Merge branch 'syncback-tests' into syncback-merge

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

@@ -10,7 +10,7 @@ expression: contents
     <Item class="LocalizationTable" referent="1">
       <Properties>
         <string name="Name">normal</string>
-        <string name="Contents">[{"key":"Count","examples":"A number demonstrating issue 145","source":"3","values":{"es":"7"}}]</string>
+        <string name="Contents">[{"key":"Count","example":"A number demonstrating issue 145","source":"3","values":{"es":"7"}}]</string>
       </Properties>
     </Item>
   </Item>

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

@@ -10,7 +10,7 @@ expression: contents
     <Item class="LocalizationTable" referent="1">
       <Properties>
         <string name="Name">normal</string>
-        <string name="Contents">[{"key":"Ack","examples":"An exclamation of despair","source":"Ack!","values":{"es":"¡Ay!"}}]</string>
+        <string name="Contents">[{"key":"Ack","example":"An exclamation of despair","source":"Ack!","values":{"es":"¡Ay!"}}]</string>
       </Properties>
     </Item>
   </Item>

rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__all_middleware.snap

@@ -1,15 +1,14 @@
 ---
 source: tests/rojo_test/syncback_util.rs
-assertion_line: 48
 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)"
 ---
 added_files:
   - "src\\csv.csv"
   - "src\\csv_init\\init.csv"
   - "src\\dir\\client_script.client.luau"
-  - "src\\dir\\init_client_script\\init.client.lua"
-  - "src\\dir\\init_module_script\\init.lua"
-  - "src\\dir\\init_server_script\\init.server.lua"
+  - "src\\dir\\init_client_script\\init.client.luau"
+  - "src\\dir\\init_module_script\\init.luau"
+  - "src\\dir\\init_server_script\\init.server.luau"
   - "src\\dir\\module_script.luau"
   - "src\\dir\\server_script.server.luau"
   - "src\\model_json.model.json"
@@ -26,4 +25,3 @@ added_dirs:
   - "src\\dir\\init_server_script"
 removed_files: []
 removed_dirs: []
-

rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_all_middleware.snap

@@ -1,15 +1,14 @@
 ---
 source: tests/rojo_test/syncback_util.rs
-assertion_line: 48
 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)"
 ---
 added_files:
   - src/client_script.client.luau
   - src/csv.csv
   - "src/csv_init\\init.csv"
-  - "src/init_client_script\\init.client.lua"
-  - "src/init_module_script\\init.lua"
-  - "src/init_server_script\\init.server.lua"
+  - "src/init_client_script\\init.client.luau"
+  - "src/init_module_script\\init.luau"
+  - "src/init_server_script\\init.server.luau"
   - src/model_json.model.json
   - src/module_script.luau
   - src/project_json.project.json
@@ -23,4 +22,3 @@ added_dirs:
   - src/init_server_script
 removed_files: []
 removed_dirs: []
-

rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_init.snap

@@ -1,12 +1,10 @@
 ---
 source: tests/rojo_test/syncback_util.rs
-assertion_line: 48
 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)"
 ---
 added_files:
-  - "src\\init.lua"
+  - "src\\init.luau"
 added_dirs:
   - src
 removed_files: []
 removed_dirs: []
-

rojo-test/syncback-tests/all_middleware/expected/src/rbxmx.rbxmx

@@ -8,6 +8,7 @@
       <int64 name="SourceAssetId">-1</int64>
       <BinaryString name="Tags"></BinaryString>
       <string name="Text">fireplace addicted army cow stock</string>
+      <UniqueId name="UniqueId">56ddeac77c86b55f061dbf5000007cca</UniqueId>
     </Properties>
   </Item>
 </roblox>

rojo-test/syncback-tests/project_all_middleware/expected/src/rbxmx.rbxmx

@@ -8,6 +8,7 @@
       <int64 name="SourceAssetId">-1</int64>
       <BinaryString name="Tags"></BinaryString>
       <string name="Text">ripe alike review heart dry</string>
+      <UniqueId name="UniqueId">288c54239308069206387a3a00004126</UniqueId>
     </Properties>
   </Item>
 </roblox>

