Integration tests, changelog, and minor fixes for array transforms. (#1108)

9 files changed, 381 insertions, 10 deletions

diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md
index e17cf56..9f53593 100644
--- a/Firestore/CHANGELOG.md
+++ b/Firestore/CHANGELOG.md
@@ -1,4 +1,8 @@
 # Unreleased
+- [feature] Added FieldValue.arrayUnion() and FieldValue.arrayRemove() methods
+  which can be used inside setData() or updateData() calls to atomically add
+  or remove specific elements to an array field in a document without using a
+  transaction.
 - [changed] Replaced the `DocumentListenOptions` object with a simple boolean.
   Instead of calling
   `addSnapshotListener(options: DocumentListenOptions.includeMetadataChanges(true))`
diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj
index de4cf17..d20dc48 100644
--- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj
+++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj
@@ -134,6 +134,7 @@
 		6ED54761B845349D43DB6B78 /* Pods_Firestore_Example.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 75A6FE51C1A02DF38F62FAAD /* Pods_Firestore_Example.framework */; };
 		71719F9F1E33DC2100824A3D /* LaunchScreen.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 71719F9D1E33DC2100824A3D /* LaunchScreen.storyboard */; };
 		7346E61D20325C6900FD6CEF /* FSTDispatchQueueTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 7346E61C20325C6900FD6CEF /* FSTDispatchQueueTests.mm */; };
+		73866AA12082B0A5009BB4FF /* FIRArrayTransformTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 73866A9F2082B069009BB4FF /* FIRArrayTransformTests.mm */; };
 		873B8AEB1B1F5CCA007FD442 /* Main.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 873B8AEA1B1F5CCA007FD442 /* Main.storyboard */; };
 		AB356EF7200EA5EB0089B766 /* field_value_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB356EF6200EA5EB0089B766 /* field_value_test.cc */; };
 		AB380CFB2019388600D97691 /* target_id_generator_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB380CF82019382300D97691 /* target_id_generator_test.cc */; };
@@ -358,6 +359,7 @@
 		69F6A10DBD6187489481CD76 /* Pods_Firestore_Tests.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Tests.framework; sourceTree = BUILT_PRODUCTS_DIR; };
 		71719F9E1E33DC2100824A3D /* Base */ = {isa = PBXFileReference; lastKnownFileType = file.storyboard; name = Base; path = Base.lproj/LaunchScreen.storyboard; sourceTree = "<group>"; };
 		7346E61C20325C6900FD6CEF /* FSTDispatchQueueTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FSTDispatchQueueTests.mm; sourceTree = "<group>"; };
+		73866A9F2082B069009BB4FF /* FIRArrayTransformTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FIRArrayTransformTests.mm; sourceTree = "<group>"; };
 		75A6FE51C1A02DF38F62FAAD /* Pods_Firestore_Example.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Example.framework; sourceTree = BUILT_PRODUCTS_DIR; };
 		873B8AEA1B1F5CCA007FD442 /* Main.storyboard */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.storyboard; name = Main.storyboard; path = Base.lproj/Main.storyboard; sourceTree = "<group>"; };
 		8E002F4AD5D9B6197C940847 /* Firestore.podspec */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text; name = Firestore.podspec; path = ../Firestore.podspec; sourceTree = "<group>"; };
