-
Notifications
You must be signed in to change notification settings - Fork 86
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
Conversation
Pull Request Test Coverage Report for Build 8287079256Warning: 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
💛 - Coveralls |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
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. |
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. |
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;
}
} |
@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. |
Fixes #1449
Replaces #1493
Depends on realm/realm-core#7426