src/cli/syncback.rs

@@ -79,6 +79,19 @@ impl SyncbackCommand {
         log::debug!("Old root: {}", dom_old.inner().root().class);
         log::debug!("New root: {}", dom_new.root().class);
 
+        if log::log_enabled!(log::Level::Trace) {
+            log::trace!("Children of old root:");
+            for child in dom_old.inner().root().children() {
+                let inst = dom_old.get_instance(*child).unwrap();
+                log::trace!("{} (class: {})", inst.name(), inst.class_name());
+            }
+            log::trace!("Children of new root:");
+            for child in dom_new.root().children() {
+                let inst = dom_new.get_by_ref(*child).unwrap();
+                log::trace!("{} (class: {})", inst.name, inst.class);
+            }
+        }
+
         let start = Instant::now();
         log::info!("Beginning syncback...");
         let snapshot = syncback_loop(
@@ -137,49 +150,66 @@ impl SyncbackCommand {
 
 fn read_dom(path: &Path, file_kind: FileKind) -> anyhow::Result<WeakDom> {
     let content = File::open(path)?;
-    Ok(match file_kind {
+    match file_kind {
         FileKind::Rbxl => {
             // HACK: A custom reflection database is used to deserialize UniqueId
             let db = binary_db().expect("A custom ReflectionDatabase to deserialize UniqueId");
             let binary_decoder = rbx_binary::Deserializer::new().reflection_database(&db);
-            binary_decoder.deserialize(content)?
+            binary_decoder.deserialize(content).with_context(|| {
+                format!(
+                    "Could not deserialize binary place file at {}",
+                    path.display()
+                )
+            })
         }
-        FileKind::Rbxlx => rbx_xml::from_reader(content, xml_decode_config())?,
+        FileKind::Rbxlx => rbx_xml::from_reader(content, xml_decode_config())
+            .with_context(|| format!("Could not deserialize XML place file at {}", path.display())),
         FileKind::Rbxm => {
             // HACK: A custom reflection database is used to deserialize UniqueId
             let db = binary_db().expect("A custom ReflectionDatabase to deserialize UniqueId");
             let binary_decoder = rbx_binary::Deserializer::new().reflection_database(&db);
-            let temp_tree = binary_decoder.deserialize(content)?;
-
-            let root_children = temp_tree.root().children();
-            if root_children.len() != 1 {
-                anyhow::bail!(
-                    "Rojo does not currently support models with more \
-                than one Instance at the Root!"
-                );
-            }
-            let real_root = temp_tree.get_by_ref(root_children[0]).unwrap();
-            let mut new_tree = WeakDom::new(InstanceBuilder::new(&real_root.class));
-            temp_tree.clone_multiple_into_external(real_root.children(), &mut new_tree);
-
-            new_tree
+            let temp_tree = binary_decoder.deserialize(content).with_context(|| {
+                format!(
+                    "Could not deserialize binary model file at {}",
+                    path.display()
+                )
+            })?;
+
+            process_model_dom(temp_tree)
         }
         FileKind::Rbxmx => {
-            let temp_tree = rbx_xml::from_reader(content, xml_decode_config())?;
-            let root_children = temp_tree.root().children();
-            if root_children.len() != 1 {
-                anyhow::bail!(
-                    "Rojo does not currently support models with more \
-                than one Instance at the Root!"
-                );
-            }
-            let real_root = temp_tree.get_by_ref(root_children[0]).unwrap();
-            let mut new_tree = WeakDom::new(InstanceBuilder::new(&real_root.class));
-            temp_tree.clone_multiple_into_external(real_root.children(), &mut new_tree);
+            let temp_tree =
+                rbx_xml::from_reader(content, xml_decode_config()).with_context(|| {
+                    format!("Could not deserialize XML model file at {}", path.display())
+                })?;
+            process_model_dom(temp_tree)
+        }
+    }
+}
 
+fn process_model_dom(dom: WeakDom) -> anyhow::Result<WeakDom> {
+    let temp_children = dom.root().children();
+    if temp_children.len() == 1 {
+        let real_root = dom.get_by_ref(temp_children[0]).unwrap();
+        let mut new_tree = WeakDom::new(InstanceBuilder::new(&real_root.class));
+        for (name, property) in &real_root.properties {
             new_tree
+                .root_mut()
+                .properties
+                .insert(name.to_owned(), property.to_owned());
         }
-    })
+
+        let children = dom.clone_multiple_into_external(real_root.children(), &mut new_tree);
+        for child in children {
+            new_tree.transfer_within(child, new_tree.root_ref());
+        }
+        Ok(new_tree)
+    } else {
+        anyhow::bail!(
+            "Rojo does not currently support models with more \
+        than one Instance at the Root!"
+        );
+    }
 }
 
 fn xml_decode_config() -> rbx_xml::DecodeOptions<'static> {

src/snapshot/instance_snapshot.rs

@@ -127,17 +127,6 @@ impl InstanceSnapshot {
             children,
         }
     }
-
-    pub fn from_instance(instance: &Instance) -> Self {
-        Self {
-            snapshot_id: instance.referent(),
-            metadata: InstanceMetadata::new(),
-            name: Cow::Owned(instance.name.clone()),
-            class_name: Cow::Owned(instance.class.clone()),
-            properties: instance.properties.clone(),
-            children: Vec::new(),
-        }
-    }
 }
 
 impl Default for InstanceSnapshot {

src/snapshot_middleware/csv.rs

@@ -125,7 +125,6 @@ pub fn syncback_csv<'sync>(
     }
 
     Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(new_inst),
         fs_snapshot,
         children: Vec::new(),
         removed_children: Vec::new(),
@@ -161,10 +160,7 @@ pub fn syncback_csv_init<'sync>(
         }
     }
 
