Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RJS-2680: Implement support for Mixed data type with nested collections #6513

Merged
merged 82 commits into from
Apr 2, 2024

Conversation

elle-j
Copy link
Contributor

@elle-j elle-j commented Feb 28, 2024

What, How & Why?

Adds support for storing nested lists and dictionaries as the underlying value of a Mixed property on non-synced realms.

Sets are not supported as a Mixed value.

Review notes

  • This PR extends the flat collections implementation.
    • The tests from the previous PR have been slightly reorganized to make both set of tests more cohesive.
    • That may make diffs (especially in the tests) larger without actually having changed much of the previous tests. In these cases, it may be easier to look at the file rather than the diffs, with most new tests having "nested" in its name.
  • This PR also includes refactoring of some preexisting code to alleviate this and future implementations.
    • TLDR:
      • The JS SDK has used a pattern of injecting getters and setters (computed when the realm is opened) to each collection. The corresponding definitions of those injected functions were sometimes harder to intuitively locate, so they have been centralized to each collection class file.
      • The choice to inject accessors was due to performance reasons, by doing some computations beforehand, rather than during construction of an object or collection.
    • (Injected getters/setters for the top-level Realm Object itself has not been refactored.)
  • Corresponding Core PR:

Test overview

Click to expand
  Mixed
    Collection types
      CRUD operations
        Create and access
          List
            ✓ has all primitive types (input: JS Array)
            ✓ has all primitive types (input: Realm List)
            ✓ has all primitive types (input: Default value)
            ✓ has nested lists of all primitive types
            ✓ has nested dictionaries of all primitive types
            ✓ has mix of nested collections of all types
            ✓ inserts all primitive types via `push()`
            ✓ inserts nested lists of all primitive types via `push()`
            ✓ inserts nested dictionaries of all primitive types via `push()`
            ✓ inserts mix of nested collections of all types via `push()`
            ✓ returns different reference for each access
          Dictionary
            ✓ has all primitive types (input: JS Object)
            ✓ has all primitive types (input: JS Object w/o proto)
            ✓ has all primitive types (input: Realm Dictionary)
            ✓ has all primitive types (input: Default value)
            ✓ can use the spread of embedded Realm object
            ✓ can use the spread of custom non-Realm object
            ✓ has nested lists of all primitive types
            ✓ has nested dictionaries of all primitive types
            ✓ has mix of nested collections of all types
            ✓ inserts all primitive types via setter
            ✓ inserts nested lists of all primitive types via setter
            ✓ inserts nested dictionaries of all primitive types via setter
            ✓ inserts mix of nested collections of all types via setter
            ✓ inserts mix of nested collections of all types via `set()` overloads
            ✓ returns different reference for each access
          Results
            from List
              snapshot()
                ✓ has all primitive types
                ✓ has mix of nested collections of all types
              objects().filtered()
                ✓ has all primitive types
                ✓ has mix of nested collections of all types
            from Dictionary
              objects().filtered()
                ✓ has all primitive types
                ✓ has mix of nested collections of all types
        Update
          List
            ✓ updates top-level item via setter
            ✓ updates nested item via setter
            ✓ updates itself to a new list
            ✓ updates nested list to a new list
            ✓ does not become invalidated when updated to a new list
            - self assigns
            - self assigns nested list
          Dictionary
            ✓ updates top-level entry via setter
            ✓ updates nested entry via setter
            ✓ updates itself to a new dictionary
            ✓ updates nested dictionary to a new dictionary
            ✓ does not become invalidated when updated to a new dictionary
            - self assigns
            - self assigns nested dictionary
        Remove
          List
            ✓ removes top-level item via `remove()`
            ✓ removes nested item via `remove()`
          Dictionary
            ✓ removes top-level entry via `remove()`
            ✓ removes nested entry via `remove()`
        JS collection methods
          List
            ✓ pop()
            ✓ shift()
            ✓ unshift()
            ✓ splice()
            ✓ indexOf()
          Iterators
            ✓ values() - list
            ✓ values() - dictionary
            ✓ entries() - list
            ✓ entries() - dictionary
      Filtering
        ✓ filters by query path on list of all primitive types
        ✓ filters by query path on nested list of all primitive types
        ✓ filters by query path on dictionary of all primitive types
        ✓ filters by query path on nested dictionary of all primitive types
      Invalid operations
        ✓ throws when creating a Mixed with a set
        ✓ throws when creating a set with a list
        ✓ throws when creating a set with a dictionary
        ✓ throws when updating a list item to a set
        ✓ throws when updating a dictionary entry to a set
        ✓ throws when creating a list or dictionary with an embedded object
        ✓ throws when setting a list or dictionary item to an embedded object
        ✓ throws when setting a list or dictionary outside a transaction
        ✓ throws when setting a list item out of bounds
        ✓ throws when setting a nested list item out of bounds
        ✓ throws when assigning to list snapshot (Results)
        ✓ invalidates the list when removed
        ✓ invalidates the dictionary when removed

  Observable
    Collections in Mixed
      Collection notifications
        List
          ✓ fires when inserting, updating, and deleting at top-level
          ✓ fires when inserting, updating, and deleting in nested list
          ✓ fires when inserting, updating, and deleting in nested dictionary
          ✓ does not fire when updating object at top-level
        Dictionary
          ✓ fires when inserting, updating, and deleting at top-level
          ✓ fires when inserting, updating, and deleting in nested list
          ✓ fires when inserting, updating, and deleting in nested dictionary
          ✓ does not fire when updating object at top-level
      Object notifications
        ✓ fires when inserting, updating, and deleting in top-level list
        ✓ fires when inserting, updating, and deleting in nested list
        ✓ fires when inserting, updating, and deleting in top-level dictionary
        ✓ fires when inserting, updating, and deleting in nested dictionary
        ✓ fires when inserting, updating, and deleting in nested dictionary (using key-path)

