operator== and operator!= for DocumentSnapshot. (#602)

ehsannas · web-flow · commit f331295f800e · 2021-08-19T21:06:36.000-05:00
* operator== and operator!= for DocumentSnapshot.

* Address feedback.

* Fix license.

* Address comments.

* Add missing header.

* Better code structure (fix build for Android).

* Address comments.
diff --git 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/integration_test_internal/src/document_snapshot_test.cc b/firestore/integration_test_internal/src/document_snapshot_test.cc
@@ -17,6 +17,8 @@
 #include <utility>
 
 #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"
@@ -28,9 +30,9 @@
 namespace firebase {
 namespace firestore {
 
-#if defined(__ANDROID__)
+using DocumentSnapshotTest = FirestoreIntegrationTest;
 
-using DocumentSnapshotTest = testing::Test;
+#if defined(__ANDROID__)
 
 TEST_F(DocumentSnapshotTest, Construction) {
   testutil::AssertWrapperConstructionContract<DocumentSnapshot,
@@ -44,5 +46,27 @@ TEST_F(DocumentSnapshotTest, Assignment) {
 
 #endif
 
+TEST_F(DocumentSnapshotTest, Equality) {
+  DocumentReference doc1 = Collection("col1").Document();
+  DocumentReference doc2 = Collection("col1").Document();
+  DocumentSnapshot doc1_snap1 = ReadDocument(doc1);
+  DocumentSnapshot doc1_snap2 = ReadDocument(doc1);
+  DocumentSnapshot doc2_snap1 = ReadDocument(doc2);
+  DocumentSnapshot snap3 = DocumentSnapshot();
+  DocumentSnapshot snap4 = DocumentSnapshot();
+
+  EXPECT_TRUE(doc1_snap1 == doc1_snap1);
+  EXPECT_TRUE(doc1_snap1 == doc1_snap2);
+  EXPECT_TRUE(doc1_snap1 != doc2_snap1);
+  EXPECT_TRUE(doc1_snap1 != snap3);
+  EXPECT_TRUE(snap3 == snap4);
+
+  EXPECT_FALSE(doc1_snap1 != doc1_snap1);
+  EXPECT_FALSE(doc1_snap1 != doc1_snap2);
+  EXPECT_FALSE(doc1_snap1 == doc2_snap1);
+  EXPECT_FALSE(doc1_snap1 == snap3);
+  EXPECT_FALSE(snap3 != snap4);
+}
+
 }  // namespace firestore
 }  // namespace firebase
diff --git a/firestore/src/android/document_snapshot_android.cc b/firestore/src/android/document_snapshot_android.cc
@@ -24,6 +24,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"
 
@@ -128,5 +129,10 @@ FieldValue DocumentSnapshotInternal::Get(const FieldPath& field,
   return FieldValueInternal::Create(env, field_value);
 }
 
+bool operator==(const DocumentSnapshotInternal& lhs,
+                const DocumentSnapshotInternal& rhs) {
+  return jni::EqualityCompareJni(lhs, rhs);
+}
+
 }  // namespace firestore
 }  // namespace firebase
diff --git a/firestore/src/android/document_snapshot_android.h b/firestore/src/android/document_snapshot_android.h
@@ -64,6 +64,14 @@ class DocumentSnapshotInternal : public Wrapper {
   mutable std::string cached_id_;
 };
 
+bool operator==(const DocumentSnapshotInternal& lhs,
+                const DocumentSnapshotInternal& rhs);
+
+inline bool operator!=(const DocumentSnapshotInternal& lhs,
+                       const DocumentSnapshotInternal& rhs) {
+  return !(lhs == rhs);
+}
+
 }  // namespace firestore
 }  // namespace firebase
 #endif  // FIREBASE_FIRESTORE_SRC_ANDROID_DOCUMENT_SNAPSHOT_ANDROID_H_
diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc
@@ -25,6 +25,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"
@@ -384,8 +385,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 jni::EqualityCompareJni(lhs, rhs);
 }
 
 template <typename T>
diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc
@@ -32,6 +32,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"
@@ -346,8 +347,7 @@ Local<Array<Object>> QueryInternal::ConvertFieldValues(
 }
 
 bool operator==(const QueryInternal& lhs, const QueryInternal& 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/android/query_snapshot_android.cc b/firestore/src/android/query_snapshot_android.cc
@@ -23,6 +23,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"
@@ -88,8 +89,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 jni::EqualityCompareJni(lhs, rhs);
 }
 
 }  // namespace firestore
diff --git a/firestore/src/common/document_snapshot.cc b/firestore/src/common/document_snapshot.cc
@@ -155,5 +155,9 @@ std::ostream& operator<<(std::ostream& out, const DocumentSnapshot& document) {
   return out << document.ToString();
 }
 