-    Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(new_inst),
-        ..dir_syncback
-    })
+    Ok(dir_syncback)
 }
 
 /// Struct that holds any valid row from a Roblox CSV translation table.
@@ -180,8 +176,13 @@ struct LocalizationEntry<'a> {
     #[serde(skip_serializing_if = "Option::is_none")]
     context: Option<Cow<'a, str>>,
 
-    #[serde(skip_serializing_if = "Option::is_none")]
-    examples: Option<Cow<'a, str>>,
+    // Roblox writes `examples` for LocalizationTable's Content property, which
+    // causes it to not roundtrip correctly.
+    // This is reported here: https://devforum.roblox.com/t/2908720.
+    //
+    // To support their mistake, we support an alias named `examples`.
+    #[serde(skip_serializing_if = "Option::is_none", alias = "examples")]
+    example: Option<Cow<'a, str>>,
 
     #[serde(skip_serializing_if = "Option::is_none")]
     source: Option<Cow<'a, str>>,
@@ -224,7 +225,7 @@ fn convert_localization_csv(contents: &[u8]) -> Result<String, csv::Error> {
                 "Key" => entry.key = Some(Cow::Borrowed(value)),
                 "Source" => entry.source = Some(Cow::Borrowed(value)),
                 "Context" => entry.context = Some(Cow::Borrowed(value)),
-                "Example" => entry.examples = Some(Cow::Borrowed(value)),
+                "Example" => entry.example = Some(Cow::Borrowed(value)),
                 _ => {
                     entry
                         .values
@@ -278,7 +279,7 @@ fn localization_to_csv(csv_contents: &str) -> anyhow::Result<Vec<u8>> {
         record.push(entry.key.as_deref().unwrap_or_default());
         record.push(entry.source.as_deref().unwrap_or_default());
         record.push(entry.context.as_deref().unwrap_or_default());
-        record.push(entry.examples.as_deref().unwrap_or_default());
+        record.push(entry.example.as_deref().unwrap_or_default());
 
         let values = &entry.values;
         for header in &extra_headers {

src/snapshot_middleware/dir.rs

@@ -202,7 +202,6 @@ pub fn syncback_dir_no_meta<'sync>(
     fs_snapshot.add_dir(&snapshot.path);
 
     Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(new_inst),
         fs_snapshot,
         children,
         removed_children,

src/snapshot_middleware/json_model.rs

@@ -74,7 +74,6 @@ pub fn syncback_json_model<'sync>(
     model.name = None;
 
     Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(snapshot.new_inst()),
         fs_snapshot: FsSnapshot::new().with_added_file(
             &snapshot.path,
             serde_json::to_vec_pretty(&model).context("failed to serialize new JSON Model")?,

src/snapshot_middleware/lua.rs

@@ -149,7 +149,6 @@ pub fn syncback_lua<'sync>(
     }
 
     Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(new_inst),
         fs_snapshot,
         // Scripts don't have a child!
         children: Vec::new(),
@@ -190,10 +189,7 @@ pub fn syncback_lua_init<'sync>(
         }
     }
 
-    Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(new_inst),
-        ..dir_syncback
-    })
+    Ok(dir_syncback)
 }
 
 #[cfg(test)]

