diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index fb444c37..50e211fd 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -8,7 +8,7 @@ use memofs::{DirEntry, Vfs}; use crate::{ snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource}, - syncback::{hash_instance, FsSnapshot, SyncbackReturn, SyncbackSnapshot}, + syncback::{hash_instance, slugify_name, FsSnapshot, SyncbackReturn, SyncbackSnapshot}, }; use super::{meta_file::DirectoryMetadata, snapshot_from_vfs}; @@ -134,12 +134,39 @@ pub fn syncback_dir_no_meta<'sync>( let mut children = Vec::new(); let mut removed_children = Vec::new(); - // We have to enforce unique child names for the file system. - let mut child_names = HashSet::with_capacity(new_inst.children().len()); + // Build the old child map early so it can be used for deduplication below. + let mut old_child_map = HashMap::new(); + if let Some(old_inst) = snapshot.old_inst() { + for child in old_inst.children() { + let inst = snapshot.get_old_instance(*child).unwrap(); + old_child_map.insert(inst.name(), inst); + } + } + + // Enforce unique filesystem names. Uses actual on-disk names for existing + // children and resolved names (with init-prefix) for new ones. + let mut fs_child_names = HashSet::with_capacity(new_inst.children().len()); let mut duplicate_set = HashSet::new(); for child_ref in new_inst.children() { let child = snapshot.get_new_instance(*child_ref).unwrap(); - if !child_names.insert(child.name.to_lowercase()) { + let fs_name = old_child_map + .get(child.name.as_str()) + .and_then(|old| old.metadata().relevant_paths.first()) + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()) + .map(|s| s.to_lowercase()) + .unwrap_or_else(|| { + let slug = slugify_name(&child.name); + let slug_lower = slug.to_lowercase(); + // Mirror name_for_inst's init-prefix. + if slug_lower == "init" { + format!("_{slug_lower}") + } else { + slug_lower + } + }); + + if !fs_child_names.insert(fs_name) { duplicate_set.insert(child.name.as_str()); } } @@ -153,13 +180,7 @@ pub fn syncback_dir_no_meta<'sync>( anyhow::bail!("Instance has more than 25 children with duplicate names"); } - if let Some(old_inst) = snapshot.old_inst() { - let mut old_child_map = HashMap::with_capacity(old_inst.children().len()); - for child in old_inst.children() { - let inst = snapshot.get_old_instance(*child).unwrap(); - old_child_map.insert(inst.name(), inst); - } - + if snapshot.old_inst().is_some() { for new_child_ref in new_inst.children() { let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); if let Some(old_child) = old_child_map.remove(new_child.name.as_str()) { @@ -225,6 +246,12 @@ pub fn syncback_dir_no_meta<'sync>( mod test { use super::*; + use std::path::PathBuf; + + use crate::{ + snapshot::{InstanceMetadata, InstanceSnapshot}, + Project, RojoTree, SyncbackData, SyncbackSnapshot, + }; use memofs::{InMemoryFs, VfsSnapshot}; #[test] @@ -261,4 +288,237 @@ mod test { insta::assert_yaml_snapshot!(instance_snapshot); } + + fn make_project() -> Project { + serde_json::from_str(r#"{"tree": {"$className": "DataModel"}}"#).unwrap() + } + + fn make_vfs() -> Vfs { + let mut imfs = InMemoryFs::new(); + imfs.load_snapshot("/root", VfsSnapshot::empty_dir()).unwrap(); + Vfs::new(imfs) + } + + /// Two children whose Roblox names are identical when lowercased ("Alpha" + /// and "alpha") but live at different filesystem paths because of the + /// `name` property ("Beta/" and "Alpha/" respectively). The dedup check + /// must use the actual filesystem paths, not the raw Roblox names, to + /// avoid a false-positive duplicate error. + #[test] + fn syncback_no_false_duplicate_with_name_prop() { + use rbx_dom_weak::{InstanceBuilder, WeakDom}; + + // Old child A: Roblox name "Alpha", on disk at "/root/Beta" + // (name property maps "Alpha" → "Beta" on the filesystem) + let old_child_a = InstanceSnapshot::new() + .name("Alpha") + .class_name("Folder") + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root/Beta")) + .relevant_paths(vec![PathBuf::from("/root/Beta")]), + ); + // Old child B: Roblox name "alpha", on disk at "/root/Alpha" + let old_child_b = InstanceSnapshot::new() + .name("alpha") + .class_name("Folder") + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root/Alpha")) + .relevant_paths(vec![PathBuf::from("/root/Alpha")]), + ); + let old_parent = InstanceSnapshot::new() + .name("Parent") + .class_name("Folder") + .children(vec![old_child_a, old_child_b]) + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root")) + .relevant_paths(vec![PathBuf::from("/root")]), + ); + let old_tree = RojoTree::new(old_parent); + + // New state: same two children in Roblox. + let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT")); + let new_parent = new_tree.insert( + new_tree.root_ref(), + InstanceBuilder::new("Folder").with_name("Parent"), + ); + new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Alpha")); + new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("alpha")); + + let vfs = make_vfs(); + let project = make_project(); + let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project); + let snapshot = SyncbackSnapshot { + data, + old: Some(old_tree.get_root_id()), + new: new_parent, + path: PathBuf::from("/root"), + middleware: None, + }; + + let result = syncback_dir_no_meta(&snapshot); + assert!( + result.is_ok(), + "should not error when two children have the same lowercased Roblox \ + name but map to distinct filesystem paths: {result:?}", + ); + } + + /// Two completely new children with the same non-init name would produce + /// the same filesystem entry and must be detected as a duplicate. + #[test] + fn syncback_detects_sibling_duplicate_names() { + use rbx_dom_weak::{InstanceBuilder, WeakDom}; + + let old_parent = InstanceSnapshot::new() + .name("Parent") + .class_name("Folder") + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root")) + .relevant_paths(vec![PathBuf::from("/root")]), + ); + let old_tree = RojoTree::new(old_parent); + + let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT")); + let new_parent = new_tree.insert( + new_tree.root_ref(), + InstanceBuilder::new("Folder").with_name("Parent"), + ); + // "Foo" is not a reserved name but two siblings named "Foo" still + // collide on disk. + new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo")); + new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo")); + + let vfs = make_vfs(); + let project = make_project(); + let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project); + let snapshot = SyncbackSnapshot { + data, + old: Some(old_tree.get_root_id()), + new: new_parent, + path: PathBuf::from("/root"), + middleware: None, + }; + + let result = syncback_dir_no_meta(&snapshot); + assert!( + result.is_err(), + "should error when two new children would produce the same filesystem name", + ); + } + + /// A new child named "Init" (as a ModuleScript) would naively become + /// "Init.luau", which case-insensitively matches the parent's reserved + /// "init.luau". Syncback must resolve this automatically by prefixing the + /// filesystem name with '_' (→ "_Init.luau") rather than erroring. + #[test] + fn syncback_resolves_init_name_conflict() { + use rbx_dom_weak::{InstanceBuilder, WeakDom}; + + let old_parent = InstanceSnapshot::new() + .name("Parent") + .class_name("Folder") + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root")) + .relevant_paths(vec![PathBuf::from("/root")]), + ); + let old_tree = RojoTree::new(old_parent); + + let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT")); + let new_parent = new_tree.insert( + new_tree.root_ref(), + InstanceBuilder::new("Folder").with_name("Parent"), + ); + new_tree.insert( + new_parent, + InstanceBuilder::new("ModuleScript").with_name("Init"), + ); + + let vfs = make_vfs(); + let project = make_project(); + let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project); + let snapshot = SyncbackSnapshot { + data, + old: Some(old_tree.get_root_id()), + new: new_parent, + path: PathBuf::from("/root"), + middleware: None, + }; + + let result = syncback_dir_no_meta(&snapshot); + assert!( + result.is_ok(), + "should resolve init-name conflict by prefixing '_', not error: {result:?}", + ); + // The child should have been placed at "_Init.luau", not "Init.luau". + let child_file_name = result + .unwrap() + .children + .into_iter() + .next() + .and_then(|c| c.path.file_name().map(|n| n.to_string_lossy().into_owned())) + .unwrap_or_default(); + assert!( + child_file_name.starts_with('_'), + "child filesystem name should start with '_' to avoid init collision, \ + got: {child_file_name}", + ); + } + + /// A child whose filesystem name is stored with a slugified prefix (e.g. + /// "_Init") must NOT be blocked — only the bare "init" stem is reserved. + #[test] + fn syncback_allows_slugified_init_name() { + use rbx_dom_weak::{InstanceBuilder, WeakDom}; + + // Existing child: on disk as "_Init" (slugified from a name with an + // illegal character), its stem is "_init" which is not reserved. + let old_child = InstanceSnapshot::new() + .name("Init") + .class_name("Folder") + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root/_Init")) + .relevant_paths(vec![PathBuf::from("/root/_Init")]), + ); + let old_parent = InstanceSnapshot::new() + .name("Parent") + .class_name("Folder") + .children(vec![old_child]) + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root")) + .relevant_paths(vec![PathBuf::from("/root")]), + ); + let old_tree = RojoTree::new(old_parent); + + let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT")); + let new_parent = new_tree.insert( + new_tree.root_ref(), + InstanceBuilder::new("Folder").with_name("Parent"), + ); + new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Init")); + + let vfs = make_vfs(); + let project = make_project(); + let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project); + let snapshot = SyncbackSnapshot { + data, + old: Some(old_tree.get_root_id()), + new: new_parent, + path: PathBuf::from("/root"), + middleware: None, + }; + + let result = syncback_dir_no_meta(&snapshot); + assert!( + result.is_ok(), + "should allow a child whose filesystem name is slugified away from \ + the reserved 'init' stem: {result:?}", + ); + } } diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 3a6a6fda..81cbe0cb 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -154,11 +154,18 @@ impl AdjacentMetadata { .old_inst() .and_then(|inst| inst.metadata().specified_name.clone()) .or_else(|| { - // If this is a new instance and its name is invalid for the filesystem, - // we need to specify the name in meta.json so it can be preserved + // Write name when the filesystem path doesn't match the + // instance name (invalid chars or init-prefix). if snapshot.old_inst().is_none() { let instance_name = &snapshot.new_inst().name; - if validate_file_name(instance_name).is_err() { + let fs_stem = path + .file_name() + .and_then(|n| n.to_str()) + .map(|s| s.split('.').next().unwrap_or(s)) + .unwrap_or(""); + if validate_file_name(instance_name).is_err() + || fs_stem != instance_name.as_str() + { Some(instance_name.clone()) } else { None @@ -421,11 +428,17 @@ impl DirectoryMetadata { .old_inst() .and_then(|inst| inst.metadata().specified_name.clone()) .or_else(|| { - // If this is a new instance and its name is invalid for the filesystem, - // we need to specify the name in meta.json so it can be preserved + // Write name when the directory name doesn't match the + // instance name (invalid chars or init-prefix). if snapshot.old_inst().is_none() { let instance_name = &snapshot.new_inst().name; - if validate_file_name(instance_name).is_err() { + let fs_name = path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(""); + if validate_file_name(instance_name).is_err() + || fs_name != instance_name.as_str() + { Some(instance_name.clone()) } else { None diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index ff867a83..c1d0d459 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -36,23 +36,33 @@ pub fn name_for_inst<'a>( | Middleware::ServerScriptDir | Middleware::ClientScriptDir | Middleware::ModuleScriptDir => { - if validate_file_name(&new_inst.name).is_err() { + let name = if validate_file_name(&new_inst.name).is_err() { Cow::Owned(slugify_name(&new_inst.name)) } else { - Cow::Borrowed(&new_inst.name) + Cow::Borrowed(new_inst.name.as_str()) + }; + // Prefix "init" to avoid colliding with reserved init files. + if name.to_lowercase() == "init" { + Cow::Owned(format!("_{name}")) + } else { + name } } _ => { let extension = extension_for_middleware(middleware); let slugified; - let final_name = if validate_file_name(&new_inst.name).is_err() { + let stem: &str = if validate_file_name(&new_inst.name).is_err() { slugified = slugify_name(&new_inst.name); &slugified } else { &new_inst.name }; - - Cow::Owned(format!("{final_name}.{extension}")) + // Prefix "init" stems to avoid colliding with reserved init files. + if stem.to_lowercase() == "init" { + Cow::Owned(format!("_{stem}.{extension}")) + } else { + Cow::Owned(format!("{stem}.{extension}")) + } } }) } diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index 384033d6..dfdf1a62 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -237,6 +237,24 @@ pub fn inst_path(dom: &WeakDom, referent: Ref) -> String { path.join("/") } +impl<'sync> SyncbackData<'sync> { + /// Constructs a `SyncbackData` for use in unit tests. + #[cfg(test)] + pub fn for_test( + vfs: &'sync Vfs, + old_tree: &'sync RojoTree, + new_tree: &'sync WeakDom, + project: &'sync Project, + ) -> Self { + Self { + vfs, + old_tree, + new_tree, + project, + } + } +} + #[cfg(test)] mod test { use rbx_dom_weak::{InstanceBuilder, WeakDom};