Skip to content

Commit 074de00

Browse files
rsgowmanrsgowman
authored
Rework FieldMask to use a (ordered) set of FieldPaths (#2136)
Rather than a vector. Port of firebase/firebase-android-sdk#137
1 parent 6759b0c commit 074de00

File tree

8 files changed

+49
-34
lines changed

8 files changed

+49
-34
lines changed

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
#include <cinttypes>
2424
#include <list>
25+
#include <set>
2526
#include <unordered_map>
2627
#include <utility>
2728
#include <vector>
@@ -252,18 +253,19 @@ NSComparator FSTTestDocComparator(const absl::string_view fieldPath) {
252253
BOOL merge = !updateMask.empty();
253254

254255
__block FSTObjectValue *objectValue = [FSTObjectValue objectValue];
255-
__block std::vector<FieldPath> fieldMaskPaths;
256+
__block std::set<FieldPath> fieldMaskPaths;
256257
[values enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *stop) {
257258
const FieldPath path = testutil::Field(util::MakeString(key));
258-
fieldMaskPaths.push_back(path);
259+
fieldMaskPaths.insert(path);
259260
if (![value isEqual:kDeleteSentinel]) {
260261
FSTFieldValue *parsedValue = FSTTestFieldValue(value);
261262
objectValue = [objectValue objectBySettingValue:parsedValue forPath:path];
262263
}
263264
}];
264265

265266
DocumentKey key = testutil::Key(path);
266-
FieldMask mask(merge ? updateMask : fieldMaskPaths);
267+
FieldMask mask(merge ? std::set<FieldPath>(updateMask.begin(), updateMask.end())
268+
: fieldMaskPaths);
267269
return [[FSTPatchMutation alloc] initWithKey:key
268270
fieldMask:mask
269271
value:objectValue

‎Firestore/Source/API/FSTUserDataConverter.mm

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#import "Firestore/Source/API/FSTUserDataConverter.h"
1818

1919
#include <memory>
20+
#include <set>
2021
#include <string>
2122
#include <utility>
2223
#include <vector>
@@ -130,7 +131,7 @@ - (ParsedSetData)parsedMergeData:(id)input fieldMask:(nullable NSArray<id> *)fie
130131
(FSTObjectValue *)[self parseData:input context:accumulator.RootContext()];
131132

132133
if (fieldMask) {
133-
std::vector<FieldPath> validatedFieldPaths;
134+
std::set<FieldPath> validatedFieldPaths;
134135
for (id fieldPath in fieldMask) {
135136
FieldPath path;
136137

@@ -150,7 +151,7 @@ - (ParsedSetData)parsedMergeData:(id)input fieldMask:(nullable NSArray<id> *)fie
150151
path.CanonicalString().c_str());
151152
}
152153

153-
validatedFieldPaths.push_back(path);
154+
validatedFieldPaths.insert(path);
154155
}
155156

156157
return std::move(accumulator).MergeData(updateData, FieldMask{std::move(validatedFieldPaths)});

‎Firestore/Source/Remote/FSTSerializerBeta.mm

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#import "Firestore/Source/Remote/FSTSerializerBeta.h"
1818

1919
#include <cinttypes>
20+
#include <set>
2021
#include <string>
2122
#include <utility>
2223
#include <vector>
@@ -570,10 +571,9 @@ - (GCFSDocumentMask *)encodedFieldMask:(const FieldMask &)fieldMask {
570571
}
571572

572573
- (FieldMask)decodedFieldMask:(GCFSDocumentMask *)fieldMask {
573-
std::vector<FieldPath> fields;
574-
fields.reserve(fieldMask.fieldPathsArray_Count);
574+
std::set<FieldPath> fields;
575575
for (NSString *path in fieldMask.fieldPathsArray) {
576-
fields.push_back(FieldPath::FromServerFormat(util::MakeString(path)));
576+
fields.insert(FieldPath::FromServerFormat(util::MakeString(path)));
577577
}
578578
return FieldMask(std::move(fields));
579579
}

‎Firestore/core/src/firebase/firestore/core/user_data.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_CORE_USER_DATA_H_
1919

2020
#include <memory>
21+
#include <set>
2122
#include <string>
2223
#include <utility>
2324
#include <vector>
@@ -166,7 +167,7 @@ class ParseAccumulator {
166167
// field_mask_ and field_transforms_ are shared across all active context
167168
// objects to accumulate the result. All ChildContext objects append their
168169
// results here.
169-
std::vector<model::FieldPath> field_mask_;
170+
std::set<model::FieldPath> field_mask_;
170171
std::vector<model::FieldTransform> field_transforms_;
171172
};
172173

‎Firestore/core/src/firebase/firestore/core/user_data.mm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@
5858
}
5959

6060
void ParseAccumulator::AddToFieldMask(FieldPath field_path) {
61-
field_mask_.push_back(std::move(field_path));
61+
field_mask_.insert(std::move(field_path));
6262
}
6363