@@ -882,6 +884,7 @@
 		DE51B1BC1F0D48AC0013853F /* API */ = {
 			isa = PBXGroup;
 			children = (
+				73866A9F2082B069009BB4FF /* FIRArrayTransformTests.mm */,
 				5492E070202154D600B64F25 /* FIRCursorTests.mm */,
 				5492E06C202154D500B64F25 /* FIRDatabaseTests.mm */,
 				5492E06A202154D500B64F25 /* FIRFieldsTests.mm */,
@@ -1555,6 +1558,7 @@
 			isa = PBXSourcesBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
+				73866AA12082B0A5009BB4FF /* FIRArrayTransformTests.mm in Sources */,
 				5492E076202154D600B64F25 /* FIRValidationTests.mm in Sources */,
 				5492E072202154D600B64F25 /* FIRQueryTests.mm in Sources */,
 				5491BC731FB44593008B3588 /* FSTIntegrationTestCase.mm in Sources */,
diff --git a/Firestore/Example/Tests/Integration/API/FIRArrayTransformTests.mm b/Firestore/Example/Tests/Integration/API/FIRArrayTransformTests.mm
new file mode 100644
index 0000000..5f0cf92
--- /dev/null
+++ b/Firestore/Example/Tests/Integration/API/FIRArrayTransformTests.mm
@@ -0,0 +1,289 @@
+/*
+ * Copyright 2018 Google
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#import <FirebaseFirestore/FirebaseFirestore.h>
+
+#import <XCTest/XCTest.h>
+
+#import "Firestore/Example/Tests/Util/FSTEventAccumulator.h"
+#import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"
+
+/**
+ * Note: Transforms are tested pretty thoroughly in FIRServerTimestampTests (via set, update,
+ * transactions, nested in documents, multiple transforms together, etc.) and so these tests
+ * mostly focus on the array transform semantics.
+ */
+@interface FIRArrayTransformTests : FSTIntegrationTestCase
+@end
+
+@implementation FIRArrayTransformTests {
+  // A document reference to read and write to.
+  FIRDocumentReference *_docRef;
+
+  // Accumulator used to capture events during the test.
+  FSTEventAccumulator *_accumulator;
+
+  // Listener registration for a listener maintained during the course of the test.
+  id<FIRListenerRegistration> _listenerRegistration;
+}
+
+- (void)setUp {
+  [super setUp];
+
+  _docRef = [self documentRef];
+  _accumulator = [FSTEventAccumulator accumulatorForTest:self];
+  _listenerRegistration =
+      [_docRef addSnapshotListenerWithIncludeMetadataChanges:YES
+                                                    listener:_accumulator.valueEventHandler];
+
+  // Wait for initial nil snapshot to avoid potential races.
+  FIRDocumentSnapshot *initialSnapshot = [_accumulator awaitEventWithName:@"initial event"];
+  XCTAssertFalse(initialSnapshot.exists);
+}
+
+- (void)tearDown {
+  [_listenerRegistration remove];
+
+  [super tearDown];
+}
+
+#pragma mark - Test Helpers
+
+/** Waits for a snapshot with local writes. */
+- (FIRDocumentSnapshot *)waitForLocalEvent {
+  FIRDocumentSnapshot *snapshot;
+  do {
+    snapshot = [_accumulator awaitEventWithName:@"Local event."];
+  } while (!snapshot.metadata.hasPendingWrites);
+  return snapshot;
+}
+
+/** Waits for a snapshot that has no pending writes */
+- (FIRDocumentSnapshot *)waitForRemoteEvent {
+  FIRDocumentSnapshot *snapshot;
+  do {
+    snapshot = [_accumulator awaitEventWithName:@"Remote event."];
+  } while (snapshot.metadata.hasPendingWrites);
+  return snapshot;
+}
+
+/** Writes some initial data and consumes the events generated. */
+- (void)writeInitialData:(NSDictionary<NSString *, id> *)data {
+  [self writeDocumentRef:_docRef data:data];
+  XCTAssertEqualObjects([self waitForLocalEvent].data, data);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, data);
+}
+
+#pragma mark - Test Cases
+
+- (void)testCreateDocumentWithArrayUnion {
+  [self writeDocumentRef:_docRef
+                    data:@{
+                      @"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @2 ]]
+                    }];
+  id expected = @{ @"array" : @[ @1, @2 ] };
+  XCTAssertEqualObjects([self waitForLocalEvent].data, expected);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, expected);
+}
+
+- (void)testAppendToArrayViaUpdate {
+  [self writeInitialData:@{ @"array" : @[ @1, @3 ] }];
+
+  [self updateDocumentRef:_docRef
+                     data:@{
+                       @"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @2, @1, @4 ]]
+                     }];
+
+  id expected = @{ @"array" : @[ @1, @3, @2, @4 ] };
+  XCTAssertEqualObjects([self waitForLocalEvent].data, expected);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, expected);
+}
+
+- (void)testAppendToArrayViaMergeSet {
+  [self writeInitialData:@{ @"array" : @[ @1, @3 ] }];
+
+  [self mergeDocumentRef:_docRef
+                    data:@{
+                      @"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @2, @1, @4 ]]
+                    }];
+
+  id expected = @{ @"array" : @[ @1, @3, @2, @4 ] };
+  XCTAssertEqualObjects([self waitForLocalEvent].data, expected);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, expected);
+}
+
+- (void)testAppendObjectToArrayViaUpdate {
+  [self writeInitialData:@{ @"array" : @[ @{@"a" : @"hi"} ] }];
+
+  [self updateDocumentRef:_docRef
+                     data:@{
+                       @"array" : [FIRFieldValue
+                           fieldValueForArrayUnion:@[ @{@"a" : @"hi"}, @{@"a" : @"bye"} ]]
+                     }];
+
+  id expected = @{ @"array" : @[ @{@"a" : @"hi"}, @{@"a" : @"bye"} ] };
+  XCTAssertEqualObjects([self waitForLocalEvent].data, expected);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, expected);
+}
+
+- (void)testRemoveFromArrayViaUpdate {
+  [self writeInitialData:@{ @"array" : @[ @1, @3, @1, @3 ] }];
+
+  [self updateDocumentRef:_docRef
+                     data:@{
+                       @"array" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, @4 ]]
+                     }];
+
+  id expected = @{ @"array" : @[ @3, @3 ] };
+  XCTAssertEqualObjects([self waitForLocalEvent].data, expected);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, expected);
+}
+
+- (void)testRemoveFromArrayViaMergeSet {
+  [self writeInitialData:@{ @"array" : @[ @1, @3, @1, @3 ] }];
+
+  [self mergeDocumentRef:_docRef
+                    data:@{
+                      @"array" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, @4 ]]
+                    }];
+
+  id expected = @{ @"array" : @[ @3, @3 ] };
+  XCTAssertEqualObjects([self waitForLocalEvent].data, expected);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, expected);
+}
+
+- (void)testRemoveObjectFromArrayViaUpdate {
+  [self writeInitialData:@{ @"array" : @[ @{@"a" : @"hi"}, @{@"a" : @"bye"} ] }];
+
+  [self updateDocumentRef:_docRef
+                     data:@{
+                       @"array" : [FIRFieldValue fieldValueForArrayRemove:@[ @{@"a" : @"hi"} ]]
+                     }];
+
+  id expected = @{ @"array" : @[ @{@"a" : @"bye"} ] };
+  XCTAssertEqualObjects([self waitForLocalEvent].data, expected);
+  XCTAssertEqualObjects([self waitForRemoteEvent].data, expected);
+}
+
+@end
+
+/**
+ * Unlike the FIRArrayTransformTests above, these tests intentionally avoid having any ongoing
+ * listeners so that we can test what gets stored in the offline cache based purely on the write
+ * acknowledgement (without receiving an updated document via watch). As such they also rely on
+ * persistence being enabled so documents remain in the cache after the write.
+ */
+@interface FIRArrayTransformServerApplicationTests : FSTIntegrationTestCase
+@end
+
+@implementation FIRArrayTransformServerApplicationTests {
+  // A document reference to read and write to.
+  FIRDocumentReference *_docRef;
+}
+
+- (void)setUp {
+  [super setUp];
+
+  _docRef = [self documentRef];
+}
+
+/**
+ * Helper that uses a temporary listener to read from cache (returning nil if no document seems
+ * to be in cache). Can probably be replaced with get(source=cache) in the future.
+ */
+- (FIRDocumentSnapshot *_Nullable)getFromCache {
+  FSTEventAccumulator *accumulator = [FSTEventAccumulator accumulatorForTest:self];
+  id<FIRListenerRegistration> listenerRegistration =
+      [_docRef addSnapshotListener:accumulator.valueEventHandler];
+  FIRDocumentSnapshot *snapshot = [accumulator awaitEventWithName:@"listenForOneEvent"];
+  [listenerRegistration remove];
+  if (snapshot.metadata.fromCache) {
+    return snapshot;
+  } else {
+    return nil;
+  }
+}
+
+- (void)testServerApplicationOfSetWithNoCachedBaseDoc {
+  [self writeDocumentRef:_docRef
+                    data:@{
+                      @"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @2 ]]
+                    }];
+  id expected = @{ @"array" : @[ @1, @2 ] };
+  XCTAssertEqualObjects([self getFromCache].data, expected);
+}
+
+- (void)testServerApplicationOfUpdateWithNoCachedBaseDoc {
+  // Write an initial document out-of-band so it's not in our cache
+  [self writeDocumentRef:[[self firestore] documentWithPath:_docRef.path]
+                    data:@{
+                      @"array" : @[ @42 ]
+                    }];
+
+  [self updateDocumentRef:_docRef
+                     data:@{
+                       @"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @2 ]]
+                     }];
+
+  // Nothing should be cached since it was an update and we had no base doc.
+  XCTAssertNil([self getFromCache]);
+}
+
+- (void)testServerApplicationOfMergeSetWithNoCachedBaseDoc {
+  // Write an initial document out-of-band so it's not in our cache
+  [self writeDocumentRef:[[self firestore] documentWithPath:_docRef.path]
+                    data:@{
+                      @"array" : @[ @42 ]
+                    }];
+
+  [self mergeDocumentRef:_docRef
+                    data:@{
+                      @"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @2 ]]
+                    }];
+
+  // Document will be cached but we'll be missing 42.
+  id expected = @{ @"array" : @[ @1, @2 ] };
+  XCTAssertEqualObjects([self getFromCache].data, expected);
+}
+
+- (void)testServerApplicationOfArrayUnionUpdateWithCachedBaseDoc {
+  // Cache a document with an array.
+  [self writeDocumentRef:_docRef data:@{ @"array" : @[ @42 ] }];
+
+  [self updateDocumentRef:_docRef
+                     data:@{
+                       @"array" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @2 ]]
+                     }];
+
+  // Should have merged the update with the cached doc.
+  id expected = @{ @"array" : @[ @42, @1, @2 ] };
+  XCTAssertEqualObjects([self getFromCache].data, expected);
+}
+
+- (void)testServerApplicationOfArrayRemoveUpdateWithCachedBaseDoc {
+  // Cache a document with an array.
+  [self writeDocumentRef:_docRef data:@{ @"array" : @[ @42, @1, @2 ] }];
+
+  [self updateDocumentRef:_docRef
+                     data:@{
+                       @"array" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, @2 ]]
+                     }];
+
+  // Should have merged the update with the cached doc.
+  id expected = @{ @"array" : @[ @42 ] };
+  XCTAssertEqualObjects([self getFromCache].data, expected);
+}
+@end
diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm
index 2c6a2a8..2361fd0 100644
--- a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm
+++ b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm
@@ -363,6 +363,51 @@
   }
 }
 