+bool operator==(const DocumentSnapshot& lhs, const DocumentSnapshot& rhs) {
+  return EqualityCompare(lhs.internal_, rhs.internal_);
+}
+
 }  // namespace firestore
 }  // namespace firebase
diff --git a/firestore/src/common/field_value.cc b/firestore/src/common/field_value.cc
@@ -25,6 +25,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"
@@ -341,9 +342,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
@@ -22,6 +22,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"
@@ -302,11 +303,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
@@ -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_change.h"
 #include "firestore/src/include/firebase/firestore/document_snapshot.h"
 #include "firestore/src/include/firebase/firestore/query.h"
@@ -118,11 +119,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
@@ -27,6 +27,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 <typename T>
+bool EqualityCompare(T* lhs, T* rhs) {
+  return lhs == rhs || (lhs != nullptr && rhs != nullptr && *lhs == *rhs);
+}
+
 }  // namespace firestore
 }  // namespace firebase
 
diff --git a/firestore/src/include/firebase/firestore/document_snapshot.h b/firestore/src/include/firebase/firestore/document_snapshot.h
@@ -261,6 +261,9 @@ class DocumentSnapshot {
                                   const DocumentSnapshot& document);
 
  private:
+  friend bool operator==(const DocumentSnapshot& lhs,
+                         const DocumentSnapshot& rhs);
+
   friend class DocumentChangeInternal;
   friend class EventListenerInternal;
   friend class FirestoreInternal;
@@ -276,6 +279,15 @@ class DocumentSnapshot {
   mutable DocumentSnapshotInternal* internal_ = nullptr;
 };
 
+/** Checks `lhs` and `rhs` for equality. */
+bool operator==(const DocumentSnapshot& lhs, const DocumentSnapshot& rhs);
+
+/** Checks `lhs` and `rhs` for inequality. */
+inline bool operator!=(const DocumentSnapshot& lhs,
+                       const DocumentSnapshot& rhs) {
+  return !(lhs == rhs);
+}
+
 }  // namespace firestore
 }  // namespace firebase
 
diff --git a/firestore/src/jni/compare.h b/firestore/src/jni/compare.h
@@ -0,0 +1,39 @@
+/*
+ * 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.
+ * 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.
+ */
+
+#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"
+
+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 <typename T>
+bool EqualityCompareJni(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
diff --git a/firestore/src/main/document_snapshot_main.cc b/firestore/src/main/document_snapshot_main.cc
@@ -215,5 +215,10 @@ FieldValue DocumentSnapshotInternal::ConvertServerTimestamp(
   FIRESTORE_UNREACHABLE();
 }
 
+bool operator==(const DocumentSnapshotInternal& lhs,
+                const DocumentSnapshotInternal& rhs) {
+  return lhs.snapshot_ == rhs.snapshot_;
+}
+
 }  // namespace firestore
 }  // namespace firebase
diff --git a/firestore/src/main/document_snapshot_main.h b/firestore/src/main/document_snapshot_main.h
@@ -63,6 +63,9 @@ class DocumentSnapshotInternal {
 
   DocumentReference reference() const;
 
+  friend bool operator==(const DocumentSnapshotInternal& lhs,
+                         const DocumentSnapshotInternal& rhs);
+
  private:
   using ServerTimestampBehavior = DocumentSnapshot::ServerTimestampBehavior;
 
@@ -87,6 +90,11 @@ class DocumentSnapshotInternal {
   api::DocumentSnapshot snapshot_;
 };
 
+inline bool operator!=(const DocumentSnapshotInternal& lhs,
+                       const DocumentSnapshotInternal& rhs) {
+  return !(lhs == rhs);
+}
+
 }  // namespace firestore
 }  // namespace firebase
 
diff --git a/release_build_files/readme.md b/release_build_files/readme.md
@@ -571,7 +571,7 @@ code.
 -   Changes
     -   General: Updating Android and iOS dependencies to the latest.
     -   Firestore: Added `operator==` and `operator!=` for `SnapshotMetadata`,
-        `Settings`, and `QuerySnapshot`.
+        `Settings`, `QuerySnapshot`, and `DocumentSnapshot`.
 
 ### 8.3.0
 -   Changes

Original file line number	Diff line number	Diff line change
`@@ -155,5 +155,9 @@ std::ostream& operator<<(std::ostream& out, const DocumentSnapshot& document) {`
`155`	`155`	`return out << document.ToString();`
`156`	`156`	`}`
`157`	`157`
	`158`	`+bool operator==(const DocumentSnapshot& lhs, const DocumentSnapshot& rhs) {`
	`159`	`+ return EqualityCompare(lhs.internal_, rhs.internal_);`
	`160`	`+}`
	`161`	`+`
`158`	`162`	`} // namespace firestore`
`159`	`163`	`} // namespace firebase`