Brief overview of usage

class CustomObject extends Realm.Object {
  value!: Realm.Types.Mixed;

  static schema: ObjectSchema = {
    name: "CustomObject",
    properties: {
      value: "mixed",
    },
  };
}

const realm = await Realm.open({ schema: [CustomObject] });

// Create an object with a dictionary value as the Mixed property, containing primitives
// and a list. (To use the properties of a Realm object, embedded Realm object, or a
// non-Realm object instance as a dictionary, you must spread the object into a new one).
const realmObject = realm.write(() => {
  return realm.create(CustomObject, {
    value: {
      num: 1,
      string: "hello",
      bool: true,
      list: [1, "hello", true /*, more nested collections */],
    },
  });
});

// Accessing the value returns the managed collection.
const dictionary = realmObject.value as Realm.Dictionary;
console.log(dictionary instanceof Realm.Dictionary);         // true
console.log(dictionary.list instanceof Realm.List);          // true
console.log((dictionary.list as Realm.List)[1] === "hello"); // true

// Get all objects with the Mixed `value` property being a dictionary
// containing the key `list` with the second item matching `"hello"`.
// (See "Filtering" tests for more alternatives.)
const objects = realm.objects(CustomObject);
let filtered = objects.filtered(`value.list[1] == $0`, "hello");
console.log(filtered.length); // 1

// Same as above query but `"hello"` can match on any index in the list.
filtered = objects.filtered(`value.list[*] == $0`, "hello");
console.log(filtered.length); // 1

// Update the Mixed property to a list.
realm.write(() => {
  realmObject.value = [1, "hello", { key: "value" }];
});

This closes #6344, #6345, #6245

☑️ ToDos

  • 📝 Changelog entry
  • 🚦 Tests

elle-j added 30 commits February 9, 2024 13:14
@elle-j elle-j marked this pull request as ready for review March 12, 2024 09:16
// Collections are always treated as not equal since their
// references will always be different for each access.
const NOT_FOUND = -1;
return NOT_FOUND;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use a const here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean why not just return -1? It's merely for readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you want to make clear that -1 is used when the value is not found, but for me it looks a little bit strange creating a const variable and then using it straight away in the same function. Aren't we also creating a new variable unnecessarily every time the method is called?
I think a comment would suffice in this case, or maybe move the const declaration at a class or higher level.
Just an observation though, I don't have super strong opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point 🙂 In this case, this code only runs when the value to search for is a collection, and these two statements would likely just be replaced with returning -1 in the end anyway. In this context I think just having -1 (without either the variable or a comment) would not be too bad, but I think the readability increases if you do have it.

Copy link
Contributor

@papafe papafe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me for what I can follow 😁

Copy link
Contributor

@kneth kneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After your introduction in person, I don't have many comments.

As we don't need much update to the API documentation, I suggest that we write a larger section in CHANGELOG.md when we are about to merge.

const isEmbedded =
baseType === binding.PropertyType.Object && internal.objectSchema.tableType === binding.TableType.Embedded;
toItemType(results.type) === binding.PropertyType.Object &&
internal.objectSchema.tableType === binding.TableType.Embedded;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swapping the order might improve performance a bit 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment above about internal.objectSchema.tableType throwing if we don't check for the Object property type first.

