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

RDART-910: Add support for RealmObject.objectSchema #1540

Merged
merged 8 commits into from
Mar 14, 2024
Merged

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Mar 6, 2024

Fixes #1449

Replaces #1493

Depends on realm/realm-core#7426

@nirinchev nirinchev requested a review from nielsenko March 6, 2024 15:53
@cla-bot cla-bot bot added the cla: yes label Mar 6, 2024
@nirinchev nirinchev changed the title Add support for RealmObject.objectSchema RDART-910: Add support for RealmObject.objectSchema Mar 6, 2024
Copy link

coveralls-official bot commented Mar 6, 2024

Pull Request Test Coverage Report for Build 8287079256

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 48 of 98 (48.98%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.5%) to 86.097%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/realm_dart/lib/src/configuration.dart 6 11 54.55%
packages/realm_dart/lib/src/realm_object.dart 10 18 55.56%
packages/realm_dart/lib/src/native/realm_core.dart 21 32 65.63%
packages/realm_dart/lib/src/realm_class.dart 10 36 27.78%
Files with Coverage Reduction New Missed Lines %
packages/realm_dart/lib/src/native/realm_core.dart 1 90.18%
Totals Coverage Status
Change from base Build 8287071322: -0.5%
Covered Lines: 5778
Relevant Lines: 6711

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

It is unclear to me how dynamic schema changes will work in practice with generated realm models.

How can client code reasonably react to schema changes that invalidate the generated realm objects? Short of installing a new version, or go full dynamic, which sort of defeats the point.

Also, I wonder if we should distinguish between the schema "used" during source generation, and the current dynamic schema

@@ -0,0 +1,107 @@
part of 'realm_core.dart';

class SyncSocketProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this. How is that related to dynamic schemas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, seems like something got left over when switching branches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we separate it out? I guess it belongs to platform networking?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll remove it.

/// Returns the name of this schema type.
final String name;

/// Returns the base type of this schema object.
final ObjectType baseType;

/// Creates schema instance with object type and collection of object's properties.
const SchemaObject(this.baseType, this.type, this.name, this.properties);
SchemaObject(this.baseType, this.type, this.name, Iterable<SchemaProperty> properties) : _properties = List.from(properties);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the loss of const on this ctor here really needed? Isn't it only caused by the List.from call?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, but I'm not sure how we can avoid the loss of const.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of the top of my head, I think you can use a DelegatingIterable .. or just drop making SchemaObject iterable itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that we want the _properties list to be mutable - i.e. we're adding stuff at runtime. My understanding was that this is incompatible with const-constructed objects, but it's very possible I'm missing something.

Copy link
Contributor

@nielsenko nielsenko Mar 12, 2024

Choose a reason for hiding this comment

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

Just because a ctor is marked as const doesn't mean it has to be used as such
with const arguments. Only that it can. If the backing list to DelegatingIterable is const, then you will get a runtime exception trying to modify it.

I guess that ties into my question, if we should track both the current schema, and the original schema used when generating the model. The latter could still be const constructed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still dream of using the flyweight property pattern from the old #903 PR 😄

@nirinchev
Copy link
Member Author

The expectation is that the changes to the schema will be additive (if they're breaking, then you need to indeed reinstall the app).

For how that would be used - you can access the generated classes/properties as usual - the main change is that if you get a new property through sync, you can also access that by going through the dynamic API. Similarly, if there's a new object type that you didn't know about at compile time and that gets synchronized inside a RealmValue, you'd be able to introspect its schema and access its data.

@nielsenko
Copy link
Contributor

nielsenko commented Mar 12, 2024

...
For how that would be used - you can access the generated classes/properties as usual - the main change is that if you get a new property through sync, you can also access that by going through the dynamic API. Similarly, if there's a new object type that you didn't know about at compile time and that gets synchronized inside a RealmValue, you'd be able to introspect its schema and access its data.

I just struggle to see how an end-user can make good use of this in practice, without their code becoming overly convoluted.

Anyway, that is perhaps a bit unfair to this PR. A implies B.

I still think it could be beneficial to track the two schemas separately. Ie. the one used at build time, and the one currently in effect.

@nirinchev
Copy link
Member Author

I agree, I don't think this should be something many people default to, but the use cases/annoyances would be similar to using a RelamValue - you'd need to introspect the schema and render things differently based on the type of the properties. An example would be an http server logging utility that logs some metadata for processed requests - e.g. the headers being sent. Then you can have a partially static/partially dynamic model that looks like:

class _HttpRequest {
  late ObjectId id;
  late DateTime timestamp;
  // ...
}

But then as requests are processed, columns are added dynamically for the different headers we see. Then they could do something like:

obj = realm.query<HttpRequest>().first;
for (const prop in obj.objectSchema.where(s => s.name.startsWith("header")) {
  switch (prop.propertyType) {
    case RealmPropertyType.string:
      const value = obj.dynamic.get<String>(prop.name);
      // Render value into the UI
	  break;
  }
}

@nirinchev
Copy link
Member Author

@nielsenko I've updated this to not actually update the schema at runtime since that depends on realm/realm-core#7426 which is not resolved yet. I suggest we merge it as is since it represents a minor breaking change and we can add the autoupdating aspect as a follow-up PR.

@nirinchev nirinchev merged commit 30d7db8 into main Mar 14, 2024
@nirinchev nirinchev deleted the ni/object-schema-2 branch March 14, 2024 20:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 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.

Expose RealmObjectBase.schema
2 participants