diff --git a/src/syncback/fs_snapshot.rs b/src/syncback/fs_snapshot.rs index ee9e5326..00fe2e3a 100644 --- a/src/syncback/fs_snapshot.rs +++ b/src/syncback/fs_snapshot.rs @@ -585,4 +585,169 @@ mod tests { "Non-meta file should not have substitution applied" ); } + + /// Regression test: the old approach used tentative paths (e.g., + /// "Beams1/001") as substitution keys. When multiple duplicate-named + /// instances shared the same tentative path, sequential `str::replace` + /// calls would chain — each substitution's result contained the next + /// substitution's search key as a prefix, producing corrupted paths + /// like "001~5~7~4~6~3~8~2". + /// + /// The fix uses unique per-Ref placeholders as keys, making each + /// substitution independent. This test verifies the old failure mode + /// cannot recur. + #[test] + fn fix_ref_paths_no_chaining_with_unique_placeholders() { + let mut snap = FsSnapshot::new(); + + // Simulate a Beam with Attachment0 → placeholder for one specific + // Attachment, and Attachment1 → placeholder for another. + let meta_content = r#"{ + "attributes": { + "Rojo_Ref_Attachment0": "__ROJO_REF_00000000000000000000000000000001__", + "Rojo_Ref_Attachment1": "__ROJO_REF_00000000000000000000000000000002__" + }, + "className": "Beam" +}"#; + snap.added_files.insert( + PathBuf::from("/test/BeamA.model.json5"), + meta_content.as_bytes().to_vec(), + ); + + // Each placeholder maps to a distinct dedup'd path. + let substitutions = vec![ + ( + "__ROJO_REF_00000000000000000000000000000001__".to_string(), + "Beams1/001~5".to_string(), + ), + ( + "__ROJO_REF_00000000000000000000000000000002__".to_string(), + "Beams1/002~7.model.json5".to_string(), + ), + ]; + snap.fix_ref_paths(&substitutions); + + let result = std::str::from_utf8( + snap.added_files + .get(Path::new("/test/BeamA.model.json5")) + .unwrap(), + ) + .unwrap(); + + assert!( + result.contains(r#""Rojo_Ref_Attachment0": "Beams1/001~5""#), + "Attachment0 should resolve to 001~5. Got:\n{result}" + ); + assert!( + result.contains(r#""Rojo_Ref_Attachment1": "Beams1/002~7.model.json5""#), + "Attachment1 should resolve to 002~7. Got:\n{result}" + ); + assert!( + !result.contains("__ROJO_REF_"), + "no placeholders should remain. Got:\n{result}" + ); + } + + /// Regression test: verifies that the OLD tentative-path approach + /// would chain substitutions, producing corrupted paths. This test + /// documents the exact failure mode to prevent future regressions. + #[test] + fn old_tentative_substitution_would_chain() { + let mut snap = FsSnapshot::new(); + + let meta_content = r#"{ + "attributes": { + "Rojo_Ref_Attachment0": "Beams1/001" + } +}"#; + snap.added_files.insert( + PathBuf::from("/test/BeamA.model.json5"), + meta_content.as_bytes().to_vec(), + ); + + // Simulate the old broken substitution list: same source, different targets. + // This is what happened when all 8 duplicate "001" Attachments shared + // the tentative path "Beams1/001". + let substitutions = vec![ + ("Beams1/001".to_string(), "Beams1/001~5".to_string()), + ("Beams1/001".to_string(), "Beams1/001~3".to_string()), + ("Beams1/001".to_string(), "Beams1/001~2".to_string()), + ]; + snap.fix_ref_paths(&substitutions); + + let result = std::str::from_utf8( + snap.added_files + .get(Path::new("/test/BeamA.model.json5")) + .unwrap(), + ) + .unwrap(); + + // The old approach chains: "001" → "001~5" → "001~3~5" → "001~2~3~5" + // This is the documented bug — the result is corrupted. + assert!( + result.contains("~5") && result.contains("~3") && result.contains("~2"), + "old approach should chain suffixes (documenting the bug). Got:\n{result}" + ); + assert!( + !result.contains(r#""Rojo_Ref_Attachment0": "Beams1/001~5""#), + "old approach should NOT produce the correct single-suffix result" + ); + } + + /// Tests that multiple Rojo_Ref_* attributes on the same line or in + /// the same file each resolve to their correct target independently. + #[test] + fn fix_ref_paths_multiple_refs_same_file() { + let mut snap = FsSnapshot::new(); + + let meta_content = r#"{ + "attributes": { + "Rojo_Ref_Attachment0": "__ROJO_REF_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa__", + "Rojo_Ref_Attachment1": "__ROJO_REF_bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb__", + "Rojo_Ref_PrimaryPart": "__ROJO_REF_cccccccccccccccccccccccccccccccc__" + } +}"#; + snap.added_files.insert( + PathBuf::from("/test/init.meta.json5"), + meta_content.as_bytes().to_vec(), + ); + + let substitutions = vec![ + ( + "__ROJO_REF_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa__".to_string(), + "Beams1/001~2".to_string(), + ), + ( + "__ROJO_REF_bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb__".to_string(), + "Beams1/001~5".to_string(), + ), + ( + "__ROJO_REF_cccccccccccccccccccccccccccccccc__".to_string(), + "Workspace/Model/Handle.model.json5".to_string(), + ), + ]; + snap.fix_ref_paths(&substitutions); + + let result = std::str::from_utf8( + snap.added_files + .get(Path::new("/test/init.meta.json5")) + .unwrap(), + ) + .unwrap(); + + assert!( + result.contains(r#""Rojo_Ref_Attachment0": "Beams1/001~2""#), + "Attachment0 wrong. Got:\n{result}" + ); + assert!( + result.contains(r#""Rojo_Ref_Attachment1": "Beams1/001~5""#), + "Attachment1 wrong. Got:\n{result}" + ); + assert!( + result.contains( + r#""Rojo_Ref_PrimaryPart": "Workspace/Model/Handle.model.json5""# + ), + "PrimaryPart wrong. Got:\n{result}" + ); + } } diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 62278593..666f4fb7 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -174,7 +174,8 @@ pub fn syncback_loop_with_stats( } log::debug!("Collecting referents for new DOM..."); - let deferred_referents = collect_referents(&new_tree, &pre_prune_paths, None); + let mut deferred_referents = collect_referents(&new_tree, &pre_prune_paths, None); + let placeholder_map = std::mem::take(&mut deferred_referents.placeholder_to_target); // Remove any properties that are manually blocked from syncback via the // project file. @@ -611,26 +612,26 @@ pub fn syncback_loop_with_stats( snapshots.extend(syncback.children); } - // Post-process: fix Rojo_Ref_* attribute paths with correct dedup suffixes. - // During the walk, we recorded (Ref → final_path) in the ref_path_map. - // Compare against tentative_fs_path to find instances where dedup changed the path. + // Post-process: substitute Ref placeholders with correct filesystem paths. + // During collect_referents (pre-walk), each in-DOM Ref target was assigned + // a unique placeholder string keyed by its Ref. During the walk, the + // ref_path_map was populated with (Ref → final dedup'd path). Now we + // substitute placeholder → final_path in all written meta/model files. { use ref_properties::tentative_fs_path_public; let final_map = ref_path_map.borrow(); let mut substitutions: Vec<(String, String)> = Vec::new(); - for (&inst_ref, final_path) in final_map.iter() { - let tentative = tentative_fs_path_public(&new_tree, inst_ref); - if tentative != *final_path { - substitutions.push((tentative, final_path.clone())); - } + for (placeholder, target_ref) in &placeholder_map { + let final_path = final_map + .get(target_ref) + .cloned() + .unwrap_or_else(|| tentative_fs_path_public(&new_tree, *target_ref)); + substitutions.push((placeholder.clone(), final_path)); } if !substitutions.is_empty() { - // Sort by length descending so longer paths are replaced first - // (prevents partial prefix matches on shorter paths) - substitutions.sort_by(|a, b| b.0.len().cmp(&a.0.len())); log::debug!( - "Fixing {} Rojo_Ref_* paths with dedup suffixes", + "Fixing {} Rojo_Ref_* placeholder paths", substitutions.len() ); fs_snapshot.fix_ref_paths(&substitutions); diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index be9069bd..1ef83f91 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -142,6 +142,11 @@ pub struct RefLinks { id_links: HashMap>, /// Target instances that need a Rojo_Id written (for ID-based system). targets_needing_id: HashSet, + /// Maps placeholder strings to target Refs. Populated when `final_paths` + /// is not available (CLI syncback). After the syncback walk populates the + /// `ref_path_map`, these placeholders are substituted with the correct + /// dedup-aware filesystem paths via `fix_ref_paths`. + pub placeholder_to_target: HashMap, } #[derive(PartialEq, Eq)] @@ -190,16 +195,7 @@ pub fn collect_referents( let mut path_links: HashMap> = HashMap::new(); let id_links: HashMap> = HashMap::new(); let targets_needing_id: HashSet = HashSet::new(); - - // NOTE: tentative_fs_path() does not include dedup suffixes (~N). For - // instances with duplicate names under the same parent, this produces - // ambiguous paths. Resolution via get_instance_by_path() returns the - // first matching child, which may be wrong. - // - // When `final_paths` is provided (from the syncback walk), it is used - // instead of tentative_fs_path() to get the correct dedup'd path. - // Otherwise, falls back to tentative_fs_path() (pre-dedup, potentially - // ambiguous for duplicate-named instances). + let mut placeholder_to_target: HashMap = HashMap::new(); let mut queue = VecDeque::new(); queue.push_back(dom.root_ref()); @@ -218,18 +214,25 @@ pub fn collect_referents( if dom.get_by_ref(*target_ref).is_some() { // Target exists in DOM -- use path-based system. - // Prefer final_paths (dedup-aware) when available, - // fall back to tentative_fs_path (pre-dedup). - let target_path = final_paths - .and_then(|fp| fp.get(target_ref).cloned()) - .unwrap_or_else(|| tentative_fs_path(dom, *target_ref)); + let target_path = if let Some(fp) = final_paths { + // final_paths available (dedup-aware) -- use directly. + fp.get(target_ref) + .cloned() + .unwrap_or_else(|| tentative_fs_path(dom, *target_ref)) + } else { + // No final_paths yet (pre-walk). Use a unique placeholder + // keyed by the target Ref so that duplicate-named siblings + // get distinct values. The syncback post-processing step + // substitutes these with the correct dedup'd paths. + let placeholder = ref_placeholder(*target_ref); + placeholder_to_target.insert(placeholder.clone(), *target_ref); + placeholder + }; path_links.entry(inst_ref).or_default().push(PathRefLink { name: *prop_name, path: target_path, }); } else if let Some(external_path) = pre_prune_paths.get(target_ref) { - // Target was pruned -- use pre-prune path (already in - // filesystem-name format from collect_all_paths) log::debug!( "Property {}.{} points to pruned instance at '{}', storing as path reference", tentative_fs_path(dom, inst_ref), @@ -254,9 +257,21 @@ pub fn collect_referents( path_links, id_links, targets_needing_id, + placeholder_to_target, } } +/// Prefix used for ref placeholder strings. Chosen to be unambiguous with +/// real filesystem paths (which cannot contain double underscores in this +/// position). +const REF_PLACEHOLDER_PREFIX: &str = "__ROJO_REF_"; +const REF_PLACEHOLDER_SUFFIX: &str = "__"; + +/// Build a unique placeholder string for a target Ref. +fn ref_placeholder(target: Ref) -> String { + format!("{REF_PLACEHOLDER_PREFIX}{target}{REF_PLACEHOLDER_SUFFIX}") +} + /// Writes reference attributes to instances in the DOM. /// Uses paths for unique references, IDs for non-unique (duplicate siblings). pub fn link_referents(links: RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> { @@ -406,3 +421,115 @@ fn get_existing_id_from_props(props: &rbx_dom_weak::UstrMap) -> Option< None } } + +#[cfg(test)] +mod tests { + use super::*; + use rbx_dom_weak::InstanceBuilder; + + fn make_beam_attachment_dom() -> (WeakDom, Ref, Vec, Ref) { + let mut dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let root = dom.root_ref(); + + let container = dom.insert(root, InstanceBuilder::new("Folder").with_name("Beams1")); + + let mut attachments = Vec::new(); + for _ in 0..4 { + let att = + dom.insert(container, InstanceBuilder::new("Attachment").with_name("001")); + attachments.push(att); + } + + let beam = dom.insert( + container, + InstanceBuilder::new("Beam") + .with_name("BeamA") + .with_property("Attachment0", Variant::Ref(attachments[2])), + ); + + (dom, container, attachments, beam) + } + + #[test] + fn collect_referents_uses_unique_placeholders_for_duplicate_names() { + let (dom, _, attachments, beam) = make_beam_attachment_dom(); + + let links = collect_referents(&dom, &HashMap::new(), None); + + let beam_links = links.path_links.get(&beam).expect("beam should have refs"); + assert_eq!(beam_links.len(), 1); + + let path = &beam_links[0].path; + assert!( + path.starts_with(REF_PLACEHOLDER_PREFIX), + "path should be a placeholder, got: {path}" + ); + + assert_eq!( + links.placeholder_to_target.get(path).copied(), + Some(attachments[2]), + "placeholder should map to the specific target Ref" + ); + } + + #[test] + fn placeholders_are_unique_per_target_ref() { + let mut dom2 = WeakDom::new(InstanceBuilder::new("DataModel")); + let root2 = dom2.root_ref(); + let c2 = dom2.insert(root2, InstanceBuilder::new("Folder").with_name("Beams1")); + let att_a = dom2.insert(c2, InstanceBuilder::new("Attachment").with_name("001")); + let att_b = dom2.insert(c2, InstanceBuilder::new("Attachment").with_name("001")); + dom2.insert( + c2, + InstanceBuilder::new("Beam") + .with_name("Beam1") + .with_property("Attachment0", Variant::Ref(att_a)) + .with_property("Attachment1", Variant::Ref(att_b)), + ); + + let links = collect_referents(&dom2, &HashMap::new(), None); + + let placeholders: Vec<&String> = links + .placeholder_to_target + .keys() + .collect(); + + assert!(placeholders.len() >= 2, "should have at least 2 placeholders"); + + let targets: HashSet<&Ref> = links.placeholder_to_target.values().collect(); + assert!( + targets.contains(&att_a), + "should have placeholder for att_a" + ); + assert!( + targets.contains(&att_b), + "should have placeholder for att_b" + ); + + // Crucially: the placeholder strings themselves must be distinct + let unique_placeholders: HashSet<&String> = placeholders.iter().copied().collect(); + assert_eq!( + unique_placeholders.len(), + placeholders.len(), + "all placeholders must be unique strings" + ); + } + + #[test] + fn collect_referents_with_final_paths_skips_placeholders() { + let (dom, _, attachments, beam) = make_beam_attachment_dom(); + + let mut final_paths = HashMap::new(); + final_paths.insert(attachments[2], "Beams1/001~3".to_string()); + + let links = collect_referents(&dom, &HashMap::new(), Some(&final_paths)); + + assert!( + links.placeholder_to_target.is_empty(), + "should not generate placeholders when final_paths is provided" + ); + + let beam_links = links.path_links.get(&beam).expect("beam should have refs"); + assert_eq!(beam_links[0].path, "Beams1/001~3"); + } +}