Pass in correct schema type for explicit and autodiscovered types#2259
Merged
Pass in correct schema type for explicit and autodiscovered types#2259
Conversation
LaPeste
changed the title
LaPeste
commented
Feb 19, 2021
CHANGELOG.md
Outdated
| ### Fixed | ||
| * Fixed an issue that would result in UWP apps being rejected from the Microsoft Store due to an unsupported API (`__C_specific_handler`) being used. (Issue [#2235](https://github.com/realm/realm-dotnet/issues/2235)) | ||
| * Fixed a bug where applying multiple `OrderBy` clauses on a query would result in the clauses being appended to each other as if they were `.ThenBy` rather than the last clause replacing the preceding ones. (PR [#2255](https://github.com/realm/realm-dotnet/issues/2255)) | ||
| * Fixed proper selection of Additive mode (Explicit or Discovered) to pass to the schema builder. This selection is based on whether or not the user has provided a subset of classes to the Realm Schema. More info about this subject can be found [here](https://docs.mongodb.com/realm/dotnet/objects/#provide-a-subset-of-classes-to-your-realm-schema). (PR [#2259](https://github.com/realm/realm-dotnet/pull/2259)) |
Contributor
Author
There was a problem hiding this comment.
2 1 issues with this:
- I'm not sure this should be considered an issue. After all, it was consciously decided to have a quick change to the breaking changes introduced by the splitting of the schema mode
Additive. If we agree on this, maybe internal could be a better section. Although, even internal means like a change when this must be addressed given the new change in core. So, please advice if this is necessary at all and where it should go.
2. I'm not that happy to have the PR# here just yet. This is because another PR may be needed based on the outcome of the conversation with James.
==== UPDATE
Since James has confirmed that it was chosen by design to have this matter only touch sync'd realms then point #2 isn't relevant anymore.
nirinchev
requested changes
Feb 20, 2021
nirinchev
reviewed
Feb 22, 2021
Member
nirinchev
left a comment
nirinchev
left a comment
There was a problem hiding this comment.
Mostly documentation nits from me + a suggestion to save a few lines of code
CHANGELOG.md
Outdated
| ### Fixed | ||
| * Fixed an issue that would result in UWP apps being rejected from the Microsoft Store due to an unsupported API (`__C_specific_handler`) being used. (Issue [#2235](https://github.com/realm/realm-dotnet/issues/2235)) | ||
| * Fixed a bug where applying multiple `OrderBy` clauses on a query would result in the clauses being appended to each other as if they were `.ThenBy` rather than the last clause replacing the preceding ones. (PR [#2255](https://github.com/realm/realm-dotnet/issues/2255)) | ||
| * Fixed proper selection of Additive mode (Explicit or Discovered) to pass to the schema builder. This selection is based on whether or not the user has provided a subset of classes to the Realm Schema. More info about this subject can be found [here](https://docs.mongodb.com/realm/dotnet/objects/#provide-a-subset-of-classes-to-your-realm-schema). (PR [#2259](https://github.com/realm/realm-dotnet/pull/2259)) |
Member
There was a problem hiding this comment.
This message is not something users can understand - none of the mentioned types are publicly exposed. We should instead say something like:
When explicitly specifying `SyncConfiguration.ObjectTypes`, added
a check to validate the schema and ensure all `EmbeddedObject`
classes are reachable from a class inheriting from `RealmObject`.
Realm/Realm/Sync/SchemaMode.cs
Outdated
| { | ||
| // N.B. the values must match their representation in core! | ||
| AdditiveDiscovered = 4, | ||
| AdditiveExplicit |
Member
There was a problem hiding this comment.
Let's be explicit about all values.
nirinchev
approved these changes
Feb 24, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Based on
RealmConfigurationBase.ObjectClasses"AdditiveDiscovered" or "AdditiveExplicit" is now appropriately selected and passed to core.Currently, because of the way core did it, this is a detail only relevant for synced scenarios. I'll talk to James Stone to understand better the why's'.
This PR also adds an additional exception to have a more precise error category when an issue with the schema validation arises.
==================================
UPDATE
I talked with James and he explained that the 2 types of Additive behaviours are purposedly used only for sync-d realms because unreachable embeddedObjects are only problematic for the server. Hence, when the client doesn't sync, there's no reason to check for them as these unreachable objs don't really create an issue; although being logically useless. He added that the only reason why such check had to end up in the client, although only bothering the server, is that when the server received such object,s the message sent back to the client was too obscure for the user to be of help.
Fixes #2193
TODO