Add support for optional paths (#472)

* Add PathNode with optional fields to project. This allows a path to be defined either as `"$path": "src"` or `"$path": { "optional": "src" }`

* Make $path truly optional

* Prevent rojo from erroring if no project node is resolved

* Use match instead of if-statement

* Add end-to-end tests (credit to MobiusCraftFlip for initial scenario)

* Pass option with ref inside instead of reference to option

* Empty commit to restart GitHub Actions

* Simplify build test

* Minimize serve test: it fails

* Simplify serve test even more

* Ignore failing serve test

Co-authored-by: Lucien Greathouse <me@lpghatguy.com>

rojo-test/build-test-snapshots/end_to_end__tests__build__optional.snap

@@ -0,0 +1,24 @@
+---
+source: tests/tests/build.rs
+expression: contents
+
+---
+<roblox version="4">
+  <Item class="Folder" referent="0">
+    <Properties>
+      <string name="Name">optional</string>
+    </Properties>
+    <Item class="StringValue" referent="1">
+      <Properties>
+        <string name="Name">foo-optional</string>
+        <string name="Value">Hello, from foo.txt!</string>
+      </Properties>
+    </Item>
+    <Item class="StringValue" referent="2">
+      <Properties>
+        <string name="Name">foo-required</string>
+        <string name="Value">Hello, from foo.txt!</string>
+      </Properties>
+    </Item>
+  </Item>
+</roblox>

rojo-test/build-tests/optional/default.project.json

@@ -0,0 +1,15 @@
+{
+  "name": "optional",
+  "tree": {
+    "$className": "Folder",
+    "foo-required": {
+      "$path": "foo.txt"
+    },
+    "foo-optional":{
+      "$path": { "optional": "foo.txt" }
+    },
+    "bar-optional":{
+      "$path": { "optional": "bar.txt" }
+    }
+  }
+}

rojo-test/build-tests/optional/foo.txt

@@ -0,0 +1 @@
+Hello, from foo.txt!

rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder.snap

@@ -0,0 +1,14 @@
+---
+source: tests/tests/serve.rs
+expression: redactions.redacted_yaml(info)
+
+---
+expectedPlaceIds: ~
+gameId: ~
+placeId: ~
+projectName: optional
+protocolVersion: 4
+rootInstanceId: id-2
+serverVersion: "[server-version]"
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_all-2.snap

@@ -0,0 +1,62 @@
+---
+source: tests/tests/serve.rs
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+
+---
+instances:
+  id-2:
+    Children:
+      - id-3
+    ClassName: Folder
+    Id: id-2
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: optional
+    Parent: "00000000000000000000000000000000"
+    Properties: {}
+  id-3:
+    Children:
+      - id-4
+      - id-5
+    ClassName: Folder
+    Id: id-3
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: src
+    Parent: id-2
+    Properties: {}
+  id-4:
+    Children: []
+    ClassName: StringValue
+    Id: id-4
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: foo
+    Parent: id-3
+    Properties:
+      Value:
+        String: "Hello, from foo.txt!"
+  id-5:
+    Children:
+      - id-6
+    ClassName: Folder
+    Id: id-5
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: node_modules
+    Parent: id-3
+    Properties: {}
+  id-6:
+    Children: []
+    ClassName: StringValue
+    Id: id-6
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: bar
+    Parent: id-5
+    Properties:
+      Value:
+        String: Hello from bar.txt
+messageCursor: 2
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_all.snap

@@ -0,0 +1,40 @@
+---
+source: tests/tests/serve.rs
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+
+---
+instances:
+  id-2:
+    Children:
+      - id-3
+    ClassName: Folder
+    Id: id-2
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: optional
+    Parent: "00000000000000000000000000000000"
+    Properties: {}
+  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: false
+    Name: foo
+    Parent: id-3
+    Properties:
+      Value:
+        String: "Hello, from foo.txt!"
+messageCursor: 0
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_subscribe.snap

@@ -0,0 +1,36 @@
+---
+source: tests/tests/serve.rs
+expression: "subscribe_response.intern_and_redact(&mut redactions, ())"
+
+---
+messageCursor: 2
+messages:
+  - added:
+      id-5:
+        Children:
+          - id-6
+        ClassName: Folder
+        Id: id-5
+        Metadata:
+          ignoreUnknownInstances: false
+        Name: node_modules
+        Parent: id-3
+        Properties: {}
+      id-6:
+        Children: []
+        ClassName: StringValue
+        Id: id-6
+        Metadata:
+          ignoreUnknownInstances: false
+        Name: bar
+        Parent: id-5
+        Properties:
+          Value:
+            String: Hello from bar.txt
+    removed: []
+    updated: []
+  - added: {}
+    removed: []
+    updated: []
+sessionId: id-1
+

rojo-test/serve-tests/add_optional_folder/default.project.json

@@ -0,0 +1,9 @@
+{
+  "name": "optional",
+  "tree": {
+    "$className": "Folder",
+    "create-later": {
+      "$path": { "optional": "create-later" }
+    }
+  }
+}

src/change_processor.rs

@@ -284,22 +284,14 @@ fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option<
                 // that path and use it as the source for our patch.
 
                 let snapshot = match snapshot_from_vfs(&metadata.context, &vfs, &path) {
-                    Ok(Some(snapshot)) => snapshot,
+                    Ok(snapshot) => snapshot,
-                    Ok(None) => {
-                        log::error!(
-                            "Snapshot did not return an instance from path {}",
-                            path.display()
-                        );
-                        log::error!("This may be a bug!");
-                        return None;
-                    }
                     Err(err) => {
                         log::error!("Snapshot error: {:?}", err);
                         return None;
                     }
                 };
 
-                let patch_set = compute_patch_set(&snapshot, &tree, id);
+                let patch_set = compute_patch_set(snapshot.as_ref(), &tree, id);
                 apply_patch_set(tree, patch_set)
             }
             Ok(None) => {
@@ -335,19 +327,14 @@ fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option<
             );
 
             let snapshot = match snapshot_result {
-                Ok(Some(snapshot)) => snapshot,
+                Ok(snapshot) => snapshot,
-                Ok(None) => {
-                    log::error!("Snapshot did not return an instance from a project node.");
-                    log::error!("This is a bug!");
-                    return None;
-                }
                 Err(err) => {
                     log::error!("{:?}", err);
                     return None;
                 }
             };
 
