mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
Enhance script handling and temp ID resolution in ServeSession and Reconciler
- Improved error handling in ServeSession by rolling back temp IDs if encoding fails for new scripts. - Added logic in applyPatch to remap echoed additions from the DescendantAdded handler, preventing duplicate instances. - Updated API to resolve intermediate container temp IDs to real tree references, ensuring proper parent-child relationships for new scripts. - Introduced new tests to verify the presence of server info flags and the resolution of temp IDs in scripts-only mode. These changes aim to optimize script management and enhance the overall functionality of the session handling.
This commit is contained in:
parent
4985a3fede
commit
9820ec6534
4 changed files with 175 additions and 9 deletions
|
|
@ -67,6 +67,33 @@ local function applyPatch(instanceMap, patch)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
-- Detect echoed additions from DescendantAdded handler.
|
||||||
|
-- When the plugin pre-inserts instances with temp IDs and the server
|
||||||
|
-- broadcasts the same instances with real Refs, remap the InstanceMap
|
||||||
|
-- entries rather than creating duplicates via reify.
|
||||||
|
local remapProgress = true
|
||||||
|
while remapProgress do
|
||||||
|
remapProgress = false
|
||||||
|
for id, virtualInstance in pairs(patch.added) do
|
||||||
|
local parentInstance = instanceMap.fromIds[virtualInstance.Parent]
|
||||||
|
if not parentInstance then continue end
|
||||||
|
for _, child in parentInstance:GetChildren() do
|
||||||
|
if child.Name == virtualInstance.Name
|
||||||
|
and child.ClassName == virtualInstance.ClassName then
|
||||||
|
local existingId = instanceMap.fromInstances[child]
|
||||||
|
if existingId and existingId ~= id then
|
||||||
|
Log.trace("Echo remap: {} -> {} for {}", existingId, id, child:GetFullName())
|
||||||
|
instanceMap:removeId(existingId)
|
||||||
|
instanceMap:insert(id, child)
|
||||||
|
patch.added[id] = nil
|
||||||
|
remapProgress = true
|
||||||
|
break
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
for id, virtualInstance in pairs(patch.added) do
|
for id, virtualInstance in pairs(patch.added) do
|
||||||
if instanceMap.fromIds[id] ~= nil then
|
if instanceMap.fromIds[id] ~= nil then
|
||||||
-- This instance already exists. We might've already added it in a
|
-- This instance already exists. We might've already added it in a
|
||||||
|
|
|
||||||
|
|
@ -961,7 +961,15 @@ function ServeSession:__connectScriptsOnlyWatchers()
|
||||||
end
|
end
|
||||||
|
|
||||||
local scriptTempId = string.gsub(HttpService:GenerateGUID(false), "-", ""):lower()
|
local scriptTempId = string.gsub(HttpService:GenerateGUID(false), "-", ""):lower()
|
||||||
patch.added[scriptTempId] = encodeInstance(descendant, prevParentId)
|
local encoded = encodeInstance(descendant, prevParentId)
|
||||||
|
if not encoded then
|
||||||
|
Log.warn("Scripts-only: failed to encode new script {}, rolling back temp IDs", descendant:GetFullName())
|
||||||
|
for tempId in patch.added do
|
||||||
|
self.__instanceMap:removeId(tempId)
|
||||||
|
end
|
||||||
|
return
|
||||||
|
end
|
||||||
|
patch.added[scriptTempId] = encoded
|
||||||
self.__instanceMap:insert(scriptTempId, descendant)
|
self.__instanceMap:insert(scriptTempId, descendant)
|
||||||
|
|
||||||
Log.info(
|
Log.info(
|
||||||
|
|
|
||||||
|
|
@ -547,6 +547,50 @@ impl ApiService {
|
||||||
|
|
||||||
if !request.added.is_empty() {
|
if !request.added.is_empty() {
|
||||||
let tree = self.serve_session.tree();
|
let tree = self.serve_session.tree();
|
||||||
|
|
||||||
|
// Phase 0: Resolve intermediate container temp IDs to real tree Refs.
|
||||||
|
// The DescendantAdded handler sends containers that already exist in the
|
||||||
|
// tree as "additions" with temp IDs. Resolve them so child additions
|
||||||
|
// (the actual new scripts) can find their parents.
|
||||||
|
let mut temp_to_real: HashMap<Ref, Ref> = HashMap::new();
|
||||||
|
let mut progress = true;
|
||||||
|
while progress {
|
||||||
|
progress = false;
|
||||||
|
for (guid, added) in request.added.iter() {
|
||||||
|
if temp_to_real.contains_key(guid) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
let parent = added.parent.unwrap_or(Ref::none());
|
||||||
|
let real_parent =
|
||||||
|
temp_to_real.get(&parent).copied().unwrap_or(parent);
|
||||||
|
if tree.get_instance(real_parent).is_none() {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
if let Some(existing_ref) =
|
||||||
|
Self::find_child_by_name(&tree, real_parent, &added.name)
|
||||||
|
{
|
||||||
|
temp_to_real.insert(*guid, existing_ref);
|
||||||
|
progress = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if !temp_to_real.is_empty() {
|
||||||
|
log::debug!(
|
||||||
|
"Scripts-only temp ID resolution: resolved {} intermediate container(s)",
|
||||||
|
temp_to_real.len()
|
||||||
|
);
|
||||||
|
for added in request.added.values_mut() {
|
||||||
|
if let Some(parent) = added.parent {
|
||||||
|
if let Some(&real) = temp_to_real.get(&parent) {
|
||||||
|
added.parent = Some(real);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
for guid in temp_to_real.keys() {
|
||||||
|
updated_in_place.insert(*guid);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Pre-compute duplicate sibling info in O(N) for efficient path uniqueness checks
|
// Pre-compute duplicate sibling info in O(N) for efficient path uniqueness checks
|
||||||
// This makes all subsequent path checks O(d) instead of O(d × s)
|
// This makes all subsequent path checks O(d) instead of O(d × s)
|
||||||
let duplicate_siblings_cache = Self::compute_tree_refs_with_duplicate_siblings(&tree);
|
let duplicate_siblings_cache = Self::compute_tree_refs_with_duplicate_siblings(&tree);
|
||||||
|
|
@ -3633,12 +3677,8 @@ fn filter_subscribe_message_for_scripts<'a>(
|
||||||
.unwrap_or(false)
|
.unwrap_or(false)
|
||||||
});
|
});
|
||||||
|
|
||||||
// Only remove scripts (plugin will ignore unknown IDs anyway)
|
// Removals are left unfiltered: we can't check removed instances in the
|
||||||
msg.removed.retain(|_id| {
|
// tree (they're already gone), and the plugin ignores unknown IDs.
|
||||||
// We can't check removed instances in the tree since they're gone,
|
|
||||||
// so we let all removals through - the plugin will ignore unknown IDs
|
|
||||||
true
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Creates an Instance for scripts-only mode.
|
/// Creates an Instance for scripts-only mode.
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
|
|
||||||
use librojo::web_api::{InstanceUpdate, WriteRequest};
|
use librojo::web_api::{AddedInstance, InstanceUpdate, WriteRequest};
|
||||||
|
use rbx_dom_weak::types::{Ref, Variant};
|
||||||
|
|
||||||
use crate::rojo_test::serve_util::run_serve_test;
|
use crate::rojo_test::serve_util::run_serve_test;
|
||||||
|
|
||||||
|
|
@ -121,7 +122,7 @@ fn scripts_only_serverinfo_flag() {
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn scripts_only_serverinfo_flag_absent_when_disabled() {
|
fn scripts_only_serverinfo_flag_present_for_other_fixture() {
|
||||||
run_serve_test("syncback_scripts_only", |session, _redactions| {
|
run_serve_test("syncback_scripts_only", |session, _redactions| {
|
||||||
let info = session.get_api_rojo().unwrap();
|
let info = session.get_api_rojo().unwrap();
|
||||||
assert!(
|
assert!(
|
||||||
|
|
@ -131,6 +132,17 @@ fn scripts_only_serverinfo_flag_absent_when_disabled() {
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn scripts_only_serverinfo_flag_false_when_disabled() {
|
||||||
|
run_serve_test("empty", |session, _redactions| {
|
||||||
|
let info = session.get_api_rojo().unwrap();
|
||||||
|
assert!(
|
||||||
|
!info.sync_scripts_only,
|
||||||
|
"syncScriptsOnly should default to false when not set in project"
|
||||||
|
);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn scripts_only_write_filters_non_script_updates() {
|
fn scripts_only_write_filters_non_script_updates() {
|
||||||
run_serve_test("scripts_only_read_pruning", |session, _redactions| {
|
run_serve_test("scripts_only_read_pruning", |session, _redactions| {
|
||||||
|
|
@ -278,3 +290,82 @@ fn scripts_only_websocket_injects_ancestors_for_new_script() {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn scripts_only_write_resolves_intermediate_temp_ids() {
|
||||||
|
run_serve_test("scripts_only_read_pruning", |session, _redactions| {
|
||||||
|
let info = session.get_api_rojo().unwrap();
|
||||||
|
let root_id = info.root_instance_id;
|
||||||
|
let read_response = session.get_api_read(root_id).unwrap();
|
||||||
|
|
||||||
|
let sss_id = read_response
|
||||||
|
.instances
|
||||||
|
.iter()
|
||||||
|
.find(|(_, inst)| inst.class_name == "ServerScriptService")
|
||||||
|
.map(|(id, _)| *id)
|
||||||
|
.expect("ServerScriptService should exist");
|
||||||
|
|
||||||
|
// PureParts exists on disk but is pruned from the read response.
|
||||||
|
// Simulate the DescendantAdded handler: send PureParts as an
|
||||||
|
// intermediate container with a temp ID, and a new script under it.
|
||||||
|
let temp_folder_ref = Ref::new();
|
||||||
|
let temp_script_ref = Ref::new();
|
||||||
|
|
||||||
|
let folder_added = AddedInstance {
|
||||||
|
parent: Some(sss_id),
|
||||||
|
name: "PureParts".to_string(),
|
||||||
|
class_name: "Folder".to_string(),
|
||||||
|
properties: HashMap::new(),
|
||||||
|
children: vec![],
|
||||||
|
};
|
||||||
|
|
||||||
|
let mut script_props = HashMap::new();
|
||||||
|
script_props.insert(
|
||||||
|
"Source".to_string(),
|
||||||
|
Variant::String("return 'temp id test'".to_string()),
|
||||||
|
);
|
||||||
|
let script_added = AddedInstance {
|
||||||
|
parent: Some(temp_folder_ref),
|
||||||
|
name: "TempIdScript".to_string(),
|
||||||
|
class_name: "ModuleScript".to_string(),
|
||||||
|
properties: script_props,
|
||||||
|
children: vec![],
|
||||||
|
};
|
||||||
|
|
||||||
|
let mut added_map = HashMap::new();
|
||||||
|
added_map.insert(temp_folder_ref, folder_added);
|
||||||
|
added_map.insert(temp_script_ref, script_added);
|
||||||
|
|
||||||
|
let write_request = WriteRequest {
|
||||||
|
session_id: info.session_id,
|
||||||
|
removed: vec![],
|
||||||
|
added: added_map,
|
||||||
|
updated: vec![],
|
||||||
|
stage_ids: Vec::new(),
|
||||||
|
};
|
||||||
|
|
||||||
|
session
|
||||||
|
.post_api_write(&write_request)
|
||||||
|
.expect("Write request should succeed");
|
||||||
|
|
||||||
|
std::thread::sleep(std::time::Duration::from_millis(500));
|
||||||
|
|
||||||
|
let read_after = session.get_api_read(root_id).unwrap();
|
||||||
|
let names: Vec<&str> = read_after
|
||||||
|
.instances
|
||||||
|
.values()
|
||||||
|
.map(|inst| inst.name.as_ref())
|
||||||
|
.collect();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
names.contains(&"TempIdScript"),
|
||||||
|
"TempIdScript should appear after temp ID resolution. Got: {:?}",
|
||||||
|
names
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
names.contains(&"PureParts"),
|
||||||
|
"PureParts should now be included (has script descendant). Got: {:?}",
|
||||||
|
names
|
||||||
|
);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue