From 0ac740223bf6d5750ad483fe4832b9715b078f60 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 30 Nov 2018 11:49:05 -0500 Subject: [PATCH 1/3] Rework FieldMask to use a (ordered) set of FieldPaths Rather than a vector. Port of firebase/firebase-android-sdk#137 --- .../src/firebase/firestore/model/field_mask.h | 12 ++++++---- .../firestore/model/field_mask_test.cc | 23 +++++++++++-------- .../firebase/firestore/testutil/testutil.cc | 23 +++++++++++-------- 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/model/field_mask.h b/Firestore/core/src/firebase/firestore/model/field_mask.h index 431e05ae55f..5d4398cb335 100644 --- a/Firestore/core/src/firebase/firestore/model/field_mask.h +++ b/Firestore/core/src/firebase/firestore/model/field_mask.h @@ -18,9 +18,9 @@ #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_MODEL_FIELD_MASK_H_ #include +#include #include #include -#include #include "Firestore/core/src/firebase/firestore/model/field_path.h" #include "Firestore/core/src/firebase/firestore/util/hashing.h" @@ -41,12 +41,14 @@ namespace model { */ class FieldMask { public: - using const_iterator = std::vector::const_iterator; + using const_iterator = std::set::const_iterator; FieldMask(std::initializer_list list) : fields_{list} { } - explicit FieldMask(std::vector fields) - : fields_{std::move(fields)} { + template + FieldMask(InputIt first, InputIt last) : fields_{first, last} { + } + explicit FieldMask(std::set fields) : fields_{std::move(fields)} { } const_iterator begin() const { @@ -94,7 +96,7 @@ class FieldMask { friend bool operator==(const FieldMask& lhs, const FieldMask& rhs); private: - std::vector fields_; + std::set fields_; }; inline bool operator==(const FieldMask& lhs, const FieldMask& rhs) { diff --git a/Firestore/core/test/firebase/firestore/model/field_mask_test.cc b/Firestore/core/test/firebase/firestore/model/field_mask_test.cc index 52d5951510d..c4ab1e4d415 100644 --- a/Firestore/core/test/firebase/firestore/model/field_mask_test.cc +++ b/Firestore/core/test/firebase/firestore/model/field_mask_test.cc @@ -16,7 +16,7 @@ #include "Firestore/core/src/firebase/firestore/model/field_mask.h" -#include +#include #include "Firestore/core/src/firebase/firestore/model/field_path.h" #include "gtest/gtest.h" @@ -28,27 +28,30 @@ namespace model { TEST(FieldMask, ConstructorAndEqual) { FieldMask mask_a{FieldPath::FromServerFormat("foo"), FieldPath::FromServerFormat("bar")}; - std::vector field_path_vector{FieldPath::FromServerFormat("foo"), - FieldPath::FromServerFormat("bar")}; - FieldMask mask_b{field_path_vector}; - FieldMask mask_c{std::vector{FieldPath::FromServerFormat("foo"), - FieldPath::FromServerFormat("bar")}}; + std::set field_path_set{FieldPath::FromServerFormat("foo"), + FieldPath::FromServerFormat("bar")}; + FieldMask mask_b{field_path_set}; + FieldMask mask_c{std::set{FieldPath::FromServerFormat("foo"), + FieldPath::FromServerFormat("bar")}}; + FieldMask mask_d{field_path_set.begin(), field_path_set.end()}; + EXPECT_EQ(mask_a, mask_b); EXPECT_EQ(mask_b, mask_c); + EXPECT_EQ(mask_c, mask_d); } TEST(FieldMask, Getter) { FieldMask mask{FieldPath::FromServerFormat("foo"), FieldPath::FromServerFormat("bar")}; - EXPECT_EQ(std::vector({FieldPath::FromServerFormat("foo"), - FieldPath::FromServerFormat("bar")}), - std::vector(mask.begin(), mask.end())); + EXPECT_EQ(std::set({FieldPath::FromServerFormat("foo"), + FieldPath::FromServerFormat("bar")}), + std::set(mask.begin(), mask.end())); } TEST(FieldMask, ToString) { FieldMask mask{FieldPath::FromServerFormat("foo"), FieldPath::FromServerFormat("bar")}; - EXPECT_EQ("{ foo bar }", mask.ToString()); + EXPECT_EQ("{ bar foo }", mask.ToString()); } } // namespace model diff --git a/Firestore/core/test/firebase/firestore/testutil/testutil.cc b/Firestore/core/test/firebase/firestore/testutil/testutil.cc index aefe5eb240e..c2726102db9 100644 --- a/Firestore/core/test/firebase/firestore/testutil/testutil.cc +++ b/Firestore/core/test/firebase/firestore/testutil/testutil.cc @@ -16,6 +16,8 @@ #include "Firestore/core/test/firebase/firestore/testutil/testutil.h" +#include + namespace firebase { namespace firestore { namespace testutil { @@ -25,11 +27,11 @@ std::unique_ptr PatchMutation( const model::ObjectValue::Map& values, const std::vector* update_mask) { model::FieldValue object_value = model::FieldValue::FromMap({}); - std::vector object_mask; + std::set object_mask; for (const auto& kv : values) { model::FieldPath field_path = Field(kv.first); - object_mask.push_back(field_path); + object_mask.insert(field_path); if (kv.second.string_value() != kDeleteSentinel) { object_value = object_value.Set(field_path, kv.second); } @@ -37,13 +39,16 @@ std::unique_ptr PatchMutation( bool merge = update_mask != nullptr; - // We sort the field_mask_paths to make the order deterministic in tests. - std::sort(object_mask.begin(), object_mask.end()); - - return absl::make_unique( - Key(path), std::move(object_value), - model::FieldMask(merge ? *update_mask : object_mask), - merge ? model::Precondition::None() : model::Precondition::Exists(true)); + if (merge) { + return absl::make_unique( + Key(path), std::move(object_value), + model::FieldMask(update_mask->begin(), update_mask->end()), + model::Precondition::None()); + } else { + return absl::make_unique( + Key(path), std::move(object_value), model::FieldMask(object_mask), + model::Precondition::Exists(true)); + } } } // namespace testutil From 12463003baec0c67da680388a16b7347778bc057 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 30 Nov 2018 16:32:14 -0500 Subject: [PATCH 2/3] Rework FieldMask to use a (ordered) set of FieldPaths in objc. --- Firestore/Example/Tests/Util/FSTHelpers.mm | 8 +++++--- Firestore/Source/API/FSTUserDataConverter.mm | 5 +++-- Firestore/Source/Remote/FSTSerializerBeta.mm | 6 +++--- Firestore/core/src/firebase/firestore/core/user_data.h | 3 ++- Firestore/core/src/firebase/firestore/core/user_data.mm | 2 +- 5 files changed, 14 insertions(+), 10 deletions(-) diff --git a/Firestore/Example/Tests/Util/FSTHelpers.mm b/Firestore/Example/Tests/Util/FSTHelpers.mm index 965dc395aef..15aaa96a3b3 100644 --- a/Firestore/Example/Tests/Util/FSTHelpers.mm +++ b/Firestore/Example/Tests/Util/FSTHelpers.mm @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -252,10 +253,10 @@ NSComparator FSTTestDocComparator(const absl::string_view fieldPath) { BOOL merge = !updateMask.empty(); __block FSTObjectValue *objectValue = [FSTObjectValue objectValue]; - __block std::vector fieldMaskPaths; + __block std::set fieldMaskPaths; [values enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *stop) { const FieldPath path = testutil::Field(util::MakeString(key)); - fieldMaskPaths.push_back(path); + fieldMaskPaths.insert(path); if (![value isEqual:kDeleteSentinel]) { FSTFieldValue *parsedValue = FSTTestFieldValue(value); objectValue = [objectValue objectBySettingValue:parsedValue forPath:path]; @@ -263,7 +264,8 @@ NSComparator FSTTestDocComparator(const absl::string_view fieldPath) { }]; DocumentKey key = testutil::Key(path); - FieldMask mask(merge ? updateMask : fieldMaskPaths); + FieldMask mask(merge ? std::set(updateMask.begin(), updateMask.end()) + : fieldMaskPaths); return [[FSTPatchMutation alloc] initWithKey:key fieldMask:mask value:objectValue diff --git a/Firestore/Source/API/FSTUserDataConverter.mm b/Firestore/Source/API/FSTUserDataConverter.mm index fd75a8f6279..84e717da3b9 100644 --- a/Firestore/Source/API/FSTUserDataConverter.mm +++ b/Firestore/Source/API/FSTUserDataConverter.mm @@ -17,6 +17,7 @@ #import "Firestore/Source/API/FSTUserDataConverter.h" #include +#include #include #include #include @@ -130,7 +131,7 @@ - (ParsedSetData)parsedMergeData:(id)input fieldMask:(nullable NSArray *)fie (FSTObjectValue *)[self parseData:input context:accumulator.RootContext()]; if (fieldMask) { - std::vector validatedFieldPaths; + std::set validatedFieldPaths; for (id fieldPath in fieldMask) { FieldPath path; @@ -150,7 +151,7 @@ - (ParsedSetData)parsedMergeData:(id)input fieldMask:(nullable NSArray *)fie path.CanonicalString().c_str()); } - validatedFieldPaths.push_back(path); + validatedFieldPaths.insert(path); } return std::move(accumulator).MergeData(updateData, FieldMask{std::move(validatedFieldPaths)}); diff --git a/Firestore/Source/Remote/FSTSerializerBeta.mm b/Firestore/Source/Remote/FSTSerializerBeta.mm index 2e52c3302cb..d9748a61a7f 100644 --- a/Firestore/Source/Remote/FSTSerializerBeta.mm +++ b/Firestore/Source/Remote/FSTSerializerBeta.mm @@ -17,6 +17,7 @@ #import "Firestore/Source/Remote/FSTSerializerBeta.h" #include +#include #include #include #include @@ -565,10 +566,9 @@ - (GCFSDocumentMask *)encodedFieldMask:(const FieldMask &)fieldMask { } - (FieldMask)decodedFieldMask:(GCFSDocumentMask *)fieldMask { - std::vector fields; - fields.reserve(fieldMask.fieldPathsArray_Count); + std::set fields; for (NSString *path in fieldMask.fieldPathsArray) { - fields.push_back(FieldPath::FromServerFormat(util::MakeString(path))); + fields.insert(FieldPath::FromServerFormat(util::MakeString(path))); } return FieldMask(std::move(fields)); } diff --git a/Firestore/core/src/firebase/firestore/core/user_data.h b/Firestore/core/src/firebase/firestore/core/user_data.h index 6d95f3ec661..cdb059af5c0 100644 --- a/Firestore/core/src/firebase/firestore/core/user_data.h +++ b/Firestore/core/src/firebase/firestore/core/user_data.h @@ -18,6 +18,7 @@ #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_CORE_USER_DATA_H_ #include +#include #include #include #include @@ -166,7 +167,7 @@ class ParseAccumulator { // field_mask_ and field_transforms_ are shared across all active context // objects to accumulate the result. All ChildContext objects append their // results here. - std::vector field_mask_; + std::set field_mask_; std::vector field_transforms_; }; diff --git a/Firestore/core/src/firebase/firestore/core/user_data.mm b/Firestore/core/src/firebase/firestore/core/user_data.mm index 44314340383..525c0ee212d 100644 --- a/Firestore/core/src/firebase/firestore/core/user_data.mm +++ b/Firestore/core/src/firebase/firestore/core/user_data.mm @@ -58,7 +58,7 @@ } void ParseAccumulator::AddToFieldMask(FieldPath field_path) { - field_mask_.push_back(std::move(field_path)); + field_mask_.insert(std::move(field_path)); } void ParseAccumulator::AddToFieldTransforms( From 741872662daa8a58b70bf7ae9485215960cbc966 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 14 Dec 2018 11:26:43 -0500 Subject: [PATCH 3/3] Add TODO --- Firestore/core/test/firebase/firestore/testutil/testutil.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/Firestore/core/test/firebase/firestore/testutil/testutil.cc b/Firestore/core/test/firebase/firestore/testutil/testutil.cc index c2726102db9..2910df0892a 100644 --- a/Firestore/core/test/firebase/firestore/testutil/testutil.cc +++ b/Firestore/core/test/firebase/firestore/testutil/testutil.cc @@ -25,6 +25,7 @@ namespace testutil { std::unique_ptr PatchMutation( absl::string_view path, const model::ObjectValue::Map& values, + // TODO(rsgowman): Investigate changing update_mask to a set. const std::vector* update_mask) { model::FieldValue object_value = model::FieldValue::FromMap({}); std::set object_mask;