Skip to content

Commit 0088d09

Browse files
author
Brian Chen
authored
Replace usage of transforms with update transforms (#7276)
1 parent ee05ed3 commit 0088d09

36 files changed

+880
-777
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
# Unreleased
2-
- [fixed] Fixed an issue where using `FieldValue.arrayRemove()` would only delete the
3-
first occurrence of an element in an array in a latency
2+
- [changed] A write to a document that contains FieldValue transforms is no
3+
longer split up into two separate operations. This reduces the number of
4+
writes the backend performs and allows each WriteBatch to hold 500 writes
5+
regardless of how many FieldValue transformations are attached.
6+
- [fixed] Fixed an issue where using `FieldValue.arrayRemove()` would only
7+
delete the first occurrence of an element in an array in a latency
48
compensated snapshots.
59

610
# v7.3.0

‎Firestore/Example/Tests/API/FSTUserDataConverterTests.mm

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@
2626

2727
#include "Firestore/core/src/model/database_id.h"
2828
#include "Firestore/core/src/model/field_value.h"
29-
#include "Firestore/core/src/model/transform_mutation.h"
29+
#include "Firestore/core/src/model/patch_mutation.h"
30+
#include "Firestore/core/src/model/set_mutation.h"
3031
#include "Firestore/core/src/model/transform_operation.h"
3132
#include "Firestore/core/src/nanopb/nanopb_util.h"
3233
#include "Firestore/core/test/unit/testutil/testutil.h"
@@ -40,8 +41,9 @@
4041
using firebase::firestore::model::FieldValue;
4142
using firebase::firestore::model::FieldTransform;
4243
using firebase::firestore::model::ObjectValue;
43-
using firebase::firestore::model::TransformMutation;
44+
using firebase::firestore::model::PatchMutation;
4445
using firebase::firestore::model::TransformOperation;
46+
using firebase::firestore::model::SetMutation;
4547
using firebase::firestore::nanopb::MakeNSData;
4648
using firebase::firestore::testutil::Field;
4749

@@ -218,44 +220,66 @@ - (void)testNSDatesAreConvertedToTimestamps {
218220
}
219221

220222
- (void)testCreatesArrayUnionTransforms {
221-
TransformMutation transform = FSTTestTransformMutation(@"collection/key", @{
223+
PatchMutation patchMutation = FSTTestPatchMutation(@"collection/key", @{
222224
@"foo" : [FIRFieldValue fieldValueForArrayUnion:@[ @"tag" ]],
223225
@"bar.baz" :
224226
[FIRFieldValue fieldValueForArrayUnion:@[ @YES, @{@"nested" : @{@"a" : @[ @1, @2 ]}} ]]
227+
},
228+
{});
229+
XCTAssertEqual(patchMutation.field_transforms().size(), 2u);
230+
231+
SetMutation setMutation = FSTTestSetMutation(@"collection/key", @{
232+
@"foo" : [FIRFieldValue fieldValueForArrayUnion:@[ @"tag" ]],
233+
@"bar" : [FIRFieldValue fieldValueForArrayUnion:@[ @YES, @{@"nested" : @{@"a" : @[ @1, @2 ]}} ]]
225234
});
226-
XCTAssertEqual(transform.field_transforms().size(), 2u);
235+
XCTAssertEqual(setMutation.field_transforms().size(), 2u);
227236

228-
const FieldTransform &first = transform.field_transforms()[0];
229-
XCTAssertEqual(first.path(), FieldPath({"foo"}));
237+
const FieldTransform &patchFirst = patchMutation.field_transforms()[0];
238+
XCTAssertEqual(patchFirst.path(), FieldPath({"foo"}));
239+
const FieldTransform &setFirst = setMutation.field_transforms()[0];
240+
XCTAssertEqual(setFirst.path(), FieldPath({"foo"}));
230241
{
231242
std::vector<FieldValue> expectedElements{FSTTestFieldValue(@"tag")};
232243
ArrayTransform expected(TransformOperation::Type::ArrayUnion, expectedElements);
233-
XCTAssertEqual(static_cast<const ArrayTransform &>(first.transformation()), expected);
244+
XCTAssertEqual(static_cast<const ArrayTransform &>(patchFirst.transformation()), expected);
245+
XCTAssertEqual(static_cast<const ArrayTransform &>(setFirst.transformation()), expected);
234246
}
235247

236-
const FieldTransform &second = transform.field_transforms()[1];
237-
XCTAssertEqual(second.path(), FieldPath({"bar", "baz"}));
248+
const FieldTransform &patchSecond = patchMutation.field_transforms()[1];
249+
XCTAssertEqual(patchSecond.path(), FieldPath({"bar", "baz"}));
250+
const FieldTransform &setSecond = setMutation.field_transforms()[1];
251+
XCTAssertEqual(setSecond.path(), FieldPath({"bar"}));
238252
{
239253
std::vector<FieldValue> expectedElements {
240254
FSTTestFieldValue(@YES), FSTTestFieldValue(@{@"nested" : @{@"a" : @[ @1, @2 ]}})
241255
};
242256
ArrayTransform expected(TransformOperation::Type::ArrayUnion, expectedElements);
243-
XCTAssertEqual(static_cast<const ArrayTransform &>(second.transformation()), expected);
257+
XCTAssertEqual(static_cast<const ArrayTransform &>(patchSecond.transformation()), expected);
258+
XCTAssertEqual(static_cast<const ArrayTransform &>(setSecond.transformation()), expected);
244259
}
245260
}
246261

247262
- (void)testCreatesArrayRemoveTransforms {
248-
TransformMutation transform = FSTTestTransformMutation(@"collection/key", @{
263+
PatchMutation patchMutation = FSTTestPatchMutation(@"collection/key", @{
264+
@"foo" : [FIRFieldValue fieldValueForArrayRemove:@[ @"tag" ]],
265+
},
266+
{});
267+
XCTAssertEqual(patchMutation.field_transforms().size(), 1u);
268+
269+
SetMutation setMutation = FSTTestSetMutation(@"collection/key", @{
249270
@"foo" : [FIRFieldValue fieldValueForArrayRemove:@[ @"tag" ]],
250271
});
251-
XCTAssertEqual(transform.field_transforms().size(), 1u);
272+
XCTAssertEqual(patchMutation.field_transforms().size(), 1u);
252273

253-
const FieldTransform &first = transform.field_transforms()[0];
254-
XCTAssertEqual(first.path(), FieldPath({"foo"}));
274+
const FieldTransform &patchFirst = patchMutation.field_transforms()[0];
275+
XCTAssertEqual(patchFirst.path(), FieldPath({"foo"}));
276+
const FieldTransform &setFirst = setMutation.field_transforms()[0];
277+
XCTAssertEqual(setFirst.path(), FieldPath({"foo"}));
255278
{
256279
std::vector<FieldValue> expectedElements{FSTTestFieldValue(@"tag")};
257280
const ArrayTransform expected(TransformOperation::Type::ArrayRemove, expectedElements);
258-
XCTAssertEqual(static_cast<const ArrayTransform &>(first.transformation()), expected);
281+
XCTAssertEqual(static_cast<const ArrayTransform &>(patchFirst.transformation()), expected);
282+
XCTAssertEqual(static_cast<const ArrayTransform &>(setFirst.transformation()), expected);
259283
}
260284
}
261285

‎Firestore/Example/Tests/SpecTests/FSTSpecTests.mm

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -345,8 +345,7 @@ - (void)doSet:(NSArray *)setSpec {
345345
}
346346

347347
- (void)doPatch:(NSArray *)patchSpec {
348-
[self.driver
349-
writeUserMutation:FSTTestPatchMutation(util::MakeString(patchSpec[0]), patchSpec[1], {})];
348+
[self.driver writeUserMutation:FSTTestPatchMutation(patchSpec[0], patchSpec[1], {})];
350349
}
351350

352351
- (void)doDelete:(NSString *)key {

‎Firestore/Example/Tests/Util/FSTHelpers.h

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -124,18 +124,10 @@ model::SetMutation FSTTestSetMutation(NSString *path, NSDictionary<NSString *, i
124124

125125
/** Creates a patch mutation for the document key at the given path. */
126126
model::PatchMutation FSTTestPatchMutation(
127-
absl::string_view path,
127+
NSString *path,
128128
NSDictionary<NSString *, id> *values,
129129
const std::vector<firebase::firestore::model::FieldPath> &updateMask);
130130

131-
/**
132-
* Creates a TransformMutation by parsing any FIRFieldValue sentinels in the provided data. The
133-
* data is expected to use dotted-notation for nested fields (i.e.
134-
* @{ @"foo.bar": [FIRFieldValue ...] } and must not contain any non-sentinel data.
135-
*/
136-
model::TransformMutation FSTTestTransformMutation(NSString *path,
137-
NSDictionary<NSString *, id> *data);
138-
139131
/** Creates a delete mutation for the document key at the given path. */
140132
model::DeleteMutation FSTTestDeleteMutation(NSString *path);
141133

‎Firestore/Example/Tests/Util/FSTHelpers.mm

Lines changed: 23 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#import "Firestore/Example/Tests/Util/FSTHelpers.h"
1818

19+
#import <FirebaseFirestore/FIRFieldValue.h>
1920
#import <FirebaseFirestore/FIRGeoPoint.h>
2021

2122
#include <set>
@@ -24,23 +25,17 @@
2425
#import "Firestore/Source/API/FSTUserDataConverter.h"
2526

2627
#include "Firestore/core/src/core/user_data.h"
27-
#include "Firestore/core/src/model/database_id.h"
2828
#include "Firestore/core/src/model/delete_mutation.h"
29-
#include "Firestore/core/src/model/document_key.h"
30-
#include "Firestore/core/src/model/field_mask.h"
31-
#include "Firestore/core/src/model/field_path.h"
32-
#include "Firestore/core/src/model/field_value.h"
3329
#include "Firestore/core/src/model/patch_mutation.h"
34-
#include "Firestore/core/src/model/precondition.h"
3530
#include "Firestore/core/src/model/resource_path.h"
3631
#include "Firestore/core/src/model/set_mutation.h"
37-
#include "Firestore/core/src/model/transform_mutation.h"
38-
#include "Firestore/core/src/util/string_apple.h"
39-
#include "Firestore/core/test/unit/testutil/testutil.h"
32+
33+
#import "Firestore/core/test/unit/testutil/testutil.h"
4034

4135
namespace testutil = firebase::firestore::testutil;
4236
namespace util = firebase::firestore::util;
4337

38+
using firebase::firestore::core::ParsedSetData;
4439
using firebase::firestore::core::ParsedUpdateData;
4540
using firebase::firestore::model::DatabaseId;
4641
using firebase::firestore::model::DeleteMutation;
@@ -52,7 +47,6 @@
5247
using firebase::firestore::model::PatchMutation;
5348
using firebase::firestore::model::Precondition;
5449
using firebase::firestore::model::SetMutation;
55-
using firebase::firestore::model::TransformMutation;
5650

5751
NS_ASSUME_NONNULL_BEGIN
5852

@@ -132,39 +126,33 @@ DocumentKey FSTTestDocKey(NSString *path) {
132126
}
133127

134128
SetMutation FSTTestSetMutation(NSString *path, NSDictionary<NSString *, id> *values) {
135-
return SetMutation(FSTTestDocKey(path), FSTTestObjectValue(values), Precondition::None());
129+
FSTUserDataConverter *converter = FSTTestUserDataConverter();
130+
ParsedSetData result = [converter parsedSetData:values];
131+
return SetMutation(FSTTestDocKey(path), result.data(), Precondition::None(),
132+
result.field_transforms());
136133
}
137134

138-
PatchMutation FSTTestPatchMutation(const absl::string_view path,
135+
PatchMutation FSTTestPatchMutation(NSString *path,
139136
NSDictionary<NSString *, id> *values,
140137
const std::vector<FieldPath> &updateMask) {
141-
BOOL merge = !updateMask.empty();
142-
143-
__block ObjectValue objectValue = ObjectValue::Empty();
144-
__block std::set<FieldPath> fieldMaskPaths;
145-
[values enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) {
146-
const FieldPath path = testutil::Field(util::MakeString(key));
147-
fieldMaskPaths.insert(path);
148-
if (![value isEqual:kDeleteSentinel]) {
149-
FieldValue parsedValue = FSTTestFieldValue(value);
150-
objectValue = objectValue.Set(path, std::move(parsedValue));
138+
// Replace '<DELETE>' sentinel from JSON.
139+
NSMutableDictionary *mutableValues = [values mutableCopy];
140+
[mutableValues enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) {
141+
if ([value isEqual:kDeleteSentinel]) {
142+
const FieldPath fieldPath = testutil::Field(util::MakeString(key));
143+
mutableValues[key] = [FIRFieldValue fieldValueForDelete];
151144
}
152145
}];
153146

154-
DocumentKey key = testutil::Key(path);
155-
Precondition precondition = merge ? Precondition::None() : Precondition::Exists(true);
156-
FieldMask mask(merge ? std::set<FieldPath>(updateMask.begin(), updateMask.end())
157-
: fieldMaskPaths);
158-
return PatchMutation(key, objectValue, mask, precondition);
159-
}
160-
161-
TransformMutation FSTTestTransformMutation(NSString *path, NSDictionary<NSString *, id> *data) {
162-
DocumentKey key{testutil::Resource(util::MakeString(path))};
163147
FSTUserDataConverter *converter = FSTTestUserDataConverter();
164-
ParsedUpdateData result = [converter parsedUpdateData:data];
165-
HARD_ASSERT(result.data().size() == 0,
166-
"FSTTestTransformMutation() only expects transforms; no other data");
167-
return TransformMutation(key, result.field_transforms());
148+
ParsedUpdateData parsed = [converter parsedUpdateData:mutableValues];
149+
150+
DocumentKey key = FSTTestDocKey(path);
151+
152+
BOOL merge = !updateMask.empty();
153+
Precondition precondition = merge ? Precondition::None() : Precondition::Exists(true);
154+
return PatchMutation(key, parsed.data(), parsed.fieldMask(), precondition,
155+
parsed.field_transforms());
168156
}
169157

170158
DeleteMutation FSTTestDeleteMutation(NSString *path) {

‎Firestore/core/src/api/document_reference.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ using core::ViewSnapshot;
5151
using model::DeleteMutation;
5252
using model::Document;
5353
using model::DocumentKey;
54+
using model::Mutation;
5455
using model::Precondition;
5556
using model::ResourcePath;
5657
using util::Status;
@@ -95,14 +96,14 @@ CollectionReference DocumentReference::GetCollectionReference(
9596
void DocumentReference::SetData(core::ParsedSetData&& set_data,
9697
util::StatusCallback callback) {
9798
firestore_->client()->WriteMutations(
98-
std::move(set_data).ToMutations(key(), Precondition::None()),
99+
{std::move(set_data).ToMutation(key(), Precondition::None())},
99100
std::move(callback));
100101
}
101102

102103
void DocumentReference::UpdateData(core::ParsedUpdateData&& update_data,
103104
util::StatusCallback callback) {
104105
firestore_->client()->WriteMutations(
105-
std::move(update_data).ToMutations(key(), Precondition::Exists(true)),
106+
{std::move(update_data).ToMutation(key(), Precondition::Exists(true))},
106107
std::move(callback));
107108
}
108109

‎Firestore/core/src/api/write_batch.cc

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,22 +41,18 @@ void WriteBatch::SetData(const DocumentReference& reference,
4141
VerifyNotCommitted();
4242
ValidateReference(reference);
4343

44-
std::vector<Mutation> append_mutations = std::move(set_data).ToMutations(
45-
reference.key(), model::Precondition::None());
46-
std::move(append_mutations.begin(), append_mutations.end(),
47-
std::back_inserter(mutations_));
44+
mutations_.push_back(std::move(set_data).ToMutation(
45+
reference.key(), model::Precondition::None()));
4846
}
4947

5048
void WriteBatch::UpdateData(const DocumentReference& reference,
5149
core::ParsedUpdateData&& update_data) {
5250
VerifyNotCommitted();
5351
ValidateReference(reference);
5452

55-
std::vector<Mutation> append_mutations =
53+
mutations_.push_back(
5654
std::move(update_data)
57-
.ToMutations(reference.key(), model::Precondition::Exists(true));
58-
std::move(append_mutations.begin(), append_mutations.end(),
59-
std::back_inserter(mutations_));
55+
.ToMutation(reference.key(), model::Precondition::Exists(true)));
6056
}
6157

6258
void WriteBatch::DeleteData(const DocumentReference& reference) {

‎Firestore/core/src/core/transaction.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ StatusOr<Precondition> Transaction::CreateUpdatePrecondition(
159159
}
160160

161161
void Transaction::Set(const DocumentKey& key, ParsedSetData&& data) {
162-
WriteMutations(std::move(data).ToMutations(key, CreatePrecondition(key)));
162+
WriteMutations({std::move(data).ToMutation(key, CreatePrecondition(key))});
163163
written_docs_.insert(key);
164164
}
165165

@@ -169,7 +169,7 @@ void Transaction::Update(const DocumentKey& key, ParsedUpdateData&& data) {
169169
last_write_error_ = maybe_precondition.status();
170170
} else {
171171
WriteMutations(
172-
std::move(data).ToMutations(key, maybe_precondition.ValueOrDie()));
172+
{std::move(data).ToMutation(key, maybe_precondition.ValueOrDie())});
173173
}
174174
written_docs_.insert(key);
175175
}

‎Firestore/core/src/core/user_data.cc

Lines changed: 11 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#include "Firestore/core/src/model/mutation.h"
2323
#include "Firestore/core/src/model/patch_mutation.h"
2424
#include "Firestore/core/src/model/set_mutation.h"
25-
#include "Firestore/core/src/model/transform_mutation.h"
2625
#include "Firestore/core/src/model/transform_operation.h"
2726
#include "Firestore/core/src/util/exception.h"
2827
#include "absl/memory/memory.h"
@@ -41,7 +40,6 @@ using model::ObjectValue;
4140
using model::PatchMutation;
4241
using model::Precondition;
4342
using model::SetMutation;
44-
using model::TransformMutation;
4543
using model::TransformOperation;
4644
using util::ThrowInvalidArgument;
4745

@@ -102,7 +100,7 @@ ParsedSetData ParseAccumulator::SetData(ObjectValue data) && {
102100
}
103101

104102
ParsedUpdateData ParseAccumulator::UpdateData(ObjectValue data) && {
105-
return ParsedUpdateData{data, FieldMask{std::move(field_mask_)},
103+
return ParsedUpdateData{std::move(data), FieldMask{std::move(field_mask_)},
106104
std::move(field_transforms_)};
107105
}
108106

@@ -224,24 +222,15 @@ ParsedSetData::ParsedSetData(ObjectValue data,
224222
patch_{true} {
225223
}
226224

227-
std::vector<Mutation> ParsedSetData::ToMutations(
228-
const DocumentKey& key, const Precondition& precondition) && {
229-
std::vector<Mutation> mutations;
225+
Mutation ParsedSetData::ToMutation(const DocumentKey& key,
226+
const Precondition& precondition) && {
230227
if (patch_) {
231-
PatchMutation mutation(key, std::move(data_), std::move(field_mask_),
232-
precondition);
233-
mutations.push_back(mutation);
228+
return PatchMutation(key, std::move(data_), std::move(field_mask_),
229+
precondition, std::move(field_transforms_));
234230
} else {
235-
SetMutation mutation(key, std::move(data_), precondition);
236-
mutations.push_back(mutation);
231+
return SetMutation(key, std::move(data_), precondition,
232+
std::move(field_transforms_));
237233
}
238-
239-
if (!field_transforms_.empty()) {
240-
TransformMutation mutation(key, std::move(field_transforms_));
241-
mutations.push_back(mutation);
242-
}
243-
244-
return mutations;
245234
}
246235

247236
// MARK: - ParsedUpdateData
@@ -255,20 +244,10 @@ ParsedUpdateData::ParsedUpdateData(
255244
field_transforms_{std::move(field_transforms)} {
256245
}
257246

258-
std::vector<Mutation> ParsedUpdateData::ToMutations(
259-
const DocumentKey& key, const Precondition& precondition) && {
260-
std::vector<Mutation> mutations;
261-
262-
PatchMutation mutation(key, std::move(data_), std::move(field_mask_),
263-
precondition);
264-
mutations.push_back(mutation);
265-
266-
if (!field_transforms_.empty()) {
267-
TransformMutation mutation(key, std::move(field_transforms_));
268-
mutations.push_back(mutation);
269-
}
270-
271-
return mutations;
247+
Mutation ParsedUpdateData::ToMutation(const DocumentKey& key,
248+
const Precondition& precondition) && {
249+
return PatchMutation(key, std::move(data_), std::move(field_mask_),
250+
precondition, std::move(field_transforms_));
272251
}
273252

274253
} // namespace core

0 commit comments

Comments
 (0)