STITCH-1967 Add full CRUD within .sync() #74

jsflax · 2018-10-31T00:21:59Z

Ticket Description:
This should implement the same CRUD methods that the non sync service has. All of these methods should only operate on the synced, offline documents.

For replacements, they should use the same set and unset event optimization

Add insertMany, updateMany, deleteMany, aggregate, and count to the DataSynchronizer. This is to be in parity with RemoteMongoCollection.
a. For deleteMany and updateMany, the documents need to be fetched before the delete to
retrieve the ids, due to the way canonical filters work.
b. For updateMany, this will match potential unmodified documents. Ignore those.
c. For updateMany, it may be an upsert. If it is an upsert, filter the result using the upsertedId
and emit an insert.
Replace findOneById, updateOneById and deleteOneById with find, updateOne and deleteOne, respectively.
a. For deleteOne, the document needs to be fetched before the delete to retrieve the id, due to
the way canonical filters work.
b. For updateOne, it may be an upsert. If it is an upsert, filter the result using the upsertedId
and emit an insert.
Proxy each method upwards through CoreSync.
Add AggregateOperation, CoreSyncAggregateIterable, CountOperation, InsertManyOperation, and DeleteManyOperation classes to be instantiated in SyncOperations to maintain parity with Operations (remote).
Add SyncCountOptions, SyncDeleteOptions, SyncUpdateOptions, SyncDeleteResult, SyncInsertManyResult, SyncInsertOneResult, and SyncUpdateResult to both maintain parity with the Remote*Options classes, and to create a divergence between our options and the local mongo client library.
Proxy CoreSync operations upwards through Sync (server) and Sync (Android).

Unit testing to test the new logic was critical here, especially for the bulk operations. Integration tests were limited to ensuring that each new operation operates as intended in an integrated environment. Since no new Sync cases were created, adding new conflicted/failing integration tests seemed unnecessarily cumbersome.

…gic; add count op; clean up

jsflax · 2018-10-31T00:24:58Z

As a note, the diff isn't very clean in certain areas because I opted to make the method order in parity with the remote methods as well.

jsflax · 2018-10-31T01:29:07Z

@edaniels @adamchel PTAL

edaniels

LGTM. Just questions and a comment or two on style/tests

edaniels · 2018-10-31T15:34:37Z

...st/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncUnitTestHarness.kt