packages/realm/src/List.ts Outdated Show resolved Hide resolved
// form of Symbol singletons. This is due to not being able to construct
// the actual list or dictionary in the current context.
case realm::type_List:
return ${this.addon.accessCtor("Symbol_for")}.call(_env, "Realm.List");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accessCtor seems ripe for a rename. Perhaps it should be closer linked to "injectable"?

this.realm.write(() => {
nestedList[0] = "primitive";
});
}).to.throw("Requested index 0 calling set() on list 'MixedClass.mixed' when empty");
Copy link
Member

@kraenhansen kraenhansen Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with the non-nested list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment.

this.realm.write(() => {
list[0] = "primitive";
});
}).to.throw("Requested index 0 calling set() on list 'MixedClass.mixed' when empty");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this isn't allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where Realm.List differs in behavior from a JS Array. The Core APIs for setting will throw if index >= size, whereas inserting only throws for index >= size + 1. This is the same behavior as we had before for Realm.List.

From a JS perspective, setting a value at, say, index 5 of an empty list, we'd have to set index 0-4 to undefined (although this would be converted to null values).

* Fix type bundling issue

* Inline functions into "create*Accessor*" functions

* Refactored typeHelpers out of accessors
@elle-j elle-j merged commit 9cbb1f8 into lj/collections-in-mixed Apr 2, 2024
27 of 31 checks passed
@elle-j elle-j deleted the lj/nested-collections-in-mixed branch April 2, 2024 10:42
elle-j added a commit that referenced this pull request Apr 11, 2024
…ions (#6513)

* Move geospatial helper functions to related file.

* Implement setting nested lists in Mixed.

* Implement setting nested dictionaries in Mixed.

* Implement getting nested lists in Mixed.

* Implement getting nested dictionaries in Mixed.

* Test creating and accessing nested lists and dicts.

* Make previous flat collections tests use the new 'expect' function.

* Test that max nesting level throws.

* Delegate throwing when using a Set to 'mixedToBinding()'.

* Implement setting nested collections on a dictionary via setter.

* Test nested collections on dictionary via setter.

* Minor update to names of tests.

* Combine nested and flat collections tests into same suite.

* Implement setting nested collections on a list via setter.

* Test nested collections on list via setter.

* Refactor common test logic to helper functions.

* Optimize property setter for hot-path and use friendlier err msg.

* Refactor test helper function to build collections of any depth.

* Implement inserting nested collections on a list via 'push()'.

* Test nested collections on a list via 'push()'.

* Test updating dictionary entry to nested collections via setter.

* Test updating nested list/dictionary item via setter.

* Test removing items from collections via 'remove()'.

* Test object notifications when modifying nested collections.

* Group previous notification tests into one test.

* Group collection notifications tests into 'List' and 'Dictionary'.

* Test collection notifications when modifying nested collections.

* Remove collections from test context.

* Test filtering by query path on nested collections.

* Align object schema property names in tests.

* Test filtering with int at_type.

* Implement setting nested collections on a dictionary via 'set()' overloads.

* Test JS Array method 'values()'.

* Test JS Array method 'entries()'.

* Implement getting nested collections on dictionary 'values()' and 'entries()'.

* Test 'values()' and 'entries()' on dictionary with nested collections.

* Remove unnecessary 'fromBinding()' calls.

* Refactor collection helpers from 'PropertyHelpers' into the respective collection file.

* Introduce list/dict sentinels to circumvent extra Core access.

* Rename getter to default.

* Remove redundant 'snapshotGet'.

* Add abstract 'get' and 'set' to 'OrderedCollection'.

* Rename the collection helpers to 'accessor'.

* Move tests into subsuites.

* Fix 'Results.update()'.

* Support nested collections in 'pop()', 'shift()', 'unshift()', 'splice()'.

* Test list 'pop()'.

* Test list 'shift()'.

* Test list 'unshift()'.

* Test list 'splice()'.

* Return 'not found' for collections searched for in 'indexOf()'.

* Test ordered collection 'indexOf()'.

* Support list/dict sentinels in JSI.

* Test references per access.

* Enable skipped tests after Core bug fix.

* Point to updated Core.

* Fix accessor for non-Mixed top-level collection with Mixed items.

* Enable and fix previously skipped test.

* Update 'mixed{}'.

* Update 'mixed<>'.

* Remove now-invalidated test.

* Remove unused injectable from Node bindgen template.

* Replace if-statements with switch.

* Add explicit Results and Set accessors for Mixed.

* Adapt to change in Core treating missing keys as null in queries.

* Rename insertion function.

* Include tests of Dictionary property type with Mixed.

* Test reassigning to new collection and self-assignment.

* Test mixed

* Update 'mixed[]'.

* Test results accessor.

* Update error messages.

* Make accessor helpers an object field rather than spread.

* Suggestions for "nested collections in mixed" (#6566)

* Fix type bundling issue

* Inline functions into "create*Accessor*" functions

* Refactored typeHelpers out of accessors

* Remove leftover 'Symbol_for' in node-wrapper template.

* Test not invalidating new collection.

* Remove test for max nesting level.

The max nesting level in debug in Core has been updated to be the same as for release.

* Remove reliance on issue-fix in certain tests.

* Add key path test for object listener on mixed field.

* Use '.values()' and '.entries()' in iteration.

* Update comments.

* Add CHANGELOG entry.

---------

Co-authored-by: Kræn Hansen <[email protected]>
elle-j added a commit that referenced this pull request Apr 26, 2024
…ions (#6513)

* Move geospatial helper functions to related file.

* Implement setting nested lists in Mixed.

* Implement setting nested dictionaries in Mixed.

* Implement getting nested lists in Mixed.

* Implement getting nested dictionaries in Mixed.

* Test creating and accessing nested lists and dicts.

* Make previous flat collections tests use the new 'expect' function.

* Test that max nesting level throws.

* Delegate throwing when using a Set to 'mixedToBinding()'.

* Implement setting nested collections on a dictionary via setter.

* Test nested collections on dictionary via setter.

* Minor update to names of tests.

* Combine nested and flat collections tests into same suite.

* Implement setting nested collections on a list via setter.

* Test nested collections on list via setter.

* Refactor common test logic to helper functions.

* Optimize property setter for hot-path and use friendlier err msg.

* Refactor test helper function to build collections of any depth.

* Implement inserting nested collections on a list via 'push()'.

* Test nested collections on a list via 'push()'.

* Test updating dictionary entry to nested collections via setter.

* Test updating nested list/dictionary item via setter.

* Test removing items from collections via 'remove()'.

* Test object notifications when modifying nested collections.

* Group previous notification tests into one test.

* Group collection notifications tests into 'List' and 'Dictionary'.

* Test collection notifications when modifying nested collections.

* Remove collections from test context.

* Test filtering by query path on nested collections.

* Align object schema property names in tests.

* Test filtering with int at_type.

* Implement setting nested collections on a dictionary via 'set()' overloads.

* Test JS Array method 'values()'.

* Test JS Array method 'entries()'.

* Implement getting nested collections on dictionary 'values()' and 'entries()'.

* Test 'values()' and 'entries()' on dictionary with nested collections.

* Remove unnecessary 'fromBinding()' calls.

* Refactor collection helpers from 'PropertyHelpers' into the respective collection file.

* Introduce list/dict sentinels to circumvent extra Core access.

* Rename getter to default.

* Remove redundant 'snapshotGet'.

* Add abstract 'get' and 'set' to 'OrderedCollection'.

* Rename the collection helpers to 'accessor'.

* Move tests into subsuites.

* Fix 'Results.update()'.

* Support nested collections in 'pop()', 'shift()', 'unshift()', 'splice()'.

* Test list 'pop()'.

* Test list 'shift()'.

* Test list 'unshift()'.

* Test list 'splice()'.

* Return 'not found' for collections searched for in 'indexOf()'.

* Test ordered collection 'indexOf()'.

* Support list/dict sentinels in JSI.

* Test references per access.

* Enable skipped tests after Core bug fix.

* Point to updated Core.

* Fix accessor for non-Mixed top-level collection with Mixed items.

* Enable and fix previously skipped test.

* Update 'mixed{}'.

* Update 'mixed<>'.

* Remove now-invalidated test.

* Remove unused injectable from Node bindgen template.

* Replace if-statements with switch.

* Add explicit Results and Set accessors for Mixed.

* Adapt to change in Core treating missing keys as null in queries.

* Rename insertion function.

* Include tests of Dictionary property type with Mixed.

* Test reassigning to new collection and self-assignment.

* Test mixed

* Update 'mixed[]'.

* Test results accessor.

* Update error messages.

* Make accessor helpers an object field rather than spread.

* Suggestions for "nested collections in mixed" (#6566)

* Fix type bundling issue

* Inline functions into "create*Accessor*" functions

* Refactored typeHelpers out of accessors

* Remove leftover 'Symbol_for' in node-wrapper template.

* Test not invalidating new collection.

* Remove test for max nesting level.

The max nesting level in debug in Core has been updated to be the same as for release.

* Remove reliance on issue-fix in certain tests.

* Add key path test for object listener on mixed field.

* Use '.values()' and '.entries()' in iteration.

* Update comments.

* Add CHANGELOG entry.

---------

Co-authored-by: Kræn Hansen <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants