diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c090788..b0ea0d11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ Making a new release? Simply add the new header with the version and date undern * `inf` and `nan` values in properties are now synced ([#1176]) * Fixed a bug caused by having reference properties (such as `ObjectValue.Value`) that point to an Instance not included in syncback. ([#1179]) +* Implemented support for the "name" property in meta/model JSON files. ([#1187]) * Fixed instance replacement fallback failing when too many instances needed to be replaced. ([#1192]) * Added actors and bindable/remote event/function variants to be synced back as JSON files. ([#1199]) * Fixed a bug where MacOS paths weren't being handled correctly. ([#1201]) @@ -42,6 +43,7 @@ Making a new release? Simply add the new header with the version and date undern [#1176]: https://github.com/rojo-rbx/rojo/pull/1176 [#1179]: https://github.com/rojo-rbx/rojo/pull/1179 +[#1187]: https://github.com/rojo-rbx/rojo/pull/1187 [#1192]: https://github.com/rojo-rbx/rojo/pull/1192 [#1199]: https://github.com/rojo-rbx/rojo/pull/1199 [#1201]: https://github.com/rojo-rbx/rojo/pull/1201 diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__json_model_legacy_name.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__json_model_legacy_name.snap deleted file mode 100644 index ba8299cf..00000000 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__json_model_legacy_name.snap +++ /dev/null @@ -1,16 +0,0 @@ ---- -source: tests/tests/build.rs -expression: contents ---- - - - - json_model_legacy_name - - - - Expected Name - - - - diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__model_json_name_input.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__model_json_name_input.snap new file mode 100644 index 00000000..018fcd34 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__model_json_name_input.snap @@ -0,0 +1,23 @@ +--- +source: tests/tests/build.rs +assertion_line: 109 +expression: contents +--- + + + + model_json_name_input + + + + Workspace + false + + + + /Bar + + + + + diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__slugified_name_roundtrip.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__slugified_name_roundtrip.snap new file mode 100644 index 00000000..c88e9a80 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__slugified_name_roundtrip.snap @@ -0,0 +1,20 @@ +--- +source: tests/tests/build.rs +assertion_line: 108 +expression: contents +--- + + + + slugified_name_roundtrip + + + + /Script + 0 + + + + + diff --git a/rojo-test/build-tests/json_model_legacy_name/default.project.json b/rojo-test/build-tests/json_model_legacy_name/default.project.json deleted file mode 100644 index c7c56c0d..00000000 --- a/rojo-test/build-tests/json_model_legacy_name/default.project.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "name": "json_model_legacy_name", - "tree": { - "$path": "folder" - } -} \ No newline at end of file diff --git a/rojo-test/build-tests/json_model_legacy_name/folder/Expected Name.model.json b/rojo-test/build-tests/json_model_legacy_name/folder/Expected Name.model.json deleted file mode 100644 index b816d601..00000000 --- a/rojo-test/build-tests/json_model_legacy_name/folder/Expected Name.model.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "Name": "Overridden Name", - "ClassName": "Folder" -} \ No newline at end of file diff --git a/rojo-test/build-tests/model_json_name_input/default.project.json b/rojo-test/build-tests/model_json_name_input/default.project.json new file mode 100644 index 00000000..6c01ae9e --- /dev/null +++ b/rojo-test/build-tests/model_json_name_input/default.project.json @@ -0,0 +1,11 @@ +{ + "name": "model_json_name_input", + "tree": { + "$className": "DataModel", + "Workspace": { + "$className": "Workspace", + "$path": "src" + } + } +} + diff --git a/rojo-test/build-tests/model_json_name_input/src/Bar.model.json b/rojo-test/build-tests/model_json_name_input/src/Bar.model.json new file mode 100644 index 00000000..4fef832e --- /dev/null +++ b/rojo-test/build-tests/model_json_name_input/src/Bar.model.json @@ -0,0 +1,5 @@ +{ + "name": "/Bar", + "className": "StringValue" +} + diff --git a/rojo-test/build-tests/slugified_name_roundtrip/_Script/init.meta.json b/rojo-test/build-tests/slugified_name_roundtrip/_Script/init.meta.json new file mode 100644 index 00000000..eb7e860c --- /dev/null +++ b/rojo-test/build-tests/slugified_name_roundtrip/_Script/init.meta.json @@ -0,0 +1,4 @@ +{ + "name": "/Script" +} + diff --git a/rojo-test/build-tests/slugified_name_roundtrip/_Script/init.server.luau b/rojo-test/build-tests/slugified_name_roundtrip/_Script/init.server.luau new file mode 100644 index 00000000..b29a52f4 --- /dev/null +++ b/rojo-test/build-tests/slugified_name_roundtrip/_Script/init.server.luau @@ -0,0 +1,2 @@ +print("Hello world!") + diff --git a/rojo-test/build-tests/slugified_name_roundtrip/default.project.json b/rojo-test/build-tests/slugified_name_roundtrip/default.project.json new file mode 100644 index 00000000..79f6f8a5 --- /dev/null +++ b/rojo-test/build-tests/slugified_name_roundtrip/default.project.json @@ -0,0 +1,6 @@ +{ + "name": "slugified_name_roundtrip", + "tree": { + "$path": "src" + } +} diff --git a/rojo-test/build-tests/slugified_name_roundtrip/src/_Script/init.meta.json b/rojo-test/build-tests/slugified_name_roundtrip/src/_Script/init.meta.json new file mode 100644 index 00000000..617a69b2 --- /dev/null +++ b/rojo-test/build-tests/slugified_name_roundtrip/src/_Script/init.meta.json @@ -0,0 +1,3 @@ +{ + "name": "/Script" +} diff --git a/rojo-test/build-tests/slugified_name_roundtrip/src/_Script/init.server.luau b/rojo-test/build-tests/slugified_name_roundtrip/src/_Script/init.server.luau new file mode 100644 index 00000000..658f93e6 --- /dev/null +++ b/rojo-test/build-tests/slugified_name_roundtrip/src/_Script/init.server.luau @@ -0,0 +1 @@ +print("Hello world!") diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__model_json_name-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__model_json_name-stdout.snap new file mode 100644 index 00000000..c8e32ca7 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__model_json_name-stdout.snap @@ -0,0 +1,6 @@ +--- +source: tests/rojo_test/syncback_util.rs +assertion_line: 101 +expression: "String::from_utf8_lossy(&output.stdout)" +--- + 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..13188a47 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__slugified_name-stdout.snap @@ -0,0 +1,13 @@ +--- +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.meta.json +Writing src/_Script.server.luau +Writing src +Writing src/_Folder diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__model_json_name-src__foo.model.json.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__model_json_name-src__foo.model.json.snap new file mode 100644 index 00000000..ad4017dd --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__model_json_name-src__foo.model.json.snap @@ -0,0 +1,9 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/foo.model.json +--- +{ + "name": "/Bar", + "className": "StringValue" +} 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/model_json_name/input-project/default.project.json b/rojo-test/syncback-tests/model_json_name/input-project/default.project.json new file mode 100644 index 00000000..8eaa84a8 --- /dev/null +++ b/rojo-test/syncback-tests/model_json_name/input-project/default.project.json @@ -0,0 +1,11 @@ +{ + "name": "model_json_name", + "tree": { + "$className": "DataModel", + "Workspace": { + "$className": "Workspace", + "$path": "src" + } + } +} + diff --git a/rojo-test/syncback-tests/model_json_name/input-project/src/foo.model.json b/rojo-test/syncback-tests/model_json_name/input-project/src/foo.model.json new file mode 100644 index 00000000..4fef832e --- /dev/null +++ b/rojo-test/syncback-tests/model_json_name/input-project/src/foo.model.json @@ -0,0 +1,5 @@ +{ + "name": "/Bar", + "className": "StringValue" +} + diff --git a/rojo-test/syncback-tests/model_json_name/input.rbxl b/rojo-test/syncback-tests/model_json_name/input.rbxl new file mode 100644 index 00000000..9c8f1510 Binary files /dev/null and b/rojo-test/syncback-tests/model_json_name/input.rbxl differ 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 fdc2069e..b3b5c8f4 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -35,20 +35,14 @@ pub fn snapshot_json_model( format!("File is not a valid JSON model: {}", path.display()) })?; - if let Some(top_level_name) = &instance.name { - let new_name = format!("{}.model.json", top_level_name); + // If the JSON has a name property, preserve it in metadata for syncback + let specified_name = instance.name.clone(); - log::warn!( - "Model at path {} had a top-level Name field. \ - This field has been ignored since Rojo 6.0.\n\ - Consider removing this field and renaming the file to {}.", - new_name, - path.display() - ); + // Use the name from JSON if present, otherwise fall back to filename-derived name + if instance.name.is_none() { + instance.name = Some(name.to_owned()); } - instance.name = Some(name.to_owned()); - let id = instance.id.take().map(RojoRef::new); let schema = instance.schema.take(); @@ -62,7 +56,8 @@ pub fn snapshot_json_model( .relevant_paths(vec![vfs.canonicalize(path)?]) .context(context) .specified_id(id) - .schema(schema); + .schema(schema) + .specified_name(specified_name); Ok(Some(snapshot)) } @@ -81,6 +76,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 a0058555..b777fbfe 100644 --- a/src/snapshot_middleware/lua.rs +++ b/src/snapshot_middleware/lua.rs @@ -158,8 +158,16 @@ pub fn syncback_lua<'sync>( if !meta.is_empty() { let parent_location = snapshot.path.parent_err()?; + let instance_name = &snapshot.new_inst().name; + let slugified; + let meta_name = if crate::syncback::validate_file_name(instance_name).is_err() { + slugified = crate::syncback::slugify_name(instance_name); + &slugified + } else { + instance_name + }; 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..3a6a6fda 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,26 @@ 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() + ); + } + if let Some(name) = &self.name { + snapshot.name = name.clone().into(); + } + 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 +266,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 +304,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 +417,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 +447,7 @@ impl DirectoryMetadata { path, id: None, schema, + name, })) } @@ -393,6 +457,7 @@ impl DirectoryMetadata { self.apply_properties(snapshot)?; self.apply_id(snapshot)?; self.apply_schema(snapshot)?; + self.apply_name(snapshot)?; Ok(()) } @@ -464,17 +529,33 @@ 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() + ); + } + if let Some(name) = &self.name { + snapshot.name = name.clone().into(); + } + 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..ff867a83 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -8,11 +8,11 @@ use rbx_dom_weak::Instance; use crate::{snapshot::InstanceWithMeta, snapshot_middleware::Middleware}; -pub fn name_for_inst<'old>( +pub fn name_for_inst<'a>( middleware: Middleware, - new_inst: &Instance, - old_inst: Option>, -) -> anyhow::Result> { + new_inst: &'a Instance, + old_inst: Option>, +) -> anyhow::Result> { if let Some(old_inst) = old_inst { if let Some(source) = old_inst.metadata().relevant_paths.first() { source @@ -35,14 +35,24 @@ pub fn name_for_inst<'old>( | Middleware::CsvDir | Middleware::ServerScriptDir | Middleware::ClientScriptDir - | Middleware::ModuleScriptDir => Cow::Owned(new_inst.name.clone()), + | Middleware::ModuleScriptDir => { + if validate_file_name(&new_inst.name).is_err() { + Cow::Owned(slugify_name(&new_inst.name)) + } else { + Cow::Borrowed(&new_inst.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 slugified; + let final_name = 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}")) } }) } @@ -94,6 +104,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 5dc3028f..1a1a240f 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}; diff --git a/tests/tests/build.rs b/tests/tests/build.rs index 8c533115..8c3c994a 100644 --- a/tests/tests/build.rs +++ b/tests/tests/build.rs @@ -41,7 +41,6 @@ gen_build_tests! { issue_546, json_as_lua, json_model_in_folder, - json_model_legacy_name, module_in_folder, module_init, nested_runcontext, @@ -55,6 +54,8 @@ gen_build_tests! { script_meta_disabled, server_in_folder, server_init, + slugified_name_roundtrip, + model_json_name_input, txt, txt_in_folder, unresolved_values, diff --git a/tests/tests/syncback.rs b/tests/tests/syncback.rs index 31553496..10be92a8 100644 --- a/tests/tests/syncback.rs +++ b/tests/tests/syncback.rs @@ -86,4 +86,9 @@ 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 without forcing directories for leaf scripts + slugified_name => ["src/_Script.meta.json", "src/_Script.server.luau", "src/_Folder/init.meta.json"], + // Ensures that .model.json files preserve the name property + model_json_name => ["src/foo.model.json"], }