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

Expose dynamic Realm API #495

Merged
merged 8 commits into from
Aug 18, 2022
Merged

Expose dynamic Realm API #495

merged 8 commits into from
Aug 18, 2022

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Apr 25, 2022

This exposes a set of string-based access API for the Realm. The current PR touches only the Realm entrypoint and a follow-up PR will be done to improve the experience when working with objects. The API is as follows

// Lookup all objects of type Car
final allCars = realm.dynamic.all('Car');

// The collection can be further filtered using the normal API
final carsFromJapan = allCars.query('manufacturer.domicile == "Japan"');

// Lookup an object by PK
final honda = realm.dynamic.find('Car', 'Honda');

Note: One can use the RealmObject.get<...>(...) API to then read properties on the object, but those will change in the future, so not showing them in the example.

This is a part of #70.

Fixes #690.

@cla-bot cla-bot bot added the cla: yes label Apr 25, 2022
@nirinchev nirinchev self-assigned this Apr 25, 2022
@coveralls
Copy link

coveralls commented Apr 25, 2022

Pull Request Test Coverage Report for Build 2225904910

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.677%

Totals Coverage Status
Change from base Build 2211365211: 0.0%
Covered Lines: 400
Relevant Lines: 427

💛 - Coveralls

Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

All tests pass, and we have a string based interface. However, I think the DynamicRealm should traffic in dynamics with runtime type DynamicRealmObject and that these should overload noSuchMethod to access the right properties.

@nirinchev
Copy link
Member Author

nirinchev commented Apr 29, 2022

There are three reasons why I didn't go for dynamic.

  1. We've been burned by dynamics in the .NET SDK. I know that dart doesn't yet have anything Unity-like in its ecosystem, but I would imagine MS didn't envision Unity either when they implemented dynamic in C#.
  2. The string-based API is slightly more discoverable as you get "intellisense" for it. So you have at least some pointers on what to do.
  3. It is more flexible. You can for example read the property information from the schema and then access properties by their name - e.g.
    for (final prop in properties) {
        if (prop.type == RealmPropertyType.int) {
            final intValue = obj.dynamic.get<int>(prop.name);
            // Do something with intValue
        }
    }
    You can't achieve something like that (or at least I can't see how) with the dynamic type in dart. It's not often, but we have had a handful of requests for that in the .NET SDK and I think it's quite powerful.

Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

It does expose RealmObject.get<T> which could use some polish (I don't like it returns Object?), but perhaps that can be addressed later.

@nirinchev
Copy link
Member Author

I believe RealmObject.get has always been exposed - that's what is used in the generated classes. We use it in tests, but I haven't changed the visibility of it (at least not intentionally).

@blagoev blagoev marked this pull request as draft May 20, 2022 05:42
@nirinchev nirinchev marked this pull request as ready for review June 14, 2022 18:58
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

Is it possible to have flexible sync subscription by dynamic query?

lib/src/native/realm_core.dart Outdated Show resolved Hide resolved

/// A collection of [SchemaObject] that will be used to construct the
/// [RealmSchema] once the Realm is opened.
final Iterable<SchemaObject> schemaObjects;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for reviewers: this is replacing the RealmSchema field that previously existed on the configuration. The reason is that it's now possible to provide an empty collection and have the schema be read from the file. We're now exposing the schema on the Realm instance directly as that can only be truly known after we've opened the file. I wanted to make that distinction more explicit, which is why I opted to change the field on the configuration (otherwise we'd have realm.schema != realm.config.schema and that felt very awkward).

Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

In general I think we should not be exposing the dynamic API except in migrations. We should be building a typed SDK instead of dynamic one and hence the SDK interface should be typed.
I tried to thoroughly review the changes since these are fundamental to the SDK internals

CHANGELOG.md Outdated Show resolved Hide resolved
test/configuration_test.dart Show resolved Hide resolved
lib/src/list.dart Show resolved Hide resolved
lib/src/list.dart Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
src/realm_dart_sync.cpp Outdated Show resolved Hide resolved
lib/src/realm_object.dart Outdated Show resolved Hide resolved
lib/src/realm_object.dart Outdated Show resolved Hide resolved
lib/src/realm_object.dart Outdated Show resolved Hide resolved
lib/src/realm_object.dart Show resolved Hide resolved
@blagoev
Copy link
Contributor

blagoev commented Jul 24, 2022

I have been constantly thinking and revisiting this PR for the last weeks and I think we are doing a step in the wrong direction here. I have taken a look at how Realm Swift does this and am I really leaning towards a similar solution for Realm Flutter. I think Realm .NET made a mistake by enabling a generic dynamic API available at all times, which is only required while doing migrations. Instead I believe we should be providing this API only in the migration callback as Realm Swift does. We have this generic dynamic support in Dart which allows us to provide two dynamic objects, old and new version, in the migration callback from which we can allow even nicer than Swift API, cause dynamic objects in Dart allow access to any property even non existing ones. Let's discuss later when we regroup.

@nirinchev
Copy link
Member Author

nirinchev commented Aug 1, 2022

String-based property setting/getting seems to be available via subscripts, like foo["bar"] = 123. As far as I can tell this functionality is always available, not just in migrations.

It also has dynamicObjects functionality which is roughly equivalent to what I'm proposing with this PR. It even goes further allowing creating of dynamic objects, which is not something covered by my work, but can be done as a follow up.

Virtually all of our SDKs offer dynamic and static API (except JS which only offers dynamic) and while I agree that the dynamic API are niche and mostly useful for migrations, I do not agree that exposing them is a mistake. It makes the SDK more powerful and gives developers flexibility.

@blagoev
Copy link
Contributor

blagoev commented Aug 15, 2022

We have discussed internally and it seems we need a dynamic API in the Dart SDK to support certain use cases like app users building the Realm schema at runtime etc.

My comments about the _getValues function are still valid concerns. I suggest we refactor this code not to use such late function optimizations. This function is becoming an important internal SDK function while at the same time providing weird and unreadable signature.
I guess I could help refactoring that code.

@nirinchev nirinchev requested a review from blagoev August 15, 2022 13:43
@nirinchev
Copy link
Member Author

I have inlined _getValues - it's a fair amount of code duplication, but I don't suppose that code will change that often, so it's probably fine. @blagoev please take another look and let me know if something is unclear. Happy to also do a live review session if that'll help get this PR merged faster.

@nirinchev nirinchev self-assigned this Aug 17, 2022
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

Added some questions?

lib/src/realm_class.dart Show resolved Hide resolved
Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

I resolved most previous comments, still I have some here.

lib/src/realm_class.dart Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/realm_object.dart Show resolved Hide resolved
pubspec.yaml Show resolved Hide resolved
test/dynamic_realm_test.dart Show resolved Hide resolved
@nirinchev
Copy link
Member Author

@blagoev refactored the while loops to use recursion. While I don't see much of a difference, it's not a hill I'm willing to die on, so can you take another look and see if there's anything left or we can merge.

Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

Thanks @nirinchev. Lets merge it.

@nirinchev nirinchev merged commit 8eb3b65 into master Aug 18, 2022
@nirinchev nirinchev deleted the ni/dynamic-realm branch August 18, 2022 22:30
@desistefanova
Copy link
Contributor

@blagoev refactored the while loops to use recursion. While I don't see much of a difference, it's not a hill I'm willing to die on, so can you take another look and see if there's anything left or we can merge.

It is not perfect in both ways. But it is accectable.

@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.

Fix countIds in userGetIdentities once Core changes how count is retrieved.
5 participants