Store the coerced base value in the BaseMutation (#3292)

Author: schmidt-sebastian

Firestore/CHANGELOG.md

@@ -1,4 +1,7 @@
 # Unreleased
+- [fixed] Fixed an internal assertion that was triggered when an update
+   with a `FieldValue.serverTimestamp()` and an update with a
+  `FieldValue.increment()` were pending for the same document.
 
 # 1.4.1
 - [fixed] Fixed certificate loading for non-CocoaPods builds that may not

Firestore/Example/Firestore.xcodeproj/project.pbxproj

@@ -62,7 +62,6 @@
 		12158DFCEE09D24B7988A340 /* maybe_document.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 618BBE7E20B89AAC00B5BCE7 /* maybe_document.pb.cc */; };
 		1235769322B7E99F007DDFA9 /* EncodableFieldValueTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1235769122B7E915007DDFA9 /* EncodableFieldValueTests.swift */; };
 		1235769522B86E65007DDFA9 /* FirestoreEncoderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1235769422B86E65007DDFA9 /* FirestoreEncoderTests.swift */; };
-		124C933122C16ACB00CA8C2D /* CodableIntegrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 124C932B22C1642C00CA8C2D /* CodableIntegrationTests.swift */; };
 		127CC0D222B3ADDC00A3E42A /* CodableTimestampTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7B65C996438B84DBC7616640 /* CodableTimestampTests.swift */; };
 		1291D9F5300AFACD1FBD262D /* array_sorted_map_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54EB764C202277B30088B8F3 /* array_sorted_map_test.cc */; };
 		12BB9ED1CA98AA52B92F497B /* log_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54C2294E1FECABAE007D065B /* log_test.cc */; };