src/snapshot_middleware/mod.rs

@@ -287,7 +287,7 @@ impl Middleware {
     /// set properties without needing a meta.json file.
     ///
     /// It does not cover middleware like `ServerScript` or `Csv` because they
-    /// need a meta.json file set properties that aren't their designated
+    /// need a meta.json file to set properties that aren't their designated
     /// 'special' properties.
     #[inline]
     pub fn handles_own_properties(&self) -> bool {

src/snapshot_middleware/project.rs

@@ -456,18 +456,19 @@ pub fn syncback_project<'sync>(
             }
             let new_equivalent = new_child_map.remove(child_name);
             let old_equivalent = old_child_map.remove(child_name.as_str());
-            // The panic below should never happen. If it does, something's gone
-            // wrong with the Instance matching for nodes.
             match (new_equivalent, old_equivalent) {
                 (Some(new), Some(old)) => node_queue.push_back((child_node, old, new)),
                 (_, None) => anyhow::bail!(
                     "The child '{child_name}' of Instance '{}' would be removed.\n\
-                    Syncback cannot add or remove Instances from project {}", old_inst.name(), project_path.display()),
-                (None, _) => panic!(
-                    "Invariant violated: the Instance matching of project nodes is flawed somehow.\n\
-                    Specifically, a child named {} of the node {} did not exist in the old tree.",
-                    child_name, old_inst.name()
+                    Syncback cannot add or remove Instances from project {}",
+                    old_inst.name(),
+                    project_path.display()
                 ),
+                (None, _) => anyhow::bail!(
+                    "The child '{child_name}' of Instance '{}' is present only in a project file,\n\
+                    and not the provided file. Syncback cannot add or remove Instances from project:\n{}.",
+                    old_inst.name(), project_path.display(),
+                )
             }
         }
 
@@ -515,7 +516,6 @@ pub fn syncback_project<'sync>(
     }
 
     Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(snapshot.new_inst()),
         fs_snapshot,
         children: descendant_snapshots,
         removed_children: removed_descendants,

src/snapshot_middleware/rbxm.rs

@@ -54,7 +54,6 @@ pub fn syncback_rbxm<'sync>(
         .context("failed to serialize new rbxm")?;
 
     Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(inst),
         fs_snapshot: FsSnapshot::new().with_added_file(&snapshot.path, serialized),
         children: Vec::new(),
         removed_children: Vec::new(),

src/snapshot_middleware/rbxmx.rs

@@ -65,7 +65,6 @@ pub fn syncback_rbxmx<'sync>(
     .context("failed to serialize new rbxmx")?;
 
     Ok(SyncbackReturn {
-        inst_snapshot: InstanceSnapshot::from_instance(inst),
         fs_snapshot: FsSnapshot::new().with_added_file(&snapshot.path, serialized),
         children: Vec::new(),
         removed_children: Vec::new(),

src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap

@@ -18,6 +18,6 @@ name: foo
 class_name: LocalizationTable
 properties:
   Contents:
-    String: "[{\"key\":\"Ack\",\"examples\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]"
+    String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]"
 children: []
 

src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap

@@ -18,6 +18,6 @@ name: foo
 class_name: LocalizationTable
 properties:
   Contents:
-    String: "[{\"key\":\"Ack\",\"examples\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]"
+    String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]"
 children: []