From 23957725c924282793a05fe66f2911bcfd3a52cf Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 13 Aug 2021 11:55:14 -0500 Subject: [PATCH 1/6] operator== and operator!= for DocumentChange. --- .../src/document_change_test.cc | 66 ++++++++++++++++++- firestore/src/common/document_change.cc | 5 ++ .../firebase/firestore/document_change.h | 8 +++ 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/firestore/integration_test_internal/src/document_change_test.cc b/firestore/integration_test_internal/src/document_change_test.cc index 62438d140..7461e3fbd 100644 --- a/firestore/integration_test_internal/src/document_change_test.cc +++ b/firestore/integration_test_internal/src/document_change_test.cc @@ -27,10 +27,10 @@ namespace firebase { namespace firestore { -#if defined(__ANDROID__) - using DocumentChangeTest = FirestoreIntegrationTest; +#if defined(__ANDROID__) + TEST_F(DocumentChangeTest, Construction) { testutil::AssertWrapperConstructionContract(); @@ -92,5 +92,67 @@ TEST_F(DocumentChangeTest, TestDocumentChanges) { #endif // defined(__ANDROID__) +TEST_F(DocumentChangeTest, Equality) { + DocumentChange invalid_change_1 = DocumentChange(); + DocumentChange invalid_change_2 = DocumentChange(); + + EXPECT_TRUE(invalid_change_1 == invalid_change_2); + EXPECT_FALSE(invalid_change_1 != invalid_change_2); + + CollectionReference collection = Collection(); + Query query = collection.OrderBy("a"); + + DocumentReference doc1 = collection.Document("1"); + DocumentReference doc2 = collection.Document("2"); + + TestEventListener listener("TestDocumentChanges"); + ListenerRegistration registration = listener.AttachTo(&query); + Await(listener); + QuerySnapshot snapshot = listener.last_result(); + EXPECT_EQ(snapshot.size(), 0); + + WriteDocument(doc1, MapFieldValue{{"a", FieldValue::Integer(1)}}); + Await(listener); + snapshot = listener.last_result(); + + std::vector changes = snapshot.DocumentChanges(); + EXPECT_EQ(changes.size(), 1); + // first_change: Added doc1. + auto first_change = changes[0]; + EXPECT_TRUE(first_change == first_change); + EXPECT_TRUE(first_change != invalid_change_1); + EXPECT_FALSE(first_change != first_change); + EXPECT_FALSE(first_change == invalid_change_1); + + WriteDocument(doc2, MapFieldValue{{"a", FieldValue::Integer(2)}}); + Await(listener, 2); + snapshot = listener.last_result(); + + changes = snapshot.DocumentChanges(); + EXPECT_EQ(changes.size(), 1); + // second_change: Added doc2. + auto second_change = changes[0]; + EXPECT_TRUE(second_change != first_change); + EXPECT_TRUE(second_change != invalid_change_1); + EXPECT_FALSE(second_change == first_change); + EXPECT_FALSE(second_change == invalid_change_1); + + // Make doc2 ordered before doc1. + WriteDocument(doc2, MapFieldValue{{"a", FieldValue::Integer(0)}}); + Await(listener, 3); + snapshot = listener.last_result(); + + changes = snapshot.DocumentChanges(); + EXPECT_EQ(changes.size(), 1); + // third_change: Modified doc2. + auto third_change = changes[0]; + EXPECT_TRUE(third_change != first_change); + EXPECT_TRUE(third_change != second_change); + EXPECT_TRUE(third_change != invalid_change_1); + EXPECT_FALSE(third_change == first_change); + EXPECT_FALSE(third_change == second_change); + EXPECT_FALSE(third_change == invalid_change_1); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/common/document_change.cc b/firestore/src/common/document_change.cc index edce5e7ac..6f66d9736 100644 --- a/firestore/src/common/document_change.cc +++ b/firestore/src/common/document_change.cc @@ -118,5 +118,10 @@ std::size_t DocumentChange::new_index() const { return internal_->new_index(); } +bool operator==(const DocumentChange& lhs, const DocumentChange& rhs) { + return lhs.type() == rhs.type() && lhs.old_index() == rhs.old_index() && + lhs.new_index() == rhs.new_index() && lhs.document() == rhs.document(); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/include/firebase/firestore/document_change.h b/firestore/src/include/firebase/firestore/document_change.h index 9f366029d..bb52c32e9 100644 --- a/firestore/src/include/firebase/firestore/document_change.h +++ b/firestore/src/include/firebase/firestore/document_change.h @@ -184,6 +184,14 @@ class DocumentChange { mutable DocumentChangeInternal* internal_ = nullptr; }; +/** Checks `lhs` and `rhs` for equality. */ +bool operator==(const DocumentChange& lhs, const DocumentChange& rhs); + +/** Checks `lhs` and `rhs` for inequality. */ +inline bool operator!=(const DocumentChange& lhs, const DocumentChange& rhs) { + return !(lhs == rhs); +} + } // namespace firestore } // namespace firebase From d41157dda780d36d7e9039a480b82b581ef84d2c Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 13 Aug 2021 21:45:53 -0500 Subject: [PATCH 2/6] delegate to underlying impl. --- firestore/src/android/document_change_android.cc | 6 ++++++ firestore/src/android/document_change_android.h | 8 ++++++++ firestore/src/common/document_change.cc | 7 +++++-- .../src/include/firebase/firestore/document_change.h | 2 ++ firestore/src/main/document_change_main.cc | 5 +++++ firestore/src/main/document_change_main.h | 8 ++++++++ 6 files changed, 34 insertions(+), 2 deletions(-) diff --git a/firestore/src/android/document_change_android.cc b/firestore/src/android/document_change_android.cc index 860e81f3c..ef4236f4e 100644 --- a/firestore/src/android/document_change_android.cc +++ b/firestore/src/android/document_change_android.cc @@ -69,5 +69,11 @@ std::size_t DocumentChangeInternal::new_index() const { return env.Call(obj_, kNewIndex); } +bool operator==(const DocumentChangeInternal& lhs, + const DocumentChangeInternal& rhs) { + Env env = FirestoreInternal::GetEnv(); + return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/android/document_change_android.h b/firestore/src/android/document_change_android.h index 555ffdf14..491341f37 100644 --- a/firestore/src/android/document_change_android.h +++ b/firestore/src/android/document_change_android.h @@ -38,6 +38,14 @@ class DocumentChangeInternal : public Wrapper { std::size_t new_index() const; }; +bool operator==(const DocumentChangeInternal& lhs, + const DocumentChangeInternal& rhs); + +inline bool operator!=(const DocumentChangeInternal& lhs, + const DocumentChangeInternal& rhs) { + return !(lhs == rhs); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/common/document_change.cc b/firestore/src/common/document_change.cc index 6f66d9736..11ed22f2a 100644 --- a/firestore/src/common/document_change.cc +++ b/firestore/src/common/document_change.cc @@ -119,8 +119,11 @@ std::size_t DocumentChange::new_index() const { } bool operator==(const DocumentChange& lhs, const DocumentChange& rhs) { - return lhs.type() == rhs.type() && lhs.old_index() == rhs.old_index() && - lhs.new_index() == rhs.new_index() && lhs.document() == rhs.document(); + if (!lhs.internal_ || !rhs.internal_) { + return lhs.internal_ == rhs.internal_; + } + + return lhs.internal_ == rhs.internal_ || *lhs.internal_ == *rhs.internal_; } } // namespace firestore diff --git a/firestore/src/include/firebase/firestore/document_change.h b/firestore/src/include/firebase/firestore/document_change.h index bb52c32e9..908fb3ec4 100644 --- a/firestore/src/include/firebase/firestore/document_change.h +++ b/firestore/src/include/firebase/firestore/document_change.h @@ -173,6 +173,8 @@ class DocumentChange { bool is_valid() const { return internal_ != nullptr; } private: + friend bool operator==(const DocumentChange& lhs, const DocumentChange& rhs); + friend class FirestoreInternal; friend class Wrapper; friend struct ConverterImpl; diff --git a/firestore/src/main/document_change_main.cc b/firestore/src/main/document_change_main.cc index b958047ec..717f433c8 100644 --- a/firestore/src/main/document_change_main.cc +++ b/firestore/src/main/document_change_main.cc @@ -60,5 +60,10 @@ std::size_t DocumentChangeInternal::new_index() const { return change_.new_index(); } +bool operator==(const DocumentChangeInternal& lhs, + const DocumentChangeInternal& rhs) { + return lhs.change_ == rhs.change_; +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/main/document_change_main.h b/firestore/src/main/document_change_main.h index b9413d19c..bac46e536 100644 --- a/firestore/src/main/document_change_main.h +++ b/firestore/src/main/document_change_main.h @@ -41,10 +41,18 @@ class DocumentChangeInternal { std::size_t old_index() const; std::size_t new_index() const; + friend bool operator==(const DocumentChangeInternal& lhs, + const DocumentChangeInternal& rhs); + private: api::DocumentChange change_; }; +inline bool operator!=(const DocumentChangeInternal& lhs, + const DocumentChangeInternal& rhs) { + return !(lhs == rhs); +} + } // namespace firestore } // namespace firebase From 11bf4e1823c9c6a81a8d81ec40b4e76245cdca42 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 20 Aug 2021 13:49:12 -0500 Subject: [PATCH 3/6] Address comments. --- .../src/document_change_test.cc | 36 +++++++++---------- .../src/android/document_change_android.cc | 4 +-- firestore/src/common/document_change.cc | 7 ++-- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/firestore/integration_test_internal/src/document_change_test.cc b/firestore/integration_test_internal/src/document_change_test.cc index 7461e3fbd..73cbb62a1 100644 --- a/firestore/integration_test_internal/src/document_change_test.cc +++ b/firestore/integration_test_internal/src/document_change_test.cc @@ -116,26 +116,26 @@ TEST_F(DocumentChangeTest, Equality) { snapshot = listener.last_result(); std::vector changes = snapshot.DocumentChanges(); - EXPECT_EQ(changes.size(), 1); - // first_change: Added doc1. - auto first_change = changes[0]; - EXPECT_TRUE(first_change == first_change); - EXPECT_TRUE(first_change != invalid_change_1); - EXPECT_FALSE(first_change != first_change); - EXPECT_FALSE(first_change == invalid_change_1); + ASSERT_EQ(changes.size(), 1); + // First change: Added doc1. + auto change1 = changes[0]; + EXPECT_TRUE(change1 == change1); + EXPECT_TRUE(change1 != invalid_change_1); + EXPECT_FALSE(change1 != change1); + EXPECT_FALSE(change1 == invalid_change_1); WriteDocument(doc2, MapFieldValue{{"a", FieldValue::Integer(2)}}); Await(listener, 2); snapshot = listener.last_result(); changes = snapshot.DocumentChanges(); - EXPECT_EQ(changes.size(), 1); - // second_change: Added doc2. - auto second_change = changes[0]; - EXPECT_TRUE(second_change != first_change); - EXPECT_TRUE(second_change != invalid_change_1); - EXPECT_FALSE(second_change == first_change); - EXPECT_FALSE(second_change == invalid_change_1); + ASSERT_EQ(changes.size(), 1); + // Second change: Added doc2. + auto change2 = changes[0]; + EXPECT_TRUE(change2 != change1); + EXPECT_TRUE(change2 != invalid_change_1); + EXPECT_FALSE(change2 == change1); + EXPECT_FALSE(change2 == invalid_change_1); // Make doc2 ordered before doc1. WriteDocument(doc2, MapFieldValue{{"a", FieldValue::Integer(0)}}); @@ -146,11 +146,11 @@ TEST_F(DocumentChangeTest, Equality) { EXPECT_EQ(changes.size(), 1); // third_change: Modified doc2. auto third_change = changes[0]; - EXPECT_TRUE(third_change != first_change); - EXPECT_TRUE(third_change != second_change); + EXPECT_TRUE(third_change != change1); + EXPECT_TRUE(third_change != change2); EXPECT_TRUE(third_change != invalid_change_1); - EXPECT_FALSE(third_change == first_change); - EXPECT_FALSE(third_change == second_change); + EXPECT_FALSE(third_change == change1); + EXPECT_FALSE(third_change == change2); EXPECT_FALSE(third_change == invalid_change_1); } diff --git a/firestore/src/android/document_change_android.cc b/firestore/src/android/document_change_android.cc index ef4236f4e..0f5566148 100644 --- a/firestore/src/android/document_change_android.cc +++ b/firestore/src/android/document_change_android.cc @@ -18,6 +18,7 @@ #include "firestore/src/android/document_change_type_android.h" #include "firestore/src/android/document_snapshot_android.h" +#include "firestore/src/jni/compare.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/loader.h" @@ -71,8 +72,7 @@ std::size_t DocumentChangeInternal::new_index() const { bool operator==(const DocumentChangeInternal& lhs, const DocumentChangeInternal& rhs) { - Env env = FirestoreInternal::GetEnv(); - return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); + return jni::EqualityCompareJni(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/common/document_change.cc b/firestore/src/common/document_change.cc index 11ed22f2a..99636b820 100644 --- a/firestore/src/common/document_change.cc +++ b/firestore/src/common/document_change.cc @@ -20,6 +20,7 @@ #include "firestore/src/common/cleanup.h" #include "firestore/src/common/hard_assert_common.h" +#include "firestore/src/common/util.h" #include "firestore/src/include/firebase/firestore/document_snapshot.h" #if defined(__ANDROID__) #include "firestore/src/android/document_change_android.h" @@ -119,11 +120,7 @@ std::size_t DocumentChange::new_index() const { } bool operator==(const DocumentChange& lhs, const DocumentChange& rhs) { - if (!lhs.internal_ || !rhs.internal_) { - return lhs.internal_ == rhs.internal_; - } - - return lhs.internal_ == rhs.internal_ || *lhs.internal_ == *rhs.internal_; + return EqualityCompare(lhs.internal_, rhs.internal_); } } // namespace firestore From da7efdebdda0547e7fefb3196f7b73c80b1361aa Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 20 Aug 2021 13:50:28 -0500 Subject: [PATCH 4/6] Update readme. --- release_build_files/readme.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 8bd7354aa..4edc47121 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -571,7 +571,8 @@ code. - Changes - General: Updating Android and iOS dependencies to the latest. - Firestore: Added `operator==` and `operator!=` for `SnapshotMetadata`, - `Settings`, `QuerySnapshot`, `DocumentSnapshot`, and `SetOptions`. + `Settings`, `QuerySnapshot`, `DocumentSnapshot`, `SetOptions`, and + `DocumentChange`. ### 8.3.0 - Changes From 2043c698f0caa0ad7fa0fa549eee5aee7db38e23 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 20 Aug 2021 14:04:38 -0500 Subject: [PATCH 5/6] Use ASSERT_EQ in a few more instances. --- .../integration_test_internal/src/document_change_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/firestore/integration_test_internal/src/document_change_test.cc b/firestore/integration_test_internal/src/document_change_test.cc index 73cbb62a1..9eadb5bd8 100644 --- a/firestore/integration_test_internal/src/document_change_test.cc +++ b/firestore/integration_test_internal/src/document_change_test.cc @@ -59,7 +59,7 @@ TEST_F(DocumentChangeTest, TestDocumentChanges) { snapshot = listener.last_result(); std::vector changes = snapshot.DocumentChanges(); - EXPECT_EQ(changes.size(), 1); + ASSERT_EQ(changes.size(), 1); EXPECT_EQ(changes[0].type(), DocumentChange::Type::kAdded); EXPECT_EQ(changes[0].document().id(), doc1.id()); @@ -71,7 +71,7 @@ TEST_F(DocumentChangeTest, TestDocumentChanges) { snapshot = listener.last_result(); changes = snapshot.DocumentChanges(); - EXPECT_EQ(changes.size(), 1); + ASSERT_EQ(changes.size(), 1); EXPECT_EQ(changes[0].type(), DocumentChange::Type::kAdded); EXPECT_EQ(changes[0].document().id(), doc2.id()); EXPECT_EQ(changes[0].old_index(), DocumentChange::npos); @@ -83,7 +83,7 @@ TEST_F(DocumentChangeTest, TestDocumentChanges) { snapshot = listener.last_result(); changes = snapshot.DocumentChanges(); - EXPECT_EQ(changes.size(), 1); + ASSERT_EQ(changes.size(), 1); EXPECT_EQ(changes[0].type(), DocumentChange::Type::kModified); EXPECT_EQ(changes[0].document().id(), doc2.id()); EXPECT_EQ(changes[0].old_index(), 1); @@ -143,7 +143,7 @@ TEST_F(DocumentChangeTest, Equality) { snapshot = listener.last_result(); changes = snapshot.DocumentChanges(); - EXPECT_EQ(changes.size(), 1); + ASSERT_EQ(changes.size(), 1); // third_change: Modified doc2. auto third_change = changes[0]; EXPECT_TRUE(third_change != change1); From 708cb3940ac4b104f0a3daabbafc7dbb86d4db6f Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 20 Aug 2021 14:08:56 -0500 Subject: [PATCH 6/6] rename third_change to change3. --- .../src/document_change_test.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/firestore/integration_test_internal/src/document_change_test.cc b/firestore/integration_test_internal/src/document_change_test.cc index 9eadb5bd8..1fda35873 100644 --- a/firestore/integration_test_internal/src/document_change_test.cc +++ b/firestore/integration_test_internal/src/document_change_test.cc @@ -144,14 +144,14 @@ TEST_F(DocumentChangeTest, Equality) { changes = snapshot.DocumentChanges(); ASSERT_EQ(changes.size(), 1); - // third_change: Modified doc2. - auto third_change = changes[0]; - EXPECT_TRUE(third_change != change1); - EXPECT_TRUE(third_change != change2); - EXPECT_TRUE(third_change != invalid_change_1); - EXPECT_FALSE(third_change == change1); - EXPECT_FALSE(third_change == change2); - EXPECT_FALSE(third_change == invalid_change_1); + // Third change: Modified doc2. + auto change3 = changes[0]; + EXPECT_TRUE(change3 != change1); + EXPECT_TRUE(change3 != change2); + EXPECT_TRUE(change3 != invalid_change_1); + EXPECT_FALSE(change3 == change1); + EXPECT_FALSE(change3 == change2); + EXPECT_FALSE(change3 == invalid_change_1); } } // namespace firestore