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

RealmDictionary feature #1239

Merged
merged 54 commits into from
Mar 10, 2023
Merged

RealmDictionary feature #1239

merged 54 commits into from
Mar 10, 2023

Conversation

edualonso
Copy link
Contributor

@edualonso edualonso commented Jan 25, 2023

Closes #537

Dictionary API

A RealmDictionary is a Map<K, V>, where keys are always strings and values are Realm primitives or objects, be it regular or embedded. Dictionaries can also be expressed as:

  • MutableSet<MutableMap.MutableEntry<K, V>> by calling RealmDictionary.entries. The entry set is a live collection connected to its underlying dictionary. Deletions and insertions are allowed and will result in an updated dictionary. Individual entries can also be modified and will result in an update key-value dictionary pair. Unmanaged entries can also be instantiated via factory - see "Factories" section below. Managed entries are also live key-value pairs and mutating them will update the dictionary accordingly.
  • MutableSet<K> by calling RealmDictionary.keys. The key set is a live collection connected to its underlying dictionary. Deletions are allowed though insertions aren't - it isn't possible to insert a key without a value. Core delivers the keys as realm_results_t but subscribing for key changes is not exposed for now.
  • MutableCollection<V> by calling RealmDictionary.values. The value collection is a live collection connected to its underlying dictionary. Deletions are allowed though insertions aren't - it isn't possible to insert a value without a key. Core delivers the values as realm_results_t so queries on dictionary objects are funnelled through these.

RealmDictionary and RealmMap abstractions

Thanks to how the compiler plugin extracts information from the type system it is (almost) possible to work at a RealmMap level, though we aren't exposing a public implementation of it and RealmMap fields aren't allowed either.

To make the implementation more readable and less painful, a number of type aliases have been added:

  • internal typealias RealmMapEntrySet<K, V> = MutableSet<MutableMap.MutableEntry<K, V>>
  • internal typealias RealmMapMutableEntry<K, V> = MutableMap.MutableEntry<K, V>
  • public typealias RealmDictionaryEntrySet<V> = RealmMapEntrySet<String, V>
  • public typealias RealmDictionaryMutableEntry<V> = RealmMapMutableEntry<String, V>

Object dictionaries

Object dictionaries may contain either regular or embedded objects and null values are allowed. RealmDictionary of objects must be nullable - not the field itself, which is not allowed for any type of Realm collection, but the collection type instead.

Factories

Unmanaged dictionaries can be instantiated by calling the realmDictionaryOf functions that allow the following input values:

  • realmDictionaryOf(vararg elements: Pair<String, T>)
  • realmDictionaryOf(elements: Collection<Pair<String, T>>)

Unmanaged RealmDictionaryMutableEntry instances can be used to add key-value pairs to the dictionary through the dictionary's entry set and can be created by calling the realmDictionaryEntryOf functions which accept the following input values:

  • realmDictionaryEntryOf(pair: Pair<String, V>)
  • realmDictionaryEntryOf(key: String, value: V)
  • realmDictionaryEntryOf(entry: Map.Entry<String, V>)

Queries on dictionaries

There is no direct way to parse a query on a dictionary object through the C-API. However, we can use the realm_results_t object yielded by the C-API when calling RealmDictionary.values to parse a query on the results.

Notifications and change sets

A dictionary change set contains:

  • the deleted keys
  • the inserted keys
  • the changed keys

Pull request plan:

