Actually include attribute-defined properties in patch computation (#944)

2026-04-20 20:55:50 +00:00 · 2024-08-19 15:41:02 -07:00
parent 30f439caec
commit 5e1cab2e75
5 changed files with 361 additions and 1 deletions
--- a/rojo-test/serve-test-snapshots/end_to_endtestsserve__ref_properties_patch_update_all-2.snap
+++ b/rojo-test/serve-test-snapshots/end_to_endtestsserve__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: 0
+sessionId: id-1
--- a/rojo-test/serve-test-snapshots/end_to_endtestsserve__ref_properties_patch_update_all.snap
+++ b/rojo-test/serve-test-snapshots/end_to_endtestsserve__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
--- a/rojo-test/serve-test-snapshots/end_to_endtestsserve__ref_properties_patch_update_info.snap
+++ b/rojo-test/serve-test-snapshots/end_to_endtestsserve__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
--- 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<String, Option<Variant>> {
+    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::*;
--- a/tests/tests/serve.rs
+++ b/tests/tests/serve.rs
@@ -477,3 +477,42 @@ 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.
+    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)
+        );
+    });
+}