From f7c0d23745bd223cc96f813aeff2e4c3a989d544 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 11 Aug 2021 20:25:46 -0500 Subject: [PATCH 1/7] operator== and operator!= for DocumentSnapshot. --- .../src/document_snapshot_test.cc | 28 +++++++++++++++++-- .../src/android/document_snapshot_android.cc | 6 ++++ .../src/android/document_snapshot_android.h | 8 ++++++ firestore/src/common/document_snapshot.cc | 8 ++++++ .../firebase/firestore/document_snapshot.h | 12 ++++++++ firestore/src/main/document_snapshot_main.cc | 5 ++++ firestore/src/main/document_snapshot_main.h | 8 ++++++ release_build_files/readme.md | 2 +- 8 files changed, 74 insertions(+), 3 deletions(-) diff --git a/firestore/integration_test_internal/src/document_snapshot_test.cc b/firestore/integration_test_internal/src/document_snapshot_test.cc index 70e0f90a5c..be2e78a319 100644 --- a/firestore/integration_test_internal/src/document_snapshot_test.cc +++ b/firestore/integration_test_internal/src/document_snapshot_test.cc @@ -3,6 +3,8 @@ #include #include "firebase/firestore.h" +#include "firestore_integration_test.h" + #if defined(__ANDROID__) #include "firestore/src/android/document_snapshot_android.h" #include "firestore/src/common/wrapper_assertions.h" @@ -14,9 +16,9 @@ namespace firebase { namespace firestore { -#if defined(__ANDROID__) +using DocumentSnapshotTest = FirestoreIntegrationTest; -using DocumentSnapshotTest = testing::Test; +#if defined(__ANDROID__) TEST_F(DocumentSnapshotTest, Construction) { testutil::AssertWrapperConstructionContract Date: Fri, 13 Aug 2021 14:32:34 -0500 Subject: [PATCH 2/7] Address feedback. --- firestore/CMakeLists.txt | 1 + .../src/android/document_snapshot_android.cc | 4 +-- firestore/src/android/field_value_android.cc | 4 +-- firestore/src/android/query_android.cc | 4 +-- .../src/android/query_snapshot_android.cc | 4 +-- firestore/src/common/document_snapshot.cc | 6 +---- firestore/src/common/field_value.cc | 5 ++-- firestore/src/common/query.cc | 7 ++---- firestore/src/common/query_snapshot.cc | 7 ++---- firestore/src/common/util.h | 7 ++++++ firestore/src/jni/compare.h | 25 +++++++++++++++++++ 11 files changed, 48 insertions(+), 26 deletions(-) create mode 100644 firestore/src/jni/compare.h diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index afe6d7db72..c6732b4ef1 100644 --- a/firestore/CMakeLists.txt +++ b/firestore/CMakeLists.txt @@ -143,6 +143,7 @@ set(android_SRCS src/jni/class.h src/jni/collection.cc src/jni/collection.h + src/jni/compare.h src/jni/declaration.h src/jni/double.cc src/jni/double.h diff --git a/firestore/src/android/document_snapshot_android.cc b/firestore/src/android/document_snapshot_android.cc index 6e69be6abf..e4cf14934f 100644 --- a/firestore/src/android/document_snapshot_android.cc +++ b/firestore/src/android/document_snapshot_android.cc @@ -10,6 +10,7 @@ #include "firestore/src/android/server_timestamp_behavior_android.h" #include "firestore/src/android/snapshot_metadata_android.h" #include "firestore/src/include/firebase/firestore.h" +#include "firestore/src/jni/compare.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/loader.h" @@ -116,8 +117,7 @@ FieldValue DocumentSnapshotInternal::Get(const FieldPath& field, bool operator==(const DocumentSnapshotInternal& lhs, const DocumentSnapshotInternal& rhs) { - Env env = FirestoreInternal::GetEnv(); - return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); + return CompareJni(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index 2c8eb74255..e68e7310f6 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -11,6 +11,7 @@ #include "firestore/src/jni/array_list.h" #include "firestore/src/jni/boolean.h" #include "firestore/src/jni/class.h" +#include "firestore/src/jni/compare.h" #include "firestore/src/jni/double.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/hash_map.h" @@ -370,8 +371,7 @@ FieldValue FieldValueInternal::DoubleIncrement(double by_value) { } bool operator==(const FieldValueInternal& lhs, const FieldValueInternal& rhs) { - Env env = FieldValueInternal::GetEnv(); - return Object::Equals(env, lhs.object_, rhs.object_); + return CompareJni(lhs, rhs); } template diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc index 845bfcc4e8..fcf86d7a84 100644 --- a/firestore/src/android/query_android.cc +++ b/firestore/src/android/query_android.cc @@ -18,6 +18,7 @@ #include "firestore/src/include/firebase/firestore.h" #include "firestore/src/jni/array.h" #include "firestore/src/jni/array_list.h" +#include "firestore/src/jni/compare.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/loader.h" #include "firestore/src/jni/task.h" @@ -332,8 +333,7 @@ Local> QueryInternal::ConvertFieldValues( } bool operator==(const QueryInternal& lhs, const QueryInternal& rhs) { - Env env = FirestoreInternal::GetEnv(); - return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); + return CompareJni(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/android/query_snapshot_android.cc b/firestore/src/android/query_snapshot_android.cc index 6765f2538c..bb5d84245a 100644 --- a/firestore/src/android/query_snapshot_android.cc +++ b/firestore/src/android/query_snapshot_android.cc @@ -9,6 +9,7 @@ #include "firestore/src/android/query_android.h" #include "firestore/src/android/snapshot_metadata_android.h" #include "firestore/src/android/util_android.h" +#include "firestore/src/jni/compare.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/list.h" #include "firestore/src/jni/loader.h" @@ -74,8 +75,7 @@ std::size_t QuerySnapshotInternal::size() const { bool operator==(const QuerySnapshotInternal& lhs, const QuerySnapshotInternal& rhs) { - Env env = FirestoreInternal::GetEnv(); - return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); + return CompareJni(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/common/document_snapshot.cc b/firestore/src/common/document_snapshot.cc index 2fc7ea6f96..4377833cf1 100644 --- a/firestore/src/common/document_snapshot.cc +++ b/firestore/src/common/document_snapshot.cc @@ -142,11 +142,7 @@ std::ostream& operator<<(std::ostream& out, const DocumentSnapshot& document) { } bool operator==(const DocumentSnapshot& lhs, const DocumentSnapshot& 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 diff --git a/firestore/src/common/field_value.cc b/firestore/src/common/field_value.cc index 6f915c13d4..fa87d82cfe 100644 --- a/firestore/src/common/field_value.cc +++ b/firestore/src/common/field_value.cc @@ -11,6 +11,7 @@ #include "firebase/firestore/timestamp.h" #include "firestore/src/common/hard_assert_common.h" #include "firestore/src/common/to_string.h" +#include "firestore/src/common/util.h" #include "firestore/src/include/firebase/firestore/document_reference.h" #if defined(__ANDROID__) #include "firestore/src/android/field_value_android.h" @@ -327,9 +328,7 @@ std::string FieldValue::ToString() const { } bool operator==(const FieldValue& lhs, const FieldValue& rhs) { - return lhs.internal_ == rhs.internal_ || - (lhs.internal_ != nullptr && rhs.internal_ != nullptr && - *lhs.internal_ == *rhs.internal_); + return EqualityCompare(lhs.internal_, rhs.internal_); } std::ostream& operator<<(std::ostream& out, const FieldValue& value) { diff --git a/firestore/src/common/query.cc b/firestore/src/common/query.cc index 0b352d646f..17a7836430 100644 --- a/firestore/src/common/query.cc +++ b/firestore/src/common/query.cc @@ -8,6 +8,7 @@ #include "firestore/src/common/event_listener.h" #include "firestore/src/common/futures.h" #include "firestore/src/common/hard_assert_common.h" +#include "firestore/src/common/util.h" #include "firestore/src/include/firebase/firestore/document_snapshot.h" #include "firestore/src/include/firebase/firestore/field_path.h" #include "firestore/src/include/firebase/firestore/field_value.h" @@ -288,11 +289,7 @@ ListenerRegistration Query::AddSnapshotListener( } bool operator==(const Query& lhs, const Query& 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 diff --git a/firestore/src/common/query_snapshot.cc b/firestore/src/common/query_snapshot.cc index 5201cca28c..338f01cd31 100644 --- a/firestore/src/common/query_snapshot.cc +++ b/firestore/src/common/query_snapshot.cc @@ -6,6 +6,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_change.h" #include "firestore/src/include/firebase/firestore/document_snapshot.h" #include "firestore/src/include/firebase/firestore/query.h" @@ -104,11 +105,7 @@ std::size_t QuerySnapshot::size() const { } bool operator==(const QuerySnapshot& lhs, const QuerySnapshot& 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 diff --git a/firestore/src/common/util.h b/firestore/src/common/util.h index 1f63ab39da..287c9087eb 100644 --- a/firestore/src/common/util.h +++ b/firestore/src/common/util.h @@ -13,6 +13,13 @@ namespace firestore { // a string to refer to. const std::string& EmptyString(); +// Returns true if the two given pointers are equal or the underlying objects +// that they point to are equal. Returns false otherwise. +template +bool EqualityCompare(T* lhs, T* rhs) { + return lhs == rhs || (lhs != nullptr && rhs != nullptr && *lhs == *rhs); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/jni/compare.h b/firestore/src/jni/compare.h new file mode 100644 index 0000000000..a876460ebf --- /dev/null +++ b/firestore/src/jni/compare.h @@ -0,0 +1,25 @@ +/* + * Copyright 2021 Google + * + * 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. + */ + +/** + * Returns the result of `Object::Equals` for the given `lhs` and `rhs` in the + * current Firestore `jni::Env`. + */ +template +bool CompareJni(const T& lhs, const T& rhs) { + Env env = FirestoreInternal::GetEnv(); + return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); +} From 73c4180b4e495ea675519c17db041975d9f86d06 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 13 Aug 2021 14:35:32 -0500 Subject: [PATCH 3/7] Fix license. --- firestore/src/jni/compare.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/src/jni/compare.h b/firestore/src/jni/compare.h index a876460ebf..a46ff26394 100644 --- a/firestore/src/jni/compare.h +++ b/firestore/src/jni/compare.h @@ -1,5 +1,5 @@ /* - * Copyright 2021 Google + * Copyright 2021 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 736ea5c3e8163161d5514352f924d22c2cd53163 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 13 Aug 2021 15:02:17 -0500 Subject: [PATCH 4/7] Address comments. --- firestore/src/jni/compare.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/firestore/src/jni/compare.h b/firestore/src/jni/compare.h index a46ff26394..27daaae4fa 100644 --- a/firestore/src/jni/compare.h +++ b/firestore/src/jni/compare.h @@ -14,12 +14,17 @@ * limitations under the License. */ +#include "firestore/src/android/exception_android.h" +#include "firestore/src/android/wrapper.h" +#include "firestore/src/jni/object.h" + /** * Returns the result of `Object::Equals` for the given `lhs` and `rhs` in the * current Firestore `jni::Env`. */ template bool CompareJni(const T& lhs, const T& rhs) { - Env env = FirestoreInternal::GetEnv(); + Env env; + env.SetUnhandledExceptionHandler(GlobalUnhandledExceptionHandler, nullptr); return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); } From c8be2d42702491c9f4d2b28e9cf6a1de39e1235b Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 13 Aug 2021 15:05:25 -0500 Subject: [PATCH 5/7] Add missing header. --- firestore/src/jni/compare.h | 1 + 1 file changed, 1 insertion(+) diff --git a/firestore/src/jni/compare.h b/firestore/src/jni/compare.h index 27daaae4fa..d6ef9800e2 100644 --- a/firestore/src/jni/compare.h +++ b/firestore/src/jni/compare.h @@ -16,6 +16,7 @@ #include "firestore/src/android/exception_android.h" #include "firestore/src/android/wrapper.h" +#include "firestore/src/jni/env.h" #include "firestore/src/jni/object.h" /** From 0093a4e8cb2b1e142065f856f7dbe840140b7738 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 13 Aug 2021 20:59:55 -0500 Subject: [PATCH 6/7] Better code structure (fix build for Android). --- firestore/src/android/document_snapshot_android.cc | 2 +- firestore/src/android/field_value_android.cc | 2 +- firestore/src/android/query_android.cc | 2 +- firestore/src/android/query_snapshot_android.cc | 2 +- firestore/src/jni/compare.h | 10 +++++++++- 5 files changed, 13 insertions(+), 5 deletions(-) diff --git a/firestore/src/android/document_snapshot_android.cc b/firestore/src/android/document_snapshot_android.cc index e4cf14934f..a9798a8611 100644 --- a/firestore/src/android/document_snapshot_android.cc +++ b/firestore/src/android/document_snapshot_android.cc @@ -117,7 +117,7 @@ FieldValue DocumentSnapshotInternal::Get(const FieldPath& field, bool operator==(const DocumentSnapshotInternal& lhs, const DocumentSnapshotInternal& rhs) { - return CompareJni(lhs, rhs); + return jni::EqualityCompare(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index e68e7310f6..566e5c7df0 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -371,7 +371,7 @@ FieldValue FieldValueInternal::DoubleIncrement(double by_value) { } bool operator==(const FieldValueInternal& lhs, const FieldValueInternal& rhs) { - return CompareJni(lhs, rhs); + return jni::EqualityCompare(lhs, rhs); } template diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc index fcf86d7a84..bfef3c8ef3 100644 --- a/firestore/src/android/query_android.cc +++ b/firestore/src/android/query_android.cc @@ -333,7 +333,7 @@ Local> QueryInternal::ConvertFieldValues( } bool operator==(const QueryInternal& lhs, const QueryInternal& rhs) { - return CompareJni(lhs, rhs); + return jni::EqualityCompare(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/android/query_snapshot_android.cc b/firestore/src/android/query_snapshot_android.cc index bb5d84245a..5b13949df3 100644 --- a/firestore/src/android/query_snapshot_android.cc +++ b/firestore/src/android/query_snapshot_android.cc @@ -75,7 +75,7 @@ std::size_t QuerySnapshotInternal::size() const { bool operator==(const QuerySnapshotInternal& lhs, const QuerySnapshotInternal& rhs) { - return CompareJni(lhs, rhs); + return jni::EqualityCompare(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/jni/compare.h b/firestore/src/jni/compare.h index d6ef9800e2..c4d9364310 100644 --- a/firestore/src/jni/compare.h +++ b/firestore/src/jni/compare.h @@ -19,13 +19,21 @@ #include "firestore/src/jni/env.h" #include "firestore/src/jni/object.h" +namespace firebase { +namespace firestore { +namespace jni { + /** * Returns the result of `Object::Equals` for the given `lhs` and `rhs` in the * current Firestore `jni::Env`. */ template -bool CompareJni(const T& lhs, const T& rhs) { +bool EqualityCompare(const T& lhs, const T& rhs) { Env env; env.SetUnhandledExceptionHandler(GlobalUnhandledExceptionHandler, nullptr); return Object::Equals(env, lhs.ToJava(), rhs.ToJava()); } + +} // namespace jni +} // namespace firestore +} // namespace firebase From 6c76b25c5a15ddc328b3c4ec11469942af88c39b Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Thu, 19 Aug 2021 10:36:23 -0500 Subject: [PATCH 7/7] Address comments. --- firestore/src/android/document_snapshot_android.cc | 2 +- firestore/src/android/field_value_android.cc | 2 +- firestore/src/android/query_android.cc | 2 +- firestore/src/android/query_snapshot_android.cc | 2 +- firestore/src/jni/compare.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/firestore/src/android/document_snapshot_android.cc b/firestore/src/android/document_snapshot_android.cc index a9798a8611..e7f0d84a0b 100644 --- a/firestore/src/android/document_snapshot_android.cc +++ b/firestore/src/android/document_snapshot_android.cc @@ -117,7 +117,7 @@ FieldValue DocumentSnapshotInternal::Get(const FieldPath& field, bool operator==(const DocumentSnapshotInternal& lhs, const DocumentSnapshotInternal& rhs) { - return jni::EqualityCompare(lhs, rhs); + return jni::EqualityCompareJni(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index 566e5c7df0..d46332f68a 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -371,7 +371,7 @@ FieldValue FieldValueInternal::DoubleIncrement(double by_value) { } bool operator==(const FieldValueInternal& lhs, const FieldValueInternal& rhs) { - return jni::EqualityCompare(lhs, rhs); + return jni::EqualityCompareJni(lhs, rhs); } template diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc index bfef3c8ef3..57bbb663b2 100644 --- a/firestore/src/android/query_android.cc +++ b/firestore/src/android/query_android.cc @@ -333,7 +333,7 @@ Local> QueryInternal::ConvertFieldValues( } bool operator==(const QueryInternal& lhs, const QueryInternal& rhs) { - return jni::EqualityCompare(lhs, rhs); + return jni::EqualityCompareJni(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/android/query_snapshot_android.cc b/firestore/src/android/query_snapshot_android.cc index 5b13949df3..4f9fef1f4e 100644 --- a/firestore/src/android/query_snapshot_android.cc +++ b/firestore/src/android/query_snapshot_android.cc @@ -75,7 +75,7 @@ std::size_t QuerySnapshotInternal::size() const { bool operator==(const QuerySnapshotInternal& lhs, const QuerySnapshotInternal& rhs) { - return jni::EqualityCompare(lhs, rhs); + return jni::EqualityCompareJni(lhs, rhs); } } // namespace firestore diff --git a/firestore/src/jni/compare.h b/firestore/src/jni/compare.h index c4d9364310..d8a9dd55bd 100644 --- a/firestore/src/jni/compare.h +++ b/firestore/src/jni/compare.h @@ -28,7 +28,7 @@ namespace jni { * current Firestore `jni::Env`. */ template -bool EqualityCompare(const T& lhs, const T& rhs) { +bool EqualityCompareJni(const T& lhs, const T& rhs) { Env env; env.SetUnhandledExceptionHandler(GlobalUnhandledExceptionHandler, nullptr); return Object::Equals(env, lhs.ToJava(), rhs.ToJava());