diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index c05338877fd..afa0f58da9a 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,3 +1,6 @@ +# Unreleased +- [fixed] Fix the flaky offline behaviour when using `arrayRemove` on `Map` object. (#12378) + # 10.21.0 - Add an error when trying to build Firestore's binary SPM distribution for visionOS (#12279). See Firestore's 10.12.0 release note for a supported diff --git a/Firestore/core/src/model/value_util.cc b/Firestore/core/src/model/value_util.cc index 012c36955a7..61c4a8c865f 100644 --- a/Firestore/core/src/model/value_util.cc +++ b/Firestore/core/src/model/value_util.cc @@ -109,19 +109,22 @@ void SortFields(google_firestore_v1_ArrayValue& value) { } } +void SortFields(google_firestore_v1_MapValue& value) { + std::sort(value.fields, value.fields + value.fields_count, + [](const google_firestore_v1_MapValue_FieldsEntry& lhs, + const google_firestore_v1_MapValue_FieldsEntry& rhs) { + return nanopb::MakeStringView(lhs.key) < + nanopb::MakeStringView(rhs.key); + }); + + for (pb_size_t i = 0; i < value.fields_count; ++i) { + SortFields(value.fields[i].value); + } +} + void SortFields(google_firestore_v1_Value& value) { if (IsMap(value)) { - google_firestore_v1_MapValue& map_value = value.map_value; - std::sort(map_value.fields, map_value.fields + map_value.fields_count, - [](const google_firestore_v1_MapValue_FieldsEntry& lhs, - const google_firestore_v1_MapValue_FieldsEntry& rhs) { - return nanopb::MakeStringView(lhs.key) < - nanopb::MakeStringView(rhs.key); - }); - - for (pb_size_t i = 0; i < map_value.fields_count; ++i) { - SortFields(map_value.fields[i].value); - } + SortFields(value.map_value); } else if (IsArray(value)) { SortFields(value.array_value); } @@ -223,30 +226,31 @@ ComparisonResult CompareArrays(const google_firestore_v1_Value& left, right.array_value.values_count); } -ComparisonResult CompareObjects(const google_firestore_v1_Value& left, - const google_firestore_v1_Value& right) { - google_firestore_v1_MapValue left_map = left.map_value; - google_firestore_v1_MapValue right_map = right.map_value; +ComparisonResult CompareMaps(const google_firestore_v1_MapValue& left, + const google_firestore_v1_MapValue& right) { + // Sort the given MapValues + auto left_map = DeepClone(left); + auto right_map = DeepClone(right); + SortFields(*left_map); + SortFields(*right_map); - // Porting Note: MapValues in iOS are always kept in sorted order. We - // therefore do no need to sort them before comparing. - for (pb_size_t i = 0; i < left_map.fields_count && i < right_map.fields_count; - ++i) { - ComparisonResult key_cmp = - util::Compare(nanopb::MakeStringView(left_map.fields[i].key), - nanopb::MakeStringView(right_map.fields[i].key)); + for (pb_size_t i = 0; + i < left_map->fields_count && i < right_map->fields_count; ++i) { + const ComparisonResult key_cmp = + util::Compare(nanopb::MakeStringView(left_map->fields[i].key), + nanopb::MakeStringView(right_map->fields[i].key)); if (key_cmp != ComparisonResult::Same) { return key_cmp; } - ComparisonResult value_cmp = - Compare(left_map.fields[i].value, right.map_value.fields[i].value); + const ComparisonResult value_cmp = + Compare(left_map->fields[i].value, right_map->fields[i].value); if (value_cmp != ComparisonResult::Same) { return value_cmp; } } - return util::Compare(left_map.fields_count, right_map.fields_count); + return util::Compare(left_map->fields_count, right_map->fields_count); } ComparisonResult Compare(const google_firestore_v1_Value& left, @@ -291,7 +295,7 @@ ComparisonResult Compare(const google_firestore_v1_Value& left, return CompareArrays(left, right); case TypeOrder::kMap: - return CompareObjects(left, right); + return CompareMaps(left.map_value, right.map_value); case TypeOrder::kMaxValue: return util::ComparisonResult::Same; @@ -366,26 +370,12 @@ bool ArrayEquals(const google_firestore_v1_ArrayValue& left, return true; } -bool ObjectEquals(const google_firestore_v1_MapValue& left, - const google_firestore_v1_MapValue& right) { +bool MapValueEquals(const google_firestore_v1_MapValue& left, + const google_firestore_v1_MapValue& right) { if (left.fields_count != right.fields_count) { return false; } - - // Porting Note: MapValues in iOS are always kept in sorted order. We - // therefore do no need to sort them before comparing. - for (size_t i = 0; i < right.fields_count; ++i) { - if (nanopb::MakeStringView(left.fields[i].key) != - nanopb::MakeStringView(right.fields[i].key)) { - return false; - } - - if (left.fields[i].value != right.fields[i].value) { - return false; - } - } - - return true; + return CompareMaps(left, right) == ComparisonResult::Same; } bool Equals(const google_firestore_v1_Value& lhs, @@ -436,10 +426,10 @@ bool Equals(const google_firestore_v1_Value& lhs, return ArrayEquals(lhs.array_value, rhs.array_value); case TypeOrder::kMap: - return ObjectEquals(lhs.map_value, rhs.map_value); + return MapValueEquals(lhs.map_value, rhs.map_value); case TypeOrder::kMaxValue: - return ObjectEquals(lhs.map_value, rhs.map_value); + return MapValueEquals(lhs.map_value, rhs.map_value); default: HARD_FAIL("Invalid type value: %s", left_type); @@ -794,27 +784,11 @@ Message DeepClone( break; case google_firestore_v1_Value_array_value_tag: - target->array_value.values_count = source.array_value.values_count; - target->array_value.values = nanopb::MakeArray( - source.array_value.values_count); - for (pb_size_t i = 0; i < source.array_value.values_count; ++i) { - target->array_value.values[i] = - *DeepClone(source.array_value.values[i]).release(); - } + target->array_value = *DeepClone(source.array_value).release(); break; case google_firestore_v1_Value_map_value_tag: - target->map_value.fields_count = source.map_value.fields_count; - target->map_value.fields = - nanopb::MakeArray( - source.map_value.fields_count); - for (pb_size_t i = 0; i < source.map_value.fields_count; ++i) { - target->map_value.fields[i].key = - nanopb::MakeBytesArray(source.map_value.fields[i].key->bytes, - source.map_value.fields[i].key->size); - target->map_value.fields[i].value = - *DeepClone(source.map_value.fields[i].value).release(); - } + target->map_value = *DeepClone(source.map_value).release(); break; } return target; @@ -832,6 +806,20 @@ Message DeepClone( return target; } +Message DeepClone( + const google_firestore_v1_MapValue& source) { + Message target{source}; + target->fields_count = source.fields_count; + target->fields = nanopb::MakeArray( + source.fields_count); + for (pb_size_t i = 0; i < source.fields_count; ++i) { + target->fields[i].key = nanopb::MakeBytesArray(source.fields[i].key->bytes, + source.fields[i].key->size); + target->fields[i].value = *DeepClone(source.fields[i].value).release(); + } + return target; +} + } // namespace model } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/model/value_util.h b/Firestore/core/src/model/value_util.h index c39fc8da39b..91e26a21ebb 100644 --- a/Firestore/core/src/model/value_util.h +++ b/Firestore/core/src/model/value_util.h @@ -183,6 +183,10 @@ nanopb::Message DeepClone( nanopb::Message DeepClone( const google_firestore_v1_ArrayValue& source); +/** Creates a copy of the contents of the MapValue proto. */ +nanopb::Message DeepClone( + const google_firestore_v1_MapValue& source); + /** Returns true if `value` is a INTEGER_VALUE. */ inline bool IsInteger(const absl::optional& value) { return value && diff --git a/Firestore/core/test/unit/model/value_util_test.cc b/Firestore/core/test/unit/model/value_util_test.cc index 50ee1b1add2..d4db43dfe20 100644 --- a/Firestore/core/test/unit/model/value_util_test.cc +++ b/Firestore/core/test/unit/model/value_util_test.cc @@ -545,6 +545,24 @@ TEST_F(ValueUtilTest, DeepClone) { VerifyDeepClone(Map("a", Array("b", Map("c", GeoPoint(30, 60))))); } +TEST_F(ValueUtilTest, CompareMaps) { + auto left_1 = Map("a", 7, "b", 0); + auto right_1 = Map("a", 7, "b", 0); + EXPECT_EQ(model::Compare(*left_1, *right_1), ComparisonResult::Same); + + auto left_2 = Map("a", 3, "b", 5); + auto right_2 = Map("b", 5, "a", 3); + EXPECT_EQ(model::Compare(*left_2, *right_2), ComparisonResult::Same); + + auto left_3 = Map("a", 8, "b", 10, "c", 5); + auto right_3 = Map("a", 8, "b", 10); + EXPECT_EQ(model::Compare(*left_3, *right_3), ComparisonResult::Descending); + + auto left_4 = Map("a", 7, "b", 0); + auto right_4 = Map("a", 7, "b", 10); + EXPECT_EQ(model::Compare(*left_4, *right_4), ComparisonResult::Ascending); +} + } // namespace } // namespace model