Correct issue with default.project.json files with no name being named default after change (#917)

Co-authored-by: Kenneth Loeffler <kenloef@gmail.com>

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

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

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

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"
+    }
+  ]
+}

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!"
+        }
+    }
+}

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)

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,
+        project_file_location: PathBuf,
-    ) -> Result<Self, ProjectError> {
+        fallback_name: Option<&str>,
+    ) -> Result<Self, Error> {
         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;
-        project.file_location = project_file_location.to_path_buf();
         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<Option<Self>, 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<Option<Self>, ProjectError> {
         if let Some(project_path) = Self::locate(fuzzy_project_location) {
-            let project = Self::load_exact(&project_path)?;
+            let contents = vfs.read(&project_path).map_err(Error::from)?;
-
+            Ok(Some(Self::load_from_slice(&contents, project_path, None)?))
-            Ok(Some(project))
         } else {
             Ok(None)
         }
     }
 
-    fn load_exact(project_file_location: &Path) -> Result<Self, Error> {
+    /// Loads a Project from a path.
-        let contents = fs::read_to_string(project_file_location)?;
+    pub fn load_exact(
-
+        vfs: &Vfs,
-        let mut project: Project =
+        project_file_location: &Path,
-            serde_json::from_str(&contents).map_err(|source| Error::Json {
+        fallback_name: Option<&str>,
-                source,
+    ) -> Result<Self, ProjectError> {
-                path: project_file_location.to_owned(),
+        let project_path = project_file_location.to_path_buf();
-            })?;
+        let contents = vfs.read(&project_path).map_err(Error::from)?;
-
+        Ok(Self::load_from_slice(
-        project.file_location = project_file_location.to_path_buf();
+            &contents,
-        project.check_compatibility();
+            project_path,
-
+            fallback_name,
-        Ok(project)
+        )?)
     }
 
     /// Checks if there are any compatibility issues with this project file and

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()? {
+        let root_project = match Project::load_exact(&vfs, &project_path, None) {
-            Some(contents) => Project::load_from_slice(&contents, &project_path)?,
+            Ok(project) => project,
-            None => {
+            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]

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<Vec<SyncRule>> = OnceLock::new();
 
     DEFAULT_SYNC_RULES.get_or_init(|| {

src/snapshot_middleware/project.rs

@@ -22,9 +22,12 @@ pub fn snapshot_project(
     path: &Path,
     name: &str,
 ) -> anyhow::Result<Option<InstanceSnapshot>> {
-    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();

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: []

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<ServerInfoResponse, reqwest::Error> {
         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<ReadResponse, reqwest::Error> {
         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<SubscribeResponse<'static>, reqwest::Error> {
         let url = format!("http://localhost:{}/api/subscribe/{}", self.port, cursor);
 
-        reqwest::blocking::get(&url)?.json()
+        reqwest::blocking::get(url)?.json()
     }
 }
 

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)
+        );
     });
 }