diff --git a/firestore/src/android/field_path_portable.cc b/firestore/src/android/field_path_portable.cc index 405f9db75c..20bd224841 100644 --- a/firestore/src/android/field_path_portable.cc +++ b/firestore/src/android/field_path_portable.cc @@ -90,6 +90,22 @@ std::vector SplitOnDots(const std::string& input) { return result; } +void ValidateSegments(const std::vector& segments) { + if (segments.empty()) { + SimpleThrowInvalidArgument( + "Invalid field path. Provided names must not be empty."); + } + + for (size_t i = 0; i < segments.size(); ++i) { + if (segments[i].empty()) { + std::ostringstream message; + message << "Invalid field name at index " << i + << ". Field names must not be empty."; + SimpleThrowInvalidArgument(message.str()); + } + } +} + } // anonymous namespace std::string FieldPathPortable::CanonicalString() const { @@ -118,6 +134,12 @@ bool FieldPathPortable::IsKeyFieldPath() const { return size() == 1 && segments_[0] == FieldPathPortable::kDocumentKeyPath; } +FieldPathPortable FieldPathPortable::FromSegments( + std::vector segments) { + ValidateSegments(segments); + return FieldPathPortable(Move(segments)); +} + FieldPathPortable FieldPathPortable::FromDotSeparatedString( const std::string& path) { if (path.find_first_of("~*/[]") != std::string::npos) { diff --git a/firestore/src/android/field_path_portable.h b/firestore/src/android/field_path_portable.h index d23d5258db..ca3b420e96 100644 --- a/firestore/src/android/field_path_portable.h +++ b/firestore/src/android/field_path_portable.h @@ -65,8 +65,16 @@ class FieldPathPortable { return segments_ >= rhs.segments_; } - // Creates and returns a new path from a dot-separated field-path string, - // where path segments are separated by a dot ".". + /** + * Creates and returns a new path from an explicitly pre-split list of + * segments. + */ + static FieldPathPortable FromSegments(std::vector segments); + + /** + * Creates and returns a new path from a dot-separated field-path string, + * where path segments are separated by a dot ".". + */ static FieldPathPortable FromDotSeparatedString(const std::string& path); static FieldPathPortable KeyFieldPath(); diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index 2133e46988..24c1b879ad 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -61,6 +61,8 @@ namespace { using jni::Constructor; using jni::Env; +using jni::Global; +using jni::HashMap; using jni::Loader; using jni::Local; using jni::Long; @@ -170,15 +172,15 @@ class JavaFirestoreMap { private: // Ensures that the backing map is initialized. // Prerequisite: `mutex_` must be locked before calling this method. - jni::HashMap& GetMapLocked(Env& env) { + HashMap& GetMapLocked(Env& env) { if (!firestores_) { - firestores_ = jni::HashMap::Create(env); + firestores_ = HashMap::Create(env); } return firestores_; } Mutex mutex_; - jni::Global firestores_; + Global firestores_; }; // init_mutex protects all the globals below. @@ -496,34 +498,34 @@ void FirestoreInternal::ClearListeners() { listener_registrations_.clear(); } -jni::Env FirestoreInternal::GetEnv() { - jni::Env env; +Env FirestoreInternal::GetEnv() { + Env env; env.SetUnhandledExceptionHandler(GlobalUnhandledExceptionHandler, nullptr); return env; } CollectionReference FirestoreInternal::NewCollectionReference( - jni::Env& env, const jni::Object& reference) const { + Env& env, const jni::Object& reference) const { return MakePublic(env, mutable_this(), reference); } DocumentReference FirestoreInternal::NewDocumentReference( - jni::Env& env, const jni::Object& reference) const { + Env& env, const jni::Object& reference) const { return MakePublic(env, mutable_this(), reference); } DocumentSnapshot FirestoreInternal::NewDocumentSnapshot( - jni::Env& env, const jni::Object& snapshot) const { + Env& env, const jni::Object& snapshot) const { return MakePublic(env, mutable_this(), snapshot); } -Query FirestoreInternal::NewQuery(jni::Env& env, +Query FirestoreInternal::NewQuery(Env& env, const jni::Object& query) const { return MakePublic(env, mutable_this(), query); } QuerySnapshot FirestoreInternal::NewQuerySnapshot( - jni::Env& env, const jni::Object& snapshot) const { + Env& env, const jni::Object& snapshot) const { return MakePublic(env, mutable_this(), snapshot); } diff --git a/firestore/src/android/transaction_android.cc b/firestore/src/android/transaction_android.cc index e76b5b04ed..7ac66bb3ea 100644 --- a/firestore/src/android/transaction_android.cc +++ b/firestore/src/android/transaction_android.cc @@ -1,5 +1,6 @@ #include "firestore/src/android/transaction_android.h" +#include #include #include "app/meta/move.h" @@ -10,9 +11,11 @@ #include "firestore/src/android/field_value_android.h" #include "firestore/src/android/set_options_android.h" #include "firestore/src/android/util_android.h" +#include "firestore/src/common/macros.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/hash_map.h" #include "firestore/src/jni/loader.h" +#include "Firestore/core/src/util/firestore_exceptions.h" namespace firebase { namespace firestore { @@ -121,9 +124,10 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document, } if (!ExceptionInternal::IsFirestoreException(env, exception)) { - // We would only preserve exception if it is not - // FirebaseFirestoreException. The user should decide whether to raise the - // error or let the transaction succeed. + // Only preserve the exception if it is not `FirebaseFirestoreException`. + // For Firestore exceptions, the user decides whether to raise the error + // or let the transaction succeed through the error code/message the + // `TransactionFunction` returns. PreserveException(env, Move(exception)); } return DocumentSnapshot{}; @@ -141,9 +145,18 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document, } Env TransactionInternal::GetEnv() { +#if FIRESTORE_HAVE_EXCEPTIONS + // If exceptions are enabled, report Java exceptions by translating them to + // their C++ equivalents in the usual way. These will throw out to the + // user-supplied TransactionFunction and ultimately out to + // `TransactionFunctionNativeApply`, below. + return FirestoreInternal::GetEnv(); + +#else Env env; env.SetUnhandledExceptionHandler(ExceptionHandler, this); return env; +#endif } void TransactionInternal::ExceptionHandler(Env& env, @@ -195,7 +208,32 @@ jobject TransactionInternal::TransactionFunctionNativeApply( new TransactionInternal(firestore, Object(java_transaction))); std::string message; - Error code = transaction_function->Apply(transaction, message); + Error code; + +#if FIRESTORE_HAVE_EXCEPTIONS + try { +#endif + + code = transaction_function->Apply(transaction, message); + +#if FIRESTORE_HAVE_EXCEPTIONS + } catch (const FirestoreException& e) { + message = e.what(); + code = e.code(); + } catch (const std::invalid_argument& e) { + message = e.what(); + code = Error::kErrorInvalidArgument; + } catch (const std::logic_error& e) { + message = e.what(); + code = Error::kErrorFailedPrecondition; + } catch (const std::exception& e) { + message = std::string("Unknown exception: ") + e.what(); + code = Error::kErrorUnknown; + } catch (...) { + message = "Unknown exception"; + code = Error::kErrorUnknown; + } +#endif // FIRESTORE_HAVE_EXCEPTIONS // Verify that `internal_` is not null before using it. It could be set to // `nullptr` if the `FirestoreInternal` is destroyed during the invocation of diff --git a/firestore/src/common/field_path.cc b/firestore/src/common/field_path.cc index a5d48e5f36..1de85336fe 100644 --- a/firestore/src/common/field_path.cc +++ b/firestore/src/common/field_path.cc @@ -24,6 +24,8 @@ #include #endif // defined(_STLPORT_VERSION) +#include "app/meta/move.h" + #if defined(__ANDROID__) || defined(FIRESTORE_STUB_BUILD) #include "firestore/src/android/field_path_portable.h" #else @@ -38,11 +40,11 @@ FieldPath::FieldPath() {} #if !defined(_STLPORT_VERSION) FieldPath::FieldPath(std::initializer_list field_names) - : internal_(new FieldPathInternal{field_names}) {} + : internal_(InternalFromSegments(std::vector(field_names))) {} #endif // !defined(_STLPORT_VERSION) FieldPath::FieldPath(const std::vector& field_names) - : internal_(new FieldPathInternal{std::vector{field_names}}) {} + : internal_(InternalFromSegments(field_names)) {} FieldPath::FieldPath(const FieldPath& path) : internal_(new FieldPathInternal{*path.internal_}) {} @@ -78,6 +80,12 @@ FieldPath FieldPath::DocumentId() { return FieldPath{new FieldPathInternal{FieldPathInternal::KeyFieldPath()}}; } +FieldPath::FieldPathInternal* FieldPath::InternalFromSegments( + std::vector field_names) { + return new FieldPathInternal( + FieldPathInternal::FromSegments(Move(field_names))); +} + /* static */ FieldPath FieldPath::FromDotSeparatedString(const std::string& path) { return FieldPath{ diff --git a/firestore/src/include/firebase/firestore/field_path.h b/firestore/src/include/firebase/firestore/field_path.h index 644aacdce8..807515c492 100644 --- a/firestore/src/include/firebase/firestore/field_path.h +++ b/firestore/src/include/firebase/firestore/field_path.h @@ -170,6 +170,9 @@ class FieldPath final { explicit FieldPath(FieldPathInternal* internal); + static FieldPathInternal* InternalFromSegments( + std::vector field_names); + static FieldPath FromDotSeparatedString(const std::string& path); FieldPathInternal* internal_ = nullptr; diff --git a/firestore/src/ios/user_data_converter_ios.cc b/firestore/src/ios/user_data_converter_ios.cc index 44a2004c9a..1daf033c55 100644 --- a/firestore/src/ios/user_data_converter_ios.cc +++ b/firestore/src/ios/user_data_converter_ios.cc @@ -36,6 +36,13 @@ using nanopb::ByteString; using Type = FieldValue::Type; +FIRESTORE_ATTRIBUTE_NORETURN +void ThrowInvalidData(const ParseContext& context, const std::string& message) { + std::string full_message = + "Invalid data. " + message + context.FieldDescription(); + SimpleThrowInvalidArgument(full_message); +} + void ParseDelete(ParseContext&& context) { if (context.data_source() == UserDataSource::MergeSet) { // No transform to add for a delete, but we need to add it to our field mask @@ -49,32 +56,16 @@ void ParseDelete(ParseContext&& context) { !context.path()->empty(), "FieldValue.Delete() at the top level should have already been " "handled."); - // TODO(b/147444199): use string formatting. - // ThrowInvalidArgument( - // "FieldValue::Delete() can only appear at the top level of your " - // "update data%s", - // context.FieldDescription()); - auto message = - std::string( - "FieldValue::Delete() can only appear at the top level of your " - "update data") + - context.FieldDescription(); - SimpleThrowInvalidArgument(message); + ThrowInvalidData(context, + "FieldValue::Delete() can only appear at the top level of " + "your update data"); } // We shouldn't encounter delete sentinels for queries or non-merge `Set` // calls. - // TODO(b/147444199): use string formatting. - // ThrowInvalidArgument( - // "FieldValue::Delete() can only be used with Update() and Set() with " - // "merge == true%s", - // context.FieldDescription()); - auto message = - std::string( - "FieldValue::Delete() can only be used with Update() and Set() with " - "merge == true") + - context.FieldDescription(); - SimpleThrowInvalidArgument(message); + ThrowInvalidData(context, + "FieldValue::Delete() can only be used with Update() and " + "Set() with merge == true"); } void ParseServerTimestamp(ParseContext&& context) { @@ -143,7 +134,7 @@ FieldMask CreateFieldMask(const ParseAccumulator& accumulator, // path.CanonicalString()); auto message = std::string("Field '") + path.CanonicalString() + - "' is specified in your field mask but missing from your input data."; + "' is specified in your field mask but not in your input data."; SimpleThrowInvalidArgument(message); } @@ -276,7 +267,7 @@ model::FieldValue::Array UserDataConverter::ParseArray( // disable this validation. if (context.array_element() && context.data_source() != core::UserDataSource::ArrayArgument) { - SimpleThrowInvalidArgument("Nested arrays are not supported"); + ThrowInvalidData(context, "Nested arrays are not supported"); } model::FieldValue::Array result; @@ -323,21 +314,21 @@ void UserDataConverter::ParseSentinel(const FieldValue& value, // Sentinels are only supported with writes, and not within arrays. if (!context.write()) { // TODO(b/147444199): use string formatting. - // ThrowInvalidArgument("%s can only be used with Update() and Set()%s", - // Describe(value.type()), context.FieldDescription()); - auto message = Describe(value.type()) + - " can only be used with Update() and Set()" + - context.FieldDescription(); - SimpleThrowInvalidArgument(message); + // ThrowInvalidData( + // context, "%s can only be used with Update() and Set()%s", + // Describe(value.type()), context.FieldDescription()); + auto message = + Describe(value.type()) + " can only be used with Update() and Set()"; + ThrowInvalidData(context, message); } if (!context.path()) { // TODO(b/147444199): use string formatting. - // ThrowInvalidArgument("%s is not currently supported inside arrays", - // Describe(value.type())); + // ThrowInvalidData(context, "%s is not currently supported inside arrays", + // Describe(value.type())); auto message = Describe(value.type()) + " is not currently supported inside arrays"; - SimpleThrowInvalidArgument(message); + ThrowInvalidData(context, message); } switch (value.type()) { @@ -406,7 +397,8 @@ model::FieldValue UserDataConverter::ParseScalar(const FieldValue& value, GetInternal(reference.firestore())->database_id(); if (other != *database_id_) { // TODO(b/147444199): use string formatting. - // ThrowInvalidArgument( + // ThrowInvalidData( + // context, // "DocumentReference is for database %s/%s but should be for " // "database %s/%s%s", // other.project_id(), other.database_id(), @@ -415,10 +407,9 @@ model::FieldValue UserDataConverter::ParseScalar(const FieldValue& value, auto actual_db = other.project_id() + "/" + other.database_id(); auto expected_db = database_id_->project_id() + "/" + database_id_->database_id(); - auto message = std::string("DocumentReference is for database ") + - actual_db + " but should be for database " + - expected_db + context.FieldDescription(); - SimpleThrowInvalidArgument(message); + auto message = std::string("Document reference is for database ") + + actual_db + " but should be for database " + expected_db; + ThrowInvalidData(context, message); } const model::DocumentKey& key = GetInternal(&reference)->key(); diff --git a/firestore/src/tests/validation_test.cc b/firestore/src/tests/validation_test.cc index 5a5fb17086..0ff12762b0 100644 --- a/firestore/src/tests/validation_test.cc +++ b/firestore/src/tests/validation_test.cc @@ -5,6 +5,8 @@ #include #include +#include "app/src/log.h" +#include "auth/src/include/firebase/auth.h" #include "firestore/src/common/macros.h" #include "firestore/src/include/firebase/firestore.h" #include "firestore/src/tests/firestore_integration_test.h" @@ -32,6 +34,7 @@ namespace firestore { #if FIRESTORE_HAVE_EXCEPTIONS +using firebase::auth::Auth; using testing::AnyOf; using testing::Property; using testing::StrEq; @@ -41,6 +44,23 @@ using testing::Throws; EXPECT_THAT([&] { stmt; }, Throws(Property( \ &std::exception::what, StrEq(message)))); +/** + * Creates a Matcher that matches any of a number of alternative error messages. + * This is useful temporarily because the different platforms produce different + * error messages from the same validation criteria. + * + * Every usage of this method is an indication of a disagreement between these + * platforms and ultimately must be eliminated. + * + * TODO(b/171990785): Unify Android and C++ validation error messages. + * Remove this method once that's done. + */ +template +auto AnyMessageEq(Messages&&... messages) + -> decltype(AnyOf(StrEq(messages)...)) { + return AnyOf(StrEq(messages)...); +} + class ValidationTest : public FirestoreIntegrationTest { protected: /** @@ -93,12 +113,10 @@ class ValidationTest : public FirestoreIntegrationTest { [data, reason, include_sets, include_updates, document]( Transaction& transaction, std::string& error_message) -> Error { if (include_sets) { - // TODO(b/149105903): Android does expectError here. - transaction.Set(document, data); + EXPECT_ERROR(transaction.Set(document, data), reason); } if (include_updates) { - // TODO(b/149105903): Android does expectError here. - transaction.Update(document, data); + EXPECT_ERROR(transaction.Update(document, data), reason); } return Error::kErrorOk; })); @@ -147,20 +165,15 @@ class ValidationTest : public FirestoreIntegrationTest { document.Update(MapFieldValue{{path, FieldValue::Integer(1)}}); FAIL() << "should throw exception"; } catch (const std::invalid_argument& exception) { - // TODO(b/171990785): Unify Android and C++ validation error messages. EXPECT_THAT( exception.what(), - AnyOf( - // When validated by iOS or common C++ code - StrEq(reason), - // When validated by Android Java code - StrEq("Use FieldPath.of() for field names containing '~*/[]'."))); + AnyMessageEq( + reason, + "Use FieldPath.of() for field names containing '~*/[]'.")); } } }; -#if defined(__ANDROID__) - // PORT_NOTE: Does not apply to C++ as host parameter is passed by value. TEST_F(ValidationTest, FirestoreSettingsNullHostFails) {} @@ -173,12 +186,16 @@ TEST_F(ValidationTest, ChangingSettingsAfterUseFails) { try { TestFirestore()->set_settings(setting); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ( - "FirebaseFirestore has already been started and its settings can no " - "longer be changed. You can only call setFirestoreSettings() before " - "calling any other methods on a FirebaseFirestore object.", - exception.what()); + } catch (const std::logic_error& exception) { + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Firestore instance has already been started and its settings can " + "no longer be changed. You can only set settings before calling " + "any other methods on a Firestore instance.", + "FirebaseFirestore has already been started and its settings can " + "no longer be changed. You can only call setFirestoreSettings() " + "before calling any other methods on a FirebaseFirestore object.")); } } @@ -188,7 +205,7 @@ TEST_F(ValidationTest, DisableSslWithoutSettingHostFails) { try { TestFirestore()->set_settings(setting); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::logic_error& exception) { EXPECT_STREQ( "You can't set the 'sslEnabled' setting unless you also set a " "non-default 'host'.", @@ -202,9 +219,29 @@ TEST_F(ValidationTest, FirestoreGetInstanceWithNullAppFails) {} TEST_F(ValidationTest, FirestoreGetInstanceWithNonNullAppReturnsNonNullInstance) { try { - InitResult result; - Firestore::GetInstance(app(), &result); - EXPECT_EQ(kInitResultSuccess, result); + // The app instance is deleted by FirestoreIntegrationTest. + auto* app = this->app(); + + InitResult init_result; + auto auth = UniquePtr(Auth::GetAuth(app, &init_result)); +#if defined(__ANDROID__) + if (init_result != kInitResultSuccess) { + // On Android, it's possible for the Auth library built at head to be too + // new for the version of Play Services available in the Android emulator. + // In this case, Auth will fail to initialize. Meanwhile, there's no + // simple way to detect if the Android app is running in an emulator + // running on Forge. Consequently, just punt if Auth fails to initialize. + LogWarning( + "Skipped FirestoreGetInstanceWithNonNullAppReturnsNonNullInstance " + "test: Auth missing or failed to initialize"); + return; + } +#else + ASSERT_EQ(init_result, kInitResultSuccess); +#endif + + auto db = UniquePtr(Firestore::GetInstance(app, &init_result)); + EXPECT_EQ(kInitResultSuccess, init_result); } catch (const std::exception& exception) { FAIL() << "shouldn't throw exception"; } @@ -253,25 +290,25 @@ TEST_F(ValidationTest, PathsMustNotHaveEmptySegments) { try { db->Collection(path); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_EQ(reason, exception.what()); } try { db->Document(path); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_EQ(reason, exception.what()); } try { collection.Document(path); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_EQ(reason, exception.what()); } try { document.Collection(path); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_EQ(reason, exception.what()); } } @@ -292,13 +329,13 @@ TEST_F(ValidationTest, DocumentPathsMustBeEvenLength) { try { db->Document(bad_absolute_paths[i]); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_EQ(expect_errors[i], exception.what()); } try { base_collection.Document(bad_relative_paths[i]); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_EQ(expect_errors[i], exception.what()); } } @@ -361,14 +398,12 @@ TEST_F(ValidationTest, WritesMustNotContainReservedFieldNames) { MapFieldValue{ {"foo", FieldValue::Map({{"__baz__", FieldValue::Integer(1)}})}}, "Invalid data. Document fields cannot begin and end with \"__\" (found " - "in " - "field foo.__baz__)"); + "in field foo.__baz__)"); ExpectWriteError( MapFieldValue{ {"__baz__", FieldValue::Map({{"foo", FieldValue::Integer(1)}})}}, "Invalid data. Document fields cannot begin and end with \"__\" (found " - "in " - "field __baz__)"); + "in field __baz__)"); ExpectUpdateError(MapFieldValue{{"__baz__", FieldValue::Integer(1)}}, "Invalid data. Document fields cannot begin and end with " @@ -381,19 +416,37 @@ TEST_F(ValidationTest, WritesMustNotContainReservedFieldNames) { TEST_F(ValidationTest, SetsMustNotContainFieldValueDelete) { SCOPED_TRACE("SetsMustNotContainFieldValueDelete"); - ExpectSetError( - MapFieldValue{{"foo", FieldValue::Delete()}}, + // TODO(b/171990785): Unify Android and C++ validation error messages. +#if defined(__ANDROID__) + std::string message = "Invalid data. FieldValue.delete() can only be used with update() and " - "set() with SetOptions.merge() (found in field foo)"); + "set() with SetOptions.merge() (found in field foo)"; +#else + std::string message = + "Invalid data. FieldValue::Delete() can only be used with Update() and " + "Set() with merge == true (found in field foo)"; +#endif // defined(__ANDROID__) + + ExpectSetError(MapFieldValue{{"foo", FieldValue::Delete()}}, message); } TEST_F(ValidationTest, UpdatesMustNotContainNestedFieldValueDeletes) { SCOPED_TRACE("UpdatesMustNotContainNestedFieldValueDeletes"); + // TODO(b/171990785): Unify Android and C++ validation error messages. +#if defined(__ANDROID__) + std::string message = + "Invalid data. FieldValue.delete() can only appear at the top level of " + "your update data (found in field foo.bar)"; +#else + std::string message = + "Invalid data. FieldValue::Delete() can only appear at the top level of " + "your update data (found in field foo.bar)"; +#endif // defined(__ANDROID__) + ExpectUpdateError( MapFieldValue{{"foo", FieldValue::Map({{"bar", FieldValue::Delete()}})}}, - "Invalid data. FieldValue.delete() can only appear at the top level of " - "your update data (found in field foo.bar)"); + message); } TEST_F(ValidationTest, BatchWritesRequireCorrectDocumentReferences) { @@ -414,8 +467,6 @@ TEST_F(ValidationTest, BatchWritesRequireCorrectDocumentReferences) { TEST_F(ValidationTest, TransactionsRequireCorrectDocumentReferences) {} -#endif // defined(__ANDROID__) - TEST_F(ValidationTest, FieldPathsMustNotHaveEmptySegments) { SCOPED_TRACE("FieldPathsMustNotHaveEmptySegments"); @@ -443,8 +494,6 @@ TEST_F(ValidationTest, FieldPathsMustNotHaveInvalidSegments) { } } -#if defined(__ANDROID__) - TEST_F(ValidationTest, FieldNamesMustNotBeEmpty) { DocumentSnapshot snapshot = ReadDocument(Document()); // PORT_NOTE: We do not enforce any logic for invalid C++ object. In @@ -458,24 +507,27 @@ TEST_F(ValidationTest, FieldNamesMustNotBeEmpty) { // EXPECT_STREQ("Invalid field path. Provided path must not be empty.", // exception.what()); // } - try { snapshot.Get(FieldPath{""}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid field name at argument 1. Field names must not be null or " - "empty.", - exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid field name at index 0. Field names must not be empty.", + "Invalid field name at argument 1. Field names must not be null or " + "empty.")); } try { snapshot.Get(FieldPath{"foo", ""}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid field name at argument 2. Field names must not be null or " - "empty.", - exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid field name at index 1. Field names must not be empty.", + "Invalid field name at argument 2. Field names must not be null or " + "empty.")); } } @@ -488,10 +540,13 @@ TEST_F(ValidationTest, ArrayTransformsFailInQueries) { {{"test", FieldValue::ArrayUnion({FieldValue::Integer(1)})}})); FAIL() << "should throw exception"; } catch (const std::invalid_argument& exception) { - EXPECT_STREQ( - "Invalid data. FieldValue.arrayUnion() can only be used with set() and " - "update() (found in field test)", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid data. FieldValue::ArrayUnion() can only be used with " + "Update() and Set() (found in field test)", + "Invalid data. FieldValue.arrayUnion() can only be used with set() " + "and update() (found in field test)")); } try { @@ -501,10 +556,13 @@ TEST_F(ValidationTest, ArrayTransformsFailInQueries) { {{"test", FieldValue::ArrayRemove({FieldValue::Integer(1)})}})); FAIL() << "should throw exception"; } catch (const std::invalid_argument& exception) { - EXPECT_STREQ( - "Invalid data. FieldValue.arrayRemove() can only be used with set() " - "and update() (found in field test)", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid data. FieldValue::ArrayRemove() can only be used with " + "Update() and Set() (found in field test)", + "Invalid data. FieldValue.arrayRemove() can only be used with " + "set() and update() (found in field test)")); } } @@ -520,7 +578,7 @@ TEST_F(ValidationTest, ArrayTransformsRejectArrays) { {FieldValue::Integer(1), FieldValue::Array({FieldValue::String("nested")})})}}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_STREQ("Invalid data. Nested arrays are not supported", exception.what()); } @@ -530,7 +588,7 @@ TEST_F(ValidationTest, ArrayTransformsRejectArrays) { {FieldValue::Integer(1), FieldValue::Array({FieldValue::String("nested")})})}}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_STREQ("Invalid data. Nested arrays are not supported", exception.what()); } @@ -541,7 +599,7 @@ TEST_F(ValidationTest, QueriesWithNonPositiveLimitFail) { try { collection.Limit(0); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_STREQ( "Invalid Query. Query limit (0) is invalid. Limit must be positive.", exception.what()); @@ -549,7 +607,7 @@ TEST_F(ValidationTest, QueriesWithNonPositiveLimitFail) { try { collection.Limit(-1); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { + } catch (const std::invalid_argument& exception) { EXPECT_STREQ( "Invalid Query. Query limit (-1) is invalid. Limit must be positive.", exception.what()); @@ -568,37 +626,40 @@ TEST_F(ValidationTest, QueriesCannotBeCreatedFromDocumentsMissingSortValues) { EXPECT_THAT(snapshot.GetData(), testing::ContainerEq(MapFieldValue{ {"k", FieldValue::String("f")}, {"nosort", FieldValue::Double(1.0)}})); - const char* reason = + + auto matches = AnyMessageEq( "Invalid query. You are trying to start or end a query using a document " - "for which the field 'sort' (used as the orderBy) does not exist."; + "for which the field 'sort' (used as the order by) does not exist.", + "Invalid query. You are trying to start or end a query using a document " + "for which the field 'sort' (used as the orderBy) does not exist."); + try { query.StartAt(snapshot); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT(exception.what(), matches); } try { query.StartAfter(snapshot); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT(exception.what(), matches); } try { query.EndBefore(snapshot); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT(exception.what(), matches); } try { query.EndAt(snapshot); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT(exception.what(), matches); } } -TEST_F(ValidationTest, - QueriesCannotBeSortedByAnUncommittedServerTimestamp) { +TEST_F(ValidationTest, QueriesCannotBeSortedByAnUncommittedServerTimestamp) { CollectionReference collection = Collection(); EventAccumulator accumulator; accumulator.listener()->AttachTo(&collection); @@ -631,27 +692,28 @@ TEST_F(ValidationTest, const std::string&) {})); } - TEST_F(ValidationTest, QueriesMustNotHaveMoreComponentsThanOrderBy) { CollectionReference collection = Collection(); Query query = collection.OrderBy("foo"); - const char* reason = + auto matches = AnyMessageEq( + "Invalid query. You are trying to start or end a query using more values " + "than were specified in the order by.", "Too many arguments provided to startAt(). The number of arguments must " - "be less than or equal to the number of orderBy() clauses."; + "be less than or equal to the number of orderBy() clauses."); try { query.StartAt({FieldValue::Integer(1), FieldValue::Integer(2)}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT(exception.what(), matches); } try { query.OrderBy("bar").StartAt({FieldValue::Integer(1), FieldValue::Integer(2), FieldValue::Integer(3)}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT(exception.what(), matches); } } @@ -661,21 +723,27 @@ TEST_F(ValidationTest, QueryOrderByKeyBoundsMustBeStringsWithoutSlashes) { try { query.StartAt({FieldValue::Integer(1)}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. Expected a string for document ID in startAt(), but " - "got 1.", - exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. Expected a string for the document ID.", + "Invalid query. Expected a string for document ID in startAt(), " + "but got 1.")); } try { query.StartAt({FieldValue::String("foo/bar")}); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. When querying a collection and ordering by " - "FieldPath.documentId(), the value passed to startAt() must be a plain " - "document ID, but 'foo/bar' contains a slash.", - exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. When querying a collection and ordering by " + "document ID, you must pass a plain document ID, but 'foo/bar' " + "contains a slash.", + "Invalid query. When querying a collection and ordering by " + "FieldPath.documentId(), the value passed to startAt() must be a " + "plain document ID, but 'foo/bar' contains a slash.")); } } @@ -685,33 +753,49 @@ TEST_F(ValidationTest, QueriesWithDifferentInequalityFieldsFail) { .WhereGreaterThan("x", FieldValue::Integer(32)) .WhereLessThan("y", FieldValue::String("cat")); FAIL() << "should throw exception"; - } catch (const std::exception& exception) { - EXPECT_STREQ( - "All where filters with an inequality (notEqualTo, notIn, lessThan, " - "lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on " - "the same field. But you have filters on 'x' and 'y'", - exception.what()); + } catch (const std::invalid_argument& exception) { + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid Query. All where filters with an inequality (notEqual, " + "lessThan, lessThanOrEqual, greaterThan, or greaterThanOrEqual) " + "must be on the same field. But you have inequality filters on 'x' " + "and 'y'", + "All where filters with an inequality (notEqualTo, notIn, " + "lessThan, lessThanOrEqualTo, greaterThan, or " + "greaterThanOrEqualTo) must be on the same field. But you have " + "filters on 'x' and 'y'")); } } TEST_F(ValidationTest, QueriesWithInequalityDifferentThanFirstOrderByFail) { - CollectionReference collection = Collection(); + // TODO(b/171990785): Unify Android and C++ validation error messages. +#if defined(__ANDROID__) const char* reason = "Invalid query. You have an inequality where filter (whereLessThan(), " "whereGreaterThan(), etc.) on field 'x' and so you must also have 'x' as " "your first orderBy() field, but your first orderBy() is currently on " "field 'y' instead."; +#else + const char* reason = + "Invalid query. You have a where filter with an inequality (notEqual, " + "lessThan, lessThanOrEqual, greaterThan, or greaterThanOrEqual) on field " + "'x' and so you must also use 'x' as your first queryOrderedBy field, " + "but your first queryOrderedBy is currently on field 'y' instead."; +#endif // defined(__ANDROID__) + + CollectionReference collection = Collection(); try { collection.WhereGreaterThan("x", FieldValue::Integer(32)).OrderBy("y"); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + EXPECT_STREQ(exception.what(), reason); } try { collection.OrderBy("y").WhereGreaterThan("x", FieldValue::Integer(32)); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + EXPECT_STREQ(exception.what(), reason); } try { collection.WhereGreaterThan("x", FieldValue::Integer(32)) @@ -719,14 +803,14 @@ TEST_F(ValidationTest, QueriesWithInequalityDifferentThanFirstOrderByFail) { .OrderBy("x"); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + EXPECT_STREQ(exception.what(), reason); } try { collection.OrderBy("y").OrderBy("x").WhereGreaterThan( "x", FieldValue::Integer(32)); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ(reason, exception.what()); + EXPECT_STREQ(exception.what(), reason); } } @@ -737,9 +821,13 @@ TEST_F(ValidationTest, QueriesWithMultipleArrayContainsFiltersFail) { .WhereArrayContains("foo", FieldValue::Integer(2)); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid Query. You cannot use more than one 'array_contains' filter.", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid Query. You cannot use more than one 'arrayContains' " + "filter.", + "Invalid Query. You cannot use more than one 'array_contains' " + "filter.")); } } @@ -750,37 +838,49 @@ TEST_F(ValidationTest, QueriesMustNotSpecifyStartingOrEndingPointAfterOrderBy) { query.StartAt({FieldValue::Integer(1)}).OrderBy("bar"); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. You must not call Query.startAt() or " - "Query.startAfter() before calling Query.orderBy().", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. You must not specify a starting point before " + "specifying the order by.", + "Invalid query. You must not call Query.startAt() or " + "Query.startAfter() before calling Query.orderBy().")); } try { query.StartAfter({FieldValue::Integer(1)}).OrderBy("bar"); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. You must not call Query.startAt() or " - "Query.startAfter() before calling Query.orderBy().", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. You must not specify a starting point before " + "specifying the order by.", + "Invalid query. You must not call Query.startAt() or " + "Query.startAfter() before calling Query.orderBy().")); } try { query.EndAt({FieldValue::Integer(1)}).OrderBy("bar"); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. You must not call Query.endAt() or " - "Query.endBefore() before calling Query.orderBy().", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. You must not specify an ending point before " + "specifying the order by.", + "Invalid query. You must not call Query.endAt() or " + "Query.endBefore() before calling Query.orderBy().")); } try { query.EndBefore({FieldValue::Integer(1)}).OrderBy("bar"); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. You must not call Query.endAt() or " - "Query.endBefore() before calling Query.orderBy().", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. You must not specify an ending point before " + "specifying the order by.", + "Invalid query. You must not call Query.endAt() or " + "Query.endBefore() before calling Query.orderBy().")); } } @@ -792,10 +892,13 @@ TEST_F(ValidationTest, FieldValue::String("")); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. When querying with FieldPath.documentId() you must " - "provide a valid document ID, but it was an empty string.", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. When querying by document ID you must provide a " + "valid document ID, but it was an empty string.", + "Invalid query. When querying with FieldPath.documentId() you must " + "provide a valid document ID, but it was an empty string.")); } try { @@ -803,11 +906,15 @@ TEST_F(ValidationTest, FieldValue::String("foo/bar/baz")); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. When querying a collection by FieldPath.documentId() " - "you must provide a plain document ID, but 'foo/bar/baz' contains a " - "'/' character.", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. When querying a collection by document ID you must " + "provide a plain document ID, but 'foo/bar/baz' contains a '/' " + "character.", + "Invalid query. When querying a collection by " + "FieldPath.documentId() you must provide a plain document ID, but " + "'foo/bar/baz' contains a '/' character.")); } try { @@ -815,11 +922,15 @@ TEST_F(ValidationTest, FieldValue::Integer(1)); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. When querying with FieldPath.documentId() you must " - "provide a valid String or DocumentReference, but it was of type: " - "java.lang.Long", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. When querying by document ID you must provide a " + "valid string or DocumentReference, but it was of type: " + "FieldValue::Integer()", + "Invalid query. When querying with FieldPath.documentId() you must " + "provide a valid String or DocumentReference, but it was of type: " + "java.lang.Long")); } try { @@ -827,14 +938,16 @@ TEST_F(ValidationTest, FieldValue::Integer(1)); FAIL() << "should throw exception"; } catch (const std::exception& exception) { - EXPECT_STREQ( - "Invalid query. You can't perform 'array_contains' queries on " - "FieldPath.documentId().", - exception.what()); + EXPECT_THAT( + exception.what(), + AnyMessageEq( + "Invalid query. You can't perform arrayContains queries on " + "document ID since document IDs are not arrays.", + "Invalid query. You can't perform 'array_contains' queries on " + "FieldPath.documentId().")); } } -#endif // defined(__ANDROID__) #endif // FIRESTORE_HAVE_EXCEPTIONS } // namespace firestore