From 969f2ed8e9baf2ebb45fee285eb45dd13476fa4d Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 23 Feb 2023 17:33:31 -0800 Subject: [PATCH 01/14] add unchanged names to existence filter and modify spec test runner --- .../Example/Tests/SpecTests/FSTSpecTests.mm | 35 +++++- .../json/existence_filter_spec_test.json | 114 ++++++++++-------- .../Tests/SpecTests/json/limbo_spec_test.json | 24 ++-- .../Tests/SpecTests/json/limit_spec_test.json | 12 +- Firestore/core/src/remote/existence_filter.h | 18 ++- Firestore/core/src/remote/serializer.cc | 2 +- 6 files changed, 137 insertions(+), 68 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index c6c5ba015c9..9cabc78bb41 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -70,12 +70,16 @@ #include "Firestore/core/src/util/string_apple.h" #include "Firestore/core/src/util/to_string.h" #include "Firestore/core/test/unit/testutil/testutil.h" + #include "absl/memory/memory.h" +#include "absl/strings/escaping.h" #include "absl/types/optional.h" namespace objc = firebase::firestore::objc; using firebase::firestore::Error; using firebase::firestore::google_firestore_v1_ArrayValue; +using firebase::firestore::google_firestore_v1_BitSequence; +using firebase::firestore::google_firestore_v1_BloomFilter; using firebase::firestore::google_firestore_v1_Value; using firebase::firestore::api::LoadBundleTask; using firebase::firestore::bundle::BundleReader; @@ -325,6 +329,27 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version { return Version(version.longLongValue); } +- (google_firestore_v1_BloomFilter)parseBloomFilter:(NSDictionary *_Nullable)bloomFilterProto { + NSDictionary *bitsData = bloomFilterProto[@"bits"]; + NSString *bitmapData = bitsData[@"bitmap"]; + + // Decode base64 json string: bitmap + std::string bitmap; + absl::Base64Unescape([bitmapData UTF8String], &bitmap); + + int32_t padding = [bitsData[@"padding"] intValue]; + int32_t hashCount = [bloomFilterProto[@"hashCount"] intValue]; + + // Parse data into bloom filter proto type + google_firestore_v1_BitSequence bits; + bits.bitmap = nanopb::MakeBytesArray(bitmap); + bits.padding = padding; + google_firestore_v1_BloomFilter filter; + filter.hash_count = hashCount; + + return filter; +} + - (DocumentViewChange)parseChange:(NSDictionary *)jsonDoc ofType:(DocumentViewChange::Type)type { NSNumber *version = jsonDoc[@"version"]; NSDictionary *options = jsonDoc[@"options"]; @@ -463,13 +488,15 @@ - (void)doWatchEntity:(NSDictionary *)watchEntity { } } -- (void)doWatchFilter:(NSArray *)watchFilter { - NSArray *targets = watchFilter[0]; +- (void)doWatchFilter:(NSDictionary *)watchFilter { + NSArray *targets = watchFilter[@"targetIds"]; HARD_ASSERT(targets.count == 1, "ExistenceFilters currently support exactly one target only."); - int keyCount = watchFilter.count == 0 ? 0 : (int)watchFilter.count - 1; + NSArray *keys = watchFilter[@"keys"]; + int keyCount = keys ? keys.count : 0; - ExistenceFilter filter{keyCount}; + google_firestore_v1_BloomFilter bloomFilter = [self parseBloomFilter:watchFilter[@"bloomFilter"]]; + ExistenceFilter filter{keyCount, bloomFilter}; ExistenceFilterWatchChange change{filter, targets[0].intValue}; [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()]; } diff --git a/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json b/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json index 3083b762224..75722e8ebcb 100644 --- a/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json @@ -128,12 +128,14 @@ ] }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1" ], - "collection/1" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { @@ -341,13 +343,15 @@ ] }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1", + "collection/2" ], - "collection/1", - "collection/2" - ] + "targetIds": [ + 2 + ] + } }, { "watchEntity": { @@ -694,11 +698,13 @@ } }, { - "watchFilter": [ - [ + "watchFilter": { + "keys": [ + ], + "targetIds": [ 2 ] - ] + } }, { "watchRemove": { @@ -871,12 +877,14 @@ ] }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1" ], - "collection/1" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { @@ -1156,12 +1164,14 @@ ] }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1" ], - "collection/1" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { @@ -1265,12 +1275,14 @@ } }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1" ], - "collection/1" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { @@ -1436,12 +1448,14 @@ ] }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1" ], - "collection/1" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { @@ -1783,12 +1797,14 @@ ] }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1" ], - "collection/1" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { @@ -2078,11 +2094,13 @@ ] }, { - "watchFilter": [ - [ + "watchFilter": { + "keys": [ + ], + "targetIds": [ 2 ] - ] + } }, { "watchSnapshot": { @@ -2193,12 +2211,14 @@ ] }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/1" ], - "collection/1" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { diff --git a/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json b/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json index 0b6abe08a2b..214a9940fc9 100644 --- a/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json @@ -3386,11 +3386,13 @@ ] }, { - "watchFilter": [ - [ + "watchFilter": { + "keys": [ + ], + "targetIds": [ 1 ] - ] + } }, { "watchCurrent": [ @@ -8036,14 +8038,16 @@ } }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/b1", + "collection/b2", + "collection/b3" ], - "collection/b1", - "collection/b2", - "collection/b3" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { diff --git a/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json b/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json index 40e48205956..856e0505d91 100644 --- a/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json @@ -5455,12 +5455,14 @@ } }, { - "watchFilter": [ - [ - 2 + "watchFilter": { + "keys": [ + "collection/b" ], - "collection/b" - ] + "targetIds": [ + 2 + ] + } }, { "watchSnapshot": { diff --git a/Firestore/core/src/remote/existence_filter.h b/Firestore/core/src/remote/existence_filter.h index 3f4019c8987..7be936912cb 100644 --- a/Firestore/core/src/remote/existence_filter.h +++ b/Firestore/core/src/remote/existence_filter.h @@ -17,6 +17,8 @@ #ifndef FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_ #define FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_ +#include "Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.h" + namespace firebase { namespace firestore { namespace remote { @@ -27,16 +29,30 @@ class ExistenceFilter { explicit ExistenceFilter(int count) : count_{count} { } + explicit ExistenceFilter(int count, + google_firestore_v1_BloomFilter unchangedNames) + : count_{count}, unchanged_names_{unchangedNames} { + } + int count() const { return count_; } + google_firestore_v1_BloomFilter unchanged_names() const { + return unchanged_names_; + } + private: int count_ = 0; + google_firestore_v1_BloomFilter unchanged_names_; }; inline bool operator==(const ExistenceFilter& lhs, const ExistenceFilter& rhs) { - return lhs.count() == rhs.count(); + return lhs.count() == rhs.count() && + lhs.unchanged_names().hash_count == rhs.unchanged_names().hash_count && + lhs.unchanged_names().bits.padding == + rhs.unchanged_names().bits.padding && + lhs.unchanged_names().bits.bitmap == rhs.unchanged_names().bits.bitmap; } } // namespace remote diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index 402197739b9..dea5503347b 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -1425,7 +1425,7 @@ std::unique_ptr Serializer::DecodeDocumentRemove( std::unique_ptr Serializer::DecodeExistenceFilterWatchChange( ReadContext*, const google_firestore_v1_ExistenceFilter& filter) const { - ExistenceFilter existence_filter{filter.count}; + ExistenceFilter existence_filter{filter.count, filter.unchanged_names}; return absl::make_unique(existence_filter, filter.target_id); } From 1df9410ba9bb8351c4343b915319c78a804d011b Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 28 Feb 2023 12:24:56 -0800 Subject: [PATCH 02/14] use BloomFilter instead of proto --- .../Firestore.xcodeproj/project.pbxproj | 16 ++++++++++++++++ .../Example/Tests/SpecTests/FSTSpecTests.mm | 17 ++++++----------- Firestore/core/src/remote/bloom_filter.cc | 6 ++++++ Firestore/core/src/remote/bloom_filter.h | 14 +++++++++++++- Firestore/core/src/remote/existence_filter.h | 18 ++++++------------ Firestore/core/src/remote/serializer.cc | 8 +++++--- Firestore/core/src/remote/watch_change.h | 2 +- .../core/test/unit/local/local_store_test.cc | 2 +- .../core/test/unit/remote/remote_event_test.cc | 5 +++-- .../core/test/unit/remote/serializer_test.cc | 2 +- .../core/test/unit/remote/watch_change_test.cc | 2 +- 11 files changed, 59 insertions(+), 33 deletions(-) diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index cae3f9fb903..14a56e7864e 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -342,6 +342,7 @@ 3FFFC1FE083D8BE9C4D9A148 /* string_util_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB380CFC201A2EE200D97691 /* string_util_test.cc */; }; 40431BF2A368D0C891229F6E /* FSTMemorySpecTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E02F20213FFC00B64F25 /* FSTMemorySpecTests.mm */; }; 409C0F2BFC2E1BECFFAC4D32 /* testutil.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54A0352820A3B3BD003E0143 /* testutil.cc */; }; + 416B653E930A609FA121C5E3 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 651321349CA7230B0E4377DA /* md5_testing.cc */; }; 4173B61CB74EB4CD1D89EE68 /* latlng.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 618BBE9220B89AAC00B5BCE7 /* latlng.pb.cc */; }; 4194B7BB8B0352E1AC5D69B9 /* precondition_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 549CCA5520A36E1F00BCEB75 /* precondition_test.cc */; }; 41EAC526C543064B8F3F7EDA /* field_path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B686F2AD2023DDB20028D6BE /* field_path_test.cc */; }; @@ -445,6 +446,7 @@ 544129DD21C2DDC800EFB9CC /* document.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 544129D821C2DDC800EFB9CC /* document.pb.cc */; }; 544129DE21C2DDC800EFB9CC /* write.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 544129D921C2DDC800EFB9CC /* write.pb.cc */; }; 54511E8E209805F8005BD28F /* hashing_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54511E8D209805F8005BD28F /* hashing_test.cc */; }; + 545D42F58BF326180B845350 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 651321349CA7230B0E4377DA /* md5_testing.cc */; }; 5467FB01203E5717009C9584 /* FIRFirestoreTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5467FAFF203E56F8009C9584 /* FIRFirestoreTests.mm */; }; 5467FB08203E6A44009C9584 /* app_testing.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5467FB07203E6A44009C9584 /* app_testing.mm */; }; 546877D52248206A005E3DE0 /* collection_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 54DA129C1F315EE100DD57A1 /* collection_spec_test.json */; }; @@ -883,6 +885,7 @@ 939C898FE9D129F6A2EA259C /* FSTHelpers.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E03A2021401F00B64F25 /* FSTHelpers.mm */; }; 93C8F772F4DC5A985FA3D815 /* task_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 899FC22684B0F7BEEAE13527 /* task_test.cc */; }; 93E5620E3884A431A14500B0 /* document_key_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6152AD5202A5385000E5744 /* document_key_test.cc */; }; + 9484B1C8E9447D397C54B100 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 651321349CA7230B0E4377DA /* md5_testing.cc */; }; 94854FAEAEA75A1AC77A0515 /* memory_bundle_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB4AB1388538CD3CB19EB028 /* memory_bundle_cache_test.cc */; }; 94BBB23B93E449D03FA34F87 /* mutation_queue_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 3068AA9DFBBA86C1FE2A946E /* mutation_queue_test.cc */; }; 95C0F55813DA51E6B8C439E1 /* status_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5493A423225F9990006DE7BA /* status_apple_test.mm */; }; @@ -977,6 +980,7 @@ AB38D93020236E21000A432D /* database_info_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB38D92E20235D22000A432D /* database_info_test.cc */; }; AB6B908420322E4D00CC290A /* document_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB6B908320322E4D00CC290A /* document_test.cc */; }; AB6D588EB21A2C8D40CEB408 /* byte_stream_cpp_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 01D10113ECC5B446DB35E96D /* byte_stream_cpp_test.cc */; }; + AB6FE7D0D8EDD7369E94C0A4 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 651321349CA7230B0E4377DA /* md5_testing.cc */; }; AB7BAB342012B519001E0872 /* geo_point_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB7BAB332012B519001E0872 /* geo_point_test.cc */; }; AB8209455BAA17850D5E196D /* http.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 618BBE9720B89AAC00B5BCE7 /* http.pb.cc */; }; AB9FF792C60FC581909EF381 /* recovery_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 9C1AFCC9E616EC33D6E169CF /* recovery_spec_test.json */; }; @@ -1208,6 +1212,7 @@ D98A0B6007E271E32299C79D /* FIRGeoPointTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E048202154AA00B64F25 /* FIRGeoPointTests.mm */; }; D9DA467E7903412DC6AECDE4 /* grpc_connection_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6D9649021544D4F00EB9CFB /* grpc_connection_test.cc */; }; D9EF7FC0E3F8646B272B427E /* FSTAPIHelpers.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04E202154AA00B64F25 /* FSTAPIHelpers.mm */; }; + DA179A5E064DDFE734DC205E /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 651321349CA7230B0E4377DA /* md5_testing.cc */; }; DA1D665B12AA1062DCDEA6BD /* async_queue_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6FB467B208E9A8200554BA2 /* async_queue_test.cc */; }; DA4303684707606318E1914D /* target_id_generator_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB380CF82019382300D97691 /* target_id_generator_test.cc */; }; DABB9FB61B1733F985CBF713 /* executor_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6FB4688208F9B9100554BA2 /* executor_test.cc */; }; @@ -1314,6 +1319,7 @@ ED420D8F49DA5C41EEF93913 /* FIRSnapshotMetadataTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04D202154AA00B64F25 /* FIRSnapshotMetadataTests.mm */; }; ED4E2AC80CAF2A8FDDAC3DEE /* field_mask_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 549CCA5320A36E1F00BCEB75 /* field_mask_test.cc */; }; ED9DF1EB20025227B38736EC /* message_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = CE37875365497FFA8687B745 /* message_test.cc */; }; + EE3512AC48C1E4B8A190FBB3 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 651321349CA7230B0E4377DA /* md5_testing.cc */; }; EE470CC3C8FBCDA5F70A8466 /* local_store_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 307FF03D0297024D59348EBD /* local_store_test.cc */; }; EE6DBFB0874A50578CE97A7F /* leveldb_remote_document_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 0840319686A223CC4AD3FAB1 /* leveldb_remote_document_cache_test.cc */; }; EECC1EC64CA963A8376FA55C /* persistence_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 9113B6F513D0473AEABBAF1F /* persistence_testing.cc */; }; @@ -1504,6 +1510,7 @@ 403DBF6EFB541DFD01582AA3 /* path_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = path_test.cc; sourceTree = ""; }; 40F9D09063A07F710811A84F /* value_util_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = value_util_test.cc; sourceTree = ""; }; 4132F30044D5DF1FB15B2A9D /* fake_credentials_provider.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = fake_credentials_provider.h; sourceTree = ""; }; + 4196784E9EDA47C4E68E3A20 /* md5_testing.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = md5_testing.h; sourceTree = ""; }; 432C71959255C5DBDF522F52 /* byte_stream_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = byte_stream_test.cc; sourceTree = ""; }; 4334F87873015E3763954578 /* status_testing.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = status_testing.h; sourceTree = ""; }; 444B7AB3F5A2929070CB1363 /* hard_assert_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = hard_assert_test.cc; sourceTree = ""; }; @@ -1658,6 +1665,7 @@ 62E103B28B48A81D682A0DE9 /* Pods_Firestore_Example_tvOS.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Example_tvOS.framework; sourceTree = BUILT_PRODUCTS_DIR; }; 63136A2371C0C013EC7A540C /* target_index_matcher_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = target_index_matcher_test.cc; sourceTree = ""; }; 64AA92CFA356A2360F3C5646 /* filesystem_testing.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = filesystem_testing.h; sourceTree = ""; }; + 651321349CA7230B0E4377DA /* md5_testing.cc */ = {isa = PBXFileReference; includeInIndex = 1; path = md5_testing.cc; sourceTree = ""; }; 69E6C311558EC77729A16CF1 /* Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS.debug.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS.debug.xcconfig"; sourceTree = ""; }; 6AE927CDFC7A72BF825BE4CB /* Pods-Firestore_Tests_tvOS.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Tests_tvOS.release.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Tests_tvOS/Pods-Firestore_Tests_tvOS.release.xcconfig"; sourceTree = ""; }; 6E8302DE210222ED003E1EA3 /* FSTFuzzTestFieldPath.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = FSTFuzzTestFieldPath.h; sourceTree = ""; }; @@ -2024,6 +2032,8 @@ CD422AF3E4515FB8E9BE67A0 /* equals_tester.h */, BA02DA2FCD0001CFC6EB08DA /* filesystem_testing.cc */, 64AA92CFA356A2360F3C5646 /* filesystem_testing.h */, + 651321349CA7230B0E4377DA /* md5_testing.cc */, + 4196784E9EDA47C4E68E3A20 /* md5_testing.h */, 3CAA33F964042646FDDAF9F9 /* status_testing.cc */, 4334F87873015E3763954578 /* status_testing.h */, 54A0352820A3B3BD003E0143 /* testutil.cc */, @@ -3749,6 +3759,7 @@ 3F6C9F8A993CF4B0CD51E7F0 /* lru_garbage_collector_test.cc in Sources */, 12158DFCEE09D24B7988A340 /* maybe_document.pb.cc in Sources */, 52BC453A24D8335C5A57C775 /* md5_test.cc in Sources */, + DA179A5E064DDFE734DC205E /* md5_testing.cc in Sources */, FA43BA0195DA90CE29B29D36 /* memory_bundle_cache_test.cc in Sources */, 8F2055702DB5EE8DA4BACD7C /* memory_document_overlay_cache_test.cc in Sources */, CFF1EBC60A00BA5109893C6E /* memory_index_manager_test.cc in Sources */, @@ -3959,6 +3970,7 @@ 1F56F51EB6DF0951B1F4F85B /* lru_garbage_collector_test.cc in Sources */, 88FD82A1FC5FEC5D56B481D8 /* maybe_document.pb.cc in Sources */, 88C1B719AC0AABE9EEEA71BA /* md5_test.cc in Sources */, + 9484B1C8E9447D397C54B100 /* md5_testing.cc in Sources */, 9611A0FAA2E10A6B1C1AC2EA /* memory_bundle_cache_test.cc in Sources */, 75C6CECF607CA94F56260BAB /* memory_document_overlay_cache_test.cc in Sources */, 3987A3E8534BAA496D966735 /* memory_index_manager_test.cc in Sources */, @@ -4185,6 +4197,7 @@ 913F6E57AF18F84C5ECFD414 /* lru_garbage_collector_test.cc in Sources */, 6F511ABFD023AEB81F92DB12 /* maybe_document.pb.cc in Sources */, 861EA75409AB15BADCD5793F /* md5_test.cc in Sources */, + 545D42F58BF326180B845350 /* md5_testing.cc in Sources */, FF6333B8BD9732C068157221 /* memory_bundle_cache_test.cc in Sources */, 5F6FD840AC2D729B50991CCB /* memory_document_overlay_cache_test.cc in Sources */, E6B825EE85BF20B88AF3E3CD /* memory_index_manager_test.cc in Sources */, @@ -4411,6 +4424,7 @@ 95CE3F5265B9BB7297EE5A6B /* lru_garbage_collector_test.cc in Sources */, C19214F5B43AA745A7FC2FC1 /* maybe_document.pb.cc in Sources */, 82473F290CC7D9579D64A175 /* md5_test.cc in Sources */, + 416B653E930A609FA121C5E3 /* md5_testing.cc in Sources */, 94854FAEAEA75A1AC77A0515 /* memory_bundle_cache_test.cc in Sources */, 053C11420E49AE1A77E21C20 /* memory_document_overlay_cache_test.cc in Sources */, 4D8367018652104A8803E8DB /* memory_index_manager_test.cc in Sources */, @@ -4631,6 +4645,7 @@ 1290FA77A922B76503AE407C /* lru_garbage_collector_test.cc in Sources */, 618BBEA720B89AAC00B5BCE7 /* maybe_document.pb.cc in Sources */, 4EC3518D09A8C37A28763052 /* md5_test.cc in Sources */, + AB6FE7D0D8EDD7369E94C0A4 /* md5_testing.cc in Sources */, A0E1C7F5C7093A498F65C5CF /* memory_bundle_cache_test.cc in Sources */, E56EEC9DAC455E2BE77D110A /* memory_document_overlay_cache_test.cc in Sources */, 3B47CC43DBA24434E215B8ED /* memory_index_manager_test.cc in Sources */, @@ -4876,6 +4891,7 @@ 4DF18D15AC926FB7A4888313 /* lru_garbage_collector_test.cc in Sources */, 12E04A12ABD5533B616D552A /* maybe_document.pb.cc in Sources */, 816375E636FD0EAECFA1E764 /* md5_test.cc in Sources */, + EE3512AC48C1E4B8A190FBB3 /* md5_testing.cc in Sources */, 479A392EAB42453D49435D28 /* memory_bundle_cache_test.cc in Sources */, 5CEB0E83DA68652927D2CF07 /* memory_document_overlay_cache_test.cc in Sources */, 90FE088B8FD9EC06EEED1F39 /* memory_index_manager_test.cc in Sources */, diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index 9cabc78bb41..ce71327b244 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -56,6 +56,7 @@ #include "Firestore/core/src/model/types.h" #include "Firestore/core/src/nanopb/message.h" #include "Firestore/core/src/nanopb/nanopb_util.h" +#include "Firestore/core/src/remote/bloom_filter.h" #include "Firestore/core/src/remote/existence_filter.h" #include "Firestore/core/src/remote/serializer.h" #include "Firestore/core/src/remote/watch_change.h" @@ -78,8 +79,6 @@ namespace objc = firebase::firestore::objc; using firebase::firestore::Error; using firebase::firestore::google_firestore_v1_ArrayValue; -using firebase::firestore::google_firestore_v1_BitSequence; -using firebase::firestore::google_firestore_v1_BloomFilter; using firebase::firestore::google_firestore_v1_Value; using firebase::firestore::api::LoadBundleTask; using firebase::firestore::bundle::BundleReader; @@ -102,6 +101,7 @@ using firebase::firestore::nanopb::ByteString; using firebase::firestore::nanopb::MakeByteString; using firebase::firestore::nanopb::Message; +using firebase::firestore::remote::BloomFilter; using firebase::firestore::remote::DocumentWatchChange; using firebase::firestore::remote::ExistenceFilter; using firebase::firestore::remote::ExistenceFilterWatchChange; @@ -329,10 +329,10 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version { return Version(version.longLongValue); } -- (google_firestore_v1_BloomFilter)parseBloomFilter:(NSDictionary *_Nullable)bloomFilterProto { +- (BloomFilter)parseBloomFilter:(NSDictionary *_Nullable)bloomFilterProto { NSDictionary *bitsData = bloomFilterProto[@"bits"]; - NSString *bitmapData = bitsData[@"bitmap"]; + NSString *bitmapData = bitsData[@"bitmap"]; // Decode base64 json string: bitmap std::string bitmap; absl::Base64Unescape([bitmapData UTF8String], &bitmap); @@ -340,12 +340,7 @@ - (google_firestore_v1_BloomFilter)parseBloomFilter:(NSDictionary *_Nullable)blo int32_t padding = [bitsData[@"padding"] intValue]; int32_t hashCount = [bloomFilterProto[@"hashCount"] intValue]; - // Parse data into bloom filter proto type - google_firestore_v1_BitSequence bits; - bits.bitmap = nanopb::MakeBytesArray(bitmap); - bits.padding = padding; - google_firestore_v1_BloomFilter filter; - filter.hash_count = hashCount; + BloomFilter filter{bitmap, padding, hashCount}; return filter; } @@ -495,7 +490,7 @@ - (void)doWatchFilter:(NSDictionary *)watchFilter { NSArray *keys = watchFilter[@"keys"]; int keyCount = keys ? keys.count : 0; - google_firestore_v1_BloomFilter bloomFilter = [self parseBloomFilter:watchFilter[@"bloomFilter"]]; + BloomFilter bloomFilter = [self parseBloomFilter:watchFilter[@"bloomFilter"]]; ExistenceFilter filter{keyCount, bloomFilter}; ExistenceFilterWatchChange change{filter, targets[0].intValue}; [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()]; diff --git a/Firestore/core/src/remote/bloom_filter.cc b/Firestore/core/src/remote/bloom_filter.cc index b42624a681c..d9ecd690a54 100644 --- a/Firestore/core/src/remote/bloom_filter.cc +++ b/Firestore/core/src/remote/bloom_filter.cc @@ -122,6 +122,12 @@ bool BloomFilter::MightContain(absl::string_view value) const { return true; } +bool operator==(const BloomFilter& lhs, const BloomFilter& rhs) { + return lhs.bit_count() == rhs.bit_count() && + lhs.hash_count() == rhs.hash_count() && + equal(lhs.bitmap().begin(), lhs.bitmap().end(), rhs.bitmap().begin()); +} + } // namespace remote } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/remote/bloom_filter.h b/Firestore/core/src/remote/bloom_filter.h index e406611b1cc..c04c5f69f85 100644 --- a/Firestore/core/src/remote/bloom_filter.h +++ b/Firestore/core/src/remote/bloom_filter.h @@ -57,11 +57,21 @@ class BloomFilter final { */ bool MightContain(absl::string_view value) const; - /** Get the `bit_count_` field. Used for testing purpose. */ + /** Get the `bit_count_` field. */ int32_t bit_count() const { return bit_count_; } + /** Get the `hash_count_` field. */ + int32_t hash_count() const { + return hash_count_; + } + + /** Get the `bitmap_` field. */ + const std::vector& bitmap() const { + return bitmap_; + } + private: /** * When checking membership of a key in bitmap, the first step is to generate @@ -104,6 +114,8 @@ class BloomFilter final { std::vector bitmap_; }; +bool operator==(const BloomFilter& lhs, const BloomFilter& rhs); + } // namespace remote } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/remote/existence_filter.h b/Firestore/core/src/remote/existence_filter.h index 7be936912cb..7ceade64faa 100644 --- a/Firestore/core/src/remote/existence_filter.h +++ b/Firestore/core/src/remote/existence_filter.h @@ -17,7 +17,7 @@ #ifndef FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_ #define FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_ -#include "Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.h" +#include "Firestore/core/src/remote/bloom_filter.h" namespace firebase { namespace firestore { @@ -26,33 +26,27 @@ namespace remote { class ExistenceFilter { public: ExistenceFilter() = default; - explicit ExistenceFilter(int count) : count_{count} { - } - explicit ExistenceFilter(int count, - google_firestore_v1_BloomFilter unchangedNames) - : count_{count}, unchanged_names_{unchangedNames} { + ExistenceFilter(int count, absl::optional unchangedNames) + : count_{count}, unchanged_names_{std::move(unchangedNames)} { } int count() const { return count_; } - google_firestore_v1_BloomFilter unchanged_names() const { + const absl::optional& unchanged_names() const { return unchanged_names_; } private: int count_ = 0; - google_firestore_v1_BloomFilter unchanged_names_; + absl::optional unchanged_names_; }; inline bool operator==(const ExistenceFilter& lhs, const ExistenceFilter& rhs) { return lhs.count() == rhs.count() && - lhs.unchanged_names().hash_count == rhs.unchanged_names().hash_count && - lhs.unchanged_names().bits.padding == - rhs.unchanged_names().bits.padding && - lhs.unchanged_names().bits.bitmap == rhs.unchanged_names().bits.bitmap; + lhs.unchanged_names() == rhs.unchanged_names(); } } // namespace remote diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index dea5503347b..35b9404f5b9 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -1425,9 +1425,11 @@ std::unique_ptr Serializer::DecodeDocumentRemove( std::unique_ptr Serializer::DecodeExistenceFilterWatchChange( ReadContext*, const google_firestore_v1_ExistenceFilter& filter) const { - ExistenceFilter existence_filter{filter.count, filter.unchanged_names}; - return absl::make_unique(existence_filter, - filter.target_id); + // TODO(Mila) + // BloomFilter bloomFilter{} + ExistenceFilter existence_filter{filter.count, absl::nullopt}; + return absl::make_unique( + std::move(existence_filter), filter.target_id); } bool Serializer::IsLocalResourceName(const ResourcePath& path) const { diff --git a/Firestore/core/src/remote/watch_change.h b/Firestore/core/src/remote/watch_change.h index ba2951c66c2..20e6121a7b5 100644 --- a/Firestore/core/src/remote/watch_change.h +++ b/Firestore/core/src/remote/watch_change.h @@ -115,7 +115,7 @@ bool operator==(const DocumentWatchChange& lhs, const DocumentWatchChange& rhs); class ExistenceFilterWatchChange : public WatchChange { public: ExistenceFilterWatchChange(ExistenceFilter filter, model::TargetId target_id) - : filter_{filter}, target_id_{target_id} { + : filter_{std::move(filter)}, target_id_{target_id} { } Type type() const override { diff --git a/Firestore/core/test/unit/local/local_store_test.cc b/Firestore/core/test/unit/local/local_store_test.cc index 0c3e1a1e595..683cc806eb0 100644 --- a/Firestore/core/test/unit/local/local_store_test.cc +++ b/Firestore/core/test/unit/local/local_store_test.cc @@ -146,7 +146,7 @@ RemoteEvent ExistenceFilterEvent(TargetId target_id, remote::FakeTargetMetadataProvider metadata_provider; metadata_provider.SetSyncedKeys(synced_keys, target_data); - ExistenceFilter existence_filter{remote_count}; + ExistenceFilter existence_filter{remote_count, absl::nullopt}; WatchChangeAggregator aggregator{&metadata_provider}; ExistenceFilterWatchChange existence_filter_watch_change{existence_filter, target_id}; diff --git a/Firestore/core/test/unit/remote/remote_event_test.cc b/Firestore/core/test/unit/remote/remote_event_test.cc index 8764a3aae90..355fd530f2b 100644 --- a/Firestore/core/test/unit/remote/remote_event_test.cc +++ b/Firestore/core/test/unit/remote/remote_event_test.cc @@ -547,7 +547,7 @@ TEST_F(RemoteEventTest, ExistenceFilterMismatchClearsTarget) { // The existence filter mismatch will remove the document from target 1, // but not synthesize a document delete. - ExistenceFilterWatchChange change4{ExistenceFilter{1}, 1}; + ExistenceFilterWatchChange change4{ExistenceFilter{1, absl::nullopt}, 1}; aggregator.HandleExistenceFilter(change4); event = aggregator.CreateRemoteEvent(testutil::Version(4)); @@ -578,7 +578,8 @@ TEST_F(RemoteEventTest, ExistenceFilterMismatchRemovesCurrentChanges) { // The existence filter mismatch will remove the document from target 1, but // not synthesize a document delete. - ExistenceFilterWatchChange existence_filter{ExistenceFilter{0}, 1}; + ExistenceFilterWatchChange existence_filter{ExistenceFilter{0, absl::nullopt}, + 1}; aggregator.HandleExistenceFilter(existence_filter); RemoteEvent event = aggregator.CreateRemoteEvent(testutil::Version(3)); diff --git a/Firestore/core/test/unit/remote/serializer_test.cc b/Firestore/core/test/unit/remote/serializer_test.cc index 5c8d08216ed..41b3120cfe3 100644 --- a/Firestore/core/test/unit/remote/serializer_test.cc +++ b/Firestore/core/test/unit/remote/serializer_test.cc @@ -1680,7 +1680,7 @@ TEST_F(SerializerTest, DecodesListenResponseWithDocumentRemove) { } TEST_F(SerializerTest, DecodesListenResponseWithExistenceFilter) { - ExistenceFilterWatchChange model(ExistenceFilter(2), 100); + ExistenceFilterWatchChange model(ExistenceFilter(2, absl::nullopt), 100); v1::ListenResponse proto; diff --git a/Firestore/core/test/unit/remote/watch_change_test.cc b/Firestore/core/test/unit/remote/watch_change_test.cc index 67a7e7d3a6f..d9459a656ac 100644 --- a/Firestore/core/test/unit/remote/watch_change_test.cc +++ b/Firestore/core/test/unit/remote/watch_change_test.cc @@ -41,7 +41,7 @@ TEST(WatchChangeTest, CanCreateDocumentWatchChange) { } TEST(WatchChangeTest, CanCreateExistenceFilterWatchChange) { - ExistenceFilter filter{7}; + ExistenceFilter filter{7, absl::nullopt}; ExistenceFilterWatchChange change{filter, 5}; EXPECT_EQ(change.filter().count(), 7); EXPECT_EQ(change.target_id(), 5); From 1e67033c932d90b2eb7d61713d839d49a23ab82d Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 28 Feb 2023 15:20:12 -0800 Subject: [PATCH 03/14] update serializer --- Firestore/Example/Tests/SpecTests/FSTSpecTests.mm | 5 +++-- Firestore/core/src/remote/existence_filter.h | 2 ++ Firestore/core/src/remote/serializer.cc | 10 ++++++++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index ce71327b244..86445e036e4 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -336,12 +336,12 @@ - (BloomFilter)parseBloomFilter:(NSDictionary *_Nullable)bloomFilterProto { // Decode base64 json string: bitmap std::string bitmap; absl::Base64Unescape([bitmapData UTF8String], &bitmap); + std::vector bitmapVector(bitmap.begin(), bitmap.end()); int32_t padding = [bitsData[@"padding"] intValue]; int32_t hashCount = [bloomFilterProto[@"hashCount"] intValue]; - BloomFilter filter{bitmap, padding, hashCount}; - + BloomFilter filter(bitmapVector, padding, hashCount); return filter; } @@ -491,6 +491,7 @@ - (void)doWatchFilter:(NSDictionary *)watchFilter { int keyCount = keys ? keys.count : 0; BloomFilter bloomFilter = [self parseBloomFilter:watchFilter[@"bloomFilter"]]; + ExistenceFilter filter{keyCount, bloomFilter}; ExistenceFilterWatchChange change{filter, targets[0].intValue}; [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()]; diff --git a/Firestore/core/src/remote/existence_filter.h b/Firestore/core/src/remote/existence_filter.h index 7ceade64faa..76e3adaec89 100644 --- a/Firestore/core/src/remote/existence_filter.h +++ b/Firestore/core/src/remote/existence_filter.h @@ -19,6 +19,8 @@ #include "Firestore/core/src/remote/bloom_filter.h" +#include + namespace firebase { namespace firestore { namespace remote { diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index 35b9404f5b9..2ee82a9d983 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include "Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.h" #include "Firestore/Protos/nanopb/google/firestore/v1/firestore.nanopb.h" @@ -1425,8 +1426,13 @@ std::unique_ptr Serializer::DecodeDocumentRemove( std::unique_ptr Serializer::DecodeExistenceFilterWatchChange( ReadContext*, const google_firestore_v1_ExistenceFilter& filter) const { - // TODO(Mila) - // BloomFilter bloomFilter{} + + // TODO(Mila): convert pb_bytes_array_t into std::vector + ByteString bitmap_string(filter.unchanged_names.bits.bitmap); + std::vector bitmap = MakeVector(bitmap_string); + + BloomFilter bloomFilter(bitmap, filter.unchanged_names.bits.padding, + filter.unchanged_names.hash_count); ExistenceFilter existence_filter{filter.count, absl::nullopt}; return absl::make_unique( std::move(existence_filter), filter.target_id); From 899e3cc380444be74a6ac72c98a422c3db0ff6ca Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 2 Mar 2023 16:53:29 -0800 Subject: [PATCH 04/14] add bloomfilter equality test --- Firestore/core/test/unit/local/local_store_test.cc | 3 ++- .../core/test/unit/remote/bloom_filter_test.cc | 13 +++++++++++++ .../core/test/unit/remote/remote_event_test.cc | 7 ++++--- Firestore/core/test/unit/remote/serializer_test.cc | 3 ++- .../core/test/unit/remote/watch_change_test.cc | 2 +- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/Firestore/core/test/unit/local/local_store_test.cc b/Firestore/core/test/unit/local/local_store_test.cc index 683cc806eb0..a9762c8fd5c 100644 --- a/Firestore/core/test/unit/local/local_store_test.cc +++ b/Firestore/core/test/unit/local/local_store_test.cc @@ -146,7 +146,8 @@ RemoteEvent ExistenceFilterEvent(TargetId target_id, remote::FakeTargetMetadataProvider metadata_provider; metadata_provider.SetSyncedKeys(synced_keys, target_data); - ExistenceFilter existence_filter{remote_count, absl::nullopt}; + ExistenceFilter existence_filter{remote_count, + /*unchangedNames=*/absl::nullopt}; WatchChangeAggregator aggregator{&metadata_provider}; ExistenceFilterWatchChange existence_filter_watch_change{existence_filter, target_id}; diff --git a/Firestore/core/test/unit/remote/bloom_filter_test.cc b/Firestore/core/test/unit/remote/bloom_filter_test.cc index 953cbf93260..a41a630132a 100644 --- a/Firestore/core/test/unit/remote/bloom_filter_test.cc +++ b/Firestore/core/test/unit/remote/bloom_filter_test.cc @@ -107,6 +107,19 @@ TEST(BloomFilterTest, CreateShouldReturnNotOKStatusIfPaddingIsTooLarge) { EXPECT_EQ(maybe_bloom_filter.status().error_message(), "Invalid padding: 8"); } +TEST(BloomFilterTest, CanCheckBloomFiltersEquality) { + { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{1}, 1, 1); + EXPECT_TRUE(bloom_filter1 == bloom_filter2); + } + { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{1}, 2, 1); + EXPECT_FALSE(bloom_filter1 == bloom_filter2); + } +} + TEST(BloomFilterTest, MightContainCanProcessNonStandardCharacters) { // A non-empty BloomFilter object with 1 insertion : "ÀÒ∑" BloomFilter bloom_filter(std::vector{237, 5}, 5, 8); diff --git a/Firestore/core/test/unit/remote/remote_event_test.cc b/Firestore/core/test/unit/remote/remote_event_test.cc index 355fd530f2b..fc8f956d4f6 100644 --- a/Firestore/core/test/unit/remote/remote_event_test.cc +++ b/Firestore/core/test/unit/remote/remote_event_test.cc @@ -547,7 +547,8 @@ TEST_F(RemoteEventTest, ExistenceFilterMismatchClearsTarget) { // The existence filter mismatch will remove the document from target 1, // but not synthesize a document delete. - ExistenceFilterWatchChange change4{ExistenceFilter{1, absl::nullopt}, 1}; + ExistenceFilterWatchChange change4{ + ExistenceFilter{1, /*unchangedNames=*/absl::nullopt}, 1}; aggregator.HandleExistenceFilter(change4); event = aggregator.CreateRemoteEvent(testutil::Version(4)); @@ -578,8 +579,8 @@ TEST_F(RemoteEventTest, ExistenceFilterMismatchRemovesCurrentChanges) { // The existence filter mismatch will remove the document from target 1, but // not synthesize a document delete. - ExistenceFilterWatchChange existence_filter{ExistenceFilter{0, absl::nullopt}, - 1}; + ExistenceFilterWatchChange existence_filter{ + ExistenceFilter{0, /*unchangedNames=*/absl::nullopt}, 1}; aggregator.HandleExistenceFilter(existence_filter); RemoteEvent event = aggregator.CreateRemoteEvent(testutil::Version(3)); diff --git a/Firestore/core/test/unit/remote/serializer_test.cc b/Firestore/core/test/unit/remote/serializer_test.cc index 41b3120cfe3..1000107ab20 100644 --- a/Firestore/core/test/unit/remote/serializer_test.cc +++ b/Firestore/core/test/unit/remote/serializer_test.cc @@ -1680,7 +1680,8 @@ TEST_F(SerializerTest, DecodesListenResponseWithDocumentRemove) { } TEST_F(SerializerTest, DecodesListenResponseWithExistenceFilter) { - ExistenceFilterWatchChange model(ExistenceFilter(2, absl::nullopt), 100); + ExistenceFilterWatchChange model( + ExistenceFilter(2, /*unchangedNames=*/absl::nullopt), 100); v1::ListenResponse proto; diff --git a/Firestore/core/test/unit/remote/watch_change_test.cc b/Firestore/core/test/unit/remote/watch_change_test.cc index d9459a656ac..9dca6e13e4a 100644 --- a/Firestore/core/test/unit/remote/watch_change_test.cc +++ b/Firestore/core/test/unit/remote/watch_change_test.cc @@ -41,7 +41,7 @@ TEST(WatchChangeTest, CanCreateDocumentWatchChange) { } TEST(WatchChangeTest, CanCreateExistenceFilterWatchChange) { - ExistenceFilter filter{7, absl::nullopt}; + ExistenceFilter filter{7, /*unchangedNames=*/absl::nullopt}; ExistenceFilterWatchChange change{filter, 5}; EXPECT_EQ(change.filter().count(), 7); EXPECT_EQ(change.target_id(), 5); From 468f967cc98c7968fb39cf4556485623b5928704 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 7 Mar 2023 10:07:45 -0800 Subject: [PATCH 05/14] make unchanged_names optional in proto --- .../Example/Firestore.xcodeproj/project.pbxproj | 14 ++++---------- .../nanopb/google/firestore/v1/write.nanopb.cc | 8 +++++--- .../nanopb/google/firestore/v1/write.nanopb.h | 5 +++-- .../protos/google/firestore/v1/write.options | 4 ++++ Firestore/core/src/remote/existence_filter.h | 4 ++-- Firestore/core/src/remote/serializer.cc | 14 ++++++++------ 6 files changed, 26 insertions(+), 23 deletions(-) diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index fb1cf85d921..bf0365539d3 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -245,7 +245,6 @@ 264AAB492E24318C5EEB0649 /* memory_lru_garbage_collector_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 9765D47FA12FA283F4EFAD02 /* memory_lru_garbage_collector_test.cc */; }; 26777815544F549DD18D87AF /* message_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = CE37875365497FFA8687B745 /* message_test.cc */; }; 268FC3360157A2DCAF89F92D /* snapshot_version_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = ABA495B9202B7E79008A7851 /* snapshot_version_test.cc */; }; - 2695C8DE8DCAD5D6E9C9407C /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 4007021A5284243AAE77292A /* md5_testing.cc */; }; 26B52236C9D049847042E1BD /* FSTMockDatastore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E02D20213FFC00B64F25 /* FSTMockDatastore.mm */; }; 26B6F5D7571279F4FC06581A /* leveldb_query_engine_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = DB1F1E1B1ED15E8D042144B1 /* leveldb_query_engine_test.cc */; }; 26C4E52128C8E7B5B96BECC4 /* defer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 8ABAC2E0402213D837F73DC3 /* defer_test.cc */; }; @@ -613,6 +612,7 @@ 55427A6CFFB22E069DCC0CC4 /* target_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 526D755F65AC676234F57125 /* target_test.cc */; }; 555161D6DB2DDC8B57F72A70 /* comparison_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 548DB928200D59F600E00ABC /* comparison_test.cc */; }; 5556B648B9B1C2F79A706B4F /* common.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 544129D221C2DDC800EFB9CC /* common.pb.cc */; }; + 55CF4295DAB9B5E7D4DA3D67 /* Validation_BloomFilterTest_MD5_1_1_membership_test_result.json in Resources */ = {isa = PBXBuildFile; fileRef = 0DB24C79879C8290D46F106D /* Validation_BloomFilterTest_MD5_1_1_membership_test_result.json */; }; 55E84644D385A70E607A0F91 /* leveldb_local_store_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 5FF903AEFA7A3284660FA4C5 /* leveldb_local_store_test.cc */; }; 568EC1C0F68A7B95E57C8C6C /* leveldb_key_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54995F6E205B6E12004EFFA0 /* leveldb_key_test.cc */; }; 56D85436D3C864B804851B15 /* string_format_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 9CFD366B783AE27B9E79EE7A /* string_format_apple_test.mm */; }; @@ -964,7 +964,6 @@ 925BE64990449E93242A00A2 /* memory_mutation_queue_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 74FBEFA4FE4B12C435011763 /* memory_mutation_queue_test.cc */; }; 92D7081085679497DC112EDB /* persistence_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 9113B6F513D0473AEABBAF1F /* persistence_testing.cc */; }; 92EFF0CC2993B43CBC7A61FF /* grpc_streaming_reader_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6D964922154AB8F00EB9CFB /* grpc_streaming_reader_test.cc */; }; - 937265DEC2441FA018104C27 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 4007021A5284243AAE77292A /* md5_testing.cc */; }; 9382BE7190E7750EE7CCCE7C /* write_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 54DA12A51F315EE100DD57A1 /* write_spec_test.json */; }; 938F2AF6EC5CD0B839300DB0 /* query.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 544129D621C2DDC800EFB9CC /* query.pb.cc */; }; 939C898FE9D129F6A2EA259C /* FSTHelpers.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E03A2021401F00B64F25 /* FSTHelpers.mm */; }; @@ -1188,7 +1187,6 @@ BB1A6F7D8F06E74FB6E525C5 /* document_key_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6152AD5202A5385000E5744 /* document_key_test.cc */; }; BB3F35B1510FE5449E50EC8A /* bundle_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = F7FC06E0A47D393DE1759AE1 /* bundle_cache_test.cc */; }; BB894A81FDF56EEC19CC29F8 /* FIRQuerySnapshotTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04F202154AA00B64F25 /* FIRQuerySnapshotTests.mm */; }; - BBC5E4D41F51A8BB3F32A627 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 4007021A5284243AAE77292A /* md5_testing.cc */; }; BBDFE0000C4D7E529E296ED4 /* mutation.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 618BBE8220B89AAC00B5BCE7 /* mutation.pb.cc */; }; BC0C98A9201E8F98B9A176A9 /* FIRWriteBatchTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E06F202154D600B64F25 /* FIRWriteBatchTests.mm */; }; BC0EF4474F799EFB1849FE26 /* Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json in Resources */ = {isa = PBXBuildFile; fileRef = 3F8374AFD04A9FF22693AA19 /* Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json */; }; @@ -1313,7 +1311,6 @@ D5B252EE3F4037405DB1ECE3 /* FIRNumericTransformTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = D5B25E7E7D6873CBA4571841 /* FIRNumericTransformTests.mm */; }; D5B25CBF07F65E885C9D68AB /* perf_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = D5B2593BCB52957D62F1C9D3 /* perf_spec_test.json */; }; D5E9954FC1C5ABBC7A180B33 /* FSTSpecTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E03020213FFC00B64F25 /* FSTSpecTests.mm */; }; - D609BD9C3CB32EDFF46D7817 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 4007021A5284243AAE77292A /* md5_testing.cc */; }; D6486C7FFA8BE6F9C7D2F4C4 /* filesystem_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = F51859B394D01C0C507282F1 /* filesystem_test.cc */; }; D658E6DA5A218E08810E1688 /* byte_string_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 5342CDDB137B4E93E2E85CCA /* byte_string_test.cc */; }; D6962E598CEDABA312D87760 /* bundle_reader_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 6ECAF7DE28A19C69DF386D88 /* bundle_reader_test.cc */; }; @@ -1373,7 +1370,6 @@ DF7ABEB48A650117CBEBCD26 /* object_value_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 214877F52A705012D6720CA0 /* object_value_test.cc */; }; DF96816EC67F9B8DF19B0CFD /* document_overlay_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = FFCA39825D9678A03D1845D0 /* document_overlay_cache_test.cc */; }; DF983A9C1FBF758AF3AF110D /* aggregation_result.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = D872D754B8AD88E28AF28B28 /* aggregation_result.pb.cc */; }; - E0008075F6D28D0D8BC46407 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 4007021A5284243AAE77292A /* md5_testing.cc */; }; E0239CE5666437FA1F353C41 /* Validation_BloomFilterTest_MD5_50000_01_membership_test_result.json in Resources */ = {isa = PBXBuildFile; fileRef = 2CF440979CE992E038A54427 /* Validation_BloomFilterTest_MD5_50000_01_membership_test_result.json */; }; E04607A1E2964684184E8AEA /* index_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 8C7278B604B8799F074F4E8C /* index_spec_test.json */; }; E08297B35E12106105F448EB /* ordered_code_benchmark.cc in Sources */ = {isa = PBXBuildFile; fileRef = 0473AFFF5567E667A125347B /* ordered_code_benchmark.cc */; }; @@ -1416,6 +1412,7 @@ E6B0FD6D70A7FFC14719B503 /* Validation_BloomFilterTest_MD5_500_1_bloom_filter_proto.json in Resources */ = {isa = PBXBuildFile; fileRef = 435B519FF670450EF8ECD6B4 /* Validation_BloomFilterTest_MD5_500_1_bloom_filter_proto.json */; }; E6B825EE85BF20B88AF3E3CD /* memory_index_manager_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = DB5A1E760451189DA36028B3 /* memory_index_manager_test.cc */; }; E6F8EB02A0E499F25160BB40 /* FIRFieldPathTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04C202154AA00B64F25 /* FIRFieldPathTests.mm */; }; + E748D5354E230ABA231F4720 /* Validation_BloomFilterTest_MD5_500_0001_membership_test_result.json in Resources */ = {isa = PBXBuildFile; fileRef = 1C46787BE2535DD3B1D5CC71 /* Validation_BloomFilterTest_MD5_500_0001_membership_test_result.json */; }; E764F0F389E7119220EB212C /* target_id_generator_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB380CF82019382300D97691 /* target_id_generator_test.cc */; }; E7CE4B1ECD008983FAB90F44 /* string_format_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54131E9620ADE678001DF3FF /* string_format_test.cc */; }; E7D415B8717701B952C344E5 /* executor_std_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6FB4687208F9B9100554BA2 /* executor_std_test.cc */; }; @@ -1523,7 +1520,6 @@ FABE084FA7DA6E216A41EE80 /* status_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54A0352C20A3B3D7003E0143 /* status_test.cc */; }; FAD97B82766AEC29B7B5A1B7 /* index_manager_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AE4A9E38D65688EE000EE2A1 /* index_manager_test.cc */; }; FAE5DA6ED3E1842DC21453EE /* fake_target_metadata_provider.cc in Sources */ = {isa = PBXBuildFile; fileRef = 71140E5D09C6E76F7C71B2FC /* fake_target_metadata_provider.cc */; }; - FB17BF00D4D6FA1A7E710250 /* md5_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 4007021A5284243AAE77292A /* md5_testing.cc */; }; FB2111D9205822CC8E7368C2 /* FIRDocumentReferenceTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E049202154AA00B64F25 /* FIRDocumentReferenceTests.mm */; }; FB2D5208A6B5816A7244D77A /* query_engine_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B8A853940305237AFDA8050B /* query_engine_test.cc */; }; FB3D9E01547436163C456A3C /* message_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = CE37875365497FFA8687B745 /* message_test.cc */; }; @@ -1663,7 +1659,6 @@ 3F0992A4B83C60841C52E960 /* Pods-Firestore_Example_iOS.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Example_iOS.release.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Example_iOS/Pods-Firestore_Example_iOS.release.xcconfig"; sourceTree = ""; }; 3F8374AFD04A9FF22693AA19 /* Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json */ = {isa = PBXFileReference; includeInIndex = 1; name = Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json; path = bloom_filter_golden_test_data/Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json; sourceTree = ""; }; 3FBAA6F05C0B46A522E3B5A7 /* bundle_cache_test.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = bundle_cache_test.h; sourceTree = ""; }; - 4007021A5284243AAE77292A /* md5_testing.cc */ = {isa = PBXFileReference; includeInIndex = 1; path = md5_testing.cc; sourceTree = ""; }; 403DBF6EFB541DFD01582AA3 /* path_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = path_test.cc; sourceTree = ""; }; 40F9D09063A07F710811A84F /* value_util_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = value_util_test.cc; sourceTree = ""; }; 4132F30044D5DF1FB15B2A9D /* fake_credentials_provider.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = fake_credentials_provider.h; sourceTree = ""; }; @@ -1965,7 +1960,6 @@ CF39535F2C41AB0006FA6C0E /* create_noop_connectivity_monitor.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = create_noop_connectivity_monitor.cc; sourceTree = ""; }; CF39ECA1293D21A0A2AB2626 /* FIRTransactionOptionsTests.mm */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.objcpp; path = FIRTransactionOptionsTests.mm; sourceTree = ""; }; D0A6E9136804A41CEC9D55D4 /* delayed_constructor_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = delayed_constructor_test.cc; sourceTree = ""; }; - D225CE12C489975D3D758414 /* md5_testing.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = md5_testing.h; sourceTree = ""; }; D3CC3DC5338DCAF43A211155 /* README.md */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = net.daringfireball.markdown; name = README.md; path = ../README.md; sourceTree = ""; }; D5B2593BCB52957D62F1C9D3 /* perf_spec_test.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = perf_spec_test.json; sourceTree = ""; }; D5B25E7E7D6873CBA4571841 /* FIRNumericTransformTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FIRNumericTransformTests.mm; sourceTree = ""; }; @@ -3411,7 +3405,7 @@ 67439E2FC35308F50C0643D9 /* Validation_BloomFilterTest_MD5_1_01_bloom_filter_proto.json in Resources */, E9ABBFCEC5A3D2AAEEC961B5 /* Validation_BloomFilterTest_MD5_1_01_membership_test_result.json in Resources */, 9D6CE441655A093BB2EC7693 /* Validation_BloomFilterTest_MD5_1_1_bloom_filter_proto.json in Resources */, - EE2404A60CE148E22EF44851 /* Validation_BloomFilterTest_MD5_1_1_membership_test_result.json in Resources */, + 55CF4295DAB9B5E7D4DA3D67 /* Validation_BloomFilterTest_MD5_1_1_membership_test_result.json in Resources */, 7F7D236E5DF9EBD84E831DAC /* Validation_BloomFilterTest_MD5_50000_0001_bloom_filter_proto.json in Resources */, 951F06B3CCE4EE1568A75B30 /* Validation_BloomFilterTest_MD5_50000_0001_membership_test_result.json in Resources */, 50ED640EA94CEC891C61A914 /* Validation_BloomFilterTest_MD5_50000_01_bloom_filter_proto.json in Resources */, @@ -3425,7 +3419,7 @@ 83A4D0CFF283CBD70EB11B2D /* Validation_BloomFilterTest_MD5_5000_1_bloom_filter_proto.json in Resources */, 0908F0C7C15A1974CF3D20C3 /* Validation_BloomFilterTest_MD5_5000_1_membership_test_result.json in Resources */, A31AC114D37771C1E430CDC4 /* Validation_BloomFilterTest_MD5_500_0001_bloom_filter_proto.json in Resources */, - 941B56B83A1C5771990E14DE /* Validation_BloomFilterTest_MD5_500_0001_membership_test_result.json in Resources */, + E748D5354E230ABA231F4720 /* Validation_BloomFilterTest_MD5_500_0001_membership_test_result.json in Resources */, E8805A3A1DFC1E4EDA65524F /* Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json in Resources */, 2374EA106F4F320120852E81 /* Validation_BloomFilterTest_MD5_500_01_membership_test_result.json in Resources */, 012498C63EE01921671FA0B6 /* Validation_BloomFilterTest_MD5_500_1_bloom_filter_proto.json in Resources */, diff --git a/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.cc b/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.cc index 56275531bd4..407c3d9b38a 100644 --- a/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.cc +++ b/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.cc @@ -95,7 +95,7 @@ const pb_field_t google_firestore_v1_DocumentRemove_fields[4] = { const pb_field_t google_firestore_v1_ExistenceFilter_fields[4] = { PB_FIELD( 1, INT32 , SINGULAR, STATIC , FIRST, google_firestore_v1_ExistenceFilter, target_id, target_id, 0), PB_FIELD( 2, INT32 , SINGULAR, STATIC , OTHER, google_firestore_v1_ExistenceFilter, count, target_id, 0), - PB_FIELD( 3, MESSAGE , SINGULAR, STATIC , OTHER, google_firestore_v1_ExistenceFilter, unchanged_names, count, &google_firestore_v1_BloomFilter_fields), + PB_FIELD( 3, MESSAGE , OPTIONAL, STATIC , OTHER, google_firestore_v1_ExistenceFilter, unchanged_names, count, &google_firestore_v1_BloomFilter_fields), PB_LAST_FIELD }; @@ -317,8 +317,10 @@ std::string google_firestore_v1_ExistenceFilter::ToString(int indent) const { target_id, indent + 1, false); tostring_result += PrintPrimitiveField("count: ", count, indent + 1, false); - tostring_result += PrintMessageField("unchanged_names ", - unchanged_names, indent + 1, false); + if (has_unchanged_names) { + tostring_result += PrintMessageField("unchanged_names ", + unchanged_names, indent + 1, true); + } std::string tostring_tail = PrintTail(indent); return tostring_header + tostring_result + tostring_tail; diff --git a/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.h b/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.h index 3e22c2bc42a..f7dc73102dc 100644 --- a/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.h +++ b/Firestore/Protos/nanopb/google/firestore/v1/write.nanopb.h @@ -110,6 +110,7 @@ typedef struct _google_firestore_v1_DocumentTransform_FieldTransform { typedef struct _google_firestore_v1_ExistenceFilter { int32_t target_id; int32_t count; + bool has_unchanged_names; google_firestore_v1_BloomFilter unchanged_names; std::string ToString(int indent = 0) const; @@ -155,7 +156,7 @@ typedef struct _google_firestore_v1_WriteResult { #define google_firestore_v1_DocumentChange_init_default {google_firestore_v1_Document_init_default, 0, NULL, 0, NULL} #define google_firestore_v1_DocumentDelete_init_default {NULL, false, google_protobuf_Timestamp_init_default, 0, NULL} #define google_firestore_v1_DocumentRemove_init_default {NULL, 0, NULL, google_protobuf_Timestamp_init_default} -#define google_firestore_v1_ExistenceFilter_init_default {0, 0, google_firestore_v1_BloomFilter_init_default} +#define google_firestore_v1_ExistenceFilter_init_default {0, 0, false, google_firestore_v1_BloomFilter_init_default} #define google_firestore_v1_Write_init_zero {0, {google_firestore_v1_Document_init_zero}, false, google_firestore_v1_DocumentMask_init_zero, false, google_firestore_v1_Precondition_init_zero, 0, NULL} #define google_firestore_v1_DocumentTransform_init_zero {NULL, 0, NULL} #define google_firestore_v1_DocumentTransform_FieldTransform_init_zero {NULL, 0, {_google_firestore_v1_DocumentTransform_FieldTransform_ServerValue_MIN}} @@ -163,7 +164,7 @@ typedef struct _google_firestore_v1_WriteResult { #define google_firestore_v1_DocumentChange_init_zero {google_firestore_v1_Document_init_zero, 0, NULL, 0, NULL} #define google_firestore_v1_DocumentDelete_init_zero {NULL, false, google_protobuf_Timestamp_init_zero, 0, NULL} #define google_firestore_v1_DocumentRemove_init_zero {NULL, 0, NULL, google_protobuf_Timestamp_init_zero} -#define google_firestore_v1_ExistenceFilter_init_zero {0, 0, google_firestore_v1_BloomFilter_init_zero} +#define google_firestore_v1_ExistenceFilter_init_zero {0, 0, false, google_firestore_v1_BloomFilter_init_zero} /* Field tags (for use in manual encoding/decoding) */ #define google_firestore_v1_DocumentTransform_document_tag 1 diff --git a/Firestore/Protos/protos/google/firestore/v1/write.options b/Firestore/Protos/protos/google/firestore/v1/write.options index 91805bd55a9..b7955f853c6 100644 --- a/Firestore/Protos/protos/google/firestore/v1/write.options +++ b/Firestore/Protos/protos/google/firestore/v1/write.options @@ -29,3 +29,7 @@ google.firestore.v1.Write.update_mask proto3:false # update_time should not be set for deletes. google.firestore.v1.WriteResult.update_time proto3:false + +# `unchanged_names` field might not be set if ListenRequest doesn't have +# `expected_count` field. +google.firestore.v1.ExistenceFilter.unchanged_names proto3:false \ No newline at end of file diff --git a/Firestore/core/src/remote/existence_filter.h b/Firestore/core/src/remote/existence_filter.h index 76e3adaec89..5e7532e35b0 100644 --- a/Firestore/core/src/remote/existence_filter.h +++ b/Firestore/core/src/remote/existence_filter.h @@ -17,10 +17,10 @@ #ifndef FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_ #define FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_ -#include "Firestore/core/src/remote/bloom_filter.h" - #include +#include "Firestore/core/src/remote/bloom_filter.h" + namespace firebase { namespace firestore { namespace remote { diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index 2ee82a9d983..35c71dc0f39 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -1426,14 +1426,16 @@ std::unique_ptr Serializer::DecodeDocumentRemove( std::unique_ptr Serializer::DecodeExistenceFilterWatchChange( ReadContext*, const google_firestore_v1_ExistenceFilter& filter) const { + absl::optional bloom_filter = absl::nullopt; + if (filter.has_unchanged_names) { + ByteString bitmap_string(filter.unchanged_names.bits.bitmap); + std::vector bitmap = MakeVector(bitmap_string); - // TODO(Mila): convert pb_bytes_array_t into std::vector - ByteString bitmap_string(filter.unchanged_names.bits.bitmap); - std::vector bitmap = MakeVector(bitmap_string); + bloom_filter = BloomFilter(bitmap, filter.unchanged_names.bits.padding, + filter.unchanged_names.hash_count); + } - BloomFilter bloomFilter(bitmap, filter.unchanged_names.bits.padding, - filter.unchanged_names.hash_count); - ExistenceFilter existence_filter{filter.count, absl::nullopt}; + ExistenceFilter existence_filter{filter.count, bloom_filter}; return absl::make_unique( std::move(existence_filter), filter.target_id); } From 23f0fb19d86a9db120d4b410430e070d7699d225 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 7 Mar 2023 15:44:54 -0800 Subject: [PATCH 06/14] handle invalid unchanged-names input with BloomFilter::Create --- .../Example/Tests/SpecTests/FSTSpecTests.mm | 29 ++++++++++++------- .../firestore/v1/bloom_filter.nanopb.cc | 6 ++-- .../google/firestore/v1/bloom_filter.nanopb.h | 5 ++-- .../google/firestore/v1/bloom_filter.options | 21 ++++++++++++++ Firestore/core/src/remote/serializer.cc | 27 ++++++++++++----- Firestore/core/src/remote/serializer.h | 3 ++ 6 files changed, 70 insertions(+), 21 deletions(-) create mode 100644 Firestore/Protos/protos/google/firestore/v1/bloom_filter.options diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index 86445e036e4..05ca08ba9e8 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -119,6 +119,7 @@ using firebase::firestore::util::MakeStringPtr; using firebase::firestore::util::Path; using firebase::firestore::util::Status; +using firebase::firestore::util::StatusOr; using firebase::firestore::util::TimerId; using firebase::firestore::util::ToString; using firebase::firestore::util::WrapCompare; @@ -329,20 +330,28 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version { return Version(version.longLongValue); } -- (BloomFilter)parseBloomFilter:(NSDictionary *_Nullable)bloomFilterProto { +- (absl::optional)parseBloomFilter:(NSDictionary *_Nullable)bloomFilterProto { + if (bloomFilterProto == nil) { + return absl::nullopt; + } NSDictionary *bitsData = bloomFilterProto[@"bits"]; - NSString *bitmapData = bitsData[@"bitmap"]; - // Decode base64 json string: bitmap - std::string bitmap; - absl::Base64Unescape([bitmapData UTF8String], &bitmap); - std::vector bitmapVector(bitmap.begin(), bitmap.end()); + // Decode base64 string into uint8_t vector. If not specified, will default bitmap to empty + // vector. + NSString *bitmapEncoded = bitsData[@"bitmap"]; + std::string bitmapDecoded; + absl::Base64Unescape([bitmapEncoded UTF8String], &bitmapDecoded); + std::vector bitmap(bitmapDecoded.begin(), bitmapDecoded.end()); + // If not specified, will default padding and hashCount to 0. int32_t padding = [bitsData[@"padding"] intValue]; int32_t hashCount = [bloomFilterProto[@"hashCount"] intValue]; + StatusOr maybeBloomFilter = BloomFilter::Create(bitmap, padding, hashCount); + if (maybeBloomFilter.ok()) { + return maybeBloomFilter.ValueOrDie(); + } - BloomFilter filter(bitmapVector, padding, hashCount); - return filter; + return absl::nullopt; } - (DocumentViewChange)parseChange:(NSDictionary *)jsonDoc ofType:(DocumentViewChange::Type)type { @@ -488,9 +497,9 @@ - (void)doWatchFilter:(NSDictionary *)watchFilter { HARD_ASSERT(targets.count == 1, "ExistenceFilters currently support exactly one target only."); NSArray *keys = watchFilter[@"keys"]; - int keyCount = keys ? keys.count : 0; + int keyCount = keys ? (int)keys.count : 0; - BloomFilter bloomFilter = [self parseBloomFilter:watchFilter[@"bloomFilter"]]; + absl::optional bloomFilter = [self parseBloomFilter:watchFilter[@"bloomFilter"]]; ExistenceFilter filter{keyCount, bloomFilter}; ExistenceFilterWatchChange change{filter, targets[0].intValue}; diff --git a/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.cc b/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.cc index 2050bc400fd..45da032ef97 100644 --- a/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.cc +++ b/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.cc @@ -44,7 +44,7 @@ const pb_field_t google_firestore_v1_BitSequence_fields[3] = { }; const pb_field_t google_firestore_v1_BloomFilter_fields[3] = { - PB_FIELD( 1, MESSAGE , SINGULAR, STATIC , FIRST, google_firestore_v1_BloomFilter, bits, bits, &google_firestore_v1_BitSequence_fields), + PB_FIELD( 1, MESSAGE , OPTIONAL, STATIC , FIRST, google_firestore_v1_BloomFilter, bits, bits, &google_firestore_v1_BitSequence_fields), PB_FIELD( 2, INT32 , SINGULAR, STATIC , OTHER, google_firestore_v1_BloomFilter, hash_count, bits, 0), PB_LAST_FIELD }; @@ -96,7 +96,9 @@ std::string google_firestore_v1_BloomFilter::ToString(int indent) const { std::string tostring_header = PrintHeader(indent, "BloomFilter", this); std::string tostring_result; - tostring_result += PrintMessageField("bits ", bits, indent + 1, false); + if (has_bits) { + tostring_result += PrintMessageField("bits ", bits, indent + 1, true); + } tostring_result += PrintPrimitiveField("hash_count: ", hash_count, indent + 1, false); diff --git a/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.h b/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.h index e0347c466a5..925d70db281 100644 --- a/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.h +++ b/Firestore/Protos/nanopb/google/firestore/v1/bloom_filter.nanopb.h @@ -42,6 +42,7 @@ typedef struct _google_firestore_v1_BitSequence { } google_firestore_v1_BitSequence; typedef struct _google_firestore_v1_BloomFilter { + bool has_bits; google_firestore_v1_BitSequence bits; int32_t hash_count; @@ -53,9 +54,9 @@ typedef struct _google_firestore_v1_BloomFilter { /* Initializer values for message structs */ #define google_firestore_v1_BitSequence_init_default {NULL, 0} -#define google_firestore_v1_BloomFilter_init_default {google_firestore_v1_BitSequence_init_default, 0} +#define google_firestore_v1_BloomFilter_init_default {false, google_firestore_v1_BitSequence_init_default, 0} #define google_firestore_v1_BitSequence_init_zero {NULL, 0} -#define google_firestore_v1_BloomFilter_init_zero {google_firestore_v1_BitSequence_init_zero, 0} +#define google_firestore_v1_BloomFilter_init_zero {false, google_firestore_v1_BitSequence_init_zero, 0} /* Field tags (for use in manual encoding/decoding) */ #define google_firestore_v1_BitSequence_bitmap_tag 1 diff --git a/Firestore/Protos/protos/google/firestore/v1/bloom_filter.options b/Firestore/Protos/protos/google/firestore/v1/bloom_filter.options new file mode 100644 index 00000000000..652655cd9d5 --- /dev/null +++ b/Firestore/Protos/protos/google/firestore/v1/bloom_filter.options @@ -0,0 +1,21 @@ +# Copyright 2023 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# In proto3 mode, Nanopb doesn't allow distinguishing between unset fields and +# fields having default values, even for non-primitive types. Using the +# workaround suggested in +# https://github.com/nanopb/nanopb/issues/255#issuecomment-291842903 + +# `bits` might not be set if the BloomFilter is empty. +google.firestore.v1.BloomFilter.bits proto3:false \ No newline at end of file diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index 35c71dc0f39..9b746487ed1 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -1426,18 +1426,31 @@ std::unique_ptr Serializer::DecodeDocumentRemove( std::unique_ptr Serializer::DecodeExistenceFilterWatchChange( ReadContext*, const google_firestore_v1_ExistenceFilter& filter) const { + ExistenceFilter existence_filter = DecodeExistenceFilter(filter); + return absl::make_unique( + std::move(existence_filter), filter.target_id); +} + +ExistenceFilter Serializer::DecodeExistenceFilter( + const google_firestore_v1_ExistenceFilter& filter) const { + // Create bloom filter if there is an unchanged_names present in the filter + // and inputs are valid, otherwise keep it null. absl::optional bloom_filter = absl::nullopt; - if (filter.has_unchanged_names) { + if (filter.has_unchanged_names && filter.unchanged_names.has_bits) { + // TODO(Mila): Ensure bloom filter with invalid inputs are handled correctly + // in next PR. ByteString bitmap_string(filter.unchanged_names.bits.bitmap); std::vector bitmap = MakeVector(bitmap_string); - - bloom_filter = BloomFilter(bitmap, filter.unchanged_names.bits.padding, - filter.unchanged_names.hash_count); + int32_t padding = filter.unchanged_names.bits.padding; + int32_t hash_count = filter.unchanged_names.hash_count; + StatusOr maybe_bloom_filter = + BloomFilter::Create(bitmap, padding, hash_count); + if (maybe_bloom_filter.ok()) { + bloom_filter = maybe_bloom_filter.ValueOrDie(); + } } - ExistenceFilter existence_filter{filter.count, bloom_filter}; - return absl::make_unique( - std::move(existence_filter), filter.target_id); + return {filter.count, bloom_filter}; } bool Serializer::IsLocalResourceName(const ResourcePath& path) const { diff --git a/Firestore/core/src/remote/serializer.h b/Firestore/core/src/remote/serializer.h index ab9370b441a..c42c6c3ac1b 100644 --- a/Firestore/core/src/remote/serializer.h +++ b/Firestore/core/src/remote/serializer.h @@ -341,6 +341,9 @@ class Serializer { util::ReadContext* context, const google_firestore_v1_ExistenceFilter& filter) const; + ExistenceFilter DecodeExistenceFilter( + const google_firestore_v1_ExistenceFilter& filter) const; + model::DatabaseId database_id_; // TODO(varconst): Android caches the result of calling `EncodeDatabaseName` // as well, consider implementing that. From 475fffb4281b0f1dc4f31a10fcde049140c08d8b Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 7 Mar 2023 15:58:03 -0800 Subject: [PATCH 07/14] format the comments --- Firestore/core/src/remote/bloom_filter.h | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/Firestore/core/src/remote/bloom_filter.h b/Firestore/core/src/remote/bloom_filter.h index c04c5f69f85..52178784d2f 100644 --- a/Firestore/core/src/remote/bloom_filter.h +++ b/Firestore/core/src/remote/bloom_filter.h @@ -57,17 +57,24 @@ class BloomFilter final { */ bool MightContain(absl::string_view value) const; - /** Get the `bit_count_` field. */ + /** + * The number of bits in the bloom filter. Guaranteed to be non-negative, and + * less than the max number of bits `bitmap_` can represent, i.e., + * bitmap_.size() * 8. + */ int32_t bit_count() const { return bit_count_; } - /** Get the `hash_count_` field. */ + /** + * The number of hash functions used to construct the filter. Guaranteed to + * be non-negative. + */ int32_t hash_count() const { return hash_count_; } - /** Get the `bitmap_` field. */ + /** Bloom filter's bitmap. */ const std::vector& bitmap() const { return bitmap_; } @@ -97,20 +104,10 @@ class BloomFilter final { /** Return whether the bit at the given index in the bitmap is set to 1. */ bool IsBitSet(int32_t index) const; - /** - * The number of bits in the bloom filter. Guaranteed to be non-negative, and - * less than the max number of bits `bitmap_` can represent, i.e., - * bitmap_.size() * 8. - */ int32_t bit_count_ = 0; - /** - * The number of hash functions used to construct the filter. Guaranteed to - * be non-negative. - */ int32_t hash_count_ = 0; - /** Bloom filter's bitmap. */ std::vector bitmap_; }; From 05caaebd40acd70c01f1f4df2ff9d269add5e535 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 13 Mar 2023 16:07:04 -0700 Subject: [PATCH 08/14] change unchanged_names to Bloom_filter --- Firestore/core/src/remote/existence_filter.h | 13 ++++++------- Firestore/core/test/unit/local/local_store_test.cc | 2 +- .../core/test/unit/remote/remote_event_test.cc | 4 ++-- Firestore/core/test/unit/remote/serializer_test.cc | 2 +- .../core/test/unit/remote/watch_change_test.cc | 2 +- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/Firestore/core/src/remote/existence_filter.h b/Firestore/core/src/remote/existence_filter.h index 5e7532e35b0..968341df588 100644 --- a/Firestore/core/src/remote/existence_filter.h +++ b/Firestore/core/src/remote/existence_filter.h @@ -29,26 +29,25 @@ class ExistenceFilter { public: ExistenceFilter() = default; - ExistenceFilter(int count, absl::optional unchangedNames) - : count_{count}, unchanged_names_{std::move(unchangedNames)} { + ExistenceFilter(int count, absl::optional bloom_filter) + : count_{count}, bloom_filter_{std::move(bloom_filter)} { } int count() const { return count_; } - const absl::optional& unchanged_names() const { - return unchanged_names_; + const absl::optional& bloom_filter() const { + return bloom_filter_; } private: int count_ = 0; - absl::optional unchanged_names_; + absl::optional bloom_filter_; }; inline bool operator==(const ExistenceFilter& lhs, const ExistenceFilter& rhs) { - return lhs.count() == rhs.count() && - lhs.unchanged_names() == rhs.unchanged_names(); + return lhs.count() == rhs.count() && lhs.bloom_filter() == rhs.bloom_filter(); } } // namespace remote diff --git a/Firestore/core/test/unit/local/local_store_test.cc b/Firestore/core/test/unit/local/local_store_test.cc index a9762c8fd5c..834a20a364c 100644 --- a/Firestore/core/test/unit/local/local_store_test.cc +++ b/Firestore/core/test/unit/local/local_store_test.cc @@ -147,7 +147,7 @@ RemoteEvent ExistenceFilterEvent(TargetId target_id, metadata_provider.SetSyncedKeys(synced_keys, target_data); ExistenceFilter existence_filter{remote_count, - /*unchangedNames=*/absl::nullopt}; + /*bloom_filter=*/absl::nullopt}; WatchChangeAggregator aggregator{&metadata_provider}; ExistenceFilterWatchChange existence_filter_watch_change{existence_filter, target_id}; diff --git a/Firestore/core/test/unit/remote/remote_event_test.cc b/Firestore/core/test/unit/remote/remote_event_test.cc index fc8f956d4f6..05f06722578 100644 --- a/Firestore/core/test/unit/remote/remote_event_test.cc +++ b/Firestore/core/test/unit/remote/remote_event_test.cc @@ -548,7 +548,7 @@ TEST_F(RemoteEventTest, ExistenceFilterMismatchClearsTarget) { // The existence filter mismatch will remove the document from target 1, // but not synthesize a document delete. ExistenceFilterWatchChange change4{ - ExistenceFilter{1, /*unchangedNames=*/absl::nullopt}, 1}; + ExistenceFilter{1, /*bloom_filter=*/absl::nullopt}, 1}; aggregator.HandleExistenceFilter(change4); event = aggregator.CreateRemoteEvent(testutil::Version(4)); @@ -580,7 +580,7 @@ TEST_F(RemoteEventTest, ExistenceFilterMismatchRemovesCurrentChanges) { // The existence filter mismatch will remove the document from target 1, but // not synthesize a document delete. ExistenceFilterWatchChange existence_filter{ - ExistenceFilter{0, /*unchangedNames=*/absl::nullopt}, 1}; + ExistenceFilter{0, /*bloom_filter=*/absl::nullopt}, 1}; aggregator.HandleExistenceFilter(existence_filter); RemoteEvent event = aggregator.CreateRemoteEvent(testutil::Version(3)); diff --git a/Firestore/core/test/unit/remote/serializer_test.cc b/Firestore/core/test/unit/remote/serializer_test.cc index f67e45b6735..7bd10ff3838 100644 --- a/Firestore/core/test/unit/remote/serializer_test.cc +++ b/Firestore/core/test/unit/remote/serializer_test.cc @@ -1739,7 +1739,7 @@ TEST_F(SerializerTest, DecodesListenResponseWithDocumentRemove) { TEST_F(SerializerTest, DecodesListenResponseWithExistenceFilter) { ExistenceFilterWatchChange model( - ExistenceFilter(2, /*unchangedNames=*/absl::nullopt), 100); + ExistenceFilter(2, /*bloom_filter=*/absl::nullopt), 100); v1::ListenResponse proto; diff --git a/Firestore/core/test/unit/remote/watch_change_test.cc b/Firestore/core/test/unit/remote/watch_change_test.cc index 9dca6e13e4a..39c3a049c3f 100644 --- a/Firestore/core/test/unit/remote/watch_change_test.cc +++ b/Firestore/core/test/unit/remote/watch_change_test.cc @@ -41,7 +41,7 @@ TEST(WatchChangeTest, CanCreateDocumentWatchChange) { } TEST(WatchChangeTest, CanCreateExistenceFilterWatchChange) { - ExistenceFilter filter{7, /*unchangedNames=*/absl::nullopt}; + ExistenceFilter filter{7, /*bloom_filter=*/absl::nullopt}; ExistenceFilterWatchChange change{filter, 5}; EXPECT_EQ(change.filter().count(), 7); EXPECT_EQ(change.target_id(), 5); From d56f4dfd3c9f641760263196938de31c427a7d1e Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 13 Mar 2023 21:23:31 -0700 Subject: [PATCH 09/14] remove the parsing unchanged_names to BloomFilter logic --- .../Example/Tests/SpecTests/FSTSpecTests.mm | 24 ++++--------------- Firestore/core/src/remote/serializer.cc | 12 ++++------ 2 files changed, 9 insertions(+), 27 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index 0ee79ac7a2f..d14b4aceead 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -331,27 +331,11 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version { } - (absl::optional)parseBloomFilter:(NSDictionary *_Nullable)bloomFilterProto { - if (bloomFilterProto == nil) { - return absl::nullopt; - } - NSDictionary *bitsData = bloomFilterProto[@"bits"]; - - // Decode base64 string into uint8_t vector. If not specified, will default bitmap to empty - // vector. - NSString *bitmapEncoded = bitsData[@"bitmap"]; - std::string bitmapDecoded; - absl::Base64Unescape([bitmapEncoded UTF8String], &bitmapDecoded); - std::vector bitmap(bitmapDecoded.begin(), bitmapDecoded.end()); - - // If not specified, will default padding and hashCount to 0. - int32_t padding = [bitsData[@"padding"] intValue]; - int32_t hashCount = [bloomFilterProto[@"hashCount"] intValue]; - StatusOr maybeBloomFilter = BloomFilter::Create(bitmap, padding, hashCount); - if (maybeBloomFilter.ok()) { - return maybeBloomFilter.ValueOrDie(); - } - + // TODO(Mila): None of the ported spec tests actually has the bloom filter json string, so hard + // code a null value for now. Actual parsing code will be written in the next PR, where we can + // validate the parsing result. return absl::nullopt; + ; } - (DocumentViewChange)parseChange:(NSDictionary *)jsonDoc ofType:(DocumentViewChange::Type)type { diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index 82ca1eefb16..5c92d481e07 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -1441,14 +1441,12 @@ ExistenceFilter Serializer::DecodeExistenceFilter( // and inputs are valid, otherwise keep it null. absl::optional bloom_filter = absl::nullopt; if (filter.has_unchanged_names && filter.unchanged_names.has_bits) { - // TODO(Mila): Ensure bloom filter with invalid inputs are handled correctly - // in next PR. - ByteString bitmap_string(filter.unchanged_names.bits.bitmap); - std::vector bitmap = MakeVector(bitmap_string); - int32_t padding = filter.unchanged_names.bits.padding; - int32_t hash_count = filter.unchanged_names.hash_count; + // TODO(Mila): None of the ported spec tests here actually has the bloom + // filter json string, so hard code an empty bloom filter for now. The + // actual parsing code will be written in the next PR, where we can validate + // the parsing result. StatusOr maybe_bloom_filter = - BloomFilter::Create(bitmap, padding, hash_count); + BloomFilter::Create(std::vector{}, 0, 0); if (maybe_bloom_filter.ok()) { bloom_filter = maybe_bloom_filter.ValueOrDie(); } From 91a42e632364a57e7db19316085b19994f1b5ec4 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 16 Mar 2023 14:56:24 -0700 Subject: [PATCH 10/14] resolve comments --- .../Example/Tests/SpecTests/FSTSpecTests.mm | 4 +- Firestore/core/src/remote/bloom_filter.cc | 22 ++++++++- Firestore/core/src/remote/bloom_filter.h | 8 ++- Firestore/core/src/remote/serializer.cc | 7 +-- .../test/unit/remote/bloom_filter_test.cc | 49 ++++++++++++++----- .../test/unit/remote/remote_event_test.cc | 1 + .../core/test/unit/remote/serializer_test.cc | 1 + .../test/unit/remote/watch_change_test.cc | 1 + 8 files changed, 73 insertions(+), 20 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index d14b4aceead..009aca8a542 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -485,8 +485,8 @@ - (void)doWatchFilter:(NSDictionary *)watchFilter { absl::optional bloomFilter = [self parseBloomFilter:watchFilter[@"bloomFilter"]]; - ExistenceFilter filter{keyCount, bloomFilter}; - ExistenceFilterWatchChange change{filter, targets[0].intValue}; + ExistenceFilter filter{keyCount, std::move(bloomFilter)}; + ExistenceFilterWatchChange change{std::move(filter), targets[0].intValue}; [self.driver receiveWatchChange:change snapshotVersion:SnapshotVersion::None()]; } diff --git a/Firestore/core/src/remote/bloom_filter.cc b/Firestore/core/src/remote/bloom_filter.cc index d893677bd1d..ad4c5bcbe7c 100644 --- a/Firestore/core/src/remote/bloom_filter.cc +++ b/Firestore/core/src/remote/bloom_filter.cc @@ -115,8 +115,26 @@ bool BloomFilter::MightContain(absl::string_view value) const { bool operator==(const BloomFilter& lhs, const BloomFilter& rhs) { return lhs.bit_count() == rhs.bit_count() && - lhs.hash_count() == rhs.hash_count() && - equal(lhs.bitmap().begin(), lhs.bitmap().end(), rhs.bitmap().begin()); + lhs.hash_count() == rhs.hash_count() && CompareBits(lhs, rhs); +} + +bool operator!=(const BloomFilter& lhs, const BloomFilter& rhs) { + return !(lhs == rhs); +} + +bool CompareBits(const BloomFilter& lhs, const BloomFilter& rhs) { + for (int32_t i = 0; i < lhs.bit_count(); ++i) { + int32_t offset = i % 8; + const uint8_t& lhs_byte = lhs.bitmap()[i / 8]; + const uint8_t& rhs_byte = rhs.bitmap()[i / 8]; + const bool bit1 = (lhs_byte & (static_cast(0x01) << offset)); + const bool bit2 = (rhs_byte & (static_cast(0x01) << offset)); + if (bit1 != bit2) { + return false; + } + } + + return true; } } // namespace remote diff --git a/Firestore/core/src/remote/bloom_filter.h b/Firestore/core/src/remote/bloom_filter.h index 52178784d2f..b5bb2003f93 100644 --- a/Firestore/core/src/remote/bloom_filter.h +++ b/Firestore/core/src/remote/bloom_filter.h @@ -59,8 +59,8 @@ class BloomFilter final { /** * The number of bits in the bloom filter. Guaranteed to be non-negative, and - * less than the max number of bits `bitmap_` can represent, i.e., - * bitmap_.size() * 8. + * less than the max number of bits the bitmap can represent, i.e., + * bitmap().size() * 8. */ int32_t bit_count() const { return bit_count_; @@ -113,6 +113,10 @@ class BloomFilter final { bool operator==(const BloomFilter& lhs, const BloomFilter& rhs); +bool operator!=(const BloomFilter& lhs, const BloomFilter& rhs); + +bool CompareBits(const BloomFilter& lhs, const BloomFilter& rhs); + } // namespace remote } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index 5c92d481e07..fbdc82e6eae 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -1439,20 +1439,21 @@ ExistenceFilter Serializer::DecodeExistenceFilter( const google_firestore_v1_ExistenceFilter& filter) const { // Create bloom filter if there is an unchanged_names present in the filter // and inputs are valid, otherwise keep it null. - absl::optional bloom_filter = absl::nullopt; + absl::optional bloom_filter; if (filter.has_unchanged_names && filter.unchanged_names.has_bits) { // TODO(Mila): None of the ported spec tests here actually has the bloom // filter json string, so hard code an empty bloom filter for now. The // actual parsing code will be written in the next PR, where we can validate // the parsing result. + // TODO(Mila): handle bloom filter creation failure. StatusOr maybe_bloom_filter = BloomFilter::Create(std::vector{}, 0, 0); if (maybe_bloom_filter.ok()) { - bloom_filter = maybe_bloom_filter.ValueOrDie(); + bloom_filter = std::move(maybe_bloom_filter).ValueOrDie(); } } - return {filter.count, bloom_filter}; + return {filter.count, std::move(bloom_filter)}; } bool Serializer::IsLocalResourceName(const ResourcePath& path) const { diff --git a/Firestore/core/test/unit/remote/bloom_filter_test.cc b/Firestore/core/test/unit/remote/bloom_filter_test.cc index 640061a72e6..93583f63d0f 100644 --- a/Firestore/core/test/unit/remote/bloom_filter_test.cc +++ b/Firestore/core/test/unit/remote/bloom_filter_test.cc @@ -118,17 +118,44 @@ TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusIfPaddingIsTooLarge) { EXPECT_EQ(maybe_bloom_filter.status().error_message(), "Invalid padding: 8"); } -TEST(BloomFilterTest, CanCheckBloomFiltersEquality) { - { - BloomFilter bloom_filter1(std::vector{1}, 1, 1); - BloomFilter bloom_filter2(std::vector{1}, 1, 1); - EXPECT_TRUE(bloom_filter1 == bloom_filter2); - } - { - BloomFilter bloom_filter1(std::vector{1}, 1, 1); - BloomFilter bloom_filter2(std::vector{1}, 2, 1); - EXPECT_FALSE(bloom_filter1 == bloom_filter2); - } +TEST(BloomFilterTest, CheckBloomFiltersEqualityWithSameInput) { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{1}, 1, 1); + EXPECT_TRUE(bloom_filter1 == bloom_filter2); + EXPECT_FALSE(bloom_filter1 != bloom_filter2); +} + +TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentBitmap) { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{2}, 1, 1); + EXPECT_FALSE(bloom_filter1 == bloom_filter2); + EXPECT_TRUE(bloom_filter1 != bloom_filter2); +} + +TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentPadding) { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{1}, 2, 1); + EXPECT_FALSE(bloom_filter1 == bloom_filter2); + EXPECT_TRUE(bloom_filter1 != bloom_filter2); +} + +TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentHashCount) { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{1}, 1, 2); + EXPECT_FALSE(bloom_filter1 == bloom_filter2); + EXPECT_TRUE(bloom_filter1 != bloom_filter2); +} + +TEST(BloomFilterTest, + BloomFiltersEqualityCheckShouldIgnoreBitsInPaddingIndexes) { + // In BloomFilter bitmap, padding is guaranteed to be less than 8, and + // starts counting from the leftmost indexes of the last byte. + BloomFilter bloom_filter1(std::vector{255, 127}, 1, + 1); // bitmap -> 11111111 01111111 + BloomFilter bloom_filter2(std::vector{255, 255}, 1, + 1); // bitmap -> 11111111 11111111 + EXPECT_TRUE(bloom_filter1 == bloom_filter2); + EXPECT_FALSE(bloom_filter1 != bloom_filter2); } TEST(BloomFilterTest, MightContainCanProcessNonStandardCharacters) { diff --git a/Firestore/core/test/unit/remote/remote_event_test.cc b/Firestore/core/test/unit/remote/remote_event_test.cc index 05f06722578..5e68c22ed66 100644 --- a/Firestore/core/test/unit/remote/remote_event_test.cc +++ b/Firestore/core/test/unit/remote/remote_event_test.cc @@ -512,6 +512,7 @@ TEST_F(RemoteEventTest, NoChangeWillStillMarkTheAffectedTargets) { ASSERT_TRUE(event.target_changes().at(1) == target_change); } +// TODO(Mila): Add test coverage for when the bloom filter is not null TEST_F(RemoteEventTest, ExistenceFilterMismatchClearsTarget) { std::unordered_map target_map = ActiveQueries({1, 2}); diff --git a/Firestore/core/test/unit/remote/serializer_test.cc b/Firestore/core/test/unit/remote/serializer_test.cc index 7bd10ff3838..23e119f3922 100644 --- a/Firestore/core/test/unit/remote/serializer_test.cc +++ b/Firestore/core/test/unit/remote/serializer_test.cc @@ -1737,6 +1737,7 @@ TEST_F(SerializerTest, DecodesListenResponseWithDocumentRemove) { ExpectDeserializationRoundTrip(model, proto); } +// TODO(Mila): Add test coverage for when the bloom filter is not null TEST_F(SerializerTest, DecodesListenResponseWithExistenceFilter) { ExistenceFilterWatchChange model( ExistenceFilter(2, /*bloom_filter=*/absl::nullopt), 100); diff --git a/Firestore/core/test/unit/remote/watch_change_test.cc b/Firestore/core/test/unit/remote/watch_change_test.cc index 39c3a049c3f..c560d63a3f5 100644 --- a/Firestore/core/test/unit/remote/watch_change_test.cc +++ b/Firestore/core/test/unit/remote/watch_change_test.cc @@ -40,6 +40,7 @@ TEST(WatchChangeTest, CanCreateDocumentWatchChange) { EXPECT_EQ(change.new_document(), doc); } +// TODO(Mila): Add test coverage for when the bloom filter is not null TEST(WatchChangeTest, CanCreateExistenceFilterWatchChange) { ExistenceFilter filter{7, /*bloom_filter=*/absl::nullopt}; ExistenceFilterWatchChange change{filter, 5}; From a2b7408d9dc87fabbb70edbc152a300dd0d7bde4 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 16 Mar 2023 15:51:03 -0700 Subject: [PATCH 11/14] format --- Firestore/Example/Tests/SpecTests/FSTSpecTests.mm | 1 - Firestore/core/src/remote/bloom_filter.cc | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index 08a5d28bb4b..703e3807df6 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -71,7 +71,6 @@ #include "Firestore/core/src/util/string_apple.h" #include "Firestore/core/src/util/to_string.h" #include "Firestore/core/test/unit/testutil/testutil.h" - #include "absl/memory/memory.h" #include "absl/strings/escaping.h" #include "absl/types/optional.h" diff --git a/Firestore/core/src/remote/bloom_filter.cc b/Firestore/core/src/remote/bloom_filter.cc index ad4c5bcbe7c..d7fa5eee7f6 100644 --- a/Firestore/core/src/remote/bloom_filter.cc +++ b/Firestore/core/src/remote/bloom_filter.cc @@ -123,6 +123,10 @@ bool operator!=(const BloomFilter& lhs, const BloomFilter& rhs) { } bool CompareBits(const BloomFilter& lhs, const BloomFilter& rhs) { + if (lhs.bit_count() != rhs.bit_count()) { + return false; + } + for (int32_t i = 0; i < lhs.bit_count(); ++i) { int32_t offset = i % 8; const uint8_t& lhs_byte = lhs.bitmap()[i / 8]; From 2a71c206d5f288c86e61cf6c9bfa2380f76dd5a3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 17 Mar 2023 12:35:48 -0700 Subject: [PATCH 12/14] resolve comments --- Firestore/core/src/remote/bloom_filter.cc | 54 ++++++++++--------- Firestore/core/src/remote/bloom_filter.h | 6 +-- .../test/unit/remote/bloom_filter_test.cc | 38 +++++++++---- 3 files changed, 61 insertions(+), 37 deletions(-) diff --git a/Firestore/core/src/remote/bloom_filter.cc b/Firestore/core/src/remote/bloom_filter.cc index d7fa5eee7f6..8f3d45107ab 100644 --- a/Firestore/core/src/remote/bloom_filter.cc +++ b/Firestore/core/src/remote/bloom_filter.cc @@ -29,6 +29,35 @@ namespace remote { using util::Status; using util::StatusOr; +namespace { +bool HasSameBits(const BloomFilter& lhs, const BloomFilter& rhs) { + if (lhs.bit_count() != rhs.bit_count()) { + return false; + } + if (lhs.bit_count() == 0) { + return true; + } + + const std::vector& bitmap1 = lhs.bitmap(); + const std::vector& bitmap2 = rhs.bitmap(); + const auto byte_count = static_cast(bitmap1.size()); + + // Compare all bytes from the bitmap, except for the last byte. + for (int32_t i = 0; i < byte_count - 1; ++i) { + if (bitmap1[i] != bitmap2[i]) { + return false; + } + } + + // Compare the last byte, ignoring the padding. + const int32_t padding = (byte_count * 8) - lhs.bit_count(); + const uint8_t last_byte1 = bitmap1[byte_count - 1] << padding; + const uint8_t last_byte2 = bitmap2[byte_count - 1] << padding; + + return (last_byte1 == last_byte2); +} +} // namespace + BloomFilter::Hash BloomFilter::Md5HashDigest(absl::string_view key) const { std::array md5_digest{util::CalculateMd5Digest(key)}; @@ -115,30 +144,7 @@ bool BloomFilter::MightContain(absl::string_view value) const { bool operator==(const BloomFilter& lhs, const BloomFilter& rhs) { return lhs.bit_count() == rhs.bit_count() && - lhs.hash_count() == rhs.hash_count() && CompareBits(lhs, rhs); -} - -bool operator!=(const BloomFilter& lhs, const BloomFilter& rhs) { - return !(lhs == rhs); -} - -bool CompareBits(const BloomFilter& lhs, const BloomFilter& rhs) { - if (lhs.bit_count() != rhs.bit_count()) { - return false; - } - - for (int32_t i = 0; i < lhs.bit_count(); ++i) { - int32_t offset = i % 8; - const uint8_t& lhs_byte = lhs.bitmap()[i / 8]; - const uint8_t& rhs_byte = rhs.bitmap()[i / 8]; - const bool bit1 = (lhs_byte & (static_cast(0x01) << offset)); - const bool bit2 = (rhs_byte & (static_cast(0x01) << offset)); - if (bit1 != bit2) { - return false; - } - } - - return true; + lhs.hash_count() == rhs.hash_count() && HasSameBits(lhs, rhs); } } // namespace remote diff --git a/Firestore/core/src/remote/bloom_filter.h b/Firestore/core/src/remote/bloom_filter.h index b5bb2003f93..c88d5687865 100644 --- a/Firestore/core/src/remote/bloom_filter.h +++ b/Firestore/core/src/remote/bloom_filter.h @@ -113,9 +113,9 @@ class BloomFilter final { bool operator==(const BloomFilter& lhs, const BloomFilter& rhs); -bool operator!=(const BloomFilter& lhs, const BloomFilter& rhs); - -bool CompareBits(const BloomFilter& lhs, const BloomFilter& rhs); +inline bool operator!=(const BloomFilter& lhs, const BloomFilter& rhs) { + return !(lhs == rhs); +}; } // namespace remote } // namespace firestore diff --git a/Firestore/core/test/unit/remote/bloom_filter_test.cc b/Firestore/core/test/unit/remote/bloom_filter_test.cc index 93583f63d0f..43015e3981c 100644 --- a/Firestore/core/test/unit/remote/bloom_filter_test.cc +++ b/Firestore/core/test/unit/remote/bloom_filter_test.cc @@ -126,10 +126,18 @@ TEST(BloomFilterTest, CheckBloomFiltersEqualityWithSameInput) { } TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentBitmap) { - BloomFilter bloom_filter1(std::vector{1}, 1, 1); - BloomFilter bloom_filter2(std::vector{2}, 1, 1); - EXPECT_FALSE(bloom_filter1 == bloom_filter2); - EXPECT_TRUE(bloom_filter1 != bloom_filter2); + { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{2}, 1, 1); + EXPECT_FALSE(bloom_filter1 == bloom_filter2); + EXPECT_TRUE(bloom_filter1 != bloom_filter2); + } + { + BloomFilter bloom_filter1(std::vector{1}, 1, 1); + BloomFilter bloom_filter2(std::vector{1, 1}, 1, 1); + EXPECT_FALSE(bloom_filter1 == bloom_filter2); + EXPECT_TRUE(bloom_filter1 != bloom_filter2); + } } TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentPadding) { @@ -150,12 +158,22 @@ TEST(BloomFilterTest, BloomFiltersEqualityCheckShouldIgnoreBitsInPaddingIndexes) { // In BloomFilter bitmap, padding is guaranteed to be less than 8, and // starts counting from the leftmost indexes of the last byte. - BloomFilter bloom_filter1(std::vector{255, 127}, 1, - 1); // bitmap -> 11111111 01111111 - BloomFilter bloom_filter2(std::vector{255, 255}, 1, - 1); // bitmap -> 11111111 11111111 - EXPECT_TRUE(bloom_filter1 == bloom_filter2); - EXPECT_FALSE(bloom_filter1 != bloom_filter2); + { + BloomFilter bloom_filter1(std::vector{255, 127}, 1, + 1); // bitmap -> 11111111 01111111 + BloomFilter bloom_filter2(std::vector{255, 255}, 1, + 1); // bitmap -> 11111111 11111111 + EXPECT_TRUE(bloom_filter1 == bloom_filter2); + EXPECT_FALSE(bloom_filter1 != bloom_filter2); + } + { + BloomFilter bloom_filter1(std::vector{255, 207}, 4, + 1); // bitmap -> 11111111 11001111 + BloomFilter bloom_filter2(std::vector{255, 255}, 4, + 1); // bitmap -> 11111111 11111111 + EXPECT_TRUE(bloom_filter1 == bloom_filter2); + EXPECT_FALSE(bloom_filter1 != bloom_filter2); + } } TEST(BloomFilterTest, MightContainCanProcessNonStandardCharacters) { From 4a976306933108f568c059db1f1ee2a37664d2ea Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 17 Mar 2023 14:16:45 -0700 Subject: [PATCH 13/14] resolve comments --- Firestore/Example/Tests/SpecTests/FSTSpecTests.mm | 1 - Firestore/core/src/remote/bloom_filter.h | 2 +- Firestore/core/src/remote/serializer.cc | 5 ++--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index 703e3807df6..7ab419f357d 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -334,7 +334,6 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version { // code a null value for now. Actual parsing code will be written in the next PR, where we can // validate the parsing result. return absl::nullopt; - ; } - (DocumentViewChange)parseChange:(NSDictionary *)jsonDoc ofType:(DocumentViewChange::Type)type { diff --git a/Firestore/core/src/remote/bloom_filter.h b/Firestore/core/src/remote/bloom_filter.h index c88d5687865..6c0facaad10 100644 --- a/Firestore/core/src/remote/bloom_filter.h +++ b/Firestore/core/src/remote/bloom_filter.h @@ -115,7 +115,7 @@ bool operator==(const BloomFilter& lhs, const BloomFilter& rhs); inline bool operator!=(const BloomFilter& lhs, const BloomFilter& rhs) { return !(lhs == rhs); -}; +} } // namespace remote } // namespace firestore diff --git a/Firestore/core/src/remote/serializer.cc b/Firestore/core/src/remote/serializer.cc index fbdc82e6eae..2b9133bf422 100644 --- a/Firestore/core/src/remote/serializer.cc +++ b/Firestore/core/src/remote/serializer.cc @@ -1430,9 +1430,8 @@ std::unique_ptr Serializer::DecodeDocumentRemove( std::unique_ptr Serializer::DecodeExistenceFilterWatchChange( ReadContext*, const google_firestore_v1_ExistenceFilter& filter) const { - ExistenceFilter existence_filter = DecodeExistenceFilter(filter); return absl::make_unique( - std::move(existence_filter), filter.target_id); + DecodeExistenceFilter(filter), filter.target_id); } ExistenceFilter Serializer::DecodeExistenceFilter( @@ -1440,7 +1439,7 @@ ExistenceFilter Serializer::DecodeExistenceFilter( // Create bloom filter if there is an unchanged_names present in the filter // and inputs are valid, otherwise keep it null. absl::optional bloom_filter; - if (filter.has_unchanged_names && filter.unchanged_names.has_bits) { + if (filter.has_unchanged_names) { // TODO(Mila): None of the ported spec tests here actually has the bloom // filter json string, so hard code an empty bloom filter for now. The // actual parsing code will be written in the next PR, where we can validate From 3eec288085122d4962b4a34d2b290506fc5e05c3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 17 Mar 2023 14:17:27 -0700 Subject: [PATCH 14/14] Update bloom_filter.cc --- Firestore/core/src/remote/bloom_filter.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Firestore/core/src/remote/bloom_filter.cc b/Firestore/core/src/remote/bloom_filter.cc index 8f3d45107ab..7ca39e1b00d 100644 --- a/Firestore/core/src/remote/bloom_filter.cc +++ b/Firestore/core/src/remote/bloom_filter.cc @@ -143,8 +143,7 @@ bool BloomFilter::MightContain(absl::string_view value) const { } bool operator==(const BloomFilter& lhs, const BloomFilter& rhs) { - return lhs.bit_count() == rhs.bit_count() && - lhs.hash_count() == rhs.hash_count() && HasSameBits(lhs, rhs); + return lhs.hash_count() == rhs.hash_count() && HasSameBits(lhs, rhs); } } // namespace remote