-            let patch_set = compute_patch_set(&snapshot, &tree, id);
+            let patch_set = compute_patch_set(snapshot.as_ref(), &tree, id);
             apply_patch_set(tree, patch_set)
         }
     };

src/project.rs

@@ -174,6 +174,35 @@ impl Project {
     }
 }
 
+#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)]
+pub struct OptionalPathNode {
+    #[serde(serialize_with = "crate::path_serializer::serialize_absolute")]
+    pub optional: PathBuf,
+}
+
+impl OptionalPathNode {
+    pub fn new(optional: PathBuf) -> Self {
+        OptionalPathNode { optional }
+    }
+}
+
+/// Describes a path that is either optional or required
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(untagged)]
+pub enum PathNode {
+    Required(#[serde(serialize_with = "crate::path_serializer::serialize_absolute")] PathBuf),
+    Optional(OptionalPathNode),
+}
+
+impl PathNode {
+    pub fn path(&self) -> &Path {
+        match self {
+            PathNode::Required(pathbuf) => &pathbuf,
+            PathNode::Optional(OptionalPathNode { optional }) => &optional,
+        }
+    }
+}
+
 /// Describes an instance and its descendants in a project.
 #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)]
 pub struct ProjectNode {
@@ -224,12 +253,8 @@ pub struct ProjectNode {
     /// path can point to any file type supported by Rojo, including Lua files
     /// (`.lua`), Roblox models (`.rbxm`, `.rbxmx`), and localization table
     /// spreadsheets (`.csv`).
-    #[serde(
+    #[serde(rename = "$path", skip_serializing_if = "Option::is_none")]
-        rename = "$path",
+    pub path: Option<PathNode>,
-        serialize_with = "crate::path_serializer::serialize_option_absolute",
-        skip_serializing_if = "Option::is_none"
-    )]
-    pub path: Option<PathBuf>,
 }
 
 impl ProjectNode {
@@ -249,3 +274,106 @@ impl ProjectNode {
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn path_node_required() {
+        let path_node: PathNode = serde_json::from_str(r#""src""#).unwrap();
+        assert_eq!(path_node, PathNode::Required(PathBuf::from("src")));
+    }
+
+    #[test]
+    fn path_node_optional() {
+        let path_node: PathNode = serde_json::from_str(r#"{ "optional": "src" }"#).unwrap();
+        assert_eq!(
+            path_node,
+            PathNode::Optional(OptionalPathNode::new(PathBuf::from("src")))
+        );
+    }
+
+    #[test]
+    fn project_node_required() {
+        let project_node: ProjectNode = serde_json::from_str(
+            r#"{
+                "$path": "src"
+            }"#,
+        )
+        .unwrap();
+
+        assert_eq!(
+            project_node.path,
+            Some(PathNode::Required(PathBuf::from("src")))
+        );
+    }
+
+    #[test]
+    fn project_node_optional() {
+        let project_node: ProjectNode = serde_json::from_str(
+            r#"{
+                "$path": { "optional": "src" }
+            }"#,
+        )
+        .unwrap();
+
+        assert_eq!(
+            project_node.path,
+            Some(PathNode::Optional(OptionalPathNode::new(PathBuf::from(
+                "src"
+            ))))
+        );
+    }
+
+    #[test]
+    fn project_node_none() {
+        let project_node: ProjectNode = serde_json::from_str(
+            r#"{
+                "$className": "Folder"
+            }"#,
+        )
+        .unwrap();
+
+        assert_eq!(project_node.path, None);
+    }
+
+    #[test]
+    fn project_node_optional_serialize_absolute() {
+        let project_node: ProjectNode = serde_json::from_str(
+            r#"{
+                "$path": { "optional": "..\\src" }
+            }"#,
+        )
+        .unwrap();
+
+        let serialized = serde_json::to_string(&project_node).unwrap();
+        assert_eq!(serialized, r#"{"$path":{"optional":"../src"}}"#);
+    }
+
+    #[test]
+    fn project_node_optional_serialize_absolute_no_change() {
+        let project_node: ProjectNode = serde_json::from_str(
+            r#"{
+                "$path": { "optional": "../src" }
+            }"#,
+        )
+        .unwrap();
+
+        let serialized = serde_json::to_string(&project_node).unwrap();
+        assert_eq!(serialized, r#"{"$path":{"optional":"../src"}}"#);
+    }
+
+    #[test]
+    fn project_node_optional_serialize_optional() {
+        let project_node: ProjectNode = serde_json::from_str(
+            r#"{
+                "$path": "..\\src"
+            }"#,
+        )
+        .unwrap();
+
+        let serialized = serde_json::to_string(&project_node).unwrap();
+        assert_eq!(serialized, r#"{"$path":"../src"}"#);
+    }
+}