+#pragma mark - ArrayUnion / ArrayRemove Validation
+
+- (void)testArrayTransformsInQueriesFail {
+  FSTAssertThrows(
+      [[self collectionRef] queryWhereField:@"test"
+                                  isEqualTo:@{
+                                    @"test" : [FIRFieldValue fieldValueForArrayUnion:@[ @1 ]]
+                                  }],
+      @"FieldValue.arrayUnion() can only be used with updateData() and setData() (found in field "
+       "test)");
+
+  FSTAssertThrows(
+      [[self collectionRef] queryWhereField:@"test"
+                                  isEqualTo:@{
+                                    @"test" : [FIRFieldValue fieldValueForArrayRemove:@[ @1 ]]
+                                  }],
+      @"FieldValue.arrayRemove() can only be used with updateData() and setData() (found in field "
+      @"test)");
+}
+
+- (void)testInvalidArrayTransformElementFails {
+  [self expectWrite:@{
+    @"foo" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, self ]]
+  }
+      toFailWithReason:@"Unsupported type: FIRValidationTests"];
+
+  [self expectWrite:@{
+    @"foo" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, self ]]
+  }
+      toFailWithReason:@"Unsupported type: FIRValidationTests"];
+}
+
+- (void)testArraysInArrayTransformsFail {
+  // This would result in a directly nested array which is not supported.
+  [self expectWrite:@{
+    @"foo" : [FIRFieldValue fieldValueForArrayUnion:@[ @1, @[ @"nested" ] ]]
+  }
+      toFailWithReason:@"Nested arrays are not supported"];
+
+  [self expectWrite:@{
+    @"foo" : [FIRFieldValue fieldValueForArrayRemove:@[ @1, @[ @"nested" ] ]]
+  }
+      toFailWithReason:@"Nested arrays are not supported"];
+}
+
 #pragma mark - Query Validation
 
 - (void)testQueryWithNonPositiveLimitFails {
diff --git a/Firestore/Example/Tests/Model/FSTMutationTests.mm b/Firestore/Example/Tests/Model/FSTMutationTests.mm
index 0bb7518..3183efd 100644
--- a/Firestore/Example/Tests/Model/FSTMutationTests.mm
+++ b/Firestore/Example/Tests/Model/FSTMutationTests.mm
@@ -145,7 +145,9 @@ using firebase::firestore::model::TransformOperation;
 - (void)testCreateArrayUnionTransform {
   FSTTransformMutation *transform = FSTTestTransformMutation(@"collection/key", @{
     @"foo" : [FIRFieldValue fieldValueForArrayUnion:@[ @"tag" ]],
-    @"bar.baz" : [FIRFieldValue fieldValueForArrayUnion:@[ @YES, @[ @1, @2 ], @{@"a" : @"b"} ]]
+    @"bar.baz" :
+        [FIRFieldValue fieldValueForArrayUnion:@[ @YES,
+                                                  @{ @"nested" : @{@"a" : @[ @1, @2 ]} } ]]
   });
   XCTAssertEqual(transform.fieldTransforms.size(), 2);
 
@@ -161,7 +163,7 @@ using firebase::firestore::model::TransformOperation;
   XCTAssertEqual(second.path(), FieldPath({"bar", "baz"}));
   {
     std::vector<FSTFieldValue *> expectedElements {
-      FSTTestFieldValue(@YES), FSTTestFieldValue(@[ @1, @2 ]), FSTTestFieldValue(@{@"a" : @"b"})
+      FSTTestFieldValue(@YES), FSTTestFieldValue(@{ @"nested" : @{@"a" : @[ @1, @2 ]} })
     };
     ArrayTransform expected(TransformOperation::Type::ArrayUnion, expectedElements);
     XCTAssertEqual(static_cast<const ArrayTransform &>(second.transformation()), expected);
diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h
index 9c80799..e27491b 100644
--- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h
+++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h
@@ -82,6 +82,8 @@ extern "C" {
 
 - (void)deleteDocumentRef:(FIRDocumentReference *)ref;
 
+- (void)mergeDocumentRef:(FIRDocumentReference *)ref data:(NSDictionary<NSString *, id> *)data;
+
 - (void)disableNetwork;
 
 - (void)enableNetwork;
diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm
index 2b1f9d0..9bfdb3b 100644
--- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm
+++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm
@@ -276,6 +276,13 @@ NS_ASSUME_NONNULL_BEGIN
   [self awaitExpectations];
 }
 
+- (void)mergeDocumentRef:(FIRDocumentReference *)ref data:(NSDictionary<NSString *, id> *)data {
+  [ref setData:data
+           merge:YES
+      completion:[self completionForExpectationWithName:@"setDataWithMerge"]];
+  [self awaitExpectations];
+}
+
 - (void)disableNetwork {
   [self.db.client
       disableNetworkWithCompletion:[self completionForExpectationWithName:@"Disable Network."]];
diff --git a/Firestore/Source/API/FIRFieldValue.mm b/Firestore/Source/API/FIRFieldValue.mm
index e0ed8c7..5f7fbd7 100644
--- a/Firestore/Source/API/FIRFieldValue.mm
+++ b/Firestore/Source/API/FIRFieldValue.mm
@@ -95,6 +95,11 @@ NS_ASSUME_NONNULL_BEGIN
   }
   return self;
 }
+
+- (NSString *)methodName {
+  return @"FieldValue.arrayUnion()";
+}
+
 @end
 
 #pragma mark - FSTArrayRemoveFieldValue
@@ -110,6 +115,11 @@ NS_ASSUME_NONNULL_BEGIN
   }
   return self;
 }