-            assertTrue(eventSemaphore?.tryAcquire(10, TimeUnit.SECONDS) ?: true)
+        override fun waitForEvents(amount: Int) {
+            waitLock.lock()
+            changeEventListener.totalEventsToAccumulate = amount


[q] Why is this here?

This is to give the harness the ability to wait for multiple events. The accumulator actually does nothing– could just be a counter– as the actual events are later accumulated via capturing the values from the spy.

This is being used to test bulk writes, as multiple change events are emitted from a single method call.

edaniels · 2018-10-31T15:35:43Z

...st/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/SyncUnitTestHarness.kt

+            private val expectedEvent: ChangeEvent<BsonDocument>?,
+            var emitEventSemaphore: Semaphore?
+        ) : ChangeEventListener<BsonDocument> {
+            val eventAccumulator = mutableListOf<ChangeEvent<BsonDocument>>()


[q] Why was this added?

See answer above.

edaniels · 2018-10-31T15:42:18Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

   * Adds and returns a document with a new version to the given document.
   *
-   * @param document the document to attach a new version to.
+   * @param document   the document to attach a new version to.


Is checkstlye not catching any of these issues?

Checkstyle did not. The DataSynchronizer doesn't seem to actually be formatted, as far as I can tell, and my autoformatter "went to town" on the class. We should probably file a ticket (or just make a patch) to reformat it.

edaniels · 2018-10-31T15:42:25Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

  private static BsonDocument withNewVersion(
-          final BsonDocument document,
-          final BsonDocument newVersion
+      final BsonDocument document,


Why did this change?

edaniels · 2018-10-31T15:44:52Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

   * Any local updates to this document cause it to be resumed. An example of pausing a document
   * is when a conflict is being resolved for that document and the handler throws an exception.
-   *
+   * <p>


See above. Though technically, this is also fine.

edaniels · 2018-10-31T15:46:46Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+    // else this is an update
+    if (documentBeforeUpdate == null && updateOptions.isUpsert()) {
+      config = syncConfig.addSynchronizedDocument(namespace, documentId);
+      triggerListeningToNamespace(namespace);


Curious, do you have a test that checks that if you were to remove this line, it would fail?

I don't believe so– I can add a line in our new upsert tests that ensures that the stream is opened on the namespace.

Added to testUpsertOne and testUpsertMany in DataSynchronizerUnitTests 👍

edaniels · 2018-10-31T15:49:22Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+        final BsonDocument beforeDocument;
+        final BsonValue documentId = BsonUtils.getDocumentId(afterDocument);
+
+        if ((beforeDocument = idToBeforeDocumentMap.get(documentId)) == null


Comment here

edaniels · 2018-10-31T15:50:33Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+        final BsonDocument beforeDocument;
+        final BsonValue documentId = BsonUtils.getDocumentId(afterDocument);
+
+        if ((beforeDocument = idToBeforeDocumentMap.get(documentId)) == null


[nit] Assigning to beforeDocument like this feels kind of odd as opposed to just doing it right before the if

… upsert tests

adamchel

excellent work! I mostly just have nits and minors but I think this will be LGTM once you address my comments

adamchel · 2018-10-31T17:57:27Z

...es/mongodb-remote/src/main/java/com/mongodb/stitch/android/services/mongodb/remote/Sync.java

  boolean resumeSyncForDocument(@NonNull final BsonValue documentId);

+  /**
+   * Counts the number of documents in the collection.


[nit] should the comment be "Counts the number of documents in the collection that are synchronized with the remote collection"?

adamchel · 2018-10-31T17:58:30Z

...es/mongodb-remote/src/main/java/com/mongodb/stitch/android/services/mongodb/remote/Sync.java

+  Task<Long> count();
+
+  /**
+   * Counts the number of documents in the collection that have been synchronized from the remote


[nit] I think synchronized "with" remote would be more accurate than synchronized "from", since this may count local documents that haven't been synchronized to the remote, and therefore have never been synchronized "from" the remote

adamchel · 2018-10-31T17:58:40Z

...es/mongodb-remote/src/main/java/com/mongodb/stitch/android/services/mongodb/remote/Sync.java

+  Task<Long> count(final Bson filter);
+
+  /**
+   * Counts the number of documents in the collection that have been synchronized from the remote


[nit] ditto

and pretty much everywhere in this file, "synchronized from" should become "synchronized with". not going to comment on this anymore

adamchel · 2018-10-31T18:00:27Z

...es/mongodb-remote/src/main/java/com/mongodb/stitch/android/services/mongodb/remote/Sync.java

+      final Class<ResultT> resultClass);
+
+  /**
+   * Inserts the provided document. If the document is missing an identifier, the client should


[nit] it should generate one, but will it? if it is going to generate one it should say "If the document is missing an identifier, one will be generated."

adamchel · 2018-10-31T18:17:20Z

...n/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/AggregateOperation.java

+
+import org.bson.conversions.Bson;
+
+class AggregateOperation<T> implements Operation<Collection<T>> {


[minor] shouldn't this class be a SyncAggregateOperation? AggregateOperation already exists in another package which might get confusing. this also would make this have parity with the existing SyncFindOperation

Discussed offline. Because these are package private, we are going to keep these as is.

SyncFindOperation will be whittled down to FindOperation as well.

adamchel · 2018-10-31T19:03:22Z

core/testutils/src/main/java/com/mongodb/stitch/core/testutils/sync/ProxySyncMethods.kt

+    fun insertManyAndSync(documents: List<Document>): SyncInsertManyResult

    /**
     * Deletes a single document by the given id. It is first searched for in the local synchronized


[nit] this comment looks out of date. honestly this file probably doesn't need doc comments for the CRUD methods, this is pretty internal

adamchel · 2018-10-31T19:10:26Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

-      return UpdateResult.acknowledged(0, 0L, null);
+  void insertManyAndSync(final MongoNamespace namespace,
+                         final List<BsonDocument> documents) {
+    getLocalCollection(namespace).insertMany(documents);


[alternatively] instead of generating IDs client-side, and depending on BsonUtils.getDocumentId not failing we could look at the result of the local insertMany (and insertOne in insertOneAndSync), and use the inserted ids there to update the doc config and emit the event

Should note that there are no results from local inserts.

The local client does not return a result.

my mistake, I assumed it returned a an insertedId like Stitch

adamchel · 2018-10-31T19:14:32Z

...ain/java/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizer.java

+        filter,
+        update,
+        new FindOneAndUpdateOptions()
+            .collation(updateOptions.getCollation())


[minor] collation, bypassDocumentValidation, and arrayFilters are all unsupported by the remote MongoDB service so this might result in conflicts between local and remote where a document that is able to inserted locally is not able to be inserted remotely. we could either not support those options at all, or just do this with the assumption that we will eventually support the full mongodb surface area

There's no actual way for these to get passed downstream, if you look through the proxies– we do not actually support the options. But we seem to operate within the DataSynchronizer on localmdb without considering Remote.

adamchel · 2018-10-31T19:26:46Z

...a/com/mongodb/stitch/core/services/mongodb/remote/sync/internal/DataSynchronizerUnitTests.kt


    @Test
-    fun testDeleteOneById() {
+    fun testUpsertOne() {


[nit] one other test that might be worth adding is to do an update where upsert is specified, but there are documents that match, and we want to verify that no insert occurs

I believe that's covered here– but I can add another check to make it clearer.

adamchel · 2018-10-31T19:30:39Z

core/testutils/src/main/java/com/mongodb/stitch/core/testutils/sync/SyncIntTestProxy.kt

+    }
+
+    @Test
+    fun testDeleteManyNoConflicts() {


[nit] might be worth throwing in an upsert test here

dkaminsky · 2018-10-31T20:36:16Z

.../java/com/mongodb/stitch/android/services/mongodb/remote/internal/SyncMongoClientIntTests.kt


-        override fun insertOneAndSync(document: Document): RemoteInsertOneResult {
+        override fun count(filter: Bson): Long {
+            return Tasks.await(sync.count())


Even if not used currently, expected behavior would be for this to propagate the filter.

Suggested change

return Tasks.await(sync.count())

return Tasks.await(sync.count(filter))

jsflax · 2018-11-01T12:20:30Z

@adamchel PTAL

adamchel

LGTM

jsflax added 10 commits October 30, 2018 16:28

Add and/or replace CRUD methods; begin testing

17f2d43

DataSync reformat

9add837

Reformat

e069aec

Figuring out formatting

457971b

More formatting

2c428ec

Need to reformat

26841cd

More formatting

a2baa49

More cleanup

c8f8d7c

More formatting issues

cb41898

Write tests for datasync, coresync, int; clean up; fix bulk update lo…

bbb3c1a

…gic; add count op; clean up

jsflax requested review from edaniels and adamchel October 31, 2018 00:25

edaniels approved these changes Oct 31, 2018

View reviewed changes

jsflax added 2 commits October 31, 2018 17:11

Fix race in upsertMany; add check for triggerListeneredToNamespace in…

fb1c618

… upsert tests

Fix aggressive auto-refactor

baf70ed

adamchel suggested changes Oct 31, 2018

View reviewed changes

jsflax requested a review from dkaminsky October 31, 2018 20:29

dkaminsky reviewed Oct 31, 2018

View reviewed changes

jsflax added 2 commits November 1, 2018 11:49

Added custom codec tests; address remarks; add extra upsert check

ddcf6d6

Appease linter

c62568c

adamchel approved these changes Nov 1, 2018

View reviewed changes

jsflax merged commit ee20789 into mongodb:master Nov 1, 2018


		import org.bson.conversions.Bson;

		class AggregateOperation<T> implements Operation<Collection<T>> {

	return Tasks.await(sync.count())
	return Tasks.await(sync.count(filter))

STITCH-1967 Add full CRUD within .sync() #74

STITCH-1967 Add full CRUD within .sync() #74

Uh oh!

Conversation

jsflax commented Oct 31, 2018

Uh oh!

jsflax commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsflax commented Oct 31, 2018

Uh oh!

edaniels left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamchel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jsflax commented Oct 31, 2018 •

edited

Loading