From dccd3a691a9db7a702f9aa8c9f787d504b9b4f05 Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 30 Jan 2026 13:18:20 -0600 Subject: [PATCH] Enhance property encoding and class descriptor retrieval in rbx_dom_lua - Introduced a new function, `findClassDescriptor`, to retrieve class properties and metadata, including inherited properties, from the database. - Updated `encodeInstance` to include encoding for Tags alongside Attributes, ensuring comprehensive property serialization. - Improved property filtering logic to skip non-serializable properties and those matching default values, aligning with syncback behavior. - Enhanced comments for clarity on property handling and encoding processes. --- plugin/rbx_dom_lua/init.lua | 46 ++++++++ plugin/src/ChangeBatcher/encodeInstance.lua | 124 ++++++++++++-------- src/syncback/mod.rs | 2 +- src/syncback/property_filter.rs | 4 +- src/web/api.rs | 69 +++++++++-- src/web/interface.rs | 5 +- 6 files changed, 188 insertions(+), 62 deletions(-) diff --git a/plugin/rbx_dom_lua/init.lua b/plugin/rbx_dom_lua/init.lua index 2d390a24..e26e3c2a 100644 --- a/plugin/rbx_dom_lua/init.lua +++ b/plugin/rbx_dom_lua/init.lua @@ -2,6 +2,51 @@ local database = require(script.database) local Error = require(script.Error) local PropertyDescriptor = require(script.PropertyDescriptor) +-- Returns a class descriptor with all property names and metadata (including inherited ones) +-- Returns nil if the class doesn't exist in the database +-- The properties table maps property names to metadata (scriptability, serialization status) +-- To get a full PropertyDescriptor for encoding, use findCanonicalPropertyDescriptor +local function findClassDescriptor(className) + local classData = database.Classes[className] + if classData == nil then + return nil + end + + -- Build a table of all properties, walking up the inheritance chain + local allProperties = {} + local currentClassName = className + + repeat + local currentClass = database.Classes[currentClassName] + if currentClass == nil then + break + end + + -- Add properties from this class (don't override if already set by subclass) + for propertyName, propertyData in pairs(currentClass.Properties) do + if allProperties[propertyName] == nil then + -- Only include canonical properties (not aliases) + if propertyData.Kind and propertyData.Kind.Canonical then + allProperties[propertyName] = { + name = propertyName, + scriptability = propertyData.Scriptability, + dataType = propertyData.DataType, + serialization = propertyData.Kind.Canonical.Serialization, + } + end + end + end + + currentClassName = currentClass.Superclass + until currentClassName == nil + + return { + name = classData.Name, + superclass = classData.Superclass, + properties = allProperties, + } +end + local function findCanonicalPropertyDescriptor(className, propertyName) local currentClassName = className @@ -65,6 +110,7 @@ return { readProperty = readProperty, writeProperty = writeProperty, findCanonicalPropertyDescriptor = findCanonicalPropertyDescriptor, + findClassDescriptor = findClassDescriptor, Error = Error, EncodedValue = require(script.EncodedValue), } diff --git a/plugin/src/ChangeBatcher/encodeInstance.lua b/plugin/src/ChangeBatcher/encodeInstance.lua index 6b780213..6644fd12 100644 --- a/plugin/src/ChangeBatcher/encodeInstance.lua +++ b/plugin/src/ChangeBatcher/encodeInstance.lua @@ -29,22 +29,35 @@ local SCRIPT_CLASS_NAMES = { ModuleScript = true, } --- Properties to skip when encoding (internal Roblox properties) +-- Properties to always skip when encoding (internal Roblox properties that never serialize) local SKIP_PROPERTIES = { Parent = true, Name = true, -- We handle this separately Archivable = true, } --- Classes that become directories but may have properties (like Attributes) -local DIRECTORY_CLASSES = { - Folder = true, - Configuration = true, - Tool = true, - ScreenGui = true, - SurfaceGui = true, - BillboardGui = true, - AdGui = true, +-- Properties to skip per class (matches filter_out_property in Rust syncback) +-- These are properties whose values are encoded in the file itself, not metadata +local SKIP_PROPERTIES_BY_CLASS = { + Script = { + Source = true, -- Encoded in the .luau file + ScriptGuid = true, -- Internal + RunContext = true, -- Encoded in file suffix (.server.luau, .client.luau, etc.) + }, + LocalScript = { + Source = true, -- Encoded in the .luau file + ScriptGuid = true, -- Internal + }, + ModuleScript = { + Source = true, -- Encoded in the .luau file + ScriptGuid = true, -- Internal + }, + LocalizationTable = { + Contents = true, -- Encoded in the .csv file + }, + StringValue = { + Value = true, -- Encoded in the .txt file + }, } -- Finds duplicate names among a list of instances @@ -126,6 +139,25 @@ local function encodeAttributes(instance, properties) end end +-- Encode Tags if present on any instance +local function encodeTags(instance, properties) + -- Try to get Tags - this works for all instance types + local success, tags = pcall(function() + return instance:GetTags() + end) + + if success and tags and #tags > 0 then + -- Tags need to be encoded specially + local tagDescriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, "Tags") + if tagDescriptor then + local encodeSuccess, encodeResult = encodeProperty(instance, "Tags", tagDescriptor) + if encodeSuccess and encodeResult ~= nil then + properties.Tags = encodeResult + end + end + end +end + encodeInstance = function(instance, parentId, _skipPathCheck) -- Check if the entire path to this instance is unique (unless we're in a recursive call) -- For top-level instances, we check the entire ancestor chain for duplicates @@ -143,11 +175,12 @@ encodeInstance = function(instance, parentId, _skipPathCheck) local properties = {} - -- Always try to encode Attributes for any instance type + -- Always try to encode Attributes and Tags for any instance type encodeAttributes(instance, properties) + encodeTags(instance, properties) + -- For scripts, encode the Source property first (required for script files) if SCRIPT_CLASS_NAMES[instance.ClassName] then - -- For scripts, encode the Source property (required) local sourceDescriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, "Source") if sourceDescriptor then local encodeSuccess, encodeResult = encodeProperty(instance, "Source", sourceDescriptor) @@ -161,46 +194,42 @@ encodeInstance = function(instance, parentId, _skipPathCheck) Log.warn("Could not find Source property descriptor for {:?}", instance) return nil end + end - -- Also encode other script properties (Disabled, LinkedSource, etc.) - -- These go into the .meta.json5 file - local classDescriptor = RbxDom.findClassDescriptor(instance.ClassName) - if classDescriptor then - for propertyName, propertyDescriptor in classDescriptor.properties do - -- Skip properties we handle separately or should ignore - if SKIP_PROPERTIES[propertyName] then - continue - end - if propertyName == "Source" or propertyName == "Attributes" then - continue - end + -- Encode ALL properties for ANY instance type (matching Rust syncback behavior) + -- The server handles filtering defaults and serialization + local classDescriptor = RbxDom.findClassDescriptor(instance.ClassName) + if classDescriptor then + -- Get class-specific properties to skip + local classSkipProps = SKIP_PROPERTIES_BY_CLASS[instance.ClassName] or {} - -- Only encode serializable properties - if propertyDescriptor.scriptability == "ReadWrite" or propertyDescriptor.scriptability == "Read" then - local encodeSuccess, encodeResult = encodeProperty(instance, propertyName, propertyDescriptor) - if encodeSuccess and encodeResult ~= nil then - properties[propertyName] = encodeResult - end - end + for propertyName, propertyMeta in pairs(classDescriptor.properties) do + -- Skip universally skipped properties + if SKIP_PROPERTIES[propertyName] then + continue end - end - elseif not DIRECTORY_CLASSES[instance.ClassName] then - -- For non-directory, non-script classes, encode all relevant properties - local classDescriptor = RbxDom.findClassDescriptor(instance.ClassName) - if classDescriptor then - for propertyName, propertyDescriptor in classDescriptor.properties do - if SKIP_PROPERTIES[propertyName] then - continue - end - -- Skip Attributes here since we handle it separately above - if propertyName == "Attributes" then - continue - end + -- Skip class-specific properties (Source, RunContext for scripts, etc.) + if classSkipProps[propertyName] then + continue + end - -- Only encode serializable properties - if propertyDescriptor.scriptability == "ReadWrite" or propertyDescriptor.scriptability == "Read" then - local encodeSuccess, encodeResult = encodeProperty(instance, propertyName, propertyDescriptor) + -- Skip Attributes and Tags since we handle them separately above + if propertyName == "Attributes" or propertyName == "Tags" then + continue + end + + -- Only encode properties that: + -- 1. Are readable (ReadWrite or Read scriptability) + -- 2. Actually serialize (not "DoesNotSerialize") + local isReadable = propertyMeta.scriptability == "ReadWrite" or propertyMeta.scriptability == "Read" + local doesSerialize = propertyMeta.serialization ~= "DoesNotSerialize" + + if isReadable and doesSerialize then + -- Get the full PropertyDescriptor for encoding + local descriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, propertyName) + if descriptor then + local encodeSuccess, encodeResult = encodeProperty(instance, propertyName, descriptor) if encodeSuccess and encodeResult ~= nil then properties[propertyName] = encodeResult end @@ -208,7 +237,6 @@ encodeInstance = function(instance, parentId, _skipPathCheck) end end end - -- For directory classes (Folder, Configuration, etc.), we only encode Attributes (done above) -- Recursively encode children, skipping those with duplicate names local children = {} diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 2f5e6c9a..40dfeeed 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -31,7 +31,7 @@ use crate::{ pub use file_names::{extension_for_middleware, name_for_inst, validate_file_name}; pub use fs_snapshot::FsSnapshot; pub use hash::*; -pub use property_filter::{filter_properties, filter_properties_preallocated}; +pub use property_filter::{filter_properties, filter_properties_preallocated, should_property_serialize}; pub use snapshot::{inst_path, SyncbackData, SyncbackSnapshot}; /// The name of an enviroment variable to use to override the behavior of diff --git a/src/syncback/property_filter.rs b/src/syncback/property_filter.rs index 47678870..0a850722 100644 --- a/src/syncback/property_filter.rs +++ b/src/syncback/property_filter.rs @@ -77,7 +77,9 @@ pub fn filter_properties_preallocated<'inst>( } } -fn should_property_serialize(class_name: &str, prop_name: &str) -> bool { +/// Checks if a property should serialize based on the reflection database. +/// Returns false for properties with DoesNotSerialize serialization, true otherwise. +pub fn should_property_serialize(class_name: &str, prop_name: &str) -> bool { let database = rbx_reflection_database::get().unwrap(); let mut current_class_name = class_name; diff --git a/src/web/api.rs b/src/web/api.rs index a108918d..e66a1527 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -20,7 +20,7 @@ use rbx_dom_weak::{ use crate::{ serve_session::ServeSession, - snapshot::{InstanceWithMeta, PatchSet, PatchUpdate}, + snapshot::{InstigatingSource, InstanceWithMeta, PatchSet, PatchUpdate}, web::{ interface::{ ErrorResponse, Instance, InstanceMetadata, MessagesPacket, OpenResponse, ReadResponse, @@ -392,14 +392,19 @@ impl ApiService { ) })?; + // Get the parent Ref (required for top-level added instances) + let parent_ref = added + .parent + .context("Top-level added instance must have a parent")?; + // Find the parent instance to get its filesystem path let parent_instance = tree - .get_instance(added.parent) + .get_instance(parent_ref) .context("Parent instance not found in Rojo tree")?; // Check if the parent path is unique (no duplicate-named siblings at any level) // Uses pre-computed cache for O(d) instead of O(d × s) complexity - if !Self::is_tree_path_unique_with_cache(tree, added.parent, duplicate_siblings_cache) { + if !Self::is_tree_path_unique_with_cache(tree, parent_ref, duplicate_siblings_cache) { anyhow::bail!( "Cannot sync instance '{}' - parent path contains duplicate-named siblings", added.name @@ -445,14 +450,32 @@ impl ApiService { let instance_name = instance.name(); - // Get the filesystem path from the instance's metadata - let instance_path = instance + // Get the instigating source and handle each variant appropriately + let instigating_source = instance .metadata() .instigating_source .as_ref() - .map(|s| s.path()) .context("Instance has no filesystem path (not synced from filesystem)")?; + // Only delete instances that originate from filesystem paths, not from project nodes + let instance_path = match instigating_source { + InstigatingSource::Path(path) => path, + InstigatingSource::ProjectNode { name, .. } => { + // Instances defined in project files cannot be removed via syncback + // because that would require modifying the project file structure + log::warn!( + "Syncback: Cannot remove instance '{}' (id: {:?}) - it is defined in a project file", + name, + removed_id + ); + anyhow::bail!( + "Cannot remove instance '{}' because it is defined in a project file. \ + To remove this instance, edit the project file directly.", + name + ); + } + }; + // Check if path exists - it may have already been deleted as part of a parent removal // (e.g., if both a directory and its children are marked for removal, the directory // deletion will recursively delete all children, so we just skip them) @@ -832,7 +855,7 @@ impl ApiService { // Check if there are any properties besides Source that need to be saved // Use IndexMap for consistent ordering (like dedicated syncback) let (properties, attributes) = - self.filter_properties_for_meta(&added.properties, Some("Source")); + self.filter_properties_for_meta(&added.class_name, &added.properties, Some("Source")); if properties.is_empty() && attributes.is_empty() { return Ok(()); @@ -862,7 +885,7 @@ impl ApiService { ) -> anyhow::Result<()> { use anyhow::Context; - let (properties, attributes) = self.filter_properties_for_meta(&added.properties, None); + let (properties, attributes) = self.filter_properties_for_meta(&added.class_name, &added.properties, None); if properties.is_empty() && attributes.is_empty() { return Ok(()); @@ -891,7 +914,7 @@ impl ApiService { ) -> anyhow::Result<()> { use anyhow::Context; - let (properties, attributes) = self.filter_properties_for_meta(&added.properties, None); + let (properties, attributes) = self.filter_properties_for_meta(&added.class_name, &added.properties, None); // For non-Folder classes, we need to include className let meta = self.build_meta_object(Some(&added.class_name), properties, attributes); @@ -915,20 +938,30 @@ impl ApiService { /// Filters out: /// - Ref and UniqueId properties (don't serialize to JSON) /// - The skip_property if specified (e.g., "Source" for scripts) + /// - Properties that don't serialize (DoesNotSerialize in reflection database) + /// - Properties that match their default value /// - Internal RBX* prefixed attributes fn filter_properties_for_meta( &self, + class_name: &str, props: &HashMap, skip_property: Option<&str>, ) -> ( indexmap::IndexMap, indexmap::IndexMap, ) { + use crate::syncback::should_property_serialize; + use crate::variant_eq::variant_eq; use indexmap::IndexMap; let mut properties: IndexMap = IndexMap::new(); let mut attributes: IndexMap = IndexMap::new(); + // Get reflection database for default value comparison + let class_data = rbx_reflection_database::get() + .ok() + .and_then(|db| db.classes.get(class_name)); + for (name, value) in props { // Skip the specified property (e.g., Source for scripts) if let Some(skip) = skip_property { @@ -962,6 +995,20 @@ impl ApiService { continue; } + // Check if property should serialize (matches dedicated syncback property_filter.rs) + if !should_property_serialize(class_name, name) { + continue; + } + + // Skip properties that match their default value (matches dedicated syncback) + if let Some(data) = class_data { + if let Some(default) = data.default_properties.get(name.as_str()) { + if variant_eq(value, default) { + continue; + } + } + } + // Convert other properties if let Some(json_value) = variant_to_json(value) { properties.insert(name.clone(), json_value); @@ -1067,7 +1114,7 @@ impl ApiService { // Filter properties, skipping Source (it's in the .luau file) let (properties, attributes) = - self.filter_properties_for_meta(&added.properties, Some("Source")); + self.filter_properties_for_meta(&added.class_name, &added.properties, Some("Source")); if properties.is_empty() && attributes.is_empty() { return Ok(()); @@ -1098,7 +1145,7 @@ impl ApiService { use serde_json::json; // Filter properties matching dedicated syncback behavior - let (properties, attributes) = self.filter_properties_for_meta(&added.properties, None); + let (properties, attributes) = self.filter_properties_for_meta(&added.class_name, &added.properties, None); // Build the JSON model structure // Format: https://rojo.space/docs/v7/sync-details/#json-models diff --git a/src/web/interface.rs b/src/web/interface.rs index cacb32b3..9166114c 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -253,7 +253,10 @@ pub struct ReadResponse<'a> { #[serde(rename_all = "camelCase")] pub struct AddedInstance { /// The parent instance's Ref (must exist in Rojo's tree) - pub parent: Ref, + /// For top-level added instances, this is required. + /// For nested children, this can be omitted (parent is implied by nesting). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub parent: Option, /// The instance name pub name: String, /// The class name (e.g., "ModuleScript", "Script", "LocalScript")