@cla-bot cla-bot bot added the cla: yes label Jan 25, 2023
@edualonso edualonso requested a review from rorbech January 25, 2023 14:25
Eduardo López added 19 commits January 26, 2023 08:49
# Conflicts:
#	packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmObjectHelper.kt
#	packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmSetInternal.kt
#	packages/test-base/src/androidAndroidTest/kotlin/io/realm/kotlin/test/shared/RealmSetTests.kt
… support for Decimal128 in `copyFromRealm` (#1255)
# Conflicts:
#	packages/external/core
#	packages/jni-swig-stub/realm.i
@@ -335,7 +345,8 @@ object TypeDescriptor {
.map { RealmFieldType(CollectionType.RLM_COLLECTION_TYPE_LIST, it) }
val allSetFieldTypes = elementTypesForSet.filter { it.realmFieldType.setSupport }
.map { RealmFieldType(CollectionType.RLM_COLLECTION_TYPE_SET, it) }
// TODO Dict
val allDictionaryFieldTypes = elementTypesForDictionary.filter { it.realmFieldType.dictionarySupport }
.map { RealmFieldType(CollectionType.RLM_COLLECTION_TYPE_DICTIONARY, it) }
val allFieldTypes: List<RealmFieldType> = allSingularFieldTypes + allListFieldTypes
Copy link
Contributor

Choose a reason for hiding this comment

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

allFieldTypes is missing allSetFieldTypes and allDictionaryFieldTypes

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Only missing 3000 lines of tests now. Looking good so far 😄

}

@Test
fun deleteSourceObject() {
val child = realm.writeBlocking {
val child = this.copyToRealm(Child())

val parents = Array(5) {
val parents = (1..5).map {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick... but I find repeat(5) { } easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with repeat is that it doesn't do any mapping and we would have to add the output manually for each iteration. Otherwise I agree, it would look better.

listOf("@sum", "@min", "@max", "@avg")
.forEach { aggregator ->
listOf(
QuerySample::intListField.name, // Just test one Long value
Copy link
Contributor

Choose a reason for hiding this comment

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

I like these tests, but it seems like it would be easy to forget to update these if we add support for new types. I'm not sure it is worth adding now, but I would create an issue for tracking it. Same for the rest of these tests

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 have created #1295

* built by the compiler. Use it to generate different types of scenarios for nullability of
* the element type and the collection field itself.
*/
fun getCode(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice with a slightly more specific function name here, maybe getCodeForCollection or something like that.

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Done...I think we are missing some corner cases around equality testing for Dictionary but otherwise it looks good 🚀


@Test
fun unmanaged() {
// No need to be exhaustive here, just checking delegation works
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we need to be exhaustive? We should be sure we know how unmanaged objects work as well. That said, It would not block merging this PR for it. Maybe just create an issue and we can add testing for it later.

}

@Test
fun accessors_getter_primitive() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these testing that the exhaustive testing framework isn't testing?

It looks like it might be the object accessors, so I would rename them to something like object_accessor_getter_dictionaryWithPrimitives or something along those lines. Same for the other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests check that we import dictionaries correctly when assigning default values to an object's dictionary field. The difference between the exhaustive tests and these is we always start with empty dictionaries in the exhaustive ones. I think slightly changing the name might help here 👍

}

@Test
fun closedRealm_readFails() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about write methods? I.e. put/putAll

Copy link
Contributor

Choose a reason for hiding this comment

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

Also seems to be missing equals, hashcode and toString

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 think I simply forgot about them 🙈

These will also most likely start to fail once we remove the call to RealmReference.checkClosed() so I'll add a TODO.

}

@Test
fun entries_equals() {
Copy link
Contributor

Choose a reason for hiding this comment

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

toString testing seems to be missing

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 comparing two RealmDictionary.entries.toString() or two individual entry.toString()?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we are missing all of them.

}

@Test
fun entry_equals() {
Copy link
Contributor

Choose a reason for hiding this comment

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

toString testing seems to be missing

}

@Test
fun values_equals() {
Copy link
Contributor

Choose a reason for hiding this comment

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

toString seems to be missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as with the comment about the entry set. Do you mean the value collection as a whole or individual values?

val realm: Realm

override fun toString(): String
fun copyToRealm()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we are missing testing for equals/hashCode for the RealmDictionary itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I will add them 👍

@edualonso
Copy link
Contributor Author

Added missing toString tests and added scaffolding for future equals and hashCode tests for when #1097 is fixed

@edualonso edualonso requested a review from cmelchior February 27, 2023 14:17
Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Deferring tests to we fix the equals issue is fine, but I think the toString() implementation is making too many assumptions about dictionaries not having many elements. So we should change it to something else IMO. I proposed a solution in the comment, but happy to discuss alternatives.

@@ -657,6 +677,11 @@ internal class ManagedRealmDictionary<V> constructor(
}
}
}

override fun toString(): String = entries.joinToString { (key, value) -> "[$key,$value]" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I can see where this is coming from, but it looks pretty dangerous, i.e. lets say you have dictionaries with 100's or 1000's of elements. I would probably avoid doing this, but there doesn't seem to be a great alternative either.

Since toString is generally used for debugging I would probably rather turn this into something like RealmDictionary{owner=<classType>,objKey=<objectId>,version=<transactionVersion>}. It would be easy to compute and you would still get some context for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there was no toString implementation for the other collections I took the approach we use for Java lists but I agree it can easily backfire with large datasets. I think your proposal is fair, I would only add size for clarity too.

@edualonso edualonso requested a review from cmelchior February 27, 2023 18:41
Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Awesome. If CI is happy 🚀

Christian Melchior and others added 6 commits March 2, 2023 08:27
# Conflicts:
#	packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmResultsImpl.kt
#	packages/test-base/src/androidAndroidTest/kotlin/io/realm/kotlin/test/shared/BacklinksTests.kt
Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

Finally had a look on this. Haven't gone through all the details but only the overall structures and essential mechanisms ... And looks good to me.

@cmelchior cmelchior merged commit 5b199ef into main Mar 10, 2023
@cmelchior cmelchior deleted the el/dictionary-feature branch March 10, 2023 20:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 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.

Add support for RealmDictionary
4 participants