Optimize model serialization by using SharedObjects for reusable data #1090

katehryhorenko · 2025-01-16T21:37:53Z

🌌 BACKGROUND:
Suggestion functions generate numerous components for each space, many of which contain repeated information. This redundant data can be stored as shared objects to improve efficiency and reduce duplication.

📋 DESCRIPTION:
This PR:

Introduces SharedObjects serialization to the model, stored as a dictionary structure similar to Elements.
Adds support for skipping SharedObjects marked with the JsonIgnore attribute during serialization.
Optimizes elements referencing shared objects by:
- Removing redundant properties from elements that are already present in the referenced shared object during serialization.

🧪 TESTING:

Tested with classroom and cafe functions to ensure proper functionality.
Verified that RepresentationInstances are not saved to JSON.
Confirmed that shared objects are correctly serialized and referenced, with no loss of critical information.

This change is

andrewheumann

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @katehryhorenko)

Elements/src/Model.cs line 40 at r1 (raw file):

        /// <summary>A collection of SharedObjects keyed by their identifiers.</summary>
        [JsonProperty("SharedObjects", Required = Required.Default)]
        [System.ComponentModel.DataAnnotations.Required]

just out of curiosity, what does this annotation do for us?

[System.ComponentModel.DataAnnotations.Required]

It seems like the json annotation says Required.Default which I think means "not required" — does that contradict what DataAnnotations.Required asserts?

Elements/src/Model.cs line 478 at r1 (raw file):

        }

        private (List<Element> elements, List<SharedObject> sharedObjects) RecursiveGatherSubElementsInternal(object obj, Dictionary<Type, List<PropertyInfo>> properties, out List<Element> sharedObjectSubElements)

Instead of mixing tuple return and out parameters, can we choose one or the other and use that only? I have a slight preference for the tuple return.

In general this method (which was already pretty confusing) seems to have gotten even harder to follow. Is there a way we can clean this up a bit (at least with comments) to make it clear what this method is doing and what different cases it addresses?

Elements/src/Model.cs line 508 at r1 (raw file):

            }

            var sharedObject = obj as SharedObject;

can we annotate what's happening here, and why it's a different case from

            if (sharedObject != null)
            {
                sharedObjects.Add(sharedObject);
            }

below? (IOW — why don't we need to add the shared object here)

Elements/src/Model.cs line 542 at r1 (raw file):

            }

            var elementsFromProperties = new List<Element>();

can we annotate what elementsFromProperties and sharedObjectsFromProperties are / represent?

Elements/src/Model.cs line 597 at r1 (raw file):

                        sharedObjectsFromProperties.AddRange(gatheredSubSharedObjects);
                    }
                    sharedObjectSubElements.AddRange(elementsFromPropertyToIgnore);

we repeat this chunk of code a lot. Can we consolidate, maybe to an internal method? something like

void processSubElements((List<Element> elements, List<SharedObject> sharedObjects, List<Element> sharedObjectSubElements) gatherResult, bool hasJsonIgnore, List<Element> outSubElements)
{
    var (subElements, subSharedObjects, elementsFromSharedObjectProperties) = gatherResult;
    elementsFromProperties.AddRange(subElements);
    // do not save shared objects marked with JsonIgnore
    if (!hasJsonIgnore)
    {
        sharedObjectsFromProperties.AddRange(subSharedObjects);
    }
    outSubElements.AddRange(elementsFromSharedObjectProperties);
}

(not sure what such a method should be called. I am not clear on what these different collections represent)

Elements/src/Serialization/JSON/JsonInheritanceConverter.cs line 288 at r1 (raw file):

        }

        private static bool WritingTopLevelSharedObject(string path)

This method is nearly identical to WritingTopLevelElement — can we rename it to PathIsTopLevel and pass in "Elements" / "SharedObjects"?

katehryhorenko

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)

Elements/src/Model.cs line 40 at r1 (raw file):