Skip to content

Commit 78a79e5

Browse files
author
Brian Chen
committed
Remove baseDoc check in localTransformResults()
1 parent b4f6400 commit 78a79e5

14 files changed

+20
-49
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
longer split up into two separate operations. This reduces the number of
44
writes the backend performs and allows each WriteBatch to hold 500 writes
55
regardless of how many FieldValue transformations are attached.
6-
- [fixed] Fixed an issue where using `FieldValue.arrayRemove()` would only
6+
- [fixed] Fixed an issue where using `FieldValue.arrayRemove()` would only
77
delete the first occurrence of an element in an array in a latency
88
compensated snapshots.
99

‎Firestore/core/src/local/local_documents_view.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,8 @@ DocumentMap LocalDocumentsView::GetDocumentsMatchingCollectionQuery(
187187
absl::optional<MaybeDocument> base_doc =
188188
results.underlying_map().get(key);
189189

190-
absl::optional<MaybeDocument> mutated_doc = mutation.ApplyToLocalView(
191-
base_doc, base_doc, batch.local_write_time());
190+
absl::optional<MaybeDocument> mutated_doc =
191+
mutation.ApplyToLocalView(base_doc, batch.local_write_time());
192192

193193
if (mutated_doc && mutated_doc->is_document()) {
194194
results = results.insert(key, Document(*mutated_doc));

‎Firestore/core/src/model/delete_mutation.cc

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,7 @@ MaybeDocument DeleteMutation::Rep::ApplyToRemoteDocument(
6060
}
6161

6262
absl::optional<MaybeDocument> DeleteMutation::Rep::ApplyToLocalView(
63-
const absl::optional<MaybeDocument>& maybe_doc,
64-
const absl::optional<MaybeDocument>&,
65-
const Timestamp&) const {
63+
const absl::optional<MaybeDocument>& maybe_doc, const Timestamp&) const {
6664
VerifyKeyMatches(maybe_doc);
6765

6866
if (!precondition().IsValidFor(maybe_doc)) {

‎Firestore/core/src/model/delete_mutation.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ class DeleteMutation : public Mutation {
5858

5959
absl::optional<MaybeDocument> ApplyToLocalView(
6060
const absl::optional<MaybeDocument>& maybe_doc,
61-
const absl::optional<MaybeDocument>&,
6261
const Timestamp&) const override;
6362

6463
// Does not override Equals or Hash; Mutation's versions are sufficient.

‎Firestore/core/src/model/mutation.cc

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,8 @@ MaybeDocument Mutation::ApplyToRemoteDocument(
5656

5757
absl::optional<MaybeDocument> Mutation::ApplyToLocalView(
5858
const absl::optional<MaybeDocument>& maybe_doc,
59-
const absl::optional<MaybeDocument>& base_doc,
6059
const Timestamp& local_write_time) const {
61-
return rep().ApplyToLocalView(maybe_doc, base_doc, local_write_time);
60+
return rep().ApplyToLocalView(maybe_doc, local_write_time);
6261
}
6362

6463
absl::optional<ObjectValue> Mutation::Rep::ExtractTransformBaseValue(
@@ -147,7 +146,6 @@ std::vector<FieldValue> Mutation::Rep::ServerTransformResults(
147146

148147
std::vector<FieldValue> Mutation::Rep::LocalTransformResults(
149148
const absl::optional<MaybeDocument>& maybe_doc,
150-
const absl::optional<MaybeDocument>& base_doc,
151149
const Timestamp& local_write_time) const {
152150
std::vector<FieldValue> transform_results;
153151
for (const FieldTransform& field_transform : field_transforms_) {
@@ -158,15 +156,6 @@ std::vector<FieldValue> Mutation::Rep::LocalTransformResults(
158156
previous_value = Document(*maybe_doc).field(field_transform.path());
159157
}
160158

161-
// TODO: remove this part
162-
if (!previous_value && base_doc && base_doc->is_document()) {
163-
// If the current document does not contain a value for the mutated field,
164-
// use the value that existed before applying this mutation batch. This
165-
// solves an edge case where a PatchMutation clears the values in a nested
166-
// map before the TransformMutation is applied.
167-
previous_value = Document(*base_doc).field(field_transform.path());
168-
}
169-
170159
transform_results.push_back(
171160
transform.ApplyToLocalView(previous_value, local_write_time));
172161
}

‎Firestore/core/src/model/mutation.h

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -222,9 +222,6 @@ class Mutation {
222222
* @param maybe_doc The document to mutate. The input document can be
223223
* `nullopt` if the client has no knowledge of the pre-mutation state of
224224
* the document.
225-
* @param base_doc The state of the document prior to this mutation batch. The
226-
* input document can be nullopt if the client has no knowledge of the
227-
* pre-mutation state of the document.
228225
* @param local_write_time A timestamp indicating the local write time of the
229226
* batch this mutation is a part of.
230227
* @return The mutated document. The returned document may be nullopt, but
@@ -233,7 +230,6 @@ class Mutation {
233230
*/
234231
absl::optional<MaybeDocument> ApplyToLocalView(
235232
const absl::optional<MaybeDocument>& maybe_doc,
236-
const absl::optional<MaybeDocument>& base_doc,
237233
const Timestamp& local_write_time) const;
238234

239235
/**
@@ -300,7 +296,6 @@ class Mutation {
300296

301297
virtual absl::optional<MaybeDocument> ApplyToLocalView(
302298
const absl::optional<MaybeDocument>& maybe_doc,
303-
const absl::optional<MaybeDocument>& base_doc,
304299
const Timestamp& local_write_time) const = 0;
305300

306301
virtual absl::optional<ObjectValue> ExtractTransformBaseValue(
@@ -334,7 +329,6 @@ class Mutation {
334329
*/
335330
virtual std::vector<FieldValue> LocalTransformResults(
336331
const absl::optional<MaybeDocument>& maybe_doc,
337-
const absl::optional<MaybeDocument>& base_doc,
338332
const Timestamp& local_write_time) const;
339333

340334
virtual ObjectValue TransformObject(

‎Firestore/core/src/model/mutation_batch.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,7 @@ absl::optional<MaybeDocument> MutationBatch::ApplyToLocalDocument(
7474
// transform against a consistent set of values.
7575
for (const Mutation& mutation : base_mutations_) {
7676
if (mutation.key() == document_key) {
77-
maybe_doc =
78-
mutation.ApplyToLocalView(maybe_doc, maybe_doc, local_write_time_);
77+
maybe_doc = mutation.ApplyToLocalView(maybe_doc, local_write_time_);
7978
}
8079
}
8180

@@ -84,8 +83,7 @@ absl::optional<MaybeDocument> MutationBatch::ApplyToLocalDocument(
8483
// Second, apply all user-provided mutations.
8584
for (const Mutation& mutation : mutations_) {
8685
if (mutation.key() == document_key) {
87-
maybe_doc =
88-
mutation.ApplyToLocalView(maybe_doc, base_doc, local_write_time_);
86+
maybe_doc = mutation.ApplyToLocalView(maybe_doc, local_write_time_);
8987
}
9088
}
9189
return maybe_doc;

‎Firestore/core/src/model/patch_mutation.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,6 @@ MaybeDocument PatchMutation::Rep::ApplyToRemoteDocument(
100100

101101
absl::optional<MaybeDocument> PatchMutation::Rep::ApplyToLocalView(
102102
const absl::optional<MaybeDocument>& maybe_doc,
103-
const absl::optional<MaybeDocument>& base_doc,
104103
const Timestamp& local_write_time) const {
105104
VerifyKeyMatches(maybe_doc);
106105

@@ -109,7 +108,7 @@ absl::optional<MaybeDocument> PatchMutation::Rep::ApplyToLocalView(
109108
}
110109

111110
std::vector<FieldValue> transform_results =
112-
LocalTransformResults(maybe_doc, base_doc, local_write_time);
111+
LocalTransformResults(maybe_doc, local_write_time);
113112

114113
ObjectValue new_data = PatchDocument(maybe_doc, transform_results);
115114
SnapshotVersion version = GetPostMutationVersion(maybe_doc);

‎Firestore/core/src/model/patch_mutation.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,6 @@ class PatchMutation : public Mutation {
108108

109109
absl::optional<MaybeDocument> ApplyToLocalView(
110110
const absl::optional<MaybeDocument>& maybe_doc,
111-
const absl::optional<MaybeDocument>& base_doc,
112111
const Timestamp& local_write_time) const override;
113112

114113
bool Equals(const Mutation::Rep& other) const override;

‎Firestore/core/src/model/set_mutation.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ MaybeDocument SetMutation::Rep::ApplyToRemoteDocument(
8989

9090
absl::optional<MaybeDocument> SetMutation::Rep::ApplyToLocalView(
9191
const absl::optional<MaybeDocument>& maybe_doc,
92-
const absl::optional<MaybeDocument>& base_doc,
9392
const Timestamp& local_write_time) const {
9493
VerifyKeyMatches(maybe_doc);
9594

@@ -98,7 +97,7 @@ absl::optional<MaybeDocument> SetMutation::Rep::ApplyToLocalView(
9897
}
9998

10099
std::vector<FieldValue> transforms_results =
101-
LocalTransformResults(maybe_doc, base_doc, local_write_time);
100+
LocalTransformResults(maybe_doc, local_write_time);
102101
ObjectValue new_data = TransformObject(value_, transforms_results);
103102

104103
SnapshotVersion version = GetPostMutationVersion(maybe_doc);

‎Firestore/core/src/model/set_mutation.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,6 @@ class SetMutation : public Mutation {
7979

8080
absl::optional<MaybeDocument> ApplyToLocalView(
8181
const absl::optional<MaybeDocument>& maybe_doc,
82-
const absl::optional<MaybeDocument>& base_doc,
8382
const Timestamp& local_write_time) const override;
8483

8584
bool Equals(const Mutation::Rep& other) const override;

‎Firestore/core/src/model/verify_mutation.cc

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,7 @@ MaybeDocument VerifyMutation::Rep::ApplyToRemoteDocument(
4444
}
4545

4646
absl::optional<MaybeDocument> VerifyMutation::Rep::ApplyToLocalView(
47-
const absl::optional<MaybeDocument>&,
48-
const absl::optional<MaybeDocument>&,
49-
const Timestamp&) const {
47+
const absl::optional<MaybeDocument>&, const Timestamp&) const {
5048
HARD_FAIL("VerifyMutation should only be used in Transactions.");
5149
}
5250

‎Firestore/core/src/model/verify_mutation.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ class VerifyMutation : public Mutation {
6969

7070
absl::optional<MaybeDocument> ApplyToLocalView(
7171
const absl::optional<MaybeDocument>& maybe_doc,
72-
const absl::optional<MaybeDocument>&,
7372
const Timestamp&) const override;
7473

7574
// Does not override Equals or Hash; Mutation's versions are sufficient.

‎Firestore/core/test/unit/model/mutation_test.cc

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ TEST(MutationTest, AppliesSetsToDocuments) {
5555
Doc("collection/key", 0, Map("foo", "foo-value", "baz", "baz-value"));
5656

5757
Mutation set = SetMutation("collection/key", Map("bar", "bar-value"));
58-
auto result = set.ApplyToLocalView(base_doc, base_doc, now);
58+
auto result = set.ApplyToLocalView(base_doc, now);
5959

6060
EXPECT_EQ(result, Doc("collection/key", 0, Map("bar", "bar-value"),
6161
DocumentState::kLocalMutations));
@@ -68,7 +68,7 @@ TEST(MutationTest, AppliesPatchToDocuments) {
6868

6969
Mutation patch =
7070
PatchMutation("collection/key", Map("foo.bar", "new-bar-value"));
71-
auto result = patch.ApplyToLocalView(base_doc, base_doc, now);
71+
auto result = patch.ApplyToLocalView(base_doc, now);
7272

7373
EXPECT_EQ(result,
7474
Doc("collection/key", 0,
@@ -81,7 +81,7 @@ TEST(MutationTest, AppliesPatchWithMergeToNoDocuments) {
8181

8282
Mutation upsert = MergeMutation(
8383
"collection/key", Map("foo.bar", "new-bar-value"), {Field("foo.bar")});
84-
auto result = upsert.ApplyToLocalView(base_doc, base_doc, now);
84+
auto result = upsert.ApplyToLocalView(base_doc, now);
8585

8686
EXPECT_EQ(result,
8787
Doc("collection/key", 0, Map("foo", Map("bar", "new-bar-value")),
@@ -93,7 +93,7 @@ TEST(MutationTest, AppliesPatchWithMergeToNullDocuments) {
9393

9494
Mutation upsert = MergeMutation(
9595
"collection/key", Map("foo.bar", "new-bar-value"), {Field("foo.bar")});
96-
auto result = upsert.ApplyToLocalView(base_doc, base_doc, now);
96+
auto result = upsert.ApplyToLocalView(base_doc, now);
9797

9898
EXPECT_EQ(result,
9999
Doc("collection/key", 0, Map("foo", Map("bar", "new-bar-value")),
@@ -106,7 +106,7 @@ TEST(MutationTest, DeletesValuesFromTheFieldMask) {
106106
Map("foo", Map("bar", "bar-value", "baz", "baz-value")));
107107

108108
Mutation patch = MergeMutation("collection/key", Map(), {Field("foo.bar")});
109-
auto result = patch.ApplyToLocalView(base_doc, base_doc, now);
109+
auto result = patch.ApplyToLocalView(base_doc, now);
110110

111111
EXPECT_EQ(result,
112112
Doc("collection/key", 0, Map("foo", Map("baz", "baz-value")),
@@ -119,7 +119,7 @@ TEST(MutationTest, PatchesPrimitiveValue) {
119119

120120
Mutation patch =
121121
PatchMutation("collection/key", Map("foo.bar", "new-bar-value"));
122-
auto result = patch.ApplyToLocalView(base_doc, base_doc, now);
122+
auto result = patch.ApplyToLocalView(base_doc, now);
123123

124124
EXPECT_EQ(result,
125125
Doc("collection/key", 0,
@@ -131,7 +131,7 @@ TEST(MutationTest, PatchingDeletedDocumentsDoesNothing) {
131131
NoDocument base_doc = testutil::DeletedDoc("collection/key", 0);
132132

133133
Mutation patch = PatchMutation("collection/key", Map("foo", "bar"));
134-
auto result = patch.ApplyToLocalView(base_doc, base_doc, now);
134+
auto result = patch.ApplyToLocalView(base_doc, now);
135135

136136
EXPECT_EQ(result, base_doc);
137137
}
@@ -143,7 +143,7 @@ TEST(MutationTest, AppliesLocalServerTimestampTransformToDocuments) {
143143

144144
Mutation transform = PatchMutation("collection/key", Map(),
145145
{{"foo.bar", ServerTimestampTransform()}});
146-
auto result = transform.ApplyToLocalView(base_doc, base_doc, now);
146+
auto result = transform.ApplyToLocalView(base_doc, now);
147147

148148
// Server timestamps aren't parsed, so we manually insert it.
149149
ObjectValue expected_data =
@@ -178,7 +178,7 @@ void TransformBaseDoc(const FieldValue::Map& base_data,
178178

179179
for (const auto& transform : transforms) {
180180
Mutation mutation = PatchMutation("collection/key", Map(), {transform});
181-
auto result = mutation.ApplyToLocalView(current_doc, current_doc, now);
181+
auto result = mutation.ApplyToLocalView(current_doc, now);
182182
ASSERT_NE(result, absl::nullopt);
183183
ASSERT_EQ(result->type(), MaybeDocument::Type::Document);
184184
current_doc = Document(*result);
@@ -471,7 +471,7 @@ TEST(MutationTest, DeleteDeletes) {
471471
Document base_doc = Doc("collection/key", 0, Map("foo", "bar"));
472472

473473
Mutation del = DeleteMutation("collection/key");
474-
auto result = del.ApplyToLocalView(base_doc, base_doc, now);
474+
auto result = del.ApplyToLocalView(base_doc, now);
475475

476476
EXPECT_EQ(result, DeletedDoc("collection/key", 0));
477477
}

0 commit comments

Comments
 (0)