6464
void ParseAccumulator::AddToFieldTransforms(

‎Firestore/core/src/firebase/firestore/model/field_mask.h

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@
1818
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_FIELD_MASK_H_
1919

2020
#include <initializer_list>
21+
#include <set>
2122
#include <string>
2223
#include <utility>
23-
#include <vector>
2424

2525
#include "Firestore/core/src/firebase/firestore/model/field_path.h"
2626
#include "Firestore/core/src/firebase/firestore/util/hashing.h"
@@ -41,12 +41,14 @@ namespace model {
4141
*/
4242
class FieldMask {
4343
public:
44-
using const_iterator = std::vector<FieldPath>::const_iterator;
44+
using const_iterator = std::set<FieldPath>::const_iterator;
4545

4646
FieldMask(std::initializer_list<FieldPath> list) : fields_{list} {
4747
}
48-
explicit FieldMask(std::vector<FieldPath> fields)
49-
: fields_{std::move(fields)} {
48+
template <class InputIt>
49+
FieldMask(InputIt first, InputIt last) : fields_{first, last} {
50+
}
51+
explicit FieldMask(std::set<FieldPath> fields) : fields_{std::move(fields)} {
5052
}
5153

5254
const_iterator begin() const {
@@ -94,7 +96,7 @@ class FieldMask {
9496
friend bool operator==(const FieldMask& lhs, const FieldMask& rhs);
9597

9698
private:
97-
std::vector<FieldPath> fields_;
99+
std::set<FieldPath> fields_;
98100
};
99101

100102
inline bool operator==(const FieldMask& lhs, const FieldMask& rhs) {

‎Firestore/core/test/firebase/firestore/model/field_mask_test.cc

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616

1717
#include "Firestore/core/src/firebase/firestore/model/field_mask.h"
1818

19-
#include <vector>
19+
#include <set>
2020

2121
#include "Firestore/core/src/firebase/firestore/model/field_path.h"
2222
#include "gtest/gtest.h"
@@ -28,27 +28,30 @@ namespace model {
2828
TEST(FieldMask, ConstructorAndEqual) {
2929
FieldMask mask_a{FieldPath::FromServerFormat("foo"),
3030
FieldPath::FromServerFormat("bar")};
31-
std::vector<FieldPath> field_path_vector{FieldPath::FromServerFormat("foo"),
32-
FieldPath::FromServerFormat("bar")};
33-
FieldMask mask_b{field_path_vector};
34-
FieldMask mask_c{std::vector<FieldPath>{FieldPath::FromServerFormat("foo"),
35-
FieldPath::FromServerFormat("bar")}};
31+
std::set<FieldPath> field_path_set{FieldPath::FromServerFormat("foo"),
32+
FieldPath::FromServerFormat("bar")};
33+
FieldMask mask_b{field_path_set};
34+
FieldMask mask_c{std::set<FieldPath>{FieldPath::FromServerFormat("foo"),
35+
FieldPath::FromServerFormat("bar")}};
36+
FieldMask mask_d{field_path_set.begin(), field_path_set.end()};
37+
3638
EXPECT_EQ(mask_a, mask_b);
3739
EXPECT_EQ(mask_b, mask_c);
40+
EXPECT_EQ(mask_c, mask_d);
3841
}
3942

4043
TEST(FieldMask, Getter) {
4144
FieldMask mask{FieldPath::FromServerFormat("foo"),
4245
FieldPath::FromServerFormat("bar")};
43-
EXPECT_EQ(std::vector<FieldPath>({FieldPath::FromServerFormat("foo"),
44-
FieldPath::FromServerFormat("bar")}),
45-
std::vector<FieldPath>(mask.begin(), mask.end()));
46+
EXPECT_EQ(std::set<FieldPath>({FieldPath::FromServerFormat("foo"),
47+
FieldPath::FromServerFormat("bar")}),
48+
std::set<FieldPath>(mask.begin(), mask.end()));
4649
}
4750

4851
TEST(FieldMask, ToString) {
4952
FieldMask mask{FieldPath::FromServerFormat("foo"),
5053
FieldPath::FromServerFormat("bar")};
51-
EXPECT_EQ("{ foo bar }", mask.ToString());
54+
EXPECT_EQ("{ bar foo }", mask.ToString());
5255
}
5356

5457
} // namespace model

‎Firestore/core/test/firebase/firestore/testutil/testutil.cc

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,34 +16,40 @@
1616

1717
#include "Firestore/core/test/firebase/firestore/testutil/testutil.h"
1818

19+
#include <set>
20+
1921
namespace firebase {
2022
namespace firestore {
2123
namespace testutil {
2224

2325
std::unique_ptr<model::PatchMutation> PatchMutation(
2426
absl::string_view path,
2527
const model::ObjectValue::Map& values,
28+
// TODO(rsgowman): Investigate changing update_mask to a set.
2629
const std::vector<model::FieldPath>* update_mask) {
2730
model::FieldValue object_value = model::FieldValue::FromMap({});
28-
std::vector<model::FieldPath> object_mask;
31+
std::set<model::FieldPath> object_mask;
2932

3033
for (const auto& kv : values) {
3134
model::FieldPath field_path = Field(kv.first);
32-
object_mask.push_back(field_path);
35+
object_mask.insert(field_path);
3336
if (kv.second.string_value() != kDeleteSentinel) {
3437
object_value = object_value.Set(field_path, kv.second);
3538
}
3639
}
3740

3841
bool merge = update_mask != nullptr;
3942

40-
// We sort the field_mask_paths to make the order deterministic in tests.
41-
std::sort(object_mask.begin(), object_mask.end());
42-
43-
return absl::make_unique<model::PatchMutation>(
44-
Key(path), std::move(object_value),
45-
model::FieldMask(merge ? *update_mask : object_mask),
46-
merge ? model::Precondition::None() : model::Precondition::Exists(true));
43+
if (merge) {
44+
return absl::make_unique<model::PatchMutation>(
45+
Key(path), std::move(object_value),
46+
model::FieldMask(update_mask->begin(), update_mask->end()),
47+
model::Precondition::None());
48+
} else {
49+
return absl::make_unique<model::PatchMutation>(
50+
Key(path), std::move(object_value), model::FieldMask(object_mask),
51+
model::Precondition::Exists(true));
52+
}
4753
}
4854

4955
} // namespace testutil

0 commit comments

Comments
 (0)