@@ -1412,9 +1411,9 @@
 		54C9EDF22040E16300A969CD /* SwiftTests */ = {
 			isa = PBXGroup;
 			children = (
-				124C932A22C1635300CA8C2D /* Integration */,
 				544A20ED20F6C046004E52CD /* API */,
 				5495EB012040E90200EBA509 /* Codable */,
+				124C932A22C1635300CA8C2D /* Integration */,
 				620C1427763BA5D3CCFB5A1F /* BridgingHeader.h */,
 				54C9EDF52040E16300A969CD /* Info.plist */,
 			);
@@ -3652,7 +3651,6 @@
 				D5B252EE3F4037405DB1ECE3 /* FIRNumericTransformTests.mm in Sources */,
 				5492E072202154D600B64F25 /* FIRQueryTests.mm in Sources */,
 				5492E077202154D600B64F25 /* FIRServerTimestampTests.mm in Sources */,
-				124C933122C16ACB00CA8C2D /* CodableIntegrationTests.swift in Sources */,
 				5492E07A202154D600B64F25 /* FIRTypeTests.mm in Sources */,
 				5492E076202154D600B64F25 /* FIRValidationTests.mm in Sources */,
 				5492E078202154D600B64F25 /* FIRWriteBatchTests.mm in Sources */,

Firestore/Example/Firestore.xcodeproj/xcshareddata/xcschemes/Firestore_Tests_macOS.xcscheme

@@ -5,6 +5,18 @@
    <BuildAction
       parallelizeBuildables = "YES"
       buildImplicitDependencies = "YES">
+      <BuildActionEntries>
+         <BuildActionEntry
+            buildForRunning = "YES">
+            <BuildableReference
+               BuildableIdentifier = "primary"
+               BlueprintIdentifier = "DAFF0CF421E64AC30062958F"
+               BuildableName = "Firestore_Example_macOS.app"
+               BlueprintName = "Firestore_Example_macOS"
+               ReferencedContainer = "container:Firestore.xcodeproj">
+            </BuildableReference>
+         </BuildActionEntry>
+      </BuildActionEntries>
    </BuildAction>
    <TestAction
       buildConfiguration = "Debug"

Firestore/Example/Tests/Integration/API/FIRNumericTransformTests.mm

@@ -136,6 +136,63 @@ - (void)testDoubleIncrementWithExistingString {
   [self expectApproximateLocalAndRemoteValue:13.37];
 }
 
+- (void)testIncrementTwiceInABatch {
+  [self writeInitialData:@{@"sum" : @"overwrite"}];
+
+  FIRWriteBatch *batch = _docRef.firestore.batch;
+  [batch updateData:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}
+        forDocument:_docRef];
+  [batch updateData:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}
+        forDocument:_docRef];
+  [batch
+      commitWithCompletion:[self completionForExpectationWithName:@"testIncrementTwiceInABatch"]];
+  [self awaitExpectations];
+
+  [self expectApproximateLocalAndRemoteValue:2];
+}
+
+- (void)testIncrementDeleteIncrementInABatch {
+  [self writeInitialData:@{@"sum" : @"overwrite"}];
+
+  FIRWriteBatch *batch = _docRef.firestore.batch;
+  [batch updateData:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:1]}
+        forDocument:_docRef];
+  [batch updateData:@{@"sum" : [FIRFieldValue fieldValueForDelete]} forDocument:_docRef];
+  [batch updateData:@{@"sum" : [FIRFieldValue fieldValueForIntegerIncrement:3]}
+        forDocument:_docRef];
+  [batch commitWithCompletion:
+             [self completionForExpectationWithName:@"testIncrementDeleteIncrementInABatch"]];
+  [self awaitExpectations];
+
+  [self expectApproximateLocalAndRemoteValue:3];
+}
+
+- (void)testServerTimestampAndIncrement {
+  // This test stacks two pending transforms (a ServerTimestamp and an Increment transform)
+  // and reproduces the setup that was reported in
+  // https://github.com/firebase/firebase-android-sdk/issues/491
+  // In our original code, a NumericIncrementTransform could cause us to decode the
+  // ServerTimestamp as part of a FSTPatchMutation, which triggered an assertion failure.
+  [self writeInitialData:@{@"val" : @"overwrite"}];
+
+  [self disableNetwork];
+
+  [_docRef updateData:@{@"val" : [FIRFieldValue fieldValueForServerTimestamp]}];
+  [_docRef updateData:@{@"val" : [FIRFieldValue fieldValueForIntegerIncrement:1]}];
+
+  FIRDocumentSnapshot *snap = [_accumulator awaitLocalEvent];
+  XCTAssertNotNil([snap valueForField:@"val"
+              serverTimestampBehavior:FIRServerTimestampBehaviorEstimate]);
+
+  snap = [_accumulator awaitLocalEvent];
+  XCTAssertEqualObjects(@1, snap[@"val"]);
+
+  [self enableNetwork];
+
+  snap = [_accumulator awaitRemoteEvent];
+  XCTAssertEqualObjects(@1, snap[@"val"]);
+}
+
 - (void)testMultipleDoubleIncrements {
   [self writeInitialData:@{@"sum" : @"0.0"}];
 

Firestore/Example/Tests/Integration/API/FIRServerTimestampTests.mm

@@ -171,6 +171,12 @@ - (void)testServerTimestampsWithEstimatedValue {
 }
 
 - (void)testServerTimestampsWithPreviousValue {
+  // The following test includes an update of the nested map "deep", which updates it to contain
+  // a single ServerTimestamp. This update is split into two mutations: One that sets "deep" to
+  // an empty map and overwrites the previous ServerTimestamp value and a second transform that
+  // writes the new ServerTimestamp. This step in the test verifies that we can still access the
+  // old ServerTimestamp value (from `previousSnapshot`) even though it was removed in an
+  // intermediate step.
   [self writeDocumentRef:_docRef data:_setData];
   [self verifyTimestampsInSnapshot:[_accumulator awaitLocalEvent] fromPreviousSnapshot:nil];
   FIRDocumentSnapshot *remoteSnapshot = [_accumulator awaitRemoteEvent];

Firestore/Example/Tests/Model/FSTMutationTests.mm

@@ -529,6 +529,73 @@ - (void)testPatchWithMutationResult {
                                                DocumentState::kCommittedMutations));
 }
 
