Don't persist documents that we already have #720

schmidt-sebastian · 2019-08-20T20:37:45Z

Port of firebase/firebase-js-sdk#2099.

Also pulls in spec test changes from firebase/firebase-js-sdk#2098

schmidt-sebastian · 2019-08-20T21:38:45Z

firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

@@ -395,8 +396,16 @@ public static RemoteEvent noChangeEvent(int targetId, int version, ByteString re

  public static RemoteEvent addedRemoteEvent(
      MaybeDocument doc, List<Integer> updatedInTargets, List<Integer> removedFromTargets) {
-    DocumentChange change =
-        new DocumentChange(updatedInTargets, removedFromTargets, doc.getKey(), doc);
+    return addedRemoteEvent(Collections.singletonList(doc), updatedInTargets, removedFromTargets);


Note: the changes here are also part of https://github.com/firebase/firebase-android-sdk/pull/697/files#diff-ac4f307b0804f62b9719e8c7556226c2L396

var-const · 2019-08-21T19:14:35Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

@@ -329,50 +329,35 @@ public SnapshotVersion getLastRemoteSnapshotVersion() {
    return persistence.runTransaction(
        "Apply remote event",
        () -> {
+          SnapshotVersion remoteVersion = remoteEvent.getSnapshotVersion();


Question: this variable is now much further from its first point of use, and I didn't notice any references to it in-between. Perhaps move it back to the end of this function?

It's used in line 354. I moved it to the top of the function body - let me know if you think that's better. I can otherwise also inline the usage in line 354.

var-const · 2019-08-21T19:15:56Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

-              if (shouldPersistQueryData(oldQueryData, queryData, change)) {
-                queryCache.updateQueryData(queryData);
+            QueryData oldQueryData = targetIds.get(targetId);
+            if (oldQueryData != null) {


Optional: I personally liked the continue approach that was in place before (this function is long, and I feel the increased nestedness hurts readability).

Sounds good. Reverted.

var-const · 2019-08-21T19:17:47Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

+              queryCache.removeMatchingKeys(change.getRemovedDocuments(), targetId);
+              queryCache.addMatchingKeys(change.getAddedDocuments(), targetId);
+
+              // Update the resume token if the change includes one. Don't clear any preexisting


Question: which code is responsible for Don't clear any preexisting value -- is it shouldPersistQueryData?

Yes. I removed that part of the comment since it is a bit confusing.

var-const · 2019-08-21T19:18:57Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

          for (Map.Entry<Integer, TargetChange> entry : targetChanges.entrySet()) {
            Integer boxedTargetId = entry.getKey();
            int targetId = boxedTargetId;
            TargetChange change = entry.getValue();

-            // Do not ref/unref unassigned targetIds - it may lead to leaks.


Does this comment no longer apply?

I rewrote it below since I didn't quite understand it at first.

Ah, cool. I also found the original comment confusing.

var-const · 2019-08-21T19:20:24Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

-              // version to the remote cache.
+                || doc.getVersion().compareTo(existingDoc.getVersion()) > 0
+                || (doc.getVersion().compareTo(existingDoc.getVersion()) == 0
+                    && existingDoc.hasPendingWrites())) {


Hmm, why do we overwrite the existing doc when it has pending writes?

This is the only important question, the rest are nitpicks.

When the write stream sends us an acknowledgment, we persist a document with hasCommittedMutations. We then pretend we know the state of the document, but that is not always true (another client may have changed a field we didn't write to, or we supplied an ArrayUnion/ArrayRemove transform that was applied to a set of elements that the client didn't yet know about). By allowing the server to send us the state of a committed document, we overwrite the hasCommittedMutations version with the actual version from the backend.

Thanks for the explanation!

var-const · 2019-08-21T19:30:37Z

firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

+    Preconditions.checkArgument(!docs.isEmpty(), "Cannot pass empty docs array");
+
+    ResourcePath collectionPath = docs.get(0).getKey().getPath().popLast();
+    SnapshotVersion version = docs.get(0).getVersion();


Optional: move this declaration to the point right before it's used?

var-const · 2019-08-21T19:31:58Z

firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

@@ -407,11 +416,16 @@ public static RemoteEvent addedRemoteEvent(

              @Override
              public QueryData getQueryDataForTarget(int targetId) {
-                return queryData(targetId, QueryPurpose.LISTEN, doc.getKey().toString());
+                return queryData(targetId, QueryPurpose.LISTEN, collectionPath.toString());


Question: what's the reason for this change?

The new test passes multiple docs to addedRemoteEvent. The test helper now uses the collection path of the first doc to represent the query.

FWIW I also believe the original implementation should have returned a collection path here.

google-oss-bot · 2019-08-21T20:36:04Z

The public api surface has changed for the subproject firebase-firestore:
error: Removed class com.google.firebase.firestore.FirebaseFirestore.InstanceRegistry [RemovedInterface]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

var-const · 2019-08-21T20:38:53Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

          for (Map.Entry<Integer, TargetChange> entry : targetChanges.entrySet()) {
            Integer boxedTargetId = entry.getKey();
            int targetId = boxedTargetId;
            TargetChange change = entry.getValue();

-            // Do not ref/unref unassigned targetIds - it may lead to leaks.


Ah, cool. I also found the original comment confusing.

Don't persist documents that we already have

eb848d2

googlebot added the cla: yes Override cla label Aug 20, 2019

schmidt-sebastian requested a review from var-const August 20, 2019 20:37

google-oss-bot added the size/L label Aug 20, 2019

schmidt-sebastian assigned var-const Aug 20, 2019

schmidt-sebastian added 2 commits August 20, 2019 13:46

Add unit test

dd257b0

Add additional assert

a465132

schmidt-sebastian commented Aug 20, 2019

View reviewed changes

var-const suggested changes Aug 21, 2019

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Aug 21, 2019

Feedback

67176b3

schmidt-sebastian assigned var-const and unassigned schmidt-sebastian Aug 21, 2019

var-const approved these changes Aug 21, 2019

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Aug 21, 2019

schmidt-sebastian merged commit a1d363b into master Aug 21, 2019

schmidt-sebastian deleted the mrschmidt/noupdate branch August 21, 2019 22:38

schmidt-sebastian mentioned this pull request Aug 22, 2019

Don't persist documents that we already have firebase/firebase-ios-sdk#3650

Merged

firebase locked and limited conversation to collaborators Oct 8, 2019

Don't persist documents that we already have #720

Don't persist documents that we already have #720

Uh oh!

Conversation

schmidt-sebastian commented Aug 20, 2019

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!

google-oss-bot commented Aug 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!