Add is_valid methods to the public API classes that can be in an invalid state (#560)

Also improve the error message contained in the invalid futures returned from invalid objects.

Addresses b/172570071 and b/157227167.

Author: var-const

external/vcpkg

@@ -15,9 +15,9 @@ void ExpectAllMethodsAreNoOps(Query* ptr);
 // and return null.
 template <typename T>
 void ExpectNullFirestore(T* ptr) {
-  EXPECT_TRUE(ptr->firestore() == nullptr);
+  EXPECT_EQ(ptr->firestore(), nullptr);
   // Make sure to check both const and non-const overloads.
-  EXPECT_TRUE(static_cast<const T*>(ptr)->firestore() == nullptr);
+  EXPECT_EQ(static_cast<const T*>(ptr)->firestore(), nullptr);
 }
 
 // Checks that the given object can be copied from, and the resulting copy can
@@ -48,32 +48,36 @@ void ExpectEqualityToWork(T* ptr) {
 // value-initialized values.
 
 void ExpectAllMethodsAreNoOps(CollectionReference* ptr) {
-  EXPECT_TRUE(*ptr == CollectionReference());
+  EXPECT_FALSE(ptr->is_valid());
+
+  EXPECT_EQ(*ptr, CollectionReference());
   ExpectCopyableAndMoveable(ptr);
   ExpectEqualityToWork(ptr);
 
   ExpectAllMethodsAreNoOps(static_cast<Query*>(ptr));
 
-  EXPECT_TRUE(ptr->id() == "");
-  EXPECT_TRUE(ptr->path() == "");
+  EXPECT_EQ(ptr->id(), "");
+  EXPECT_EQ(ptr->path(), "");
 
-  EXPECT_TRUE(ptr->Document() == DocumentReference());
-  EXPECT_TRUE(ptr->Document("foo") == DocumentReference());
-  EXPECT_TRUE(ptr->Document(std::string("foo")) == DocumentReference());
+  EXPECT_EQ(ptr->Document(), DocumentReference());
+  EXPECT_EQ(ptr->Document("foo"), DocumentReference());
+  EXPECT_EQ(ptr->Document(std::string("foo")), DocumentReference());
 
-  EXPECT_TRUE(ptr->Add(MapFieldValue()) == FailedFuture<DocumentReference>());
+  EXPECT_EQ(ptr->Add(MapFieldValue()), FailedFuture<DocumentReference>());
 }
 
 void ExpectAllMethodsAreNoOps(DocumentChange* ptr) {
+  EXPECT_FALSE(ptr->is_valid());
+
   // TODO(b/137966104): implement == on `DocumentChange`
   // ExpectEqualityToWork(ptr);
   ExpectCopyableAndMoveable(ptr);
 
-  EXPECT_TRUE(ptr->type() == DocumentChange::Type());
+  EXPECT_EQ(ptr->type(), DocumentChange::Type());
   // TODO(b/137966104): implement == on `DocumentSnapshot`
   ptr->document();
-  EXPECT_TRUE(ptr->old_index() == 0);
-  EXPECT_TRUE(ptr->new_index() == 0);
+  EXPECT_EQ(ptr->old_index(), 0);
+  EXPECT_EQ(ptr->new_index(), 0);
 }
 
 void ExpectAllMethodsAreNoOps(DocumentReference* ptr) {
@@ -83,58 +87,61 @@ void ExpectAllMethodsAreNoOps(DocumentReference* ptr) {
   ExpectCopyableAndMoveable(ptr);
   ExpectNullFirestore(ptr);
 
-  EXPECT_TRUE(ptr->ToString() == "DocumentReference(invalid)");
+  EXPECT_EQ(ptr->ToString(), "DocumentReference(invalid)");
 
-  EXPECT_TRUE(ptr->id() == "");
-  EXPECT_TRUE(ptr->path() == "");
+  EXPECT_EQ(ptr->id(), "");
+  EXPECT_EQ(ptr->path(), "");
 
-  EXPECT_TRUE(ptr->Parent() == CollectionReference());
-  EXPECT_TRUE(ptr->Collection("foo") == CollectionReference());
-  EXPECT_TRUE(ptr->Collection(std::string("foo")) == CollectionReference());
+  EXPECT_EQ(ptr->Parent(), CollectionReference());
+  EXPECT_EQ(ptr->Collection("foo"), CollectionReference());
+  EXPECT_EQ(ptr->Collection(std::string("foo")), CollectionReference());
 
-  EXPECT_TRUE(ptr->Get() == FailedFuture<DocumentSnapshot>());
+  EXPECT_EQ(ptr->Get(), FailedFuture<DocumentSnapshot>());
 
-  EXPECT_TRUE(ptr->Set(MapFieldValue()) == FailedFuture<void>());
+  EXPECT_EQ(ptr->Set(MapFieldValue()), FailedFuture<void>());
 
-  EXPECT_TRUE(ptr->Update(MapFieldValue()) == FailedFuture<void>());
-  EXPECT_TRUE(ptr->Update(MapFieldPathValue()) == FailedFuture<void>());
+  EXPECT_EQ(ptr->Update(MapFieldValue()), FailedFuture<void>());
+  EXPECT_EQ(ptr->Update(MapFieldPathValue()), FailedFuture<void>());
 
-  EXPECT_TRUE(ptr->Delete() == FailedFuture<void>());
+  EXPECT_EQ(ptr->Delete(), FailedFuture<void>());
 
   ptr->AddSnapshotListener(
       [](const DocumentSnapshot&, Error, const std::string&) {});
 }
 
 void ExpectAllMethodsAreNoOps(DocumentSnapshot* ptr) {
+  EXPECT_FALSE(ptr->is_valid());
+
   // TODO(b/137966104): implement == on `DocumentSnapshot`
   // ExpectEqualityToWork(ptr);
   ExpectCopyableAndMoveable(ptr);
 
-  EXPECT_TRUE(ptr->ToString() == "DocumentSnapshot(invalid)");
+  EXPECT_EQ(ptr->ToString(), "DocumentSnapshot(invalid)");
 
-  EXPECT_TRUE(ptr->id() == "");
+  EXPECT_EQ(ptr->id(), "");
   EXPECT_FALSE(ptr->exists());
 
-  EXPECT_TRUE(ptr->reference() == DocumentReference());
+  EXPECT_EQ(ptr->reference(), DocumentReference());
   // TODO(b/137966104): implement == on `SnapshotMetadata`
   ptr->metadata();
 
-  EXPECT_TRUE(ptr->GetData() == MapFieldValue());
+  EXPECT_EQ(ptr->GetData(), MapFieldValue());
 
-  EXPECT_TRUE(ptr->Get("foo") == FieldValue());
-  EXPECT_TRUE(ptr->Get(std::string("foo")) == FieldValue());
-  EXPECT_TRUE(ptr->Get(FieldPath{"foo"}) == FieldValue());
+  EXPECT_EQ(ptr->Get("foo"), FieldValue());
+  EXPECT_EQ(ptr->Get(std::string("foo")), FieldValue());
+  EXPECT_EQ(ptr->Get(FieldPath{"foo"}), FieldValue());
 }
 
 void ExpectAllMethodsAreNoOps(FieldValue* ptr) {
+  EXPECT_FALSE(ptr->is_valid());
+
   ExpectEqualityToWork(ptr);
   ExpectCopyableAndMoveable(ptr);
 
-  EXPECT_FALSE(ptr->is_valid());
   // FieldValue doesn't have a separate "invalid" type in its enum.
   EXPECT_TRUE(ptr->is_null());
 
-  EXPECT_TRUE(ptr->type() == FieldValue::Type());
+  EXPECT_EQ(ptr->type(), FieldValue::Type());
 
   EXPECT_FALSE(ptr->is_boolean());
   EXPECT_FALSE(ptr->is_integer());
@@ -147,93 +154,100 @@ void ExpectAllMethodsAreNoOps(FieldValue* ptr) {
   EXPECT_FALSE(ptr->is_array());
   EXPECT_FALSE(ptr->is_map());
 
-  EXPECT_TRUE(ptr->boolean_value() == false);
-  EXPECT_TRUE(ptr->integer_value() == 0);
-  EXPECT_TRUE(ptr->double_value() == 0);
-  EXPECT_TRUE(ptr->timestamp_value() == Timestamp());
-  EXPECT_TRUE(ptr->string_value() == "");
-  EXPECT_TRUE(ptr->blob_value() == nullptr);
-  EXPECT_TRUE(ptr->reference_value() == DocumentReference());
-  EXPECT_TRUE(ptr->geo_point_value() == GeoPoint());
+  EXPECT_EQ(ptr->boolean_value(), false);
+  EXPECT_EQ(ptr->integer_value(), 0);
+  EXPECT_EQ(ptr->double_value(), 0);
+  EXPECT_EQ(ptr->timestamp_value(), Timestamp());
+  EXPECT_EQ(ptr->string_value(), "");
+  EXPECT_EQ(ptr->blob_value(), nullptr);
+  EXPECT_EQ(ptr->reference_value(), DocumentReference());
+  EXPECT_EQ(ptr->geo_point_value(), GeoPoint());
   EXPECT_TRUE(ptr->array_value().empty());
   EXPECT_TRUE(ptr->map_value().empty());
 }
 
 void ExpectAllMethodsAreNoOps(ListenerRegistration* ptr) {
+  EXPECT_FALSE(ptr->is_valid());
+
   // `ListenerRegistration` isn't equality comparable.
   ExpectCopyableAndMoveable(ptr);
 
   ptr->Remove();
 }
 
 void ExpectAllMethodsAreNoOps(Query* ptr) {
+  EXPECT_FALSE(ptr->is_valid());
+
   ExpectEqualityToWork(ptr);
   ExpectCopyableAndMoveable(ptr);
   ExpectNullFirestore(ptr);
 
-  EXPECT_TRUE(ptr->WhereEqualTo("foo", FieldValue()) == Query());
-  EXPECT_TRUE(ptr->WhereEqualTo(FieldPath{"foo"}, FieldValue()) == Query());
+  EXPECT_EQ(ptr->WhereEqualTo("foo", FieldValue()), Query());
+  EXPECT_EQ(ptr->WhereEqualTo(FieldPath{"foo"}, FieldValue()), Query());
 
-  EXPECT_TRUE(ptr->WhereLessThan("foo", FieldValue()) == Query());
-  EXPECT_TRUE(ptr->WhereLessThan(FieldPath{"foo"}, FieldValue()) == Query());
+  EXPECT_EQ(ptr->WhereLessThan("foo", FieldValue()), Query());
+  EXPECT_EQ(ptr->WhereLessThan(FieldPath{"foo"}, FieldValue()), Query());
 
-  EXPECT_TRUE(ptr->WhereLessThanOrEqualTo("foo", FieldValue()) == Query());
-  EXPECT_TRUE(ptr->WhereLessThanOrEqualTo(FieldPath{"foo"}, FieldValue()) ==
-              Query());
+  EXPECT_EQ(ptr->WhereLessThanOrEqualTo("foo", FieldValue()), Query());
+  EXPECT_EQ(ptr->WhereLessThanOrEqualTo(FieldPath{"foo"}, FieldValue()),
+            Query());
 
-  EXPECT_TRUE(ptr->WhereGreaterThan("foo", FieldValue()) == Query());
-  EXPECT_TRUE(ptr->WhereGreaterThan(FieldPath{"foo"}, FieldValue()) == Query());
+  EXPECT_EQ(ptr->WhereGreaterThan("foo", FieldValue()), Query());
+  EXPECT_EQ(ptr->WhereGreaterThan(FieldPath{"foo"}, FieldValue()), Query());
 
-  EXPECT_TRUE(ptr->WhereGreaterThanOrEqualTo("foo", FieldValue()) == Query());
-  EXPECT_TRUE(ptr->WhereGreaterThanOrEqualTo(FieldPath{"foo"}, FieldValue()) ==
-              Query());
+  EXPECT_EQ(ptr->WhereGreaterThanOrEqualTo("foo", FieldValue()), Query());
+  EXPECT_EQ(ptr->WhereGreaterThanOrEqualTo(FieldPath{"foo"}, FieldValue()),
+            Query());
 
-  EXPECT_TRUE(ptr->WhereArrayContains("foo", FieldValue()) == Query());
-  EXPECT_TRUE(ptr->WhereArrayContains(FieldPath{"foo"}, FieldValue()) ==
-              Query());
+  EXPECT_EQ(ptr->WhereArrayContains("foo", FieldValue()), Query());
+  EXPECT_EQ(ptr->WhereArrayContains(FieldPath{"foo"}, FieldValue()), Query());
 
-  EXPECT_TRUE(ptr->OrderBy("foo") == Query());
-  EXPECT_TRUE(ptr->OrderBy(FieldPath{"foo"}) == Query());
+  EXPECT_EQ(ptr->OrderBy("foo"), Query());
+  EXPECT_EQ(ptr->OrderBy(FieldPath{"foo"}), Query());
 
-  EXPECT_TRUE(ptr->Limit(123) == Query());
+  EXPECT_EQ(ptr->Limit(123), Query());
 
-  EXPECT_TRUE(ptr->StartAt(DocumentSnapshot()) == Query());
-  EXPECT_TRUE(ptr->StartAt(std::vector<FieldValue>()) == Query());
+  EXPECT_EQ(ptr->StartAt(DocumentSnapshot()), Query());
+  EXPECT_EQ(ptr->StartAt(std::vector<FieldValue>()), Query());
 
-  EXPECT_TRUE(ptr->StartAfter(DocumentSnapshot()) == Query());
-  EXPECT_TRUE(ptr->StartAfter(std::vector<FieldValue>()) == Query());
+  EXPECT_EQ(ptr->StartAfter(DocumentSnapshot()), Query());
+  EXPECT_EQ(ptr->StartAfter(std::vector<FieldValue>()), Query());
 
-  EXPECT_TRUE(ptr->EndBefore(DocumentSnapshot()) == Query());
-  EXPECT_TRUE(ptr->EndBefore(std::vector<FieldValue>()) == Query());
+  EXPECT_EQ(ptr->EndBefore(DocumentSnapshot()), Query());
+  EXPECT_EQ(ptr->EndBefore(std::vector<FieldValue>()), Query());
 
-  EXPECT_TRUE(ptr->EndAt(DocumentSnapshot()) == Query());
-  EXPECT_TRUE(ptr->EndAt(std::vector<FieldValue>()) == Query());
+  EXPECT_EQ(ptr->EndAt(DocumentSnapshot()), Query());
+  EXPECT_EQ(ptr->EndAt(std::vector<FieldValue>()), Query());
 
-  EXPECT_TRUE(ptr->Get() == FailedFuture<QuerySnapshot>());
+  EXPECT_EQ(ptr->Get(), FailedFuture<QuerySnapshot>());
 
-  EXPECT_TRUE(ptr->Get() == FailedFuture<QuerySnapshot>());
+  EXPECT_EQ(ptr->Get(), FailedFuture<QuerySnapshot>());
 
   ptr->AddSnapshotListener(
       [](const QuerySnapshot&, Error, const std::string&) {});
 }
 
 void ExpectAllMethodsAreNoOps(QuerySnapshot* ptr) {
+  EXPECT_FALSE(ptr->is_valid());
+
   // TODO(b/137966104): implement == on `QuerySnapshot`
   // ExpectEqualityToWork(ptr);
   ExpectCopyableAndMoveable(ptr);
 
-  EXPECT_TRUE(ptr->query() == Query());
+  EXPECT_EQ(ptr->query(), Query());
 
   // TODO(b/137966104): implement == on `SnapshotMetadata`
   ptr->metadata();
 
   EXPECT_TRUE(ptr->DocumentChanges().empty());
   EXPECT_TRUE(ptr->documents().empty());
   EXPECT_TRUE(ptr->empty());
-  EXPECT_TRUE(ptr->size() == 0);
+  EXPECT_EQ(ptr->size(), 0);
 }
 
 void ExpectAllMethodsAreNoOps(WriteBatch* ptr) {
+  EXPECT_FALSE(ptr->is_valid());
+
   // `WriteBatch` isn't equality comparable.
   ExpectCopyableAndMoveable(ptr);
 
@@ -244,7 +258,7 @@ void ExpectAllMethodsAreNoOps(WriteBatch* ptr) {
 
   ptr->Delete(DocumentReference());
 
-  EXPECT_TRUE(ptr->Commit() == FailedFuture<void>());
+  EXPECT_EQ(ptr->Commit(), FailedFuture<void>());
 }
 
 using CleanupTest = FirestoreIntegrationTest;
@@ -337,7 +351,7 @@ TEST_F(CleanupTest, FieldValueIsBlankAfterCleanup) {
   // stay valid after Firestore has shut down.
   EXPECT_TRUE(str_value.is_valid());
   EXPECT_TRUE(str_value.is_string());
-  EXPECT_TRUE(str_value.string_value() == "bar");
+  EXPECT_EQ(str_value.string_value(), "bar");
 
   // However, need to make sure that in a reference value, the reference was
   // cleaned up.
@@ -382,7 +396,7 @@ TEST_F(CleanupTest, QuerySnapshotIsBlankAfterCleanup) {
   WriteDocument(doc, MapFieldValue{{"foo", FieldValue::String("bar")}});
 
   QuerySnapshot snap = ReadDocuments(col);
-  EXPECT_TRUE(snap.size() == 1);
+  EXPECT_EQ(snap.size(), 1);
 
   DeleteFirestore(col.firestore());
   SCOPED_TRACE("QuerySnapshot.AfterCleanup");

firestore/integration_test_internal/src/cleanup_test.cc

@@ -55,8 +55,10 @@ template <typename T>
 Future<T> FailedFuture() {
   static auto* future = new Future<T>(FailedFuture<T>(
       Error::kErrorFailedPrecondition,
-      "This instance is in an invalid state. This is because the underlying "
-      "Firestore instance has been destructed."));
+      "The object that issued this future is in an invalid state. This can be "
+      "because the object was default-constructed and never reassigned, "
+      "the object was moved from, or the Firestore instance with which the "
+      "object was associated has been destroyed."));
   return *future;
 }
 

firestore/src/common/futures.h

@@ -159,6 +159,19 @@ class DocumentChange {
    */
   virtual std::size_t new_index() const;
 
+  /**
+   * @brief Returns true if this `DocumentChange` is valid, false if it is
+   * not valid. An invalid `DocumentChange` could be the result of:
+   *   - Creating a `DocumentChange` using the default constructor.
+   *   - Moving from the `DocumentChange`.
+   *   - Deleting your Firestore instance, which will invalidate all the
+   *     `DocumentChange` instances associated with it.
+   *
+   * @return true if this `DocumentChange` is valid, false if this
+   * `DocumentChange` is invalid.
+   */
+  bool is_valid() const { return internal_ != nullptr; }
+
  private:
   friend class FirestoreInternal;
   friend class Wrapper;

firestore/src/include/firebase/firestore/document_change.h

@@ -285,16 +285,17 @@ class DocumentReference {
           callback);
 
   /**
-   * @brief Returns true if this DocumentReference is valid, false if it is not
-   * valid. An invalid DocumentReference could be the result of:
-   *   - Creating a DocumentReference with the default constructor.
-   *   - Calling CollectionReference::Parent() on a CollectionReference that is
-   *     not a subcollection.
-   *   - Deleting your Firestore instance, which will invalidate all
-   *     DocumentReference instances associated with it.
-   *
-   * @return true if this DocumentReference is valid, false if this
-   * DocumentReference is invalid.
+   * @brief Returns true if this `DocumentReference` is valid, false if it is
+   * not valid. An invalid `DocumentReference` could be the result of:
+   *   - Creating a `DocumentReference` using the default constructor.
+   *   - Moving from the `DocumentReference`.
+   *   - Calling `CollectionReference::Parent()` on a `CollectionReference` that
+   *     is not a subcollection.
+   *   - Deleting your Firestore instance, which will invalidate all the
+   *     `DocumentReference` instances associated with it.
+   *
+   * @return true if this `DocumentReference` is valid, false if this
+   * `DocumentReference` is invalid.
    */
   bool is_valid() const { return internal_ != nullptr; }
 

firestore/src/include/firebase/firestore/document_reference.h

@@ -229,6 +229,19 @@ class DocumentSnapshot {
       const FieldPath& field,
       ServerTimestampBehavior stb = ServerTimestampBehavior::kDefault) const;
 
+  /**
+   * @brief Returns true if this `DocumentSnapshot` is valid, false if it is
+   * not valid. An invalid `DocumentSnapshot` could be the result of:
+   *   - Creating a `DocumentSnapshot` with the default constructor.
+   *   - Moving from the `DocumentSnapshot`.
+   *   - Deleting your Firestore instance, which will invalidate all the
+   *     `DocumentSnapshot` instances associated with it.
+   *
+   * @return true if this `DocumentSnapshot` is valid, false if this
+   * `DocumentSnapshot` is invalid.
+   */
+  bool is_valid() const { return internal_ != nullptr; }
+
   /**
    * Returns a string representation of this `DocumentSnapshot` for
    * logging/debugging purposes.