diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_all-2.snap new file mode 100644 index 00000000..2941eea8 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_all-2.snap @@ -0,0 +1,19 @@ +--- +source: tests/tests/serve.rs +assertion_line: 370 +expression: "read_response.intern_and_redact(&mut redactions, root_id)" +--- +instances: + id-2: + Children: [] + ClassName: StringValue + Id: id-2 + Metadata: + ignoreUnknownInstances: true + Name: no_name_top_level_project + Parent: "00000000000000000000000000000000" + Properties: + Value: + String: "If this isn't named `no_name_top_level_project`, something went wrong!" +messageCursor: 0 +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_all.snap new file mode 100644 index 00000000..0b25468a --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_all.snap @@ -0,0 +1,41 @@ +--- +source: tests/tests/serve.rs +assertion_line: 389 +expression: "read_response.intern_and_redact(&mut redactions, root_id)" +--- +instances: + id-2: + Children: + - id-3 + ClassName: StringValue + Id: id-2 + Metadata: + ignoreUnknownInstances: true + Name: sync_rule_no_name_project + Parent: "00000000000000000000000000000000" + Properties: + Value: + String: "This should be named `sync_rule_no_name_project` and have a child at `src/not_a_project`" + id-3: + Children: + - id-4 + ClassName: Folder + Id: id-3 + Metadata: + ignoreUnknownInstances: false + Name: src + Parent: id-2 + Properties: {} + id-4: + Children: [] + ClassName: StringValue + Id: id-4 + Metadata: + ignoreUnknownInstances: true + Name: not_a_project + Parent: id-3 + Properties: + Value: + String: "If this isn't named `not_a_project`, something has gone wrong!" +messageCursor: 0 +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_info.snap new file mode 100644 index 00000000..532e8b2e --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__sync_rule_no_name_project_info.snap @@ -0,0 +1,13 @@ +--- +source: tests/tests/serve.rs +assertion_line: 383 +expression: redactions.redacted_yaml(info) +--- +expectedPlaceIds: ~ +gameId: ~ +placeId: ~ +projectName: sync_rule_no_name_project +protocolVersion: 4 +rootInstanceId: id-2 +serverVersion: "[server-version]" +sessionId: id-1 diff --git a/rojo-test/serve-tests/sync_rule_no_name_project/default.project.json b/rojo-test/serve-tests/sync_rule_no_name_project/default.project.json new file mode 100644 index 00000000..647c0adb --- /dev/null +++ b/rojo-test/serve-tests/sync_rule_no_name_project/default.project.json @@ -0,0 +1,17 @@ +{ + "tree": { + "$className": "StringValue", + "$properties": { + "Value": "This should be named `sync_rule_no_name_project` and have a child at `src/not_a_project`" + }, + "src": { + "$path": "src" + } + }, + "syncRules": [ + { + "use": "project", + "pattern": "*.rojo" + } + ] +} \ No newline at end of file diff --git a/rojo-test/serve-tests/sync_rule_no_name_project/src/not_a_project.rojo b/rojo-test/serve-tests/sync_rule_no_name_project/src/not_a_project.rojo new file mode 100644 index 00000000..83b08b77 --- /dev/null +++ b/rojo-test/serve-tests/sync_rule_no_name_project/src/not_a_project.rojo @@ -0,0 +1,8 @@ +{ + "tree": { + "$className": "StringValue", + "$properties": { + "Value": "If this isn't named `not_a_project`, something has gone wrong!" + } + } +} \ No newline at end of file diff --git a/src/cli/fmt_project.rs b/src/cli/fmt_project.rs index 9c3e88d3..1c165974 100644 --- a/src/cli/fmt_project.rs +++ b/src/cli/fmt_project.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; use anyhow::Context; use clap::Parser; +use memofs::Vfs; use crate::project::Project; @@ -17,8 +18,11 @@ pub struct FmtProjectCommand { impl FmtProjectCommand { pub fn run(self) -> anyhow::Result<()> { + let vfs = Vfs::new_default(); + vfs.set_watch_enabled(false); + let base_path = resolve_path(&self.project); - let project = Project::load_fuzzy(&base_path)? + let project = Project::load_fuzzy(&vfs, &base_path)? .context("A project file is required to run 'rojo fmt-project'")?; let serialized = serde_json::to_string_pretty(&project) diff --git a/src/project.rs b/src/project.rs index 6e503dd6..43920a9a 100644 --- a/src/project.rs +++ b/src/project.rs @@ -1,14 +1,19 @@ use std::{ collections::{BTreeMap, HashMap, HashSet}, + ffi::OsStr, fs, io, net::IpAddr, path::{Path, PathBuf}, }; +use memofs::Vfs; use serde::{Deserialize, Serialize}; use thiserror::Error; -use crate::{glob::Glob, resolution::UnresolvedValue, snapshot::SyncRule}; +use crate::{ + glob::Glob, resolution::UnresolvedValue, snapshot::SyncRule, + snapshot_middleware::default_sync_rules, +}; static PROJECT_FILENAME: &str = "default.project.json"; @@ -19,6 +24,14 @@ pub struct ProjectError(#[from] Error); #[derive(Debug, Error)] enum Error { + #[error("The folder for the provided project cannot be used as a project name: {}\n\ + Consider setting the `name` field on this project.", .path.display())] + FolderNameInvalid { path: PathBuf }, + + #[error("The file name of the provided project cannot be used as a project name: {}.\n\ + Consider setting the `name` field on this project.", .path.display())] + ProjectNameInvalid { path: PathBuf }, + #[error(transparent)] Io { #[from] @@ -135,43 +148,97 @@ impl Project { } } - pub fn load_from_slice( + /// Sets the name of a project. The order it handles is as follows: + /// + /// - If the project is a `default.project.json`, uses the folder's name + /// - If a fallback is specified, uses that blindly + /// - Otherwise, loops through sync rules (including the default ones!) and + /// uses the name of the first one that matches and is a project file + fn set_file_name(&mut self, fallback: Option<&str>) -> Result<(), Error> { + let file_name = self + .file_location + .file_name() + .and_then(OsStr::to_str) + .ok_or_else(|| Error::ProjectNameInvalid { + path: self.file_location.clone(), + })?; + + // If you're editing this to be generic, make sure you also alter the + // snapshot middleware to support generic init paths. + if file_name == PROJECT_FILENAME { + let folder_name = self.folder_location().file_name().and_then(OsStr::to_str); + if let Some(folder_name) = folder_name { + self.name = Some(folder_name.to_string()); + } else { + return Err(Error::FolderNameInvalid { + path: self.file_location.clone(), + }); + } + } else if let Some(fallback) = fallback { + self.name = Some(fallback.to_string()); + } else { + // As of the time of writing (July 10, 2024) there is no way for + // this code path to be reachable. It can in theory be reached from + // both `load_fuzzy` and `load_exact` but in practice it's never + // invoked. + // If you're adding this codepath, make sure a test for it exists + // and that it handles sync rules appropriately. + todo!( + "set_file_name doesn't support loading project files that aren't default.project.json without a fallback provided" + ); + } + + Ok(()) + } + + /// Loads a Project file from the provided contents with its source set as + /// the provided location. + fn load_from_slice( contents: &[u8], - project_file_location: &Path, - ) -> Result { + project_file_location: PathBuf, + fallback_name: Option<&str>, + ) -> Result { let mut project: Self = serde_json::from_slice(contents).map_err(|source| Error::Json { source, - path: project_file_location.to_owned(), + path: project_file_location.clone(), })?; - - project.file_location = project_file_location.to_path_buf(); + project.file_location = project_file_location; project.check_compatibility(); + if project.name.is_none() { + project.set_file_name(fallback_name)?; + } + Ok(project) } - pub fn load_fuzzy(fuzzy_project_location: &Path) -> Result, ProjectError> { + /// Loads a Project from a path. This will find the project if it refers to + /// a `.project.json` file or if it refers to a directory that contains a + /// file named `default.project.json`. + pub fn load_fuzzy( + vfs: &Vfs, + fuzzy_project_location: &Path, + ) -> Result, ProjectError> { if let Some(project_path) = Self::locate(fuzzy_project_location) { - let project = Self::load_exact(&project_path)?; - - Ok(Some(project)) + let contents = vfs.read(&project_path).map_err(Error::from)?; + Ok(Some(Self::load_from_slice(&contents, project_path, None)?)) } else { Ok(None) } } - fn load_exact(project_file_location: &Path) -> Result { - let contents = fs::read_to_string(project_file_location)?; - - let mut project: Project = - serde_json::from_str(&contents).map_err(|source| Error::Json { - source, - path: project_file_location.to_owned(), - })?; - - project.file_location = project_file_location.to_path_buf(); - project.check_compatibility(); - - Ok(project) + /// Loads a Project from a path. + pub fn load_exact( + vfs: &Vfs, + project_file_location: &Path, + fallback_name: Option<&str>, + ) -> Result { + let project_path = project_file_location.to_path_buf(); + let contents = vfs.read(&project_path).map_err(Error::from)?; + Ok(Self::load_from_slice( + &contents, + project_path, + fallback_name, + )?) } /// Checks if there are any compatibility issues with this project file and diff --git a/src/serve_session.rs b/src/serve_session.rs index dbc841d7..ddb4dc21 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -9,7 +9,6 @@ use std::{ }; use crossbeam_channel::Sender; -use memofs::IoResultExt; use memofs::Vfs; use thiserror::Error; @@ -110,43 +109,14 @@ impl ServeSession { log::debug!("Loading project file from {}", project_path.display()); - let mut root_project = match vfs.read(&project_path).with_not_found()? { - Some(contents) => Project::load_from_slice(&contents, &project_path)?, - None => { + let root_project = match Project::load_exact(&vfs, &project_path, None) { + Ok(project) => project, + Err(_) => { return Err(ServeSessionError::NoProjectFound { path: project_path.to_path_buf(), - }); + }) } }; - if root_project.name.is_none() { - if let Some(file_name) = project_path.file_name().and_then(|s| s.to_str()) { - if file_name == "default.project.json" { - let folder_name = project_path - .parent() - .and_then(Path::file_name) - .and_then(|s| s.to_str()); - if let Some(folder_name) = folder_name { - root_project.name = Some(folder_name.to_string()); - } else { - return Err(ServeSessionError::FolderNameInvalid { - path: project_path.to_path_buf(), - }); - } - } else if let Some(trimmed) = file_name.strip_suffix(".project.json") { - root_project.name = Some(trimmed.to_string()); - } else { - return Err(ServeSessionError::ProjectNameInvalid { - path: project_path.to_path_buf(), - }); - } - } else { - return Err(ServeSessionError::ProjectNameInvalid { - path: project_path.to_path_buf(), - }); - } - } - // Rebind it to make it no longer mutable - let root_project = root_project; let mut tree = RojoTree::new(InstanceSnapshot::new()); @@ -263,14 +233,6 @@ pub enum ServeSessionError { )] NoProjectFound { path: PathBuf }, - #[error("The folder for the provided project cannot be used as a project name: {}\n\ - Consider setting the `name` field on this project.", .path.display())] - FolderNameInvalid { path: PathBuf }, - - #[error("The file name of the provided project cannot be used as a project name: {}.\n\ - Consider setting the `name` field on this project.", .path.display())] - ProjectNameInvalid { path: PathBuf }, - #[error(transparent)] Io { #[from] diff --git a/src/snapshot_middleware/mod.rs b/src/snapshot_middleware/mod.rs index 1c95154e..410d96ec 100644 --- a/src/snapshot_middleware/mod.rs +++ b/src/snapshot_middleware/mod.rs @@ -63,6 +63,9 @@ pub fn snapshot_from_vfs( if meta.is_dir() { if let Some(init_path) = get_init_path(vfs, path)? { // TODO: support user-defined init paths + // If and when we do, make sure to go support it in + // `Project::set_file_name`, as right now it special-cases + // `default.project.json` as an `init` path. for rule in default_sync_rules() { if rule.matches(&init_path) { return match rule.middleware { @@ -274,7 +277,7 @@ macro_rules! sync_rule { /// Defines the 'default' syncing rules that Rojo uses. /// These do not broadly overlap, but the order matters for some in the case of /// e.g. JSON models. -fn default_sync_rules() -> &'static [SyncRule] { +pub fn default_sync_rules() -> &'static [SyncRule] { static DEFAULT_SYNC_RULES: OnceLock> = OnceLock::new(); DEFAULT_SYNC_RULES.get_or_init(|| { diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 53e65251..0cc1a400 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -22,9 +22,12 @@ pub fn snapshot_project( path: &Path, name: &str, ) -> anyhow::Result> { - let project = Project::load_from_slice(&vfs.read(path)?, path) + let project = Project::load_exact(vfs, path, Some(name)) .with_context(|| format!("File was not a valid Rojo project: {}", path.display()))?; - let project_name = project.name.as_deref().unwrap_or(name); + let project_name = match project.name.as_deref() { + Some(name) => name, + None => panic!("Project is missing a name"), + }; let mut context = context.clone(); context.clear_sync_rules(); diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__project__test__no_name_project.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__project__test__no_name_project.snap index 971027f6..7c1978ca 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__project__test__no_name_project.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__project__test__no_name_project.snap @@ -1,6 +1,5 @@ --- source: src/snapshot_middleware/project.rs -assertion_line: 730 expression: instance_snapshot --- snapshot_id: "00000000000000000000000000000000" @@ -13,7 +12,7 @@ metadata: context: emit_legacy_scripts: true specified_id: ~ -name: no_name_project +name: foo class_name: Model properties: {} children: [] diff --git a/tests/rojo_test/serve_util.rs b/tests/rojo_test/serve_util.rs index 6f049020..df0ac235 100644 --- a/tests/rojo_test/serve_util.rs +++ b/tests/rojo_test/serve_util.rs @@ -87,7 +87,7 @@ impl TestServeSession { let port_string = port.to_string(); let rojo_process = Command::new(ROJO_PATH) - .args(&[ + .args([ "serve", project_path.to_str().unwrap(), "--port", @@ -145,14 +145,14 @@ impl TestServeSession { pub fn get_api_rojo(&self) -> Result { let url = format!("http://localhost:{}/api/rojo", self.port); - let body = reqwest::blocking::get(&url)?.text()?; + let body = reqwest::blocking::get(url)?.text()?; Ok(serde_json::from_str(&body).expect("Server returned malformed response")) } pub fn get_api_read(&self, id: Ref) -> Result { let url = format!("http://localhost:{}/api/read/{}", self.port, id); - let body = reqwest::blocking::get(&url)?.text()?; + let body = reqwest::blocking::get(url)?.text()?; Ok(serde_json::from_str(&body).expect("Server returned malformed response")) } @@ -163,7 +163,7 @@ impl TestServeSession { ) -> Result, reqwest::Error> { let url = format!("http://localhost:{}/api/subscribe/{}", self.port, cursor); - reqwest::blocking::get(&url)?.json() + reqwest::blocking::get(url)?.json() } } diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index 2e8d61b8..880ae29b 100644 --- a/tests/tests/serve.rs +++ b/tests/tests/serve.rs @@ -358,6 +358,38 @@ fn no_name_top_level_project() { "no_name_top_level_project_all", read_response.intern_and_redact(&mut redactions, root_id) ); + + let project_path = session.path().join("default.project.json"); + let mut project_contents = fs::read_to_string(&project_path).unwrap(); + project_contents.push('\n'); + fs::write(&project_path, project_contents).unwrap(); + + // The cursor shouldn't be changing so this snapshot is fine for testing + // the response. + let read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "no_name_top_level_project_all-2", + read_response.intern_and_redact(&mut redactions, root_id) + ); + }); +} + +#[test] +fn sync_rule_no_name_project() { + run_serve_test("sync_rule_no_name_project", |session, mut redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + + assert_yaml_snapshot!( + "sync_rule_no_name_project_info", + redactions.redacted_yaml(info) + ); + + let read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "sync_rule_no_name_project_all", + read_response.intern_and_redact(&mut redactions, root_id) + ); }); }