+
+- (NSString *)methodName {
+  return @"FieldValue.arrayRemove()";
+}
+
 @end
 
 #pragma mark - FIRFieldValue
diff --git a/Firestore/Source/API/FSTUserDataConverter.mm b/Firestore/Source/API/FSTUserDataConverter.mm
index cd32cef..2794398 100644
--- a/Firestore/Source/API/FSTUserDataConverter.mm
+++ b/Firestore/Source/API/FSTUserDataConverter.mm
@@ -515,6 +515,15 @@ typedef NS_ENUM(NSInteger, FSTUserDataSource) {
   input = self.preConverter(input);
   if ([input isKindOfClass:[NSDictionary class]]) {
     return [self parseDictionary:(NSDictionary *)input context:context];
+
+  } else if ([input isKindOfClass:[FIRFieldValue class]]) {
+    // FieldValues usually parse into transforms (except FieldValue.delete()) in which case we
+    // do not want to include this field in our parsed data (as doing so will overwrite the field
+    // directly prior to the transform trying to transform it). So we don't call appendToFieldMask
+    // and we return nil as our parsing result.
+    [self parseSentinelFieldValue:(FIRFieldValue *)input context:context];
+    return nil;
+
   } else {
     // If context.path is nil we are already inside an array and we don't support field mask paths
     // more granular than the top-level array.
@@ -528,11 +537,6 @@ typedef NS_ENUM(NSInteger, FSTUserDataSource) {
         FSTThrowInvalidArgument(@"Nested arrays are not supported");
       }
       return [self parseArray:(NSArray *)input context:context];
-    } else if ([input isKindOfClass:[FIRFieldValue class]]) {
-      // parseSentinelFieldValue may add an FSTFieldTransform, but we return nil since nothing
-      // should be included in the actual parsed data.
-      [self parseSentinelFieldValue:(FIRFieldValue *)input context:context];
-      return nil;
     } else {
       return [self parseScalarValue:input context:context];
     }
@@ -582,7 +586,9 @@ typedef NS_ENUM(NSInteger, FSTUserDataSource) {
 
   if ([fieldValue isKindOfClass:[FSTDeleteFieldValue class]]) {
     if (context.dataSource == FSTUserDataSourceMergeSet) {
-      // No transform to add for a delete, so we do nothing.
+      // No transform to add for a delete, but we need to add it to our fieldMask so it gets
+      // deleted.
+      [context appendToFieldMaskWithFieldPath:*context.path];
     } else if (context.dataSource == FSTUserDataSourceUpdate) {
       FSTAssert(context.path->size() > 0,
                 @"FieldValue.delete() at the top level should have already been handled.");
@@ -777,13 +783,15 @@ typedef NS_ENUM(NSInteger, FSTUserDataSource) {
 
 - (std::vector<FSTFieldValue *>)parseArrayTransformElements:(NSArray<id> *)elements {
   std::vector<FSTFieldValue *> results;
-  for (id element in elements) {
+  for (NSUInteger i = 0; i < elements.count; i++) {
+    id element = elements[i];
     // Although array transforms are used with writes, the actual elements being unioned or removed
     // are not considered writes since they cannot contain any FieldValue sentinels, etc.
     FSTParseContext *context =
         [FSTParseContext contextWithSource:FSTUserDataSourceArgument
                                       path:absl::make_unique<FieldPath>(FieldPath::EmptyPath())];
-    FSTFieldValue *parsedElement = [self parseData:element context:context];
+    FSTFieldValue *parsedElement =
+        [self parseData:element context:[context contextForArrayIndex:i]];
     FSTAssert(parsedElement && context.fieldTransforms->size() == 0,
               @"Failed to properly parse array transform element: %@", element);
     results.push_back(parsedElement);