diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__optional.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__optional.snap new file mode 100644 index 00000000..63f0494f --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__optional.snap @@ -0,0 +1,24 @@ +--- +source: tests/tests/build.rs +expression: contents + +--- + + + + optional + + + + foo-optional + Hello, from foo.txt! + + + + + foo-required + Hello, from foo.txt! + + + + diff --git a/rojo-test/build-tests/optional/default.project.json b/rojo-test/build-tests/optional/default.project.json new file mode 100644 index 00000000..9e9091f4 --- /dev/null +++ b/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" } + } + } +} \ No newline at end of file diff --git a/rojo-test/build-tests/optional/foo.txt b/rojo-test/build-tests/optional/foo.txt new file mode 100644 index 00000000..ca77661c --- /dev/null +++ b/rojo-test/build-tests/optional/foo.txt @@ -0,0 +1 @@ +Hello, from foo.txt! \ No newline at end of file diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder.snap new file mode 100644 index 00000000..42a31374 --- /dev/null +++ b/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 + diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_all-2.snap new file mode 100644 index 00000000..22432adf --- /dev/null +++ b/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 + diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_all.snap new file mode 100644 index 00000000..af3714ec --- /dev/null +++ b/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 + diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_subscribe.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__add_optional_folder_subscribe.snap new file mode 100644 index 00000000..492302c8 --- /dev/null +++ b/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 + diff --git a/rojo-test/serve-tests/add_optional_folder/default.project.json b/rojo-test/serve-tests/add_optional_folder/default.project.json new file mode 100644 index 00000000..d582e8e5 --- /dev/null +++ b/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" } + } + } +} \ No newline at end of file diff --git a/src/change_processor.rs b/src/change_processor.rs index b7ad8b20..7082a687 100644 --- a/src/change_processor.rs +++ b/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(None) => { - log::error!( - "Snapshot did not return an instance from path {}", - path.display() - ); - log::error!("This may be a bug!"); - return None; - } + Ok(snapshot) => snapshot, 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(None) => { - log::error!("Snapshot did not return an instance from a project node."); - log::error!("This is a bug!"); - return None; - } + Ok(snapshot) => snapshot, 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) } }; diff --git a/src/project.rs b/src/project.rs index d9edc820..53d52808 100644 --- a/src/project.rs +++ b/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( - rename = "$path", - serialize_with = "crate::path_serializer::serialize_option_absolute", - skip_serializing_if = "Option::is_none" - )] - pub path: Option, + #[serde(rename = "$path", skip_serializing_if = "Option::is_none")] + pub path: Option, } 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"}"#); + } +} diff --git a/src/serve_session.rs b/src/serve_session.rs index 2643cffd..2cf1dc1c 100644 --- a/src/serve_session.rs +++ b/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)? - .expect("snapshot did not return an instance"); + let snapshot = snapshot_from_vfs(&instance_context, &vfs, &start_path)?; 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); diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index 39974caf..76f0b26c 100644 --- a/src/snapshot/patch_compute.rs +++ b/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 - // 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); + compute_patch_set_internal(&mut context, snapshot, tree, id, &mut patch_set); + + // Rewrite Ref properties to refer to instance IDs instead of snapshot IDs + // 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 { diff --git a/src/snapshot/tests/compute.rs b/src/snapshot/tests/compute.rs index c62978c9..7b15d378 100644 --- a/src/snapshot/tests/compute.rs +++ b/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); diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 6c855eb8..5650c99a 100644 --- a/src/snapshot_middleware/project.rs +++ b/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 - // understand, this may panic! - let mut snapshot = - snapshot_project_node(&context, path, &project.name, &project.tree, vfs, None)?.unwrap(); + match snapshot_project_node(&context, path, &project.name, &project.tree, vfs, None)? { + Some(found_snapshot) => { + let mut snapshot = found_snapshot; + // 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 - // 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()); + // Mark this snapshot (the root node of the project file) as being + // related to the project file. + // + // 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()); - // Mark this snapshot (the root node of the project file) as being - // related to the project file. - // - // 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)) + Ok(Some(snapshot)) + } + None => Ok(None), + } } 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, - (None, Some(path), None) => path, - (None, None, Some(inference)) => inference, + (Some(project), None, None, _) => project, + (None, Some(path), None, _) => path, + (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\ diff --git a/tests/tests/build.rs b/tests/tests/build.rs index 748121fc..fc3b55fe 100644 --- a/tests/tests/build.rs +++ b/tests/tests/build.rs @@ -53,6 +53,7 @@ gen_build_tests! { txt, txt_in_folder, unresolved_values, + optional, weldconstraint, } diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index 4eb1ebe5..9cca9fac 100644 --- a/tests/tests/serve.rs +++ b/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) + ); + }); +}