From 67b6a7e19804cd581747cfa23e04cb8a8f16d622 Mon Sep 17 00:00:00 2001 From: Micah Date: Mon, 22 Jul 2024 12:11:28 -0700 Subject: [PATCH] Backport #917 to Rojo 7.4.x branch (#947) --- ...erve__no_name_top_level_project_all-2.snap | 19 ++++ src/cli/fmt_project.rs | 6 +- src/project.rs | 104 ++++++++++++++---- src/serve_session.rs | 44 +------- src/snapshot_middleware/project.rs | 15 ++- tests/rojo_test/serve_util.rs | 8 +- tests/tests/serve.rs | 15 ++- 7 files changed, 138 insertions(+), 73 deletions(-) create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_all-2.snap 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..23a9828c --- /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 +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/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 125b0e56..c7902cdd 100644 --- a/src/project.rs +++ b/src/project.rs @@ -1,10 +1,12 @@ 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; @@ -19,6 +21,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] @@ -129,43 +139,91 @@ 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 { + unimplemented!( + "7.4.X branch will hopefully never have a case where fallback isn't provided to set_file_name" + ); + } + + 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..ee1f97ab 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/project.rs b/src/snapshot_middleware/project.rs index f186830f..b0098c2c 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, collections::HashMap, path::Path}; +use std::{borrow::Cow, collections::HashMap, ffi::OsStr, path::Path}; use anyhow::{bail, Context}; use memofs::Vfs; @@ -19,7 +19,18 @@ pub fn snapshot_project( vfs: &Vfs, path: &Path, ) -> anyhow::Result> { - let project = Project::load_from_slice(&vfs.read(path)?, path) + let fallback_name = match path.file_name().and_then(OsStr::to_str) { + Some("default.project.json") => path + .parent() + .and_then(Path::file_name) + .and_then(OsStr::to_str), + Some(name) => name.strip_suffix(".project.json"), + None => anyhow::bail!( + "project file does not have valid utf-8 name: {}", + path.display() + ), + }; + let project = Project::load_exact(vfs, path, fallback_name) .with_context(|| format!("File was not a valid Rojo project: {}", path.display()))?; // This is not how I would normally do this, but this is a temporary 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 4f358b27..3204fe0d 100644 --- a/tests/tests/serve.rs +++ b/tests/tests/serve.rs @@ -296,16 +296,27 @@ fn no_name_top_level_project() { run_serve_test("no_name_top_level_project", |session, mut redactions| { let info = session.get_api_rojo().unwrap(); let root_id = info.root_instance_id; - assert_yaml_snapshot!( "no_name_top_level_project_info", redactions.redacted_yaml(info) ); - let read_response = session.get_api_read(root_id).unwrap(); assert_yaml_snapshot!( "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) + ); }); }