From 715f4472543108d287ce08e8e41e48e0f6ee4236 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Fri, 9 Feb 2024 14:14:28 -0500 Subject: [PATCH 1/4] Bug fix for flaky behaviour with arrayRemove --- Firestore/core/src/model/value_util.cc | 78 +++++++++++++------ Firestore/core/src/model/value_util.h | 4 + .../core/test/unit/model/value_util_test.cc | 6 ++ 3 files changed, 63 insertions(+), 25 deletions(-) diff --git a/Firestore/core/src/model/value_util.cc b/Firestore/core/src/model/value_util.cc index 012c36955a7..0645c80d66c 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,13 +226,21 @@ 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_Value& left, + const google_firestore_v1_Value& right) { + HARD_ASSERT(left.which_value_type == google_firestore_v1_Value_map_value_tag, + "In function CompareMaps(), left side value is not a map"); + HARD_ASSERT(right.which_value_type == google_firestore_v1_Value_map_value_tag, + "In function CompareMaps(), right side value is not a map"); + + // Sort the given MapValues + auto left_copy = DeepClone(left); + auto right_copy = DeepClone(right); + SortFields(*left_copy); + SortFields(*right_copy); + google_firestore_v1_MapValue left_map = left_copy->map_value; + google_firestore_v1_MapValue right_map = right_copy->map_value; - // 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 = @@ -240,7 +251,7 @@ ComparisonResult CompareObjects(const google_firestore_v1_Value& left, } ComparisonResult value_cmp = - Compare(left_map.fields[i].value, right.map_value.fields[i].value); + Compare(left_map.fields[i].value, right_map.fields[i].value); if (value_cmp != ComparisonResult::Same) { return value_cmp; } @@ -291,7 +302,7 @@ ComparisonResult Compare(const google_firestore_v1_Value& left, return CompareArrays(left, right); case TypeOrder::kMap: - return CompareObjects(left, right); + return CompareMaps(left, right); case TypeOrder::kMaxValue: return util::ComparisonResult::Same; @@ -371,16 +382,19 @@ bool ObjectEquals(const google_firestore_v1_MapValue& left, if (left.fields_count != right.fields_count) { return false; } + // Sort the given MapValues + auto left_copy = DeepClone(left); + auto right_copy = DeepClone(right); + SortFields(*left_copy); + SortFields(*right_copy); - // 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)) { + for (size_t i = 0; i < right_copy->fields_count; ++i) { + if (nanopb::MakeStringView(left_copy->fields[i].key) != + nanopb::MakeStringView(right_copy->fields[i].key)) { return false; } - if (left.fields[i].value != right.fields[i].value) { + if (left_copy->fields[i].value != right_copy->fields[i].value) { return false; } } @@ -832,6 +846,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..0a2883758a5 100644 --- a/Firestore/core/test/unit/model/value_util_test.cc +++ b/Firestore/core/test/unit/model/value_util_test.cc @@ -545,6 +545,12 @@ TEST_F(ValueUtilTest, DeepClone) { VerifyDeepClone(Map("a", Array("b", Map("c", GeoPoint(30, 60))))); } +TEST_F(ValueUtilTest, CompareMapsWithDifferentKeyOrders) { + auto left = Map("a", 3, "b", 5); + auto right = Map("b", 5, "a", 3); + EXPECT_EQ(model::Compare(*left, *right), ComparisonResult::Same); +} + } // namespace } // namespace model From ccd8f7379a10de85e8180e975a4edde5c5f9f5ed Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Fri, 9 Feb 2024 14:29:43 -0500 Subject: [PATCH 2/4] add changelog --- Firestore/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) 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 From 9b2e8d54d1016f7249a6822843b89e98053a20d3 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Mon, 12 Feb 2024 11:09:33 -0500 Subject: [PATCH 3/4] Address feedbacks --- Firestore/core/src/model/value_util.cc | 86 +++++++------------------- 1 file changed, 23 insertions(+), 63 deletions(-) diff --git a/Firestore/core/src/model/value_util.cc b/Firestore/core/src/model/value_util.cc index 0645c80d66c..61c4a8c865f 100644 --- a/Firestore/core/src/model/value_util.cc +++ b/Firestore/core/src/model/value_util.cc @@ -226,38 +226,31 @@ ComparisonResult CompareArrays(const google_firestore_v1_Value& left, right.array_value.values_count); } -ComparisonResult CompareMaps(const google_firestore_v1_Value& left, - const google_firestore_v1_Value& right) { - HARD_ASSERT(left.which_value_type == google_firestore_v1_Value_map_value_tag, - "In function CompareMaps(), left side value is not a map"); - HARD_ASSERT(right.which_value_type == google_firestore_v1_Value_map_value_tag, - "In function CompareMaps(), right side value is not a map"); - +ComparisonResult CompareMaps(const google_firestore_v1_MapValue& left, + const google_firestore_v1_MapValue& right) { // Sort the given MapValues - auto left_copy = DeepClone(left); - auto right_copy = DeepClone(right); - SortFields(*left_copy); - SortFields(*right_copy); - google_firestore_v1_MapValue left_map = left_copy->map_value; - google_firestore_v1_MapValue right_map = right_copy->map_value; - - 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)); + auto left_map = DeepClone(left); + auto right_map = DeepClone(right); + SortFields(*left_map); + SortFields(*right_map); + + 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.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, @@ -302,7 +295,7 @@ ComparisonResult Compare(const google_firestore_v1_Value& left, return CompareArrays(left, right); case TypeOrder::kMap: - return CompareMaps(left, right); + return CompareMaps(left.map_value, right.map_value); case TypeOrder::kMaxValue: return util::ComparisonResult::Same; @@ -377,29 +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; } - // Sort the given MapValues - auto left_copy = DeepClone(left); - auto right_copy = DeepClone(right); - SortFields(*left_copy); - SortFields(*right_copy); - - for (size_t i = 0; i < right_copy->fields_count; ++i) { - if (nanopb::MakeStringView(left_copy->fields[i].key) != - nanopb::MakeStringView(right_copy->fields[i].key)) { - return false; - } - - if (left_copy->fields[i].value != right_copy->fields[i].value) { - return false; - } - } - - return true; + return CompareMaps(left, right) == ComparisonResult::Same; } bool Equals(const google_firestore_v1_Value& lhs, @@ -450,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); @@ -808,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; From 481fe635c6de86c3433ba4c8fe59cb6ad318f571 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Mon, 12 Feb 2024 17:51:01 -0500 Subject: [PATCH 4/4] Add tests --- .../core/test/unit/model/value_util_test.cc | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/Firestore/core/test/unit/model/value_util_test.cc b/Firestore/core/test/unit/model/value_util_test.cc index 0a2883758a5..d4db43dfe20 100644 --- a/Firestore/core/test/unit/model/value_util_test.cc +++ b/Firestore/core/test/unit/model/value_util_test.cc @@ -545,10 +545,22 @@ TEST_F(ValueUtilTest, DeepClone) { VerifyDeepClone(Map("a", Array("b", Map("c", GeoPoint(30, 60))))); } -TEST_F(ValueUtilTest, CompareMapsWithDifferentKeyOrders) { - auto left = Map("a", 3, "b", 5); - auto right = Map("b", 5, "a", 3); - EXPECT_EQ(model::Compare(*left, *right), ComparisonResult::Same); +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