diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__slugified_name-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__slugified_name-stdout.snap new file mode 100644 index 00000000..b50c2448 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__slugified_name-stdout.snap @@ -0,0 +1,14 @@ +--- +source: tests/rojo_test/syncback_util.rs +assertion_line: 101 +expression: "String::from_utf8_lossy(&output.stdout)" +--- +Writing default.project.json +Writing src/Camera.rbxm +Writing src/Terrain.rbxm +Writing src/_Folder/init.meta.json +Writing src/_Script/init.meta.json +Writing src/_Script/init.server.luau +Writing src +Writing src/_Folder +Writing src/_Script diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Folder.model.json.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Folder.model.json.snap new file mode 100644 index 00000000..10829e88 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Folder.model.json.snap @@ -0,0 +1,8 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/_Folder.model.json +--- +{ + "className": "Folder" +} diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Folder__init.meta.json.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Folder__init.meta.json.snap new file mode 100644 index 00000000..eb336d02 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Folder__init.meta.json.snap @@ -0,0 +1,8 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/_Folder/init.meta.json +--- +{ + "name": "/Folder" +} diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script.meta.json.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script.meta.json.snap new file mode 100644 index 00000000..c4d203b8 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script.meta.json.snap @@ -0,0 +1,8 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/_Script.meta.json +--- +{ + "name": "/Script" +} diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script.server.luau.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script.server.luau.snap new file mode 100644 index 00000000..1a45520f --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script.server.luau.snap @@ -0,0 +1,6 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/_Script.server.luau +--- +print("Hello world!") diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script__init.meta.json.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script__init.meta.json.snap new file mode 100644 index 00000000..2798b41d --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script__init.meta.json.snap @@ -0,0 +1,8 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/_Script/init.meta.json +--- +{ + "name": "/Script" +} diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script__init.server.luau.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script__init.server.luau.snap new file mode 100644 index 00000000..ee496521 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__slugified_name-src___Script__init.server.luau.snap @@ -0,0 +1,6 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/_Script/init.server.luau +--- +print("Hello world!") diff --git a/rojo-test/syncback-tests/slugified_name/input-project/default.project.json b/rojo-test/syncback-tests/slugified_name/input-project/default.project.json new file mode 100644 index 00000000..38b17470 --- /dev/null +++ b/rojo-test/syncback-tests/slugified_name/input-project/default.project.json @@ -0,0 +1,10 @@ +{ + "name": "slugified_name", + "tree": { + "$className": "DataModel", + "Workspace": { + "$className": "Workspace", + "$path": "src" + } + } +} \ No newline at end of file diff --git a/rojo-test/syncback-tests/slugified_name/input-project/src/.gitkeep b/rojo-test/syncback-tests/slugified_name/input-project/src/.gitkeep new file mode 100644 index 00000000..e69de29b diff --git a/rojo-test/syncback-tests/slugified_name/input.rbxl b/rojo-test/syncback-tests/slugified_name/input.rbxl new file mode 100644 index 00000000..9b91b694 Binary files /dev/null and b/rojo-test/syncback-tests/slugified_name/input.rbxl differ diff --git a/src/snapshot/metadata.rs b/src/snapshot/metadata.rs index 79f7a2d0..273a7665 100644 --- a/src/snapshot/metadata.rs +++ b/src/snapshot/metadata.rs @@ -70,6 +70,12 @@ pub struct InstanceMetadata { /// A schema provided via a JSON file, if one exists. Will be `None` for /// all non-JSON middleware. pub schema: Option, + + /// A custom name specified via meta.json or model.json files. If present, + /// this name will be used for the instance while the filesystem name will + /// be slugified to remove illegal characters. + #[serde(skip_serializing_if = "Option::is_none")] + pub specified_name: Option, } impl InstanceMetadata { @@ -82,6 +88,7 @@ impl InstanceMetadata { specified_id: None, middleware: None, schema: None, + specified_name: None, } } @@ -130,6 +137,13 @@ impl InstanceMetadata { pub fn schema(self, schema: Option) -> Self { Self { schema, ..self } } + + pub fn specified_name(self, specified_name: Option) -> Self { + Self { + specified_name, + ..self + } + } } impl Default for InstanceMetadata { diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index d19792cb..185ab9ee 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -81,6 +81,7 @@ pub fn syncback_json_model<'sync>( // schemas will ever exist in one project for it to matter, but it // could have a performance cost. model.schema = old_inst.metadata().schema.clone(); + model.name = old_inst.metadata().specified_name.clone(); } Ok(SyncbackReturn { diff --git a/src/snapshot_middleware/lua.rs b/src/snapshot_middleware/lua.rs index eaa90952..f0ddea11 100644 --- a/src/snapshot_middleware/lua.rs +++ b/src/snapshot_middleware/lua.rs @@ -158,8 +158,14 @@ pub fn syncback_lua<'sync>( if !meta.is_empty() { let parent_location = snapshot.path.parent_err()?; + let instance_name = &snapshot.new_inst().name; + let meta_name = if crate::syncback::validate_file_name(instance_name).is_err() { + crate::syncback::slugify_name(instance_name) + } else { + instance_name.clone() + }; fs_snapshot.add_file( - parent_location.join(format!("{}.meta.json", new_inst.name)), + parent_location.join(format!("{}.meta.json", meta_name)), serde_json::to_vec_pretty(&meta).context("cannot serialize metadata")?, ); } diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 84ca9a80..23a18918 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -10,7 +10,10 @@ use rbx_dom_weak::{ use serde::{Deserialize, Serialize}; use crate::{ - json, resolution::UnresolvedValue, snapshot::InstanceSnapshot, syncback::SyncbackSnapshot, + json, + resolution::UnresolvedValue, + snapshot::InstanceSnapshot, + syncback::{validate_file_name, SyncbackSnapshot}, RojoRef, }; @@ -36,6 +39,9 @@ pub struct AdjacentMetadata { #[serde(default, skip_serializing_if = "IndexMap::is_empty")] pub attributes: IndexMap, + #[serde(skip_serializing_if = "Option::is_none")] + pub name: Option, + #[serde(skip)] pub path: PathBuf, } @@ -144,6 +150,24 @@ impl AdjacentMetadata { } } + let name = snapshot + .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 + if snapshot.old_inst().is_none() { + let instance_name = &snapshot.new_inst().name; + if validate_file_name(instance_name).is_err() { + Some(instance_name.clone()) + } else { + None + } + } else { + None + } + }); + Ok(Some(Self { ignore_unknown_instances: if ignore_unknown_instances { Some(true) @@ -155,6 +179,7 @@ impl AdjacentMetadata { path, id: None, schema, + name, })) } @@ -213,11 +238,23 @@ impl AdjacentMetadata { Ok(()) } + fn apply_name(&mut self, snapshot: &mut InstanceSnapshot) -> anyhow::Result<()> { + if self.name.is_some() && snapshot.metadata.specified_name.is_some() { + anyhow::bail!( + "cannot specify a name using {} (instance has a name from somewhere else)", + self.path.display() + ); + } + snapshot.metadata.specified_name = self.name.take(); + Ok(()) + } + pub fn apply_all(&mut self, snapshot: &mut InstanceSnapshot) -> anyhow::Result<()> { self.apply_ignore_unknown_instances(snapshot); self.apply_properties(snapshot)?; self.apply_id(snapshot)?; self.apply_schema(snapshot)?; + self.apply_name(snapshot)?; Ok(()) } @@ -226,11 +263,13 @@ impl AdjacentMetadata { /// /// - The number of properties and attributes is 0 /// - `ignore_unknown_instances` is None + /// - `name` is None #[inline] pub fn is_empty(&self) -> bool { self.attributes.is_empty() && self.properties.is_empty() && self.ignore_unknown_instances.is_none() + && self.name.is_none() } // TODO: Add method to allow selectively applying parts of metadata and @@ -262,6 +301,9 @@ pub struct DirectoryMetadata { #[serde(skip_serializing_if = "Option::is_none")] pub class_name: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub name: Option, + #[serde(skip)] pub path: PathBuf, } @@ -372,6 +414,24 @@ impl DirectoryMetadata { } } + let name = snapshot + .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 + if snapshot.old_inst().is_none() { + let instance_name = &snapshot.new_inst().name; + if validate_file_name(instance_name).is_err() { + Some(instance_name.clone()) + } else { + None + } + } else { + None + } + }); + Ok(Some(Self { ignore_unknown_instances: if ignore_unknown_instances { Some(true) @@ -384,6 +444,7 @@ impl DirectoryMetadata { path, id: None, schema, + name, })) } @@ -393,6 +454,7 @@ impl DirectoryMetadata { self.apply_properties(snapshot)?; self.apply_id(snapshot)?; self.apply_schema(snapshot)?; + self.apply_name(snapshot)?; Ok(()) } @@ -464,17 +526,30 @@ impl DirectoryMetadata { snapshot.metadata.schema = self.schema.take(); Ok(()) } + + fn apply_name(&mut self, snapshot: &mut InstanceSnapshot) -> anyhow::Result<()> { + if self.name.is_some() && snapshot.metadata.specified_name.is_some() { + anyhow::bail!( + "cannot specify a name using {} (instance has a name from somewhere else)", + self.path.display() + ); + } + snapshot.metadata.specified_name = self.name.take(); + Ok(()) + } /// Returns whether the metadata is 'empty', meaning it doesn't have anything /// worth persisting in it. Specifically: /// /// - The number of properties and attributes is 0 /// - `ignore_unknown_instances` is None /// - `class_name` is either None or not Some("Folder") + /// - `name` is None #[inline] pub fn is_empty(&self) -> bool { self.attributes.is_empty() && self.properties.is_empty() && self.ignore_unknown_instances.is_none() + && self.name.is_none() && if let Some(class) = &self.class_name { class == "Folder" } else { diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index 4653aabd..3369c640 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -35,14 +35,23 @@ pub fn name_for_inst<'old>( | Middleware::CsvDir | Middleware::ServerScriptDir | Middleware::ClientScriptDir - | Middleware::ModuleScriptDir => Cow::Owned(new_inst.name.clone()), + | Middleware::ModuleScriptDir => { + let name = if validate_file_name(&new_inst.name).is_err() { + Cow::Owned(slugify_name(&new_inst.name)) + } else { + Cow::Owned(new_inst.name.clone()) + }; + name + } _ => { let extension = extension_for_middleware(middleware); - let name = &new_inst.name; - validate_file_name(name).with_context(|| { - format!("name '{name}' is not legal to write to the file system") - })?; - Cow::Owned(format!("{name}.{extension}")) + let final_name = if validate_file_name(&new_inst.name).is_err() { + slugify_name(&new_inst.name) + } else { + new_inst.name.clone() + }; + + Cow::Owned(format!("{final_name}.{extension}")) } }) } @@ -94,6 +103,39 @@ const INVALID_WINDOWS_NAMES: [&str; 22] = [ /// in a file's name. const FORBIDDEN_CHARS: [char; 9] = ['<', '>', ':', '"', '/', '|', '?', '*', '\\']; +/// Slugifies a name by replacing forbidden characters with underscores +/// and ensuring the result is a valid file name +pub fn slugify_name(name: &str) -> String { + let mut result = String::with_capacity(name.len()); + + for ch in name.chars() { + if FORBIDDEN_CHARS.contains(&ch) { + result.push('_'); + } else { + result.push(ch); + } + } + + // Handle Windows reserved names by appending an underscore + let result_lower = result.to_lowercase(); + for forbidden in INVALID_WINDOWS_NAMES { + if result_lower == forbidden.to_lowercase() { + result.push('_'); + break; + } + } + + while result.ends_with(' ') || result.ends_with('.') { + result.pop(); + } + + if result.is_empty() || result.chars().all(|c| c == '_') { + result = "instance".to_string(); + } + + result +} + /// Validates a provided file name to ensure it's allowed on the file system. An /// error is returned if the name isn't allowed, indicating why. /// This takes into account rules for Windows, MacOS, and Linux. diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 61d26674..57d86f24 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -28,7 +28,7 @@ use crate::{ Project, }; -pub use file_names::{extension_for_middleware, name_for_inst, validate_file_name}; +pub use file_names::{extension_for_middleware, name_for_inst, slugify_name, validate_file_name}; pub use fs_snapshot::FsSnapshot; pub use hash::*; pub use property_filter::{filter_properties, filter_properties_preallocated}; @@ -360,6 +360,19 @@ pub fn get_best_middleware(snapshot: &SyncbackSnapshot) -> Middleware { } } + // If the instance name is invalid for the filesystem, use directory middleware + // to allow preserving the name in meta.json + if crate::syncback::file_names::validate_file_name(&inst.name).is_err() { + middleware = match middleware { + Middleware::ServerScript => Middleware::ServerScriptDir, + Middleware::ClientScript => Middleware::ClientScriptDir, + Middleware::ModuleScript => Middleware::ModuleScriptDir, + Middleware::Csv => Middleware::CsvDir, + Middleware::JsonModel | Middleware::Text => Middleware::Dir, + _ => middleware, + } + } + if middleware == Middleware::Rbxm { middleware = match env::var(DEBUG_MODEL_FORMAT_VAR) { Ok(value) if value == "1" => Middleware::Rbxmx, diff --git a/tests/tests/syncback.rs b/tests/tests/syncback.rs index 31553496..c21824ce 100644 --- a/tests/tests/syncback.rs +++ b/tests/tests/syncback.rs @@ -86,4 +86,7 @@ syncback_tests! { sync_rules => ["src/module.modulescript", "src/text.text"], // Ensures that the `syncUnscriptable` setting works unscriptable_properties => ["default.project.json"], + // Ensures that instances with names containing illegal characters get slugified filenames + // and preserve their original names in meta.json + slugified_name => ["src/_Script/init.meta.json", "src/_Script/init.server.luau", "src/_Folder/init.meta.json"], }