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

Update to Core 13.2.0 #1157

Merged
merged 71 commits into from
Jan 13, 2023
Merged

Update to Core 13.2.0 #1157

merged 71 commits into from
Jan 13, 2023

Conversation

cmelchior
Copy link
Contributor

@cmelchior cmelchior commented Dec 1, 2022

Update to Realm Core 13.2.0

The changes to Sync Connection Parameters are defined here: https://docs.google.com/document/d/1jUeN71gzQoLoYxHUawzIQaDVD2xDLbtbWkmxjIMcN-U/edit

TODO

  • closeShouldFreeMemory is failing on macOS. Turns out to only happen when running this test individually.
  • Test for Schema in synced Realms. We should only expose what is defined by the user.
  • Can we add something like "userDefined" in our internal schema metadata. Unclear if we can calculate this somewhere clever.

@cla-bot cla-bot bot added the cla: yes label Dec 1, 2022
@cmelchior cmelchior changed the title Enable writing a copy of flexible sync Realms Update to Core 13.2.0 Dec 22, 2022
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.

Looks OK to me. There are some itchy things around the schema that needs to be replicated all over the realms, but I guess cleaning it up goes better together with the standalone DynamicRealm-API and/or rework of our internal reuse of interfaces, etc. to improve the internal Realm hierarchy implementation.

@@ -519,7 +519,8 @@ class RealmModelSyntheticPropertiesGeneration(private val pluginContext: IrPlugi
property.locationOf()
)
}
val isIndexed = backingField.hasAnnotation(INDEX_ANNOTATION)
// See https://github.com/realm/realm-core/issues/6187
Copy link
Contributor

@rorbech rorbech Jan 11, 2023

Choose a reason for hiding this comment

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

I guess we didn't align on whether RealmSchema should reflect the actual schema or whether there was an index in effect. If going with the latter then I guess it should just be fixed in core. If we wan't to mimic the return value until it is fixed in core then we shouldn't change the schema here but just pass isPrimaryKey || isIndexed to the ValuePropertyType constructor in https://github.com/realm/realm-kotlin/blob/main/packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/schema/RealmPropertyImpl.kt#L47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, right now I'm adding it here, while waiting for the Core fix, but also because I'm a little afraid that Core might not do it correctly at all, so just hacking the return value would just seem to mask even more bugs? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion, I'll remove this fix for now and revert the test. This is now a "known bug" and we will talk with Core about a resolution.

@@ -107,23 +105,34 @@ public class AppConfigurationImpl constructor(
private fun initializeSyncClientConfig(): RealmSyncClientConfigurationPointer =
RealmInterop.realm_sync_client_config_new()
.also { syncClientConfig ->
// TODO use separate logger for sync or piggyback on config's?
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure that I get the full details of this ... but do we need to note this in the CHANGELOG?

@cmelchior cmelchior marked this pull request as ready for review January 12, 2023 12:43
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.

LGTM. Only a minor thing with the changelog. ... And of course when CI is green 😅

CHANGELOG.md Outdated Show resolved Hide resolved
@cmelchior cmelchior merged commit 91ce89d into main Jan 13, 2023
@cmelchior cmelchior deleted the cm/write-copy-to-flx branch January 13, 2023 08:02
@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.

None yet

2 participants