Replace usage of transforms with update transforms (#7276)

Author: Brian Chen

Firestore/CHANGELOG.md

@@ -1,6 +1,10 @@
 # Unreleased
-- [fixed] Fixed an issue where using `FieldValue.arrayRemove()` would only delete the
-  first occurrence of an element in an array in a latency
+- [changed] A write to a document that contains FieldValue transforms is no
+  longer split up into two separate operations. This reduces the number of
+  writes the backend performs and allows each WriteBatch to hold 500 writes
+  regardless of how many FieldValue transformations are attached.
+- [fixed] Fixed an issue where using `FieldValue.arrayRemove()` would only
+  delete the first occurrence of an element in an array in a latency
   compensated snapshots.
 
 # v7.3.0

Firestore/Example/Tests/API/FSTUserDataConverterTests.mm

@@ -26,7 +26,8 @@
 
 #include "Firestore/core/src/model/database_id.h"
 #include "Firestore/core/src/model/field_value.h"
-#include "Firestore/core/src/model/transform_mutation.h"
+#include "Firestore/core/src/model/patch_mutation.h"
+#include "Firestore/core/src/model/set_mutation.h"
 #include "Firestore/core/src/model/transform_operation.h"
 #include "Firestore/core/src/nanopb/nanopb_util.h"
 #include "Firestore/core/test/unit/testutil/testutil.h"
@@ -40,8 +41,9 @@
 using firebase::firestore::model::FieldValue;
 using firebase::firestore::model::FieldTransform;
 using firebase::firestore::model::ObjectValue;
-using firebase::firestore::model::TransformMutation;
+using firebase::firestore::model::PatchMutation;
 using firebase::firestore::model::TransformOperation;
+using firebase::firestore::model::SetMutation;
 using firebase::firestore::nanopb::MakeNSData;
 using firebase::firestore::testutil::Field;
 
@@ -218,44 +220,66 @@ - (void)testNSDatesAreConvertedToTimestamps {
 }
 
 - (void)testCreatesArrayUnionTransforms {
-  TransformMutation transform = FSTTestTransformMutation(@"collection/key", @{
+  PatchMutation patchMutation = FSTTestPatchMutation(@"collection/key", @{
     @"foo" : [FIRFieldValue fieldValueForArrayUnion:@[ @"tag" ]],
     @"bar.baz" :
         [FIRFieldValue fieldValueForArrayUnion:@[ @YES, @{@"nested" : @{@"a" : @[ @1, @2 ]}} ]]
+  },
+                                                     {});
+  XCTAssertEqual(patchMutation.field_transforms().size(), 2u);
+
+  SetMutation setMutation = FSTTestSetMutation(@"collection/key", @{
+    @"foo" : [FIRFieldValue fieldValueForArrayUnion:@[ @"tag" ]],
+    @"bar" : [FIRFieldValue fieldValueForArrayUnion:@[ @YES, @{@"nested" : @{@"a" : @[ @1, @2 ]}} ]]
   });
-  XCTAssertEqual(transform.field_transforms().size(), 2u);
+  XCTAssertEqual(setMutation.field_transforms().size(), 2u);
 
-  const FieldTransform &first = transform.field_transforms()[0];
-  XCTAssertEqual(first.path(), FieldPath({"foo"}));
+  const FieldTransform &patchFirst = patchMutation.field_transforms()[0];
+  XCTAssertEqual(patchFirst.path(), FieldPath({"foo"}));
+  const FieldTransform &setFirst = setMutation.field_transforms()[0];
+  XCTAssertEqual(setFirst.path(), FieldPath({"foo"}));
   {
     std::vector<FieldValue> expectedElements{FSTTestFieldValue(@"tag")};
     ArrayTransform expected(TransformOperation::Type::ArrayUnion, expectedElements);
-    XCTAssertEqual(static_cast<const ArrayTransform &>(first.transformation()), expected);
+    XCTAssertEqual(static_cast<const ArrayTransform &>(patchFirst.transformation()), expected);
+    XCTAssertEqual(static_cast<const ArrayTransform &>(setFirst.transformation()), expected);
   }
 
-  const FieldTransform &second = transform.field_transforms()[1];
-  XCTAssertEqual(second.path(), FieldPath({"bar", "baz"}));
+  const FieldTransform &patchSecond = patchMutation.field_transforms()[1];
+  XCTAssertEqual(patchSecond.path(), FieldPath({"bar", "baz"}));
+  const FieldTransform &setSecond = setMutation.field_transforms()[1];
+  XCTAssertEqual(setSecond.path(), FieldPath({"bar"}));
   {
     std::vector<FieldValue> expectedElements {
       FSTTestFieldValue(@YES), FSTTestFieldValue(@{@"nested" : @{@"a" : @[ @1, @2 ]}})
     };
     ArrayTransform expected(TransformOperation::Type::ArrayUnion, expectedElements);
-    XCTAssertEqual(static_cast<const ArrayTransform &>(second.transformation()), expected);
+    XCTAssertEqual(static_cast<const ArrayTransform &>(patchSecond.transformation()), expected);
+    XCTAssertEqual(static_cast<const ArrayTransform &>(setSecond.transformation()), expected);
   }
 }
 
 - (void)testCreatesArrayRemoveTransforms {
-  TransformMutation transform = FSTTestTransformMutation(@"collection/key", @{
+  PatchMutation patchMutation = FSTTestPatchMutation(@"collection/key", @{
+    @"foo" : [FIRFieldValue fieldValueForArrayRemove:@[ @"tag" ]],
+  },
+                                                     {});
+  XCTAssertEqual(patchMutation.field_transforms().size(), 1u);
+
+  SetMutation setMutation = FSTTestSetMutation(@"collection/key", @{
     @"foo" : [FIRFieldValue fieldValueForArrayRemove:@[ @"tag" ]],
   });
-  XCTAssertEqual(transform.field_transforms().size(), 1u);
+  XCTAssertEqual(patchMutation.field_transforms().size(), 1u);
 
-  const FieldTransform &first = transform.field_transforms()[0];
-  XCTAssertEqual(first.path(), FieldPath({"foo"}));
+  const FieldTransform &patchFirst = patchMutation.field_transforms()[0];
+  XCTAssertEqual(patchFirst.path(), FieldPath({"foo"}));
+  const FieldTransform &setFirst = setMutation.field_transforms()[0];
+  XCTAssertEqual(setFirst.path(), FieldPath({"foo"}));
   {
     std::vector<FieldValue> expectedElements{FSTTestFieldValue(@"tag")};
     const ArrayTransform expected(TransformOperation::Type::ArrayRemove, expectedElements);
-    XCTAssertEqual(static_cast<const ArrayTransform &>(first.transformation()), expected);
+    XCTAssertEqual(static_cast<const ArrayTransform &>(patchFirst.transformation()), expected);
+    XCTAssertEqual(static_cast<const ArrayTransform &>(setFirst.transformation()), expected);
   }
 }
 

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm

@@ -345,8 +345,7 @@ - (void)doSet:(NSArray *)setSpec {
 }
 
 - (void)doPatch:(NSArray *)patchSpec {
-  [self.driver
-      writeUserMutation:FSTTestPatchMutation(util::MakeString(patchSpec[0]), patchSpec[1], {})];
+  [self.driver writeUserMutation:FSTTestPatchMutation(patchSpec[0], patchSpec[1], {})];
 }
 
 - (void)doDelete:(NSString *)key {

Firestore/Example/Tests/Util/FSTHelpers.h

@@ -124,18 +124,10 @@ model::SetMutation FSTTestSetMutation(NSString *path, NSDictionary<NSString *, i
 
 /** Creates a patch mutation for the document key at the given path. */
 model::PatchMutation FSTTestPatchMutation(
-    absl::string_view path,
+    NSString *path,
     NSDictionary<NSString *, id> *values,
     const std::vector<firebase::firestore::model::FieldPath> &updateMask);
 
-/**
- * Creates a TransformMutation by parsing any FIRFieldValue sentinels in the provided data. The
- * data is expected to use dotted-notation for nested fields (i.e.
- * @{ @"foo.bar": [FIRFieldValue ...] } and must not contain any non-sentinel data.
- */
-model::TransformMutation FSTTestTransformMutation(NSString *path,
-                                                  NSDictionary<NSString *, id> *data);
-
 /** Creates a delete mutation for the document key at the given path. */
 model::DeleteMutation FSTTestDeleteMutation(NSString *path);
 

Firestore/Example/Tests/Util/FSTHelpers.mm

@@ -16,6 +16,7 @@
 
 #import "Firestore/Example/Tests/Util/FSTHelpers.h"
 
+#import <FirebaseFirestore/FIRFieldValue.h>
 #import <FirebaseFirestore/FIRGeoPoint.h>
 
 #include <set>
@@ -24,23 +25,17 @@
 #import "Firestore/Source/API/FSTUserDataConverter.h"
 
 #include "Firestore/core/src/core/user_data.h"
-#include "Firestore/core/src/model/database_id.h"
 #include "Firestore/core/src/model/delete_mutation.h"
-#include "Firestore/core/src/model/document_key.h"
-#include "Firestore/core/src/model/field_mask.h"
-#include "Firestore/core/src/model/field_path.h"
-#include "Firestore/core/src/model/field_value.h"
 #include "Firestore/core/src/model/patch_mutation.h"
-#include "Firestore/core/src/model/precondition.h"
 #include "Firestore/core/src/model/resource_path.h"
 #include "Firestore/core/src/model/set_mutation.h"
-#include "Firestore/core/src/model/transform_mutation.h"
-#include "Firestore/core/src/util/string_apple.h"
-#include "Firestore/core/test/unit/testutil/testutil.h"
+
+#import "Firestore/core/test/unit/testutil/testutil.h"
 
 namespace testutil = firebase::firestore::testutil;
 namespace util = firebase::firestore::util;
 
+using firebase::firestore::core::ParsedSetData;
 using firebase::firestore::core::ParsedUpdateData;
 using firebase::firestore::model::DatabaseId;
 using firebase::firestore::model::DeleteMutation;
@@ -52,7 +47,6 @@
 using firebase::firestore::model::PatchMutation;
 using firebase::firestore::model::Precondition;
 using firebase::firestore::model::SetMutation;
-using firebase::firestore::model::TransformMutation;
 
 NS_ASSUME_NONNULL_BEGIN
 
@@ -132,39 +126,33 @@ DocumentKey FSTTestDocKey(NSString *path) {
 }
 
 SetMutation FSTTestSetMutation(NSString *path, NSDictionary<NSString *, id> *values) {
-  return SetMutation(FSTTestDocKey(path), FSTTestObjectValue(values), Precondition::None());
+  FSTUserDataConverter *converter = FSTTestUserDataConverter();
+  ParsedSetData result = [converter parsedSetData:values];
+  return SetMutation(FSTTestDocKey(path), result.data(), Precondition::None(),
+                     result.field_transforms());
 }
 
-PatchMutation FSTTestPatchMutation(const absl::string_view path,
+PatchMutation FSTTestPatchMutation(NSString *path,
                                    NSDictionary<NSString *, id> *values,
                                    const std::vector<FieldPath> &updateMask) {
-  BOOL merge = !updateMask.empty();
-
-  __block ObjectValue objectValue = ObjectValue::Empty();
-  __block std::set<FieldPath> fieldMaskPaths;
-  [values enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) {
-    const FieldPath path = testutil::Field(util::MakeString(key));
-    fieldMaskPaths.insert(path);
-    if (![value isEqual:kDeleteSentinel]) {
-      FieldValue parsedValue = FSTTestFieldValue(value);
-      objectValue = objectValue.Set(path, std::move(parsedValue));
+  // Replace '<DELETE>' sentinel from JSON.
+  NSMutableDictionary *mutableValues = [values mutableCopy];
+  [mutableValues enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) {
+    if ([value isEqual:kDeleteSentinel]) {
+      const FieldPath fieldPath = testutil::Field(util::MakeString(key));
+      mutableValues[key] = [FIRFieldValue fieldValueForDelete];
     }
   }];
 
-  DocumentKey key = testutil::Key(path);
-  Precondition precondition = merge ? Precondition::None() : Precondition::Exists(true);
-  FieldMask mask(merge ? std::set<FieldPath>(updateMask.begin(), updateMask.end())
-                       : fieldMaskPaths);
-  return PatchMutation(key, objectValue, mask, precondition);
-}
-
-TransformMutation FSTTestTransformMutation(NSString *path, NSDictionary<NSString *, id> *data) {
-  DocumentKey key{testutil::Resource(util::MakeString(path))};
   FSTUserDataConverter *converter = FSTTestUserDataConverter();
-  ParsedUpdateData result = [converter parsedUpdateData:data];
-  HARD_ASSERT(result.data().size() == 0,
-              "FSTTestTransformMutation() only expects transforms; no other data");
-  return TransformMutation(key, result.field_transforms());
+  ParsedUpdateData parsed = [converter parsedUpdateData:mutableValues];
+
+  DocumentKey key = FSTTestDocKey(path);
+
+  BOOL merge = !updateMask.empty();
+  Precondition precondition = merge ? Precondition::None() : Precondition::Exists(true);
+  return PatchMutation(key, parsed.data(), parsed.fieldMask(), precondition,
+                       parsed.field_transforms());
 }
 
 DeleteMutation FSTTestDeleteMutation(NSString *path) {

Firestore/core/src/api/document_reference.cc

@@ -51,6 +51,7 @@ using core::ViewSnapshot;
 using model::DeleteMutation;
 using model::Document;
 using model::DocumentKey;
+using model::Mutation;
 using model::Precondition;
 using model::ResourcePath;
 using util::Status;
@@ -95,14 +96,14 @@ CollectionReference DocumentReference::GetCollectionReference(
 void DocumentReference::SetData(core::ParsedSetData&& set_data,
                                 util::StatusCallback callback) {
   firestore_->client()->WriteMutations(
-      std::move(set_data).ToMutations(key(), Precondition::None()),
+      {std::move(set_data).ToMutation(key(), Precondition::None())},
       std::move(callback));
 }
 
 void DocumentReference::UpdateData(core::ParsedUpdateData&& update_data,
                                    util::StatusCallback callback) {
   firestore_->client()->WriteMutations(
-      std::move(update_data).ToMutations(key(), Precondition::Exists(true)),
+      {std::move(update_data).ToMutation(key(), Precondition::Exists(true))},
       std::move(callback));
 }
 

Firestore/core/src/api/write_batch.cc

@@ -41,22 +41,18 @@ void WriteBatch::SetData(const DocumentReference& reference,
   VerifyNotCommitted();
   ValidateReference(reference);
 
-  std::vector<Mutation> append_mutations = std::move(set_data).ToMutations(
-      reference.key(), model::Precondition::None());
-  std::move(append_mutations.begin(), append_mutations.end(),
-            std::back_inserter(mutations_));
+  mutations_.push_back(std::move(set_data).ToMutation(
+      reference.key(), model::Precondition::None()));
 }
 
 void WriteBatch::UpdateData(const DocumentReference& reference,
                             core::ParsedUpdateData&& update_data) {
   VerifyNotCommitted();
   ValidateReference(reference);
 
-  std::vector<Mutation> append_mutations =
+  mutations_.push_back(
       std::move(update_data)
-          .ToMutations(reference.key(), model::Precondition::Exists(true));
-  std::move(append_mutations.begin(), append_mutations.end(),
-            std::back_inserter(mutations_));
+          .ToMutation(reference.key(), model::Precondition::Exists(true)));
 }
 
 void WriteBatch::DeleteData(const DocumentReference& reference) {

Firestore/core/src/core/transaction.cc

@@ -159,7 +159,7 @@ StatusOr<Precondition> Transaction::CreateUpdatePrecondition(
 }
 
 void Transaction::Set(const DocumentKey& key, ParsedSetData&& data) {
-  WriteMutations(std::move(data).ToMutations(key, CreatePrecondition(key)));
+  WriteMutations({std::move(data).ToMutation(key, CreatePrecondition(key))});
   written_docs_.insert(key);
 }
 
@@ -169,7 +169,7 @@ void Transaction::Update(const DocumentKey& key, ParsedUpdateData&& data) {
     last_write_error_ = maybe_precondition.status();
   } else {
     WriteMutations(
-        std::move(data).ToMutations(key, maybe_precondition.ValueOrDie()));
+        {std::move(data).ToMutation(key, maybe_precondition.ValueOrDie())});
   }
   written_docs_.insert(key);
 }

Firestore/core/src/core/user_data.cc

@@ -22,7 +22,6 @@
 #include "Firestore/core/src/model/mutation.h"
 #include "Firestore/core/src/model/patch_mutation.h"
 #include "Firestore/core/src/model/set_mutation.h"
-#include "Firestore/core/src/model/transform_mutation.h"
 #include "Firestore/core/src/model/transform_operation.h"
 #include "Firestore/core/src/util/exception.h"
 #include "absl/memory/memory.h"
@@ -41,7 +40,6 @@ using model::ObjectValue;
 using model::PatchMutation;
 using model::Precondition;
 using model::SetMutation;
-using model::TransformMutation;
 using model::TransformOperation;
 using util::ThrowInvalidArgument;
 
@@ -102,7 +100,7 @@ ParsedSetData ParseAccumulator::SetData(ObjectValue data) && {
 }
 
 ParsedUpdateData ParseAccumulator::UpdateData(ObjectValue data) && {
-  return ParsedUpdateData{data, FieldMask{std::move(field_mask_)},
+  return ParsedUpdateData{std::move(data), FieldMask{std::move(field_mask_)},
                           std::move(field_transforms_)};
 }
 
@@ -224,24 +222,15 @@ ParsedSetData::ParsedSetData(ObjectValue data,
       patch_{true} {
 }
 
-std::vector<Mutation> ParsedSetData::ToMutations(
-    const DocumentKey& key, const Precondition& precondition) && {
-  std::vector<Mutation> mutations;
+Mutation ParsedSetData::ToMutation(const DocumentKey& key,
+                                   const Precondition& precondition) && {
   if (patch_) {
-    PatchMutation mutation(key, std::move(data_), std::move(field_mask_),
-                           precondition);
-    mutations.push_back(mutation);
+    return PatchMutation(key, std::move(data_), std::move(field_mask_),
+                         precondition, std::move(field_transforms_));
   } else {
-    SetMutation mutation(key, std::move(data_), precondition);
-    mutations.push_back(mutation);
+    return SetMutation(key, std::move(data_), precondition,
+                       std::move(field_transforms_));
   }
-
-  if (!field_transforms_.empty()) {
-    TransformMutation mutation(key, std::move(field_transforms_));
-    mutations.push_back(mutation);
-  }
-
-  return mutations;
 }
 
 // MARK: - ParsedUpdateData
@@ -255,20 +244,10 @@ ParsedUpdateData::ParsedUpdateData(
       field_transforms_{std::move(field_transforms)} {
 }
 
-std::vector<Mutation> ParsedUpdateData::ToMutations(
-    const DocumentKey& key, const Precondition& precondition) && {
-  std::vector<Mutation> mutations;
-
-  PatchMutation mutation(key, std::move(data_), std::move(field_mask_),
-                         precondition);
-  mutations.push_back(mutation);
-
-  if (!field_transforms_.empty()) {
-    TransformMutation mutation(key, std::move(field_transforms_));
-    mutations.push_back(mutation);
-  }
-
-  return mutations;
+Mutation ParsedUpdateData::ToMutation(const DocumentKey& key,
+                                      const Precondition& precondition) && {
+  return PatchMutation(key, std::move(data_), std::move(field_mask_),
+                       precondition, std::move(field_transforms_));
 }
 
 }  // namespace core