+- (void)testNonTransformMutationBaseValue {
+  NSDictionary *docData = @{@"foo" : @"foo"};
+  FSTDocument *baseDoc = FSTTestDoc("collection/key", 0, docData, DocumentState::kSynced);
+
+  FSTMutation *set = FSTTestSetMutation(@"collection/key", @{@"foo" : @"bar"});
+  XCTAssertFalse([set extractBaseValue:baseDoc]);
+
+  FSTMutation *patch = FSTTestPatchMutation("collection/key", @{@"foo" : @"bar"}, {});
+  XCTAssertFalse([patch extractBaseValue:baseDoc]);
+
+  FSTMutation *deleter = FSTTestDeleteMutation(@"collection/key");
+  XCTAssertFalse([deleter extractBaseValue:baseDoc]);
+}
+
+- (void)testServerTimestampBaseValue {
+  NSDictionary *docData = @{@"time" : @"foo", @"nested" : @{@"time" : @"foo"}};
+  FSTDocument *baseDoc = FSTTestDoc("collection/key", 0, docData, DocumentState::kSynced);
+
+  FSTMutation *transform = FSTTestTransformMutation(@"collection/key", @{
+    @"time" : [FIRFieldValue fieldValueForServerTimestamp],
+    @"nested.time" : [FIRFieldValue fieldValueForServerTimestamp]
+  });
+
+  // Server timestamps are idempotent and don't require base values.
+  XCTAssertFalse([transform extractBaseValue:baseDoc]);
+}
+
+- (void)testNumericIncrementBaseValue {
+  NSDictionary *docData = @{
+    @"ignore" : @"foo",
+    @"double" : @42.0,
+    @"long" : @42,
+    @"string" : @"foo",
+    @"map" : @{},
+    @"nested" :
+        @{@"ignore" : @"foo", @"double" : @42.0, @"long" : @42, @"string" : @"foo", @"map" : @{}}
+  };
+  FSTDocument *baseDoc = FSTTestDoc("collection/key", 0, docData, DocumentState::kSynced);
+
+  FSTMutation *transform = FSTTestTransformMutation(@"collection/key", @{
+    @"double" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"long" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"string" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"map" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"missing" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"nested.double" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"nested.long" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"nested.string" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"nested.map" : [FIRFieldValue fieldValueForIntegerIncrement:1],
+    @"nested.missing" : [FIRFieldValue fieldValueForIntegerIncrement:1]
+  });
+
+  ObjectValue expectedBaseValue = FSTTestObjectValue(@{
+    @"double" : @42.0,
+    @"long" : @42,
+    @"string" : @0,
+    @"map" : @0,
+    @"missing" : @0,
+    @"nested" : @{@"double" : @42.0, @"long" : @42, @"string" : @0, @"map" : @0, @"missing" : @0}
+  });
+
+  // Server timestamps are idempotent and don't require base values.
+  absl::optional<ObjectValue> actualBaseValue = [transform extractBaseValue:baseDoc];
+  XCTAssertTrue([transform extractBaseValue:baseDoc]);
+  XCTAssertEqual(expectedBaseValue, *actualBaseValue);
+}
+
 #define ASSERT_VERSION_TRANSITION(mutation, base, result, expected)                         \
   do {                                                                                      \
     FSTMaybeDocument *actual = [mutation applyToRemoteDocument:base mutationResult:result]; \

Firestore/Example/Tests/Model/transform_operations_test.mm

@@ -42,8 +42,9 @@ FieldValue ApplyToRemoteDocument(const absl::optional<model::FieldValue>& /* pre
     return FieldValue::Null();
   }
 
-  bool idempotent() const override {
-    return true;
+  absl::optional<model::FieldValue> ComputeBaseValue(
+      const absl::optional<model::FieldValue>& previous_value) const override {
+    return absl::nullopt;
   }
 
   bool operator==(const TransformOperation& other) const override {

Firestore/Source/Local/FSTLocalStore.mm

@@ -49,6 +49,7 @@
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/log.h"
 #include "absl/memory/memory.h"
+#include "absl/types/optional.h"
 
 using firebase::Timestamp;
 using firebase::firestore::auth::User;
@@ -192,29 +193,17 @@ - (FSTLocalWriteResult *)locallyWriteMutations:(std::vector<FSTMutation *> &&)mu
     // transform.
     std::vector<FSTMutation *> baseMutations;
     for (FSTMutation *mutation : mutations) {
-      if (mutation.idempotent) {
-        continue;
-      }
+      auto base_document_it = existingDocuments.find(mutation.key);
+      FSTMaybeDocument *base_document =
+          base_document_it != existingDocuments.end() ? base_document_it->second : nil;
 
-      // Theoretically, we should only include non-idempotent fields in this field mask as this mask
-      // is used to prevent flicker for non-idempotent transforms by providing consistent base
-      // values. By including the fields for all DocumentTransforms, we incorrectly prevent rebasing
-      // of idempotent transforms (such as `arrayUnion()`) when any non-idempotent transforms are
-      // present.
-      // TODO(mrschmidt): Expose a method that only returns the a field mask for non-idempotent
-      // transforms
-      const FieldMask *fieldMask = [mutation fieldMask];
-      if (fieldMask) {
-        // `documentsForKeys` is guaranteed to return a (nullable) entry for every document key.
-        FSTMaybeDocument *maybeDocument = existingDocuments.find(mutation.key)->second;
-        ObjectValue baseValues = [maybeDocument isKindOfClass:[FSTDocument class]]
-                                     ? fieldMask->ApplyTo(((FSTDocument *)maybeDocument).data)
-                                     : ObjectValue::Empty();
+      absl::optional<ObjectValue> base_value = [mutation extractBaseValue:base_document];
+      if (base_value) {
         // NOTE: The base state should only be applied if there's some existing document to
         // override, so use a Precondition of exists=true
         baseMutations.push_back([[FSTPatchMutation alloc] initWithKey:mutation.key
-                                                            fieldMask:*fieldMask
-                                                                value:std::move(baseValues)
+                                                            fieldMask:base_value->ToFieldMask()
+                                                                value:*base_value
                                                          precondition:Precondition::Exists(true)]);
       }
     }

Firestore/Source/Model/FSTMutation.h

@@ -154,19 +154,25 @@ NS_ASSUME_NONNULL_BEGIN
                                        baseDocument:(nullable FSTMaybeDocument *)baseDoc
                                      localWriteTime:(const firebase::Timestamp &)localWriteTime;
 
-- (const model::DocumentKey &)key;
-
-- (const model::Precondition &)precondition;
-
 /**
- * If applicable, returns the field mask for this mutation. Fields that are not included in this
- * field mask are not modified when this mutation is applied. Mutations that replace all document
- * values return 'nullptr'.
+ * If this mutation is not idempotent, returns the base value to persist with this mutation.
+ * If a base value is returned, the mutation is always applied to this base value, even if
+ * document has already been updated.
+ *
+ * The base value is a sparse object that consists of only the document fields for which this
+ * mutation contains a non-idempotent transformation (e.g. a numeric increment). The provided
+ * value guarantees consistent behavior for non-idempotent transforms and allow us to return the
+ * same latency-compensated value even if the backend has already applied the mutation. The base
+ * value is empty for idempotent mutations, as they can be re-played even if the backend has
+ * already applied them.
+ *
+ * @return a base value to store along with the mutation, or empty for idempotent mutations.
  */
-- (nullable const model::FieldMask *)fieldMask;
+- (absl::optional<model::ObjectValue>)extractBaseValue:(nullable FSTMaybeDocument *)maybeDoc;
+
+- (const model::DocumentKey &)key;
 
-/** Returns whether all operations in the mutation are idempotent. */
-@property(nonatomic, readonly) BOOL idempotent;
+- (const model::Precondition &)precondition;
 
 @end