From 19945617a57c675c401253e8ff2065d8aaca4c22 Mon Sep 17 00:00:00 2001 From: Micah Date: Fri, 19 Jul 2024 15:58:33 -0700 Subject: [PATCH 1/3] Add test for issue 929 --- ...ve__ref_properties_patch_update_all-2.snap | 120 ++++++++++++++++++ ...erve__ref_properties_patch_update_all.snap | 120 ++++++++++++++++++ ...rve__ref_properties_patch_update_info.snap | 12 ++ tests/tests/serve.rs | 33 +++++ 4 files changed, 285 insertions(+) create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap create mode 100644 rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap new file mode 100644 index 000000000..ad68c0b19 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap @@ -0,0 +1,120 @@ +--- +source: tests/tests/serve.rs +expression: "read_response.intern_and_redact(&mut redactions, root_id)" +--- +instances: + id-10: + Children: [] + ClassName: ObjectValue + Id: id-10 + Metadata: + ignoreUnknownInstances: true + Name: ProjectPointer + Parent: id-9 + Properties: + Attributes: + Attributes: + Rojo_Target_Value: + String: project target + Value: + Ref: id-9 + id-2: + Children: + - id-3 + ClassName: DataModel + Id: id-2 + Metadata: + ignoreUnknownInstances: true + Name: ref_properties + Parent: "00000000000000000000000000000000" + Properties: {} + id-3: + Children: + - id-4 + - id-5 + - id-7 + - id-9 + ClassName: Workspace + Id: id-3 + Metadata: + ignoreUnknownInstances: true + Name: Workspace + Parent: id-2 + Properties: {} + id-4: + Children: [] + ClassName: ObjectValue + Id: id-4 + Metadata: + ignoreUnknownInstances: true + Name: CrossFormatPointer + Parent: id-3 + Properties: + Attributes: + Attributes: + Rojo_Target_Value: + String: folder target + Value: + Ref: id-5 + id-5: + Children: + - id-6 + ClassName: Folder + Id: id-5 + Metadata: + ignoreUnknownInstances: false + Name: FolderTarget + Parent: id-3 + Properties: {} + id-6: + Children: [] + ClassName: ObjectValue + Id: id-6 + Metadata: + ignoreUnknownInstances: false + Name: FolderPointer + Parent: id-5 + Properties: + Attributes: + Attributes: + Rojo_Target_Value: + String: folder target + Value: + Ref: id-5 + id-7: + Children: + - id-8 + ClassName: Folder + Id: id-7 + Metadata: + ignoreUnknownInstances: false + Name: ModelTarget + Parent: id-3 + Properties: {} + id-8: + Children: [] + ClassName: Model + Id: id-8 + Metadata: + ignoreUnknownInstances: false + Name: ModelPointer + Parent: id-7 + Properties: + Attributes: + Attributes: + Rojo_Target_PrimaryPart: + String: model target + PrimaryPart: + Ref: id-7 + id-9: + Children: + - id-10 + ClassName: Folder + Id: id-9 + Metadata: + ignoreUnknownInstances: true + Name: ProjectTarget + Parent: id-3 + Properties: {} +messageCursor: 1 +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap new file mode 100644 index 000000000..f29e43e62 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap @@ -0,0 +1,120 @@ +--- +source: tests/tests/serve.rs +expression: "read_response.intern_and_redact(&mut redactions, root_id)" +--- +instances: + id-10: + Children: [] + ClassName: ObjectValue + Id: id-10 + Metadata: + ignoreUnknownInstances: true + Name: ProjectPointer + Parent: id-9 + Properties: + Attributes: + Attributes: + Rojo_Target_Value: + String: project target + Value: + Ref: id-9 + id-2: + Children: + - id-3 + ClassName: DataModel + Id: id-2 + Metadata: + ignoreUnknownInstances: true + Name: ref_properties + Parent: "00000000000000000000000000000000" + Properties: {} + id-3: + Children: + - id-4 + - id-5 + - id-7 + - id-9 + ClassName: Workspace + Id: id-3 + Metadata: + ignoreUnknownInstances: true + Name: Workspace + Parent: id-2 + Properties: {} + id-4: + Children: [] + ClassName: ObjectValue + Id: id-4 + Metadata: + ignoreUnknownInstances: true + Name: CrossFormatPointer + Parent: id-3 + Properties: + Attributes: + Attributes: + Rojo_Target_Value: + String: folder target + Value: + Ref: id-5 + id-5: + Children: + - id-6 + ClassName: Folder + Id: id-5 + Metadata: + ignoreUnknownInstances: false + Name: FolderTarget + Parent: id-3 + Properties: {} + id-6: + Children: [] + ClassName: ObjectValue + Id: id-6 + Metadata: + ignoreUnknownInstances: false + Name: FolderPointer + Parent: id-5 + Properties: + Attributes: + Attributes: + Rojo_Target_Value: + String: folder target + Value: + Ref: id-5 + id-7: + Children: + - id-8 + ClassName: Folder + Id: id-7 + Metadata: + ignoreUnknownInstances: false + Name: ModelTarget + Parent: id-3 + Properties: {} + id-8: + Children: [] + ClassName: Model + Id: id-8 + Metadata: + ignoreUnknownInstances: false + Name: ModelPointer + Parent: id-7 + Properties: + Attributes: + Attributes: + Rojo_Target_PrimaryPart: + String: model target + PrimaryPart: + Ref: id-7 + id-9: + Children: + - id-10 + ClassName: Folder + Id: id-9 + Metadata: + ignoreUnknownInstances: true + Name: ProjectTarget + Parent: id-3 + Properties: {} +messageCursor: 0 +sessionId: id-1 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap new file mode 100644 index 000000000..ac25f16b3 --- /dev/null +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_info.snap @@ -0,0 +1,12 @@ +--- +source: tests/tests/serve.rs +expression: redactions.redacted_yaml(info) +--- +expectedPlaceIds: ~ +gameId: ~ +placeId: ~ +projectName: ref_properties +protocolVersion: 4 +rootInstanceId: id-2 +serverVersion: "[server-version]" +sessionId: id-1 diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index 880ae29ba..f8a49eb6d 100644 --- a/tests/tests/serve.rs +++ b/tests/tests/serve.rs @@ -477,3 +477,36 @@ fn ref_properties_remove() { ); }); } + +#[test] +fn ref_properties_patch_update() { + // Reusing ref_properties is fun and easy. + run_serve_test("ref_properties", |session, mut redactions| { + let info = session.get_api_rojo().unwrap(); + let root_id = info.root_instance_id; + + assert_yaml_snapshot!( + "ref_properties_patch_update_info", + redactions.redacted_yaml(info) + ); + + let read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "ref_properties_patch_update_all", + read_response.intern_and_redact(&mut redactions, root_id) + ); + + let project_path = session.path().join("default.project.json"); + let mut project_content = fs::read(&project_path).unwrap(); + // Adding a newline to force the change processor to pick up a change. + project_content.push(b'\n'); + + fs::write(project_path, project_content).unwrap(); + + let read_response = session.get_api_read(root_id).unwrap(); + assert_yaml_snapshot!( + "ref_properties_patch_update_all-2", + read_response.intern_and_redact(&mut redactions, root_id) + ); + }); +} From 6dcca657ee5033fcb303f744148f3efd1410390e Mon Sep 17 00:00:00 2001 From: Micah Date: Fri, 19 Jul 2024 15:59:48 -0700 Subject: [PATCH 2/3] Include attribute properties in patch computation --- ...ve__ref_properties_patch_update_all-2.snap | 2 +- src/snapshot/patch_compute.rs | 71 ++++++++++++++++++- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap index ad68c0b19..f29e43e62 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap @@ -116,5 +116,5 @@ instances: Name: ProjectTarget Parent: id-3 Properties: {} -messageCursor: 1 +messageCursor: 0 sessionId: id-1 diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index 5d7abc698..76c3cab62 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -8,6 +8,8 @@ use std::{ use rbx_dom_weak::types::{Ref, Variant}; +use crate::{RojoRef, REF_POINTER_ATTRIBUTE_PREFIX}; + use super::{ patch::{PatchAdd, PatchSet, PatchUpdate}, InstanceSnapshot, InstanceWithMeta, RojoTree, @@ -87,7 +89,7 @@ fn compute_patch_set_internal( .get_instance(id) .expect("Instance did not exist in tree"); - compute_property_patches(&mut snapshot, &instance, patch_set); + compute_property_patches(&mut snapshot, &instance, patch_set, tree); compute_children_patches(context, &mut snapshot, tree, id, patch_set); } @@ -95,10 +97,13 @@ fn compute_property_patches( snapshot: &mut InstanceSnapshot, instance: &InstanceWithMeta, patch_set: &mut PatchSet, + tree: &RojoTree, ) { let mut visited_properties = HashSet::new(); let mut changed_properties = HashMap::new(); + let attribute_ref_properties = compute_ref_properties(snapshot, tree); + let changed_name = if snapshot.name == instance.name() { None } else { @@ -140,6 +145,24 @@ fn compute_property_patches( changed_properties.insert(name.clone(), None); } + for (name, ref_value) in attribute_ref_properties { + match (&ref_value, instance.properties().get(&name)) { + (Some(referent), Some(instance_value)) => { + if referent != instance_value { + changed_properties.insert(name, ref_value); + } else { + changed_properties.remove(&name); + } + } + (Some(_), None) | (None, Some(_)) => { + changed_properties.insert(name, ref_value); + } + (None, None) => { + changed_properties.remove(&name); + } + } + } + if changed_properties.is_empty() && changed_name.is_none() && changed_class_name.is_none() @@ -224,6 +247,52 @@ fn compute_children_patches( } } +fn compute_ref_properties( + snapshot: &InstanceSnapshot, + tree: &RojoTree, +) -> HashMap> { + let mut map = HashMap::new(); + let attributes = match snapshot.properties.get("Attributes") { + Some(Variant::Attributes(attrs)) => attrs, + _ => return map, + }; + + for (attr_name, attr_value) in attributes.iter() { + let prop_name = match attr_name.strip_prefix(REF_POINTER_ATTRIBUTE_PREFIX) { + Some(str) => str, + None => continue, + }; + let rojo_ref = match attr_value { + Variant::String(str) => RojoRef::new(str.clone()), + Variant::BinaryString(bytes) => { + if let Ok(str) = std::str::from_utf8(bytes.as_ref()) { + RojoRef::new(str.to_string()) + } else { + log::warn!( + "IDs specified by referent property attributes must be valid UTF-8 strings" + ); + continue; + } + } + _ => { + log::warn!( + "Attribute {attr_name} is of type {:?} when it was \ + expected to be a String", + attr_value.ty() + ); + continue; + } + }; + if let Some(target_id) = tree.get_specified_id(&rojo_ref) { + map.insert(prop_name.to_string(), Some(Variant::Ref(target_id))); + } else { + map.insert(prop_name.to_string(), None); + } + } + + map +} + #[cfg(test)] mod test { use super::*; From ed7e3c2b5ca8c28db89a68096405a7900956c2ae Mon Sep 17 00:00:00 2001 From: Micah Date: Mon, 19 Aug 2024 15:37:18 -0700 Subject: [PATCH 3/3] Add a doc comment for the ref_properties_patch_update test --- tests/tests/serve.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index f8a49eb6d..67ee7dbe6 100644 --- a/tests/tests/serve.rs +++ b/tests/tests/serve.rs @@ -478,6 +478,12 @@ fn ref_properties_remove() { }); } +/// When Ref properties were first implemented, a mistake was made that resulted +/// in Ref properties defined via attributes not being included in patch +/// computation, which resulted in subsequent patches setting those properties +/// to `nil`. +/// +/// See: https://github.com/rojo-rbx/rojo/issues/929 #[test] fn ref_properties_patch_update() { // Reusing ref_properties is fun and easy.