-
Notifications
You must be signed in to change notification settings - Fork 61
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
[RKOTLIN-612] MongoClient API #1593
Conversation
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.
First round of feedback. Haven't gone over the tests yet. Most things look great.
Some docs seems to be missing and I have some reservations about the return types in MongoCollection
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/ext/UserExt.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/ext/UserExt.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/FunctionsImpl.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoDatabase.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoCollection.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoCollection.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoCollection.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoCollection.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoCollection.kt
Outdated
Show resolved
Hide resolved
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.
Nice, mostly just minor documentation issues at this point.
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoClient.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoCollection.kt
Outdated
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/mongo/MongoCollection.kt
Outdated
Show resolved
Hide resolved
packages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/UserTests.kt
Show resolved
Hide resolved
...est-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/mongo/MongoClientTests.kt
Outdated
Show resolved
Hide resolved
...t-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/mongo/MongoDatabaseTests.kt
Outdated
Show resolved
Hide resolved
...t-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/mongo/MongoDatabaseTests.kt
Outdated
Show resolved
Hide resolved
...sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/mongo/MongoCollectionTests.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Melchior <[email protected]>
# Conflicts: # CHANGELOG.md # packages/test-sync/src/commonMain/kotlin/io/realm/kotlin/test/mongodb/util/AppAdmin.kt # packages/test-sync/src/commonMain/kotlin/io/realm/kotlin/test/mongodb/util/TestAppInitializer.kt # packages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/utils/Utils.kt
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.
LGTM, but then again my exposure to this SDK is very limited
...s/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/MongoCollectionImpl.kt
Show resolved
Hide resolved
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.
Looks great, amazing job. Some minor comments.
packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/platform/RealmObject.kt
Outdated
Show resolved
Hide resolved
...sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/mongo/MongoCollectionTests.kt
Show resolved
Hide resolved
...sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/mongo/MongoCollectionTests.kt
Outdated
Show resolved
Hide resolved
...s/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/MongoCollectionImpl.kt
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
open fun findOne() = runBlocking<Unit> { |
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.
Why it has to be open?
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 was to overwrite the test in the actual implementations. I have remove the open
. Actually it is now possible to select which implementing class you want to run the test for when you click the gutter icon for a test in an interface in latest Android Studio. That is quite neat.
...sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/mongo/MongoCollectionTests.kt
Outdated
Show resolved
Hide resolved
fun findOne_extraFieldsAreDiscarded() = runBlocking<Unit> { | ||
val withDocumentClass = collection.withDocumentClass<BsonDocument>() | ||
withDocumentClass.insertOne( | ||
BsonDocument( |
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 would be great if it could be written as:
Bson {
"_id" to Random.nextInt(),
"name" to "object-1",
"extra" to "extra",
}
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 just added this convenience extension constructor in https://github.com/realm/realm-kotlin/pull/1593/files#diff-62866c3bf368e1c82bfc172e3aa985f0d097e7ae6bb93002d12e5b78978c235dR1418. More clever DSL-construct should probably be filed as an issue in https://github.com/mongodb/kbson
...s/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/MongoCollectionImpl.kt
Outdated
Show resolved
Hide resolved
...ges/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/MongoDBSerializer.kt
Outdated
Show resolved
Hide resolved
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.
Looks great
This PR adds
MongoClient
-APIs to query and update remote Atlas App Service data sources directly according to these docsThis overall flow is to obtain a
MongoCollection
though:App
->User
->MongoClient
->MongoDatabase
->MongoCollection
with:
You can obtain typed collection instances that will automatically serialize arguments by specifying the object types of the collection.
If sync is enabled and there is a schema defined for a specific
RealmObject
you can obtain theMongoCollection
directly from theMongoClient
. This allows obtaining aMongoCollection
directly though:App
->User
->MongoClient
->MongoCollection
with:
This will use the sync schema definition to map from the
RealmObject
to the collection of the corresponding type on the server.Serialization of custom types and links between RealmObjects
The typed
MongoCollection<T>
supports serialization to/from Kotlin Serialization's@Serializable
types by default.Since MongoDB represents links only by their primary keys, special serializers needs to be configured if serializing links to/from existing
RealmObject
model classes. These can be added by supplying aSerializerModule
to theejson
-serializer infrastructure when obtaining any of the relevant MongoDB APIMongoClient
,MongoDatabase
orMongoCollection
instances with:The main entry points and main public APIs are:
User.mongoClient
MongoClient
MongoDatabase
MongoCollection
Input to review
One of the main issues with this PR is how to control serialization. Kotlins serialization framework is heavily dependant of compile time derivable information which has made it a bit tricky. Some of the concerns was:
The above led me to write a generic serializer that reuses existing EJSON<->BsonDocument serializer from github.com/mongodb/kbson and control serialization from statically stored model metadata. This however needs runtime information of the
KClass
and cannot easily be added through class annotations. Alternative would have been to write a new decoder that could hold the set ofclassName
->RealmObjectCompanion
needed to construct typed link instances, as there is no other way to maintain a runtime-context during serialization. Instead I continued the approach from AppConfiguration.ejson of allowing to inject the fullStringFormat
'er. To ease configuration all realm object serialializers must be added to theStringFormat
'er as a specielSerializerModule
constructed from realmSerializerModule.TODOs
0.4.0
releaseCloses #972