src/serve_session.rs

@@ -127,11 +127,10 @@ impl ServeSession {
         let instance_context = InstanceContext::default();
 
         log::trace!("Generating snapshot of instances from VFS");
-        let snapshot = snapshot_from_vfs(&instance_context, &vfs, &start_path)?
+        let snapshot = snapshot_from_vfs(&instance_context, &vfs, &start_path)?;
-            .expect("snapshot did not return an instance");
 
         log::trace!("Computing initial patch set");
-        let patch_set = compute_patch_set(&snapshot, &tree, root_id);
+        let patch_set = compute_patch_set(snapshot.as_ref(), &tree, root_id);
 
         log::trace!("Applying initial patch set");
         apply_patch_set(&mut tree, patch_set);

src/snapshot/patch_compute.rs

@@ -10,16 +10,27 @@ use super::{
     InstanceSnapshot, InstanceWithMeta, RojoTree,
 };
 
-pub fn compute_patch_set(snapshot: &InstanceSnapshot, tree: &RojoTree, id: Ref) -> PatchSet {
+pub fn compute_patch_set(
+    snapshot: Option<&InstanceSnapshot>,
+    tree: &RojoTree,
+    id: Ref,
+) -> PatchSet {
     let mut patch_set = PatchSet::new();
-    let mut context = ComputePatchContext::default();
 
-    compute_patch_set_internal(&mut context, snapshot, tree, id, &mut patch_set);
+    if let Some(snapshot) = snapshot {
+        let mut context = ComputePatchContext::default();
 
-    // Rewrite Ref properties to refer to instance IDs instead of snapshot IDs
+        compute_patch_set_internal(&mut context, snapshot, tree, id, &mut patch_set);
-    // for all of the IDs that we know about so far.
+
-    rewrite_refs_in_updates(&context, &mut patch_set.updated_instances);
+        // Rewrite Ref properties to refer to instance IDs instead of snapshot IDs
-    rewrite_refs_in_additions(&context, &mut patch_set.added_instances);
+        // for all of the IDs that we know about so far.
+        rewrite_refs_in_updates(&context, &mut patch_set.updated_instances);
+        rewrite_refs_in_additions(&context, &mut patch_set.added_instances);
+    } else {
+        if id != tree.get_root_id() {
+            patch_set.removed_instances.push(id);
+        }
+    }
 
     patch_set
 }
@@ -246,7 +257,7 @@ mod test {
             children: Vec::new(),
         };
 
-        let patch_set = compute_patch_set(&snapshot, &tree, root_id);
+        let patch_set = compute_patch_set(Some(&snapshot), &tree, root_id);
 
         let expected_patch_set = PatchSet {
             updated_instances: vec![PatchUpdate {
@@ -296,7 +307,7 @@ mod test {
             class_name: Cow::Borrowed("foo"),
         };
 
-        let patch_set = compute_patch_set(&snapshot, &tree, root_id);
+        let patch_set = compute_patch_set(Some(&snapshot), &tree, root_id);
 
         let expected_patch_set = PatchSet {
             added_instances: vec![PatchAdd {

src/snapshot/tests/compute.rs

@@ -23,7 +23,7 @@ fn set_name_and_class_name() {
         children: Vec::new(),
     };
 
-    let patch_set = compute_patch_set(&snapshot, &tree, tree.get_root_id());
+    let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id());
     let patch_value = redactions.redacted_yaml(patch_set);
 
     assert_yaml_snapshot!(patch_value);
@@ -47,7 +47,7 @@ fn set_property() {
         children: Vec::new(),
     };
 
-    let patch_set = compute_patch_set(&snapshot, &tree, tree.get_root_id());
+    let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id());
     let patch_value = redactions.redacted_yaml(patch_set);
 
     assert_yaml_snapshot!(patch_value);
@@ -78,7 +78,7 @@ fn remove_property() {
         children: Vec::new(),
     };
 
-    let patch_set = compute_patch_set(&snapshot, &tree, tree.get_root_id());
+    let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id());
     let patch_value = redactions.redacted_yaml(patch_set);
 
     assert_yaml_snapshot!(patch_value);
@@ -107,7 +107,7 @@ fn add_child() {
         }],
     };
 
-    let patch_set = compute_patch_set(&snapshot, &tree, tree.get_root_id());
+    let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id());
     let patch_value = redactions.redacted_yaml(patch_set);
 
     assert_yaml_snapshot!(patch_value);
@@ -139,7 +139,7 @@ fn remove_child() {
         children: Vec::new(),
     };
 
-    let patch_set = compute_patch_set(&snapshot, &tree, tree.get_root_id());
+    let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id());
     let patch_value = redactions.redacted_yaml(patch_set);
 
     assert_yaml_snapshot!(patch_value);

src/snapshot_middleware/project.rs

@@ -5,7 +5,7 @@ use memofs::Vfs;
 use rbx_reflection::ClassTag;
 
 use crate::{
-    project::{Project, ProjectNode},
+    project::{PathNode, Project, ProjectNode},
     snapshot::{
         InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource, PathIgnoreRule,
     },
@@ -30,30 +30,31 @@ pub fn snapshot_project(
 
     context.add_path_ignore_rules(rules);
 
-    // TODO: If this project node is a path to an instance that Rojo doesn't
+    match snapshot_project_node(&context, path, &project.name, &project.tree, vfs, None)? {
-    // understand, this may panic!
+        Some(found_snapshot) => {
-    let mut snapshot =
+            let mut snapshot = found_snapshot;
-        snapshot_project_node(&context, path, &project.name, &project.tree, vfs, None)?.unwrap();
+            // Setting the instigating source to the project file path is a little
+            // coarse.
+            //
+            // Ideally, we'd only snapshot the project file if the project file
+            // actually changed. Because Rojo only has the concept of one
+            // relevant path -> snapshot path mapping per instance, we pick the more
+            // conservative approach of snapshotting the project file if any
+            // relevant paths changed.
+            snapshot.metadata.instigating_source = Some(path.to_path_buf().into());
 
-    // Setting the instigating source to the project file path is a little
+            // Mark this snapshot (the root node of the project file) as being
-    // coarse.
+            // related to the project file.
-    //
+            //
-    // Ideally, we'd only snapshot the project file if the project file
+            // We SHOULD NOT mark the project file as a relevant path for any
-    // actually changed. Because Rojo only has the concept of one
+            // nodes that aren't roots. They'll be updated as part of the project
-    // relevant path -> snapshot path mapping per instance, we pick the more
+            // file being updated.
-    // conservative approach of snapshotting the project file if any
+            snapshot.metadata.relevant_paths.push(path.to_path_buf());
-    // relevant paths changed.
-    snapshot.metadata.instigating_source = Some(path.to_path_buf().into());
 
-    // Mark this snapshot (the root node of the project file) as being
+            Ok(Some(snapshot))
-    // related to the project file.
+        }
-    //
+        None => Ok(None),
-    // We SHOULD NOT mark the project file as a relevant path for any
+    }
-    // nodes that aren't roots. They'll be updated as part of the project
-    // file being updated.
-    snapshot.metadata.relevant_paths.push(path.to_path_buf());
-
-    Ok(Some(snapshot))
 }
 
 pub fn snapshot_project_node(
@@ -77,7 +78,9 @@ pub fn snapshot_project_node(
     let mut children = Vec::new();
     let mut metadata = InstanceMetadata::default();
 
-    if let Some(path) = &node.path {
+    if let Some(path_node) = &node.path {
+        let path = path_node.path();
+
         // If the path specified in the project is relative, we assume it's
         // relative to the folder that the project is in, project_folder.
         let full_path = if path.is_relative() {
@@ -106,16 +109,6 @@ pub fn snapshot_project_node(
             // Take the snapshot's metadata as-is, which will be mutated later
             // on.
             metadata = snapshot.metadata;
-        } else {
-            anyhow::bail!(
-                "Rojo project referred to a file using $path that could not be turned into a Roblox Instance by Rojo.\n\
-                Check that the file exists and is a file type known by Rojo.\n\
-                \n\
-                Project path: {}\n\
-                File $path: {}",
-                project_path.display(),
-                path.display(),
-            );
         }
     }
 
@@ -125,20 +118,21 @@ pub fn snapshot_project_node(
         class_name_from_project,
         class_name_from_path,
         class_name_from_inference,
+        &node.path,
     ) {
         // These are the easy, happy paths!
-        (Some(project), None, None) => project,
+        (Some(project), None, None, _) => project,
-        (None, Some(path), None) => path,
+        (None, Some(path), None, _) => path,
-        (None, None, Some(inference)) => inference,
+        (None, None, Some(inference), _) => inference,
 
         // If the user specifies a class name, but there's an inferred class
         // name, we prefer the name listed explicitly by the user.
-        (Some(project), None, Some(_)) => project,
+        (Some(project), None, Some(_), _) => project,
 
         // If the user has a $path pointing to a folder and we're able to infer
         // a class name, let's use the inferred name. If the path we're pointing
         // to isn't a folder, though, that's a user error.
-        (None, Some(path), Some(inference)) => {
+        (None, Some(path), Some(inference), _) => {
             if path == "Folder" {
                 inference
             } else {
@@ -146,7 +140,7 @@ pub fn snapshot_project_node(
             }
         }
 
-        (Some(project), Some(path), _) => {
+        (Some(project), Some(path), _, _) => {
             if path == "Folder" {
                 project
             } else {
@@ -160,12 +154,28 @@ pub fn snapshot_project_node(
                     project,
                     path,
                     project_path.display(),
-                    node.path.as_ref().unwrap().display()
+                    node.path.as_ref().unwrap().path().display()
                 );
             }
         }
 
-        (None, None, None) => {
+        (None, None, None, Some(PathNode::Optional(_))) => {
+            return Ok(None);
+        }
+
+        (_, None, _, Some(PathNode::Required(path))) => {
+            anyhow::bail!(
+                "Rojo project referred to a file using $path that could not be turned into a Roblox Instance by Rojo.\n\
+                Check that the file exists and is a file type known by Rojo.\n\
+                \n\
+                Project path: {}\n\
+                File $path: {}",
+                project_path.display(),
+                path.display(),
+            );
+        }
+
+        (None, None, None, None) => {
             bail!(
                 "Instance \"{}\" is missing some required information.\n\
                  One of the following must be true:\n\

tests/tests/build.rs

@@ -53,6 +53,7 @@ gen_build_tests! {
     txt,
     txt_in_folder,
     unresolved_values,
+    optional,
     weldconstraint,
 }
 

tests/tests/serve.rs

@@ -220,3 +220,34 @@ fn empty_json_model() {
         );
     });
 }
+
+#[test]
+#[ignore = "Rojo does not watch missing, optional files for changes."]
+fn add_optional_folder() {
+    run_serve_test("add_optional_folder", |session, mut redactions| {
+        let info = session.get_api_rojo().unwrap();
+        let root_id = info.root_instance_id;
+
+        assert_yaml_snapshot!("add_optional_folder", redactions.redacted_yaml(info));
+
+        let read_response = session.get_api_read(root_id).unwrap();
+        assert_yaml_snapshot!(
+            "add_optional_folder_all",
+            read_response.intern_and_redact(&mut redactions, root_id)
+        );
+
+        fs::create_dir(session.path().join("create-later")).unwrap();
+
+        let subscribe_response = session.get_api_subscribe(0).unwrap();
+        assert_yaml_snapshot!(
+            "add_optional_folder_subscribe",
+            subscribe_response.intern_and_redact(&mut redactions, ())
+        );
+
+        let read_response = session.get_api_read(root_id).unwrap();
+        assert_yaml_snapshot!(
+            "add_optional_folder_all-2",
+            read_response.intern_and_redact(&mut redactions, root_id)
+        );
+    });
+}