From bd7a8df380c0a13b7e3b9f4b3c5cf35ca3e14778 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Thu, 13 Apr 2023 09:23:07 -0700 Subject: [PATCH 01/18] Mila/multi db add get instance header (#1267) --- .../src/validation_test.cc | 36 +++++++++-- firestore/src/common/firestore.cc | 32 +++++++++- firestore/src/include/firebase/firestore.h | 61 +++++++++++++++++-- 3 files changed, 117 insertions(+), 12 deletions(-) diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index 4825b8c56..cd20c3c28 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -407,11 +407,37 @@ TEST_F(ValidationTest, DisableSslWithoutSettingHostFails) { } TEST_F(ValidationTest, FirestoreGetInstanceWithNullAppFails) { - EXPECT_ERROR( - Firestore::GetInstance(/*app=*/nullptr, /*init_result=*/nullptr), - "firebase::App instance cannot be null. Use " - "firebase::App::GetInstance() without arguments if you'd like to use " - "the default instance."); + EXPECT_ERROR(Firestore::GetInstance(/*app=*/(App*)nullptr, + /*init_result=*/(InitResult*)nullptr), + "firebase::App instance cannot be null. Use other " + "firebase::App::GetInstance() if you'd like to use the default " + "instance."); +} + +TEST_F(ValidationTest, FirestoreGetInstanceWithNullDatabaseNameFails) { + EXPECT_ERROR(Firestore::GetInstance(/*db_name=*/(char*)nullptr, + /*init_result=*/(InitResult*)nullptr), + "Provided database ID must not be null. Use other " + "firebase::App::GetInstance() if you'd like to use the default " + "database ID."); +} + +TEST_F(ValidationTest, + FirestoreGetInstanceWithNonNullDatabaseIdButNullAppFails) { + EXPECT_ERROR(Firestore::GetInstance(/*app=*/(App*)nullptr, "foo", + /*init_result=*/(InitResult*)nullptr), + "firebase::App instance cannot be null. Use other " + "firebase::App::GetInstance() if you'd like to use the default " + "instance."); +} + +TEST_F(ValidationTest, + FirestoreGetInstanceWithNonNullAppButNullDatabaseNameFails) { + EXPECT_ERROR(Firestore::GetInstance(app(), /*db_name=*/(char*)nullptr, + /*init_result=*/(InitResult*)nullptr), + "Provided database ID must not be null. Use other " + "firebase::App::GetInstance() if you'd like to use the default " + "database ID."); } TEST_F(ValidationTest, diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index a7695a7b1..85d0542af 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -101,9 +101,18 @@ InitResult CheckInitialized(const FirestoreInternal& firestore) { void ValidateApp(App* app) { if (!app) { SimpleThrowInvalidArgument( - "firebase::App instance cannot be null. Use " - "firebase::App::GetInstance() without arguments if you'd like to use " - "the default instance."); + "firebase::App instance cannot be null. Use other " + "firebase::App::GetInstance() if you'd like to use the default " + "instance."); + } +} + +void ValidateDatabase(const char* db_name) { + if (!db_name) { + SimpleThrowInvalidArgument( + "Provided database ID must not be null. Use other " + "firebase::App::GetInstance() if you'd like to use the default " + "database ID."); } } @@ -133,6 +142,23 @@ Firestore* Firestore::GetInstance(InitResult* init_result_out) { return Firestore::GetInstance(app, init_result_out); } +Firestore* Firestore::GetInstance(App* app, + const char* db_name, + InitResult* init_result_out) { + ValidateApp(app); + ValidateDatabase(db_name); + // TODO(Mila): Implement code + return Firestore::GetInstance(app, init_result_out); +} + +Firestore* Firestore::GetInstance(const char* db_name, + InitResult* init_result_out) { + ValidateDatabase(db_name); + // TODO(Mila): Implement code + App* app = App::GetInstance(); + return Firestore::GetInstance(app, init_result_out); +} + Firestore* Firestore::CreateFirestore(App* app, FirestoreInternal* internal, InitResult* init_result_out) { diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index d8a461e7b..a2475a36d 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -89,7 +89,8 @@ class TransactionManager; class Firestore { public: /** - * @brief Returns an instance of Firestore corresponding to the given App. + * @brief Returns an instance of Firestore corresponding to the given App + * with default database ID. * * Firebase Firestore uses firebase::App to communicate with Firebase * Authentication to authenticate users to the Firestore server backend. @@ -104,13 +105,15 @@ class Firestore { * succeeded, or firebase::kInitResultFailedMissingDependency on Android if * Google Play services is not available on the current device. * - * @return An instance of Firestore corresponding to the given App. + * @return An instance of Firestore corresponding to the given App with + * default database ID. */ static Firestore* GetInstance(::firebase::App* app, InitResult* init_result_out = nullptr); /** - * @brief Returns an instance of Firestore corresponding to the default App. + * @brief Returns an instance of Firestore corresponding to the default App + * with default database ID. * * Firebase Firestore uses the default App to communicate with Firebase * Authentication to authenticate users to the Firestore server backend. @@ -122,10 +125,60 @@ class Firestore { * succeeded, or firebase::kInitResultFailedMissingDependency on Android if * Google Play services is not available on the current device. * - * @return An instance of Firestore corresponding to the default App. + * @return An instance of Firestore corresponding to the default App + * with default database ID. */ static Firestore* GetInstance(InitResult* init_result_out = nullptr); + /** + * @brief Returns an instance of Firestore corresponding to the given App with + * the given database ID. + * + * Firebase Firestore uses firebase::App to communicate with Firebase + * Authentication to authenticate users to the Firestore server backend. + * + * If you call GetInstance() multiple times with the same App, you will get + * the same instance of Firestore. + * + * @param[in] app Your instance of firebase::App. Firebase Firestore will use + * this to communicate with Firebase Authentication. + * @param[in] db_name Name of the database. Firebase Firestore will use + * this to communicate with Firebase Authentication. + * @param[out] init_result_out If provided, the initialization result will be + * written here. Will be set to firebase::kInitResultSuccess if initialization + * succeeded, or firebase::kInitResultFailedMissingDependency on Android if + * Google Play services is not available on the current device. + * + * @return An instance of Firestore corresponding to the given App with + * the given database ID. + */ + static Firestore* GetInstance(::firebase::App* app, + const char* db_name, + InitResult* init_result_out = nullptr); + + /** + * @brief Returns an instance of Firestore corresponding to the default App + * with the given database ID. + * + * Firebase Firestore uses firebase::App to communicate with Firebase + * Authentication to authenticate users to the Firestore server backend. + * + * If you call GetInstance() multiple times with the same App, you will get + * the same instance of Firestore. + * + * @param[in] db_name Name of the database. Firebase Firestore will use + * this to communicate with Firebase Authentication. + * @param[out] init_result_out If provided, the initialization result will be + * written here. Will be set to firebase::kInitResultSuccess if initialization + * succeeded, or firebase::kInitResultFailedMissingDependency on Android if + * Google Play services is not available on the current device. + * + * @return An instance of Firestore corresponding to the default App with + * the given database ID. + */ + static Firestore* GetInstance(const char* db_name, + InitResult* init_result_out = nullptr); + /** * @brief Destructor for the Firestore object. * From 6a9e36b532b46767370bd4f4c41f87144b2425a1 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Fri, 14 Apr 2023 11:05:34 -0700 Subject: [PATCH 02/18] Pass database_id from GetInstance() down to CreateFirestore() (#1270) --- .../src/util/integration_test_util.cc | 3 +- .../src/validation_test.cc | 24 ++++++++++ firestore/src/common/firestore.cc | 45 +++++++++---------- firestore/src/include/firebase/firestore.h | 6 ++- firestore/src/main/firestore_main.cc | 18 +++++--- firestore/src/main/firestore_main.h | 4 +- 6 files changed, 67 insertions(+), 33 deletions(-) diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index d16dc418d..192238def 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -37,7 +37,8 @@ struct TestFriend { static FirestoreInternal* CreateTestFirestoreInternal(App* app) { #if !defined(__ANDROID__) return new FirestoreInternal( - app, absl::make_unique(), + app, /*database_id=*/"(default)", + absl::make_unique(), absl::make_unique()); #else return new FirestoreInternal(app); diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index cd20c3c28..c33157bd5 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -440,6 +440,14 @@ TEST_F(ValidationTest, "database ID."); } +TEST_F(ValidationTest, + FirestoreGetInstanceWithNoArgumentsReturnsNonNullInstance) { + InitResult result; + EXPECT_NO_THROW(Firestore::GetInstance(&result)); + EXPECT_EQ(kInitResultSuccess, result); + EXPECT_NE(Firestore::GetInstance(), nullptr); +} + TEST_F(ValidationTest, FirestoreGetInstanceWithNonNullAppReturnsNonNullInstance) { InitResult result; @@ -448,6 +456,22 @@ TEST_F(ValidationTest, EXPECT_NE(Firestore::GetInstance(app()), nullptr); } +TEST_F(ValidationTest, + FirestoreGetInstanceWithAppAndDatabaseNameReturnsNonNullInstance) { + InitResult result; + EXPECT_NO_THROW(Firestore::GetInstance(app(), "foo", &result)); + EXPECT_EQ(kInitResultSuccess, result); + EXPECT_NE(Firestore::GetInstance(app(), "foo"), nullptr); +} + +TEST_F(ValidationTest, + FirestoreGetInstanceWithDatabaseNameReturnsNonNullInstance) { + InitResult result; + EXPECT_NO_THROW(Firestore::GetInstance("foo", &result)); + EXPECT_EQ(kInitResultSuccess, result); + EXPECT_NE(Firestore::GetInstance("foo"), nullptr); +} + TEST_F(ValidationTest, CollectionPathsMustBeOddLength) { Firestore* db = TestFirestore(); DocumentReference base_document = db->Document("foo/bar"); diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 85d0542af..9fe7e3c45 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -118,28 +118,29 @@ void ValidateDatabase(const char* db_name) { } // namespace -Firestore* Firestore::GetInstance(App* app, InitResult* init_result_out) { - ValidateApp(app); - - MutexLock lock(*g_firestores_lock); - - Firestore* from_cache = FindFirestoreInCache(app, init_result_out); - if (from_cache) { - return from_cache; +Firestore* Firestore::GetInstance(InitResult* init_result_out) { + App* app = App::GetInstance(); + if (!app) { + SimpleThrowInvalidArgument( + "Failed to get firebase::App instance. Please call " + "firebase::App::Create before using Firestore"); } + return Firestore::GetInstance(app, kDefaultDatabase, init_result_out); +} - return AddFirestoreToCache(new Firestore(app), init_result_out); +Firestore* Firestore::GetInstance(App* app, InitResult* init_result_out) { + return Firestore::GetInstance(app, kDefaultDatabase, init_result_out); } -Firestore* Firestore::GetInstance(InitResult* init_result_out) { +Firestore* Firestore::GetInstance(const char* db_name, + InitResult* init_result_out) { App* app = App::GetInstance(); if (!app) { SimpleThrowInvalidArgument( "Failed to get firebase::App instance. Please call " "firebase::App::Create before using Firestore"); } - - return Firestore::GetInstance(app, init_result_out); + return Firestore::GetInstance(app, db_name, init_result_out); } Firestore* Firestore::GetInstance(App* app, @@ -147,16 +148,14 @@ Firestore* Firestore::GetInstance(App* app, InitResult* init_result_out) { ValidateApp(app); ValidateDatabase(db_name); - // TODO(Mila): Implement code - return Firestore::GetInstance(app, init_result_out); -} -Firestore* Firestore::GetInstance(const char* db_name, - InitResult* init_result_out) { - ValidateDatabase(db_name); - // TODO(Mila): Implement code - App* app = App::GetInstance(); - return Firestore::GetInstance(app, init_result_out); + MutexLock lock(*g_firestores_lock); + Firestore* from_cache = FindFirestoreInCache(app, init_result_out); + if (from_cache) { + return from_cache; + } + + return AddFirestoreToCache(new Firestore(app, db_name), init_result_out); } Firestore* Firestore::CreateFirestore(App* app, @@ -190,8 +189,8 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return firestore; } -Firestore::Firestore(::firebase::App* app) - : Firestore{new FirestoreInternal{app}} {} +Firestore::Firestore(::firebase::App* app, const char* database_id) + : Firestore{new FirestoreInternal{app, database_id}} {} Firestore::Firestore(FirestoreInternal* internal) // TODO(wuandy): use make_unique once it is supported for our build here. diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index a2475a36d..a2befc375 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -72,6 +72,9 @@ class TransactionManager; } // namespace csharp +/** The default name for "unset" database ID in resource names. */ +static const char* kDefaultDatabase = "(default)"; + /** * @brief Entry point for the Firebase Firestore C++ SDK. * @@ -492,7 +495,8 @@ class Firestore { friend class csharp::ApiHeaders; friend class csharp::TransactionManager; - explicit Firestore(::firebase::App* app); + explicit Firestore(::firebase::App* app, + const char* database_id = kDefaultDatabase); explicit Firestore(FirestoreInternal* internal); static Firestore* CreateFirestore(::firebase::App* app, diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 3834ec6a8..b235f536d 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -101,17 +101,20 @@ void ValidateDoubleSlash(const char* path) { } // namespace -FirestoreInternal::FirestoreInternal(App* app) - : FirestoreInternal{app, CreateCredentialsProvider(*app), +FirestoreInternal::FirestoreInternal(App* app, const char* database_id) + : FirestoreInternal{app, database_id, CreateCredentialsProvider(*app), CreateAppCheckCredentialsProvider(*app)} {} FirestoreInternal::FirestoreInternal( App* app, + const char* database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials) : app_(NOT_NULL(app)), - firestore_core_(CreateFirestore( - app, std::move(auth_credentials), std::move(app_check_credentials))), + firestore_core_(CreateFirestore(app, + database_id, + std::move(auth_credentials), + std::move(app_check_credentials))), transaction_executor_(absl::ShareUniquePtr(Executor::CreateConcurrent( "com.google.firebase.firestore.transaction", /*threads=*/5))) { ApplyDefaultSettings(); @@ -131,13 +134,14 @@ FirestoreInternal::~FirestoreInternal() { std::shared_ptr FirestoreInternal::CreateFirestore( App* app, + const char* database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials) { const AppOptions& opt = app->options(); return std::make_shared( - DatabaseId{opt.project_id()}, app->name(), std::move(auth_credentials), - std::move(app_check_credentials), CreateWorkerQueue(), - CreateFirebaseMetadataProvider(*app), this); + DatabaseId{opt.project_id(), database_id}, app->name(), + std::move(auth_credentials), std::move(app_check_credentials), + CreateWorkerQueue(), CreateFirebaseMetadataProvider(*app), this); } CollectionReference FirestoreInternal::Collection( diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index 49b377954..4e7f164a5 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -55,7 +55,7 @@ class Executor; class FirestoreInternal { public: // Note: call `set_firestore_public` immediately after construction. - explicit FirestoreInternal(App* app); + FirestoreInternal(App* app, const char* database_id); ~FirestoreInternal(); FirestoreInternal(const FirestoreInternal&) = delete; @@ -150,12 +150,14 @@ class FirestoreInternal { FirestoreInternal( App* app, + const char* database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials); std::shared_ptr CreateFirestore( App* app, + const char* database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials); From 3c2dc25026f65ddb51db77f9ff364adf57f236a8 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 9 May 2023 10:27:56 -0400 Subject: [PATCH 03/18] Update FirestoreCache key type (#1272) --- .../src/firestore_integration_test.cc | 18 +- .../src/firestore_integration_test.h | 28 +- .../src/firestore_test.cc | 304 +++++++++++++----- .../src/util/integration_test_util.cc | 11 +- .../src/util/locate_emulator.cc | 8 + .../src/util/locate_emulator.h | 3 + .../src/validation_test.cc | 90 ++++++ firestore/src/common/firestore.cc | 54 +++- firestore/src/include/firebase/firestore.h | 1 + 9 files changed, 412 insertions(+), 105 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index df1854044..e95db2551 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -116,11 +116,19 @@ Firestore* FirestoreIntegrationTest::TestFirestore( return TestFirestoreWithProjectId(name, /*project_id=*/""); } +Firestore* FirestoreIntegrationTest::TestFirestoreWithDatabaseId( + const std::string& name, const std::string& database_id) const { + return TestFirestoreWithProjectId(name, /*project_id=*/"", database_id); +} + Firestore* FirestoreIntegrationTest::TestFirestoreWithProjectId( - const std::string& name, const std::string& project_id) const { + const std::string& name, + const std::string& project_id, + const std::string& database_id) const { for (const auto& entry : firestores_) { const FirestoreInfo& firestore_info = entry.second; - if (firestore_info.name() == name) { + if (firestore_info.name() == name && + firestore_info.database_id() == database_id) { return firestore_info.firestore(); } } @@ -129,9 +137,9 @@ Firestore* FirestoreIntegrationTest::TestFirestoreWithProjectId( if (apps_.find(app) == apps_.end()) { apps_[app] = std::unique_ptr(app); } - - Firestore* db = new Firestore(CreateTestFirestoreInternal(app)); - firestores_[db] = FirestoreInfo(name, std::unique_ptr(db)); + Firestore* db = new Firestore(CreateTestFirestoreInternal(app, database_id)); + firestores_[db] = + FirestoreInfo(name, database_id, std::unique_ptr(db)); firestore::LocateEmulator(db); return db; diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 249fbe3b0..71010583f 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include "app/meta/move.h" @@ -49,7 +50,8 @@ const int kCheckIntervalMillis = 100; // The timeout of waiting for a Future or a listener. const int kTimeOutMillis = 15000; -FirestoreInternal* CreateTestFirestoreInternal(App* app); +FirestoreInternal* CreateTestFirestoreInternal( + App* app, const std::string& database_id = kDefaultDatabase); App* GetApp(); App* GetApp(const char* name, const std::string& override_project_id); @@ -252,9 +254,16 @@ class FirestoreIntegrationTest : public testing::Test { Firestore* TestFirestore(const std::string& name = kDefaultAppName) const; // Returns a Firestore instance for an app with the given `name`, associated - // with the database with the given `project_id`. - Firestore* TestFirestoreWithProjectId(const std::string& name, - const std::string& project_id) const; + // with the database with the given `project_id` and default `database_id`. + Firestore* TestFirestoreWithProjectId( + const std::string& name, + const std::string& project_id, + const std::string& database_id = kDefaultDatabase) const; + + // Returns a Firestore instance for an app with the given `name`, associated + // with the database with the given `database_id`. + Firestore* TestFirestoreWithDatabaseId(const std::string& name, + const std::string& database_id) const; // Deletes the given `Firestore` instance, which must have been returned by a // previous invocation of `TestFirestore()`, and removes it from the cache of @@ -419,15 +428,22 @@ class FirestoreIntegrationTest : public testing::Test { class FirestoreInfo { public: FirestoreInfo() = default; - FirestoreInfo(const std::string& name, std::unique_ptr firestore) - : name_(name), firestore_(std::move(firestore)) {} + FirestoreInfo(const std::string& name, + const std::string& database_id, + std::unique_ptr firestore) + : name_(name), + database_id_(database_id), + firestore_(std::move(firestore)) {} const std::string& name() const { return name_; } Firestore* firestore() const { return firestore_.get(); } + const std::string& database_id() const { return database_id_; } + void ReleaseFirestore() { firestore_.release(); } private: std::string name_; + std::string database_id_; std::unique_ptr firestore_; }; diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 0569448f7..24b478f06 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -16,6 +16,7 @@ #include "firebase/firestore.h" +#include #include #include #include @@ -40,11 +41,13 @@ #include "util/future_test_util.h" #if !defined(__ANDROID__) #include "Firestore/core/src/util/autoid.h" +#include "firestore/src/main/firestore_main.h" #else #include "android/util_autoid.h" #endif // !defined(__ANDROID__) #include "Firestore/core/src/util/firestore_exceptions.h" #include "firebase_test_framework.h" +#include "util/locate_emulator.h" // These test cases are in sync with native iOS client SDK test // Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm @@ -60,7 +63,14 @@ using ::firebase::auth::Auth; using ::testing::ContainerEq; using ::testing::HasSubstr; -TEST_F(FirestoreIntegrationTest, GetInstance) { +class FirestoreTest : public FirestoreIntegrationTest { + protected: + const std::string GetFirestoreDatabaseId(Firestore* firestore) { + return GetInternal(firestore)->database_id().database_id(); + } +}; + +TEST_F(FirestoreTest, GetInstance) { // Create App. App* app = this->app(); EXPECT_NE(nullptr, app); @@ -87,8 +97,22 @@ TEST_F(FirestoreIntegrationTest, GetInstance) { delete auth; } +TEST_F(FirestoreTest, GetInstanceWithNamedDatabase) { + App* app = this->app(); + EXPECT_NE(nullptr, app); + + InitResult init_result; + auto instance = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result)); + ASSERT_EQ(kInitResultSuccess, init_result); + + EXPECT_NE(nullptr, instance); + EXPECT_EQ(app, instance->app()); + EXPECT_EQ(GetFirestoreDatabaseId(instance.get()), "foo"); +} + // Sanity test for stubs. -TEST_F(FirestoreIntegrationTest, TestCanCreateCollectionAndDocumentReferences) { +TEST_F(FirestoreTest, TestCanCreateCollectionAndDocumentReferences) { Firestore* db = TestFirestore(); CollectionReference c = db->Collection("a/b/c").Document("d").Parent(); DocumentReference d = db->Document("a/b").Collection("c/d/e").Parent(); @@ -102,7 +126,7 @@ TEST_F(FirestoreIntegrationTest, TestCanCreateCollectionAndDocumentReferences) { // If any of these assert, the test will fail. } -TEST_F(FirestoreIntegrationTest, TestCanReadNonExistentDocuments) { +TEST_F(FirestoreTest, TestCanReadNonExistentDocuments) { DocumentReference doc = Collection("rooms").Document(); DocumentSnapshot snap = ReadDocument(doc); @@ -110,7 +134,7 @@ TEST_F(FirestoreIntegrationTest, TestCanReadNonExistentDocuments) { EXPECT_THAT(snap.GetData(), ContainerEq(MapFieldValue())); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateAnExistingDocument) { +TEST_F(FirestoreTest, TestCanUpdateAnExistingDocument) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -131,7 +155,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateAnExistingDocument) { {"email", FieldValue::String("new@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateAnUnknownDocument) { +TEST_F(FirestoreTest, TestCanUpdateAnUnknownDocument) { DocumentReference writer_reference = TestFirestore("writer")->Collection("collection").Document(); DocumentReference reader_reference = TestFirestore("reader") @@ -165,7 +189,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateAnUnknownDocument) { EXPECT_FALSE(reader_snapshot.metadata().is_from_cache()); } -TEST_F(FirestoreIntegrationTest, TestCanOverwriteAnExistingDocumentUsingSet) { +TEST_F(FirestoreTest, TestCanOverwriteAnExistingDocumentUsingSet) { DocumentReference document = Collection("rooms").Document(); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -186,8 +210,7 @@ TEST_F(FirestoreIntegrationTest, TestCanOverwriteAnExistingDocumentUsingSet) { FieldValue::Map({{"name", FieldValue::String("Sebastian")}})}})); } -TEST_F(FirestoreIntegrationTest, - TestCanMergeDataWithAnExistingDocumentUsingSet) { +TEST_F(FirestoreTest, TestCanMergeDataWithAnExistingDocumentUsingSet) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -212,7 +235,7 @@ TEST_F(FirestoreIntegrationTest, {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanMergeServerTimestamps) { +TEST_F(FirestoreTest, TestCanMergeServerTimestamps) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{{"untouched", FieldValue::Boolean(true)}})); Await(document.Set( @@ -226,7 +249,7 @@ TEST_F(FirestoreIntegrationTest, TestCanMergeServerTimestamps) { EXPECT_TRUE(snapshot.Get("nested.time").is_timestamp()); } -TEST_F(FirestoreIntegrationTest, TestCanMergeEmptyObject) { +TEST_F(FirestoreTest, TestCanMergeEmptyObject) { DocumentReference document = Document(); EventAccumulator accumulator; ListenerRegistration registration = @@ -262,7 +285,7 @@ TEST_F(FirestoreIntegrationTest, TestCanMergeEmptyObject) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMerge) { +TEST_F(FirestoreTest, TestCanDeleteFieldUsingMerge) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -287,7 +310,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMerge) { EXPECT_FALSE(snapshot.Get("nested.foo").is_valid()); } -TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMergeFields) { +TEST_F(FirestoreTest, TestCanDeleteFieldUsingMergeFields) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -314,7 +337,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMergeFields) { FieldValue::Map({{"untouched", FieldValue::Boolean(true)}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanSetServerTimestampsUsingMergeFields) { +TEST_F(FirestoreTest, TestCanSetServerTimestampsUsingMergeFields) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -335,7 +358,7 @@ TEST_F(FirestoreIntegrationTest, TestCanSetServerTimestampsUsingMergeFields) { EXPECT_TRUE(snapshot.Get("nested.foo").is_timestamp()); } -TEST_F(FirestoreIntegrationTest, TestMergeReplacesArrays) { +TEST_F(FirestoreTest, TestMergeReplacesArrays) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -363,8 +386,7 @@ TEST_F(FirestoreIntegrationTest, TestMergeReplacesArrays) { {{"data", FieldValue::String("new")}})})}})); } -TEST_F(FirestoreIntegrationTest, - TestCanDeepMergeDataWithAnExistingDocumentUsingSet) { +TEST_F(FirestoreTest, TestCanDeepMergeDataWithAnExistingDocumentUsingSet) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"owner.data", @@ -391,7 +413,7 @@ TEST_F(FirestoreIntegrationTest, #if defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS // TODO(b/136012313): iOS currently doesn't rethrow native exceptions as C++ // exceptions. -TEST_F(FirestoreIntegrationTest, TestFieldMaskCannotContainMissingFields) { +TEST_F(FirestoreTest, TestFieldMaskCannotContainMissingFields) { DocumentReference document = Collection("rooms").Document(); try { document.Set(MapFieldValue{{"desc", FieldValue::String("NewDescription")}}, @@ -406,7 +428,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldMaskCannotContainMissingFields) { } #endif // defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS -TEST_F(FirestoreIntegrationTest, TestFieldsNotInFieldMaskAreIgnored) { +TEST_F(FirestoreTest, TestFieldsNotInFieldMaskAreIgnored) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -428,7 +450,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldsNotInFieldMaskAreIgnored) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestFieldDeletesNotInFieldMaskAreIgnored) { +TEST_F(FirestoreTest, TestFieldDeletesNotInFieldMaskAreIgnored) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -450,7 +472,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldDeletesNotInFieldMaskAreIgnored) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestFieldTransformsNotInFieldMaskAreIgnored) { +TEST_F(FirestoreTest, TestFieldTransformsNotInFieldMaskAreIgnored) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -472,7 +494,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldTransformsNotInFieldMaskAreIgnored) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanSetEmptyFieldMask) { +TEST_F(FirestoreTest, TestCanSetEmptyFieldMask) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -493,7 +515,7 @@ TEST_F(FirestoreIntegrationTest, TestCanSetEmptyFieldMask) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanSpecifyFieldsMultipleTimesInFieldMask) { +TEST_F(FirestoreTest, TestCanSpecifyFieldsMultipleTimesInFieldMask) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -518,7 +540,7 @@ TEST_F(FirestoreIntegrationTest, TestCanSpecifyFieldsMultipleTimesInFieldMask) { {"email", FieldValue::String("new@new.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanDeleteAFieldWithAnUpdate) { +TEST_F(FirestoreTest, TestCanDeleteAFieldWithAnUpdate) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -535,7 +557,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDeleteAFieldWithAnUpdate) { FieldValue::Map({{"name", FieldValue::String("Jonny")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithDots) { +TEST_F(FirestoreTest, TestCanUpdateFieldsWithDots) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{{"a.b", FieldValue::String("old")}, {"c.d", FieldValue::String("old")}, @@ -550,7 +572,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithDots) { {"e.f", FieldValue::String("old")}})); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateNestedFields) { +TEST_F(FirestoreTest, TestCanUpdateNestedFields) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"a", FieldValue::Map({{"b", FieldValue::String("old")}})}, @@ -569,7 +591,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateNestedFields) { // Verify that multiple deletes in a single update call work. // https://github.com/firebase/quickstart-unity/issues/882 -TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithMultipleDeletes) { +TEST_F(FirestoreTest, TestCanUpdateFieldsWithMultipleDeletes) { DocumentReference document = Collection("rooms").Document(); Await(document.Set(MapFieldValue{{"key1", FieldValue::String("value1")}, {"key2", FieldValue::String("value2")}, @@ -586,7 +608,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithMultipleDeletes) { {"key4", FieldValue::String("value4")}})); } -TEST_F(FirestoreIntegrationTest, TestDeleteDocument) { +TEST_F(FirestoreTest, TestDeleteDocument) { DocumentReference document = Collection("rooms").Document("eros"); WriteDocument(document, MapFieldValue{{"value", FieldValue::String("bar")}}); DocumentSnapshot snapshot = ReadDocument(document); @@ -599,7 +621,7 @@ TEST_F(FirestoreIntegrationTest, TestDeleteDocument) { EXPECT_FALSE(snapshot.exists()); } -TEST_F(FirestoreIntegrationTest, TestCannotUpdateNonexistentDocument) { +TEST_F(FirestoreTest, TestCannotUpdateNonexistentDocument) { DocumentReference document = Collection("rooms").Document(); Future future = document.Update(MapFieldValue{{"owner", FieldValue::String("abc")}}); @@ -610,7 +632,7 @@ TEST_F(FirestoreIntegrationTest, TestCannotUpdateNonexistentDocument) { EXPECT_FALSE(snapshot.exists()); } -TEST_F(FirestoreIntegrationTest, TestCanRetrieveNonexistentDocument) { +TEST_F(FirestoreTest, TestCanRetrieveNonexistentDocument) { DocumentReference document = Collection("rooms").Document(); DocumentSnapshot snapshot = ReadDocument(document); EXPECT_FALSE(snapshot.exists()); @@ -623,7 +645,7 @@ TEST_F(FirestoreIntegrationTest, TestCanRetrieveNonexistentDocument) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, +TEST_F(FirestoreTest, TestAddingToACollectionYieldsTheCorrectDocumentReference) { DocumentReference document = Collection("rooms").Document(); Await(document.Set(MapFieldValue{{"foo", FieldValue::Double(1.0)}})); @@ -633,8 +655,7 @@ TEST_F(FirestoreIntegrationTest, ContainerEq(MapFieldValue{{"foo", FieldValue::Double(1.0)}})); } -TEST_F(FirestoreIntegrationTest, - TestSnapshotsInSyncListenerFiresAfterListenersInSync) { +TEST_F(FirestoreTest, TestSnapshotsInSyncListenerFiresAfterListenersInSync) { class TestData { public: void AddEvent(const std::string& event) { @@ -723,7 +744,7 @@ TEST_F(FirestoreIntegrationTest, sync_registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQueriesAreValidatedOnClient) { +TEST_F(FirestoreTest, TestQueriesAreValidatedOnClient) { // NOTE: Failure cases are validated in ValidationTest. CollectionReference collection = Collection(); Query query = @@ -755,7 +776,7 @@ TEST_F(FirestoreIntegrationTest, TestQueriesAreValidatedOnClient) { // The test harness will generate Java JUnit test regardless whether this is // inside a #if or not. So we move #if inside instead of enclose the whole case. -TEST_F(FirestoreIntegrationTest, TestListenCanBeCalledMultipleTimes) { +TEST_F(FirestoreTest, TestListenCanBeCalledMultipleTimes) { class TestData { public: void SetDocumentSnapshot(const DocumentSnapshot& document_snapshot) { @@ -803,7 +824,7 @@ TEST_F(FirestoreIntegrationTest, TestListenCanBeCalledMultipleTimes) { ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsNonExistent) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsNonExistent) { DocumentReference document = Collection("rooms").Document(); TestEventListener listener("TestNonExistent"); ListenerRegistration registration = @@ -815,7 +836,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsNonExistent) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForAdd) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsForAdd) { DocumentReference document = Collection("rooms").Document(); TestEventListener listener("TestForAdd"); ListenerRegistration registration = @@ -839,7 +860,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForAdd) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForChange) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsForChange) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -873,7 +894,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForChange) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForDelete) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsForDelete) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -897,7 +918,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForDelete) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotErrorReporting) { +TEST_F(FirestoreTest, TestDocumentSnapshotErrorReporting) { DocumentReference document = Collection("col").Document("__badpath__"); TestEventListener listener("TestBadPath"); ListenerRegistration registration = @@ -910,7 +931,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotErrorReporting) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForAdd) { +TEST_F(FirestoreTest, TestQuerySnapshotEventsForAdd) { CollectionReference collection = Collection(); DocumentReference document = collection.Document(); TestEventListener listener("TestForCollectionAdd"); @@ -937,7 +958,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForAdd) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForChange) { +TEST_F(FirestoreTest, TestQuerySnapshotEventsForChange) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -971,7 +992,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForChange) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForDelete) { +TEST_F(FirestoreTest, TestQuerySnapshotEventsForDelete) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -995,7 +1016,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForDelete) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotErrorReporting) { +TEST_F(FirestoreTest, TestQuerySnapshotErrorReporting) { CollectionReference collection = Collection("a").Document("__badpath__").Collection("b"); TestEventListener listener("TestBadPath"); @@ -1009,8 +1030,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotErrorReporting) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, - TestMetadataOnlyChangesAreNotFiredWhenNoOptionsProvided) { +TEST_F(FirestoreTest, TestMetadataOnlyChangesAreNotFiredWhenNoOptionsProvided) { DocumentReference document = Collection().Document(); TestEventListener listener("TestForNoMetadataOnlyChanges"); ListenerRegistration registration = listener.AttachTo(&document); @@ -1027,7 +1047,7 @@ TEST_F(FirestoreIntegrationTest, registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentReferenceExposesFirestore) { +TEST_F(FirestoreTest, TestDocumentReferenceExposesFirestore) { Firestore* db = TestFirestore(); // EXPECT_EQ(db, db->Document("foo/bar").firestore()); // TODO(varconst): use the commented out check above. @@ -1041,19 +1061,19 @@ TEST_F(FirestoreIntegrationTest, TestDocumentReferenceExposesFirestore) { EXPECT_NE(nullptr, db->Document("foo/bar").firestore()); } -TEST_F(FirestoreIntegrationTest, TestCollectionReferenceExposesFirestore) { +TEST_F(FirestoreTest, TestCollectionReferenceExposesFirestore) { Firestore* db = TestFirestore(); // EXPECT_EQ(db, db->Collection("foo").firestore()); EXPECT_NE(nullptr, db->Collection("foo").firestore()); } -TEST_F(FirestoreIntegrationTest, TestQueryExposesFirestore) { +TEST_F(FirestoreTest, TestQueryExposesFirestore) { Firestore* db = TestFirestore(); // EXPECT_EQ(db, db->Collection("foo").Limit(5).firestore()); EXPECT_NE(nullptr, db->Collection("foo").Limit(5).firestore()); } -TEST_F(FirestoreIntegrationTest, TestDocumentReferenceEquality) { +TEST_F(FirestoreTest, TestDocumentReferenceEquality) { Firestore* db = TestFirestore(); DocumentReference document = db->Document("foo/bar"); EXPECT_EQ(document, db->Document("foo/bar")); @@ -1065,7 +1085,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentReferenceEquality) { EXPECT_NE(document, another_db->Document("foo/bar")); } -TEST_F(FirestoreIntegrationTest, TestQueryReferenceEquality) { +TEST_F(FirestoreTest, TestQueryReferenceEquality) { Firestore* db = TestFirestore(); Query query = db->Collection("foo").OrderBy("bar").WhereEqualTo( "baz", FieldValue::Integer(42)); @@ -1081,7 +1101,7 @@ TEST_F(FirestoreIntegrationTest, TestQueryReferenceEquality) { // So we skip the testing of two queries with different Firestore instance. } -TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionsAndDocuments) { +TEST_F(FirestoreTest, TestCanTraverseCollectionsAndDocuments) { Firestore* db = TestFirestore(); // doc path from root Firestore. @@ -1097,7 +1117,7 @@ TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionsAndDocuments) { EXPECT_EQ("a/b/c/d/e", db->Document("a/b").Collection("c/d/e").path()); } -TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionAndDocumentParents) { +TEST_F(FirestoreTest, TestCanTraverseCollectionAndDocumentParents) { Firestore* db = TestFirestore(); CollectionReference collection = db->Collection("a/b/c"); EXPECT_EQ("a/b/c", collection.path()); @@ -1112,17 +1132,17 @@ TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionAndDocumentParents) { EXPECT_FALSE(invalidDoc.is_valid()); } -TEST_F(FirestoreIntegrationTest, TestCollectionId) { +TEST_F(FirestoreTest, TestCollectionId) { EXPECT_EQ("foo", TestFirestore()->Collection("foo").id()); EXPECT_EQ("baz", TestFirestore()->Collection("foo/bar/baz").id()); } -TEST_F(FirestoreIntegrationTest, TestDocumentId) { +TEST_F(FirestoreTest, TestDocumentId) { EXPECT_EQ(TestFirestore()->Document("foo/bar").id(), "bar"); EXPECT_EQ(TestFirestore()->Document("foo/bar/baz/qux").id(), "qux"); } -TEST_F(FirestoreIntegrationTest, TestCanQueueWritesWhileOffline) { +TEST_F(FirestoreTest, TestCanQueueWritesWhileOffline) { // Arrange DocumentReference document = Collection("rooms").Document("eros"); @@ -1150,7 +1170,7 @@ TEST_F(FirestoreIntegrationTest, TestCanQueueWritesWhileOffline) { EXPECT_FALSE(snapshot.metadata().is_from_cache()); } -TEST_F(FirestoreIntegrationTest, TestCanGetDocumentsWhileOffline) { +TEST_F(FirestoreTest, TestCanGetDocumentsWhileOffline) { DocumentReference document = Collection("rooms").Document(); Await(TestFirestore()->DisableNetwork()); Future future = document.Get(); @@ -1200,7 +1220,7 @@ TEST_F(FirestoreIntegrationTest, TestCanGetDocumentsWhileOffline) { // really unit tests that have to be run in integration tests setup. The // existing Objective-C and Android tests cover these cases fairly well. -TEST_F(FirestoreIntegrationTest, TestCanDisableAndEnableNetworking) { +TEST_F(FirestoreTest, TestCanDisableAndEnableNetworking) { // There's not currently a way to check if networking is in fact disabled, // so for now just test that the method is well-behaved and doesn't throw. Firestore* db = TestFirestore(); @@ -1212,7 +1232,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDisableAndEnableNetworking) { } // TODO(varconst): split this test. -TEST_F(FirestoreIntegrationTest, TestToString) { +TEST_F(FirestoreTest, TestToString) { Settings settings; settings.set_host("foo.bar"); settings.set_ssl_enabled(false); @@ -1242,12 +1262,12 @@ TEST_F(FirestoreIntegrationTest, TestToString) { // TODO(wuandy): Enable this for other platforms when they can handle // exceptions. #if defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS -TEST_F(FirestoreIntegrationTest, ClientCallsAfterTerminateFails) { +TEST_F(FirestoreTest, ClientCallsAfterTerminateFails) { EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); EXPECT_THROW(Await(TestFirestore()->DisableNetwork()), std::logic_error); } -TEST_F(FirestoreIntegrationTest, NewOperationThrowsAfterFirestoreTerminate) { +TEST_F(FirestoreTest, NewOperationThrowsAfterFirestoreTerminate) { auto instance = TestFirestore(); DocumentReference reference = TestFirestore()->Document("abc/123"); Await(reference.Set({{"Field", FieldValue::Integer(100)}})); @@ -1273,7 +1293,7 @@ TEST_F(FirestoreIntegrationTest, NewOperationThrowsAfterFirestoreTerminate) { std::logic_error); } -TEST_F(FirestoreIntegrationTest, TerminateCanBeCalledMultipleTimes) { +TEST_F(FirestoreTest, TerminateCanBeCalledMultipleTimes) { auto instance = TestFirestore(); DocumentReference reference = instance->Document("abc/123"); Await(reference.Set({{"Field", FieldValue::Integer(100)}})); @@ -1290,7 +1310,41 @@ TEST_F(FirestoreIntegrationTest, TerminateCanBeCalledMultipleTimes) { } #endif // defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS -TEST_F(FirestoreIntegrationTest, MaintainsPersistenceAfterRestarting) { +TEST_F(FirestoreTest, CanTerminateFirestoreInstance) { + App* app = App::GetInstance(); + InitResult init_result1; + auto db1 = + std::unique_ptr(Firestore::GetInstance(app, &init_result1)); + ASSERT_EQ(kInitResultSuccess, init_result1); + + EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + + InitResult init_result2; + auto db2 = + std::unique_ptr(Firestore::GetInstance(app, &init_result2)); + ASSERT_EQ(kInitResultSuccess, init_result2); + + EXPECT_NE(db1, db2); +} + +TEST_F(FirestoreTest, CanTerminateNamedFirestoreInstance) { + App* app = App::GetInstance(); + InitResult init_result1; + auto db1 = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result1)); + ASSERT_EQ(kInitResultSuccess, init_result1); + + EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + + InitResult init_result2; + auto db2 = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result2)); + ASSERT_EQ(kInitResultSuccess, init_result2); + + EXPECT_NE(db1, db2); +} + +TEST_F(FirestoreTest, MaintainsPersistenceAfterRestarting) { Firestore* db = TestFirestore(); App* app = db->app(); DocumentReference doc = db->Collection("col1").Document("doc1"); @@ -1304,7 +1358,7 @@ TEST_F(FirestoreIntegrationTest, MaintainsPersistenceAfterRestarting) { EXPECT_TRUE(snap->exists()); } -TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) { +TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { // Get App and Settings objects to use in the test. Firestore* db_template = TestFirestore("restart_firestore_new_instance_test"); App* app = db_template->app(); @@ -1348,7 +1402,107 @@ TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) { delete db1; } -TEST_F(FirestoreIntegrationTest, CanStopListeningAfterTerminate) { +TEST_F(FirestoreTest, + CanReadDocsAfterRestartFirestoreAndCreateNewNamedDatabaseInstance) { + // TODO(Mila): Remove the emulator env check and LocateEmulator call after + // prod supports multiDB. + if (!IsUsingFirestoreEmulator()) { + GTEST_SKIP(); + } + + App* app = App::GetInstance(); + auto db1 = std::unique_ptr(Firestore::GetInstance(app, "test-db")); + firestore::LocateEmulator(db1.get()); + + // Create a document that we can use for verification later. + DocumentReference doc1 = db1->Collection("abc").Document(); + const std::string doc_path = doc1.path(); + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); + + // Terminate `db1` so that it will be removed from the instance cache. + EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + + // Verify that GetInstance() returns a new instance since the old instance has + // been terminated. + auto db2 = std::unique_ptr(Firestore::GetInstance(app, "test-db")); + firestore::LocateEmulator(db2.get()); + EXPECT_NE(db1, db2); + + // Verify that the new instance points to the same database by verifying that + // the document created with the old instance exists in the new instance. + DocumentReference doc2 = db2->Document(doc_path); + const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); + ASSERT_NE(snapshot2, nullptr); + EXPECT_TRUE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); +} + +TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { + // TODO(Mila): Remove the emulator env check after prod supports multiDB. + if (!IsUsingFirestoreEmulator()) { + GTEST_SKIP(); + } + + // Create two DB instances in the same app. + App* app = App::GetInstance(); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); + + EXPECT_NE(db1, db2); + + // Create a document in the first DB instance. + DocumentReference doc1 = db1->Collection("abc").Document(); + const std::string doc_path = doc1.path(); + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); + const DocumentSnapshot* snapshot1 = Await(doc1.Get()); + EXPECT_TRUE(snapshot1->exists()); + EXPECT_THAT(snapshot1->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); + + // Verify that the previously saved document only exists in the first DB + // instance by verifying that the document does not exist in the second + // instance. + DocumentReference doc2 = db2->Document(doc_path); + const DocumentSnapshot* snapshot2 = Await(doc2.Get()); + EXPECT_FALSE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{})); +} + +TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { + // TODO(Mila): Remove the emulator env check after prod supports multiDB. + if (!IsUsingFirestoreEmulator()) { + GTEST_SKIP(); + } + + // Create two DB instances in the same app. + App* app = App::GetInstance(); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); + + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); + + EXPECT_NE(db1, db2); + + DisableNetwork(); + + // Create a document in the first DB instance. + DocumentReference doc1 = db1->Collection("abc").Document(); + const std::string doc_path = doc1.path(); + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); + const DocumentSnapshot* snapshot1 = Await(doc1.Get(Source::kCache)); + EXPECT_TRUE(snapshot1->exists()); + EXPECT_THAT(snapshot1->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); + // Verify that the previously saved document only exists in the first DB + // instance by verifying that the document does not exist in the second + // instance. + DocumentReference doc2 = db2->Document(doc_path); + const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); + EXPECT_FALSE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{})); +} + +TEST_F(FirestoreTest, CanStopListeningAfterTerminate) { auto instance = TestFirestore(); DocumentReference reference = instance->Document("abc/123"); EventAccumulator accumulator; @@ -1364,7 +1518,7 @@ TEST_F(FirestoreIntegrationTest, CanStopListeningAfterTerminate) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, WaitForPendingWritesResolves) { +TEST_F(FirestoreTest, WaitForPendingWritesResolves) { DocumentReference document = Collection("abc").Document("123"); Await(TestFirestore()->DisableNetwork()); @@ -1390,9 +1544,9 @@ TEST_F(FirestoreIntegrationTest, WaitForPendingWritesResolves) { // TODO(wuandy): This test requires to create underlying firestore instance with // a MockCredentialProvider first. -// TEST_F(FirestoreIntegrationTest, WaitForPendingWritesFailsWhenUserChanges) {} +// TEST_F(FirestoreTest, WaitForPendingWritesFailsWhenUserChanges) {} -TEST_F(FirestoreIntegrationTest, +TEST_F(FirestoreTest, WaitForPendingWritesResolvesWhenOfflineIfThereIsNoPending) { Await(TestFirestore()->DisableNetwork()); Future await_pending_writes = TestFirestore()->WaitForPendingWrites(); @@ -1403,7 +1557,7 @@ TEST_F(FirestoreIntegrationTest, EXPECT_EQ(await_pending_writes.status(), FutureStatus::kFutureStatusComplete); } -TEST_F(FirestoreIntegrationTest, CanClearPersistenceTestHarnessVerification) { +TEST_F(FirestoreTest, CanClearPersistenceTestHarnessVerification) { // Verify that TestFirestore(), DeleteFirestore(), and DeleteApp() behave how // we expect; otherwise, the tests for ClearPersistence() could yield false // positives. @@ -1426,7 +1580,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceTestHarnessVerification) { ContainerEq(MapFieldValue{{"foo", FieldValue::Integer(42)}})); } -TEST_F(FirestoreIntegrationTest, CanClearPersistenceAfterRestarting) { +TEST_F(FirestoreTest, CanClearPersistenceAfterRestarting) { Firestore* db = TestFirestore(); App* app = db->app(); const std::string app_name = app->name(); @@ -1458,7 +1612,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceAfterRestarting) { EXPECT_EQ(await_get.error(), Error::kErrorUnavailable); } -TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) { +TEST_F(FirestoreTest, CanClearPersistenceOnANewFirestoreInstance) { Firestore* db = TestFirestore(); App* app = db->app(); const std::string app_name = app->name(); @@ -1487,7 +1641,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) { EXPECT_EQ(await_get.error(), Error::kErrorUnavailable); } -TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) { +TEST_F(FirestoreTest, ClearPersistenceWhileRunningFails) { // Call EnableNetwork() in order to ensure that Firestore is fully // initialized before clearing persistence. EnableNetwork() is chosen because // it is easy to call. @@ -1500,12 +1654,12 @@ TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) { } // Note: this test only exists in C++. -TEST_F(FirestoreIntegrationTest, DomainObjectsReferToSameFirestoreInstance) { +TEST_F(FirestoreTest, DomainObjectsReferToSameFirestoreInstance) { EXPECT_EQ(TestFirestore(), TestFirestore()->Document("foo/bar").firestore()); EXPECT_EQ(TestFirestore(), TestFirestore()->Collection("foo").firestore()); } -TEST_F(FirestoreIntegrationTest, AuthWorks) { +TEST_F(FirestoreTest, AuthWorks) { SKIP_TEST_ON_QUICK_CHECK; // This app instance is managed by the text fixture. App* app = GetApp(); @@ -1549,7 +1703,7 @@ TEST_F(FirestoreIntegrationTest, AuthWorks) { // This test is to ensure b/172986326 doesn't regress. // Note: this test only exists in C++. -TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransactionAsync) { +TEST_F(FirestoreTest, FirestoreCanBeDeletedFromTransactionAsync) { Firestore* db = TestFirestore(); DisownFirestore(db); @@ -1571,7 +1725,7 @@ TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransactionAsync) { // This test is to ensure b/172986326 doesn't regress. // Note: this test only exists in C++. -TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) { +TEST_F(FirestoreTest, FirestoreCanBeDeletedFromTransaction) { Firestore* db = TestFirestore(); DisownFirestore(db); diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index 192238def..fc7cecbc6 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -15,6 +15,7 @@ */ #include // NOLINT(build/c++11) +#include #include // NOLINT(build/c++11) #include "absl/memory/memory.h" @@ -34,10 +35,11 @@ namespace firebase { namespace firestore { struct TestFriend { - static FirestoreInternal* CreateTestFirestoreInternal(App* app) { + static FirestoreInternal* CreateTestFirestoreInternal( + App* app, const std::string& database_id) { #if !defined(__ANDROID__) return new FirestoreInternal( - app, /*database_id=*/"(default)", + app, database_id.c_str(), absl::make_unique(), absl::make_unique()); #else @@ -79,8 +81,9 @@ App* GetApp(const char* name, const std::string& override_project_id) { App* GetApp() { return GetApp(/*name=*/nullptr, /*project_id=*/""); } -FirestoreInternal* CreateTestFirestoreInternal(App* app) { - return TestFriend::CreateTestFirestoreInternal(app); +FirestoreInternal* CreateTestFirestoreInternal(App* app, + const std::string& database_id) { + return TestFriend::CreateTestFirestoreInternal(app, database_id); } } // namespace firestore diff --git a/firestore/integration_test_internal/src/util/locate_emulator.cc b/firestore/integration_test_internal/src/util/locate_emulator.cc index 89f7470e1..4e6fb4ae3 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.cc +++ b/firestore/integration_test_internal/src/util/locate_emulator.cc @@ -53,5 +53,13 @@ void LocateEmulator(Firestore* db) { db->set_settings(settings); } +// Check if firestore is using Firestore Emulator via USE_FIRESTORE_EMULATOR. +bool IsUsingFirestoreEmulator() { + // Emulator is used if the env variable is set, regardless of its value. + if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { + return false; + } + return true; +} } // namespace firestore } // namespace firebase diff --git a/firestore/integration_test_internal/src/util/locate_emulator.h b/firestore/integration_test_internal/src/util/locate_emulator.h index 99225985d..a19ce284e 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.h +++ b/firestore/integration_test_internal/src/util/locate_emulator.h @@ -24,6 +24,9 @@ namespace firestore { // Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR void LocateEmulator(Firestore* db); +// Check if firestore is using Firestore Emulator via USE_FIRESTORE_EMULATOR. +bool IsUsingFirestoreEmulator(); + } // namespace firestore } // namespace firebase diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index c33157bd5..3bd0f43ef 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -472,6 +472,96 @@ TEST_F(ValidationTest, EXPECT_NE(Firestore::GetInstance("foo"), nullptr); } +TEST_F(ValidationTest, + FirestoreGetInstanceCalledMultipleTimeReturnSameInstance) { + { + Firestore* instance1 = Firestore::GetInstance(); + Firestore* instance2 = Firestore::GetInstance(); + EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; + } + { + Firestore* instance1 = Firestore::GetInstance(app()); + Firestore* instance2 = Firestore::GetInstance(app()); + EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; + } + { + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance("foo"); + EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; + } + { + Firestore* instance1 = Firestore::GetInstance(app(), "foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "foo"); + EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; + } +} + +TEST_F(ValidationTest, + DifferentFirestoreGetInstanceCanGetSameDefaultFirestoreInstance) { + Firestore* instance1 = Firestore::GetInstance(); + Firestore* instance2 = Firestore::GetInstance(app()); + Firestore* instance3 = Firestore::GetInstance("(default)"); + Firestore* instance4 = Firestore::GetInstance(app(), "(default)"); + + EXPECT_EQ(instance1, instance2); + EXPECT_EQ(instance1, instance3); + EXPECT_EQ(instance1, instance4); + delete instance1; + delete instance2; + delete instance3; + delete instance4; +} + +TEST_F( + ValidationTest, + DifferentFirestoreGetInstanceWithSameDatabaseNameShouldGetSameFirestoreInstance) { + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "foo"); + EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; +} + +TEST_F( + ValidationTest, + DifferentFirestoreGetInstanceWithDifferentDatabaseNameShouldGetDifferentFirestoreInstance) { + { + Firestore* instance1 = Firestore::GetInstance(); + Firestore* instance2 = Firestore::GetInstance("bar"); + EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; + } + { + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance("bar"); + EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; + } + { + Firestore* instance1 = Firestore::GetInstance(app(), "foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "bar"); + EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; + } + { + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "bar"); + EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; + } +} TEST_F(ValidationTest, CollectionPathsMustBeOddLength) { Firestore* db = TestFirestore(); DocumentReference base_document = db->Document("foo/bar"); diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 9fe7e3c45..cbf469922 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -63,25 +63,37 @@ const char* GetPlatform() { #endif } +// Use the combination of the App address and database ID to identify a +// firestore instance in cache. +using FirestoreMap = std::map, Firestore*>; + +FirestoreMap::key_type MakeKey(App* app, const char* database_id) { + return std::make_pair(app, std::string(database_id)); +} + Mutex* g_firestores_lock = new Mutex(); -std::map* g_firestores = nullptr; +FirestoreMap* g_firestores = nullptr; // Ensures that the cache is initialized. // Prerequisite: `g_firestores_lock` must be locked before calling this // function. -std::map* FirestoreCache() { +FirestoreMap* FirestoreCache() { if (!g_firestores) { - g_firestores = new std::map(); + g_firestores = new FirestoreMap(); } return g_firestores; } // Prerequisite: `g_firestores_lock` must be locked before calling this // function. -Firestore* FindFirestoreInCache(App* app, InitResult* init_result_out) { +Firestore* FindFirestoreInCache(App* app, + const char* database_id, + InitResult* init_result_out) { auto* cache = FirestoreCache(); - auto found = cache->find(app); + FirestoreMap::key_type key = MakeKey(app, database_id); + FirestoreMap::iterator found = g_firestores->find(key); + if (found != cache->end()) { if (init_result_out) *init_result_out = kInitResultSuccess; return found->second; @@ -107,8 +119,8 @@ void ValidateApp(App* app) { } } -void ValidateDatabase(const char* db_name) { - if (!db_name) { +void ValidateDatabase(const char* database_id) { + if (!database_id) { SimpleThrowInvalidArgument( "Provided database ID must not be null. Use other " "firebase::App::GetInstance() if you'd like to use the default " @@ -150,12 +162,13 @@ Firestore* Firestore::GetInstance(App* app, ValidateDatabase(db_name); MutexLock lock(*g_firestores_lock); - Firestore* from_cache = FindFirestoreInCache(app, init_result_out); + Firestore* from_cache = FindFirestoreInCache(app, db_name, init_result_out); if (from_cache) { return from_cache; } - return AddFirestoreToCache(new Firestore(app, db_name), init_result_out); + return AddFirestoreToCache(new Firestore(app, db_name), db_name, + init_result_out); } Firestore* Firestore::CreateFirestore(App* app, @@ -167,14 +180,18 @@ Firestore* Firestore::CreateFirestore(App* app, MutexLock lock(*g_firestores_lock); - Firestore* from_cache = FindFirestoreInCache(app, init_result_out); + const char* database_id = internal->database_id().database_id().c_str(); + Firestore* from_cache = + FindFirestoreInCache(app, database_id, init_result_out); SIMPLE_HARD_ASSERT(from_cache == nullptr, "Firestore must not be created already"); - return AddFirestoreToCache(new Firestore(internal), init_result_out); + return AddFirestoreToCache(new Firestore(internal), database_id, + init_result_out); } Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, + const char* database_id, InitResult* init_result_out) { InitResult init_result = CheckInitialized(*firestore->internal_); if (init_result_out) { @@ -185,7 +202,8 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return nullptr; } - FirestoreCache()->emplace(firestore->app(), firestore); + FirestoreMap::key_type key = MakeKey(firestore->app(), database_id); + FirestoreCache()->emplace(key, firestore); return firestore; } @@ -226,6 +244,7 @@ void Firestore::DeleteInternal() { if (!internal_) return; App* my_app = app(); + const std::string database_id = internal_->database_id().database_id(); // Only need to unregister if internal_ is initialized. if (internal_->initialized()) { @@ -249,8 +268,10 @@ void Firestore::DeleteInternal() { delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. - FirestoreCache()->erase(my_app); - // If it's the last one, delete the map. + + FirestoreMap::key_type key = MakeKey(my_app, database_id.c_str()); + + FirestoreCache()->erase(key); if (g_firestores->empty()) { delete g_firestores; g_firestores = nullptr; @@ -362,7 +383,10 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - FirestoreCache()->erase(app()); + const std::string& database_id = internal_->database_id().database_id(); + FirestoreMap::key_type key = MakeKey(app(), database_id.c_str()); + + FirestoreCache()->erase(key); return internal_->Terminate(); } diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index a2befc375..9c745a212 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -503,6 +503,7 @@ class Firestore { FirestoreInternal* internal, InitResult* init_result_out); static Firestore* AddFirestoreToCache(Firestore* firestore, + const char* database_id, InitResult* init_result_out); static void SetClientLanguage(const std::string& language_token); From 1e4ae745b4dfa9a25be73682889134781e201547 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Mon, 15 May 2023 17:11:40 -0400 Subject: [PATCH 04/18] Enable multi database in android (#1318) --- .../src/firestore_test.cc | 62 ++++++++++++++++++- .../src/util/integration_test_util.cc | 2 +- .../src/validation_test.cc | 28 ++++----- firestore/src/android/firestore_android.cc | 9 ++- firestore/src/android/firestore_android.h | 7 ++- firestore/src/common/firestore.cc | 6 +- firestore/src/main/firestore_main.cc | 1 + firestore/src/main/firestore_main.h | 4 ++ 8 files changed, 93 insertions(+), 26 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 24b478f06..b8e33f768 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -16,7 +16,6 @@ #include "firebase/firestore.h" -#include #include #include #include @@ -24,7 +23,9 @@ #if defined(__ANDROID__) #include "android/firestore_integration_test_android.h" +#include "firestore/src/android/converter_android.h" #include "firestore/src/android/exception_android.h" +#include "firestore/src/android/firestore_android.h" #include "firestore/src/android/jni_runnable_android.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/ownership.h" @@ -41,6 +42,7 @@ #include "util/future_test_util.h" #if !defined(__ANDROID__) #include "Firestore/core/src/util/autoid.h" +#include "firestore/src/main/converter_main.h" #include "firestore/src/main/firestore_main.h" #else #include "android/util_autoid.h" @@ -66,7 +68,7 @@ using ::testing::HasSubstr; class FirestoreTest : public FirestoreIntegrationTest { protected: const std::string GetFirestoreDatabaseId(Firestore* firestore) { - return GetInternal(firestore)->database_id().database_id(); + return GetInternal(firestore)->database_name(); } }; @@ -1402,6 +1404,62 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { delete db1; } +TEST_F(FirestoreTest, CanCreateMultipleDatabases) { + // TODO(Mila): Remove the emulator env check after prod supports multiDB. + if (!IsUsingFirestoreEmulator()) { + GTEST_SKIP(); + } + + // Create two database instances in the same app. + App* app = App::GetInstance(); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); + EXPECT_NE(db1, db2); + + // Create collections with same name in different databases. + DocumentReference doc1 = db1->Collection("abc").Document(); + DocumentReference doc2 = db2->Collection("abc").Document(); + + // Databases can store and retrieve documents without interfering with each + // other. + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar1")}}), + FutureSucceeds()); + EXPECT_THAT(doc2.Set({{"foo", FieldValue::String("bar2")}}), + FutureSucceeds()); + + const DocumentSnapshot* snapshot1 = Await(doc1.Get()); + EXPECT_TRUE(snapshot1->exists()); + EXPECT_THAT(snapshot1->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar1")}})); + const DocumentSnapshot* snapshot2 = Await(doc2.Get()); + EXPECT_TRUE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar2")}})); +} + +TEST_F(FirestoreTest, CanTerminateMultipleDatabases) { + // TODO(Mila): Remove the emulator env check after prod supports multiDB. + if (!IsUsingFirestoreEmulator()) { + GTEST_SKIP(); + } + + // Create two database instances in the same app. + App* app = App::GetInstance(); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); + EXPECT_NE(db1, db2); + + // A database can be terminated without affecting other databases. + DeleteFirestore(db1); + + DocumentReference doc = db2->Collection("abc").Document(); + EXPECT_THAT(doc.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); + const DocumentSnapshot* snapshot = Await(doc.Get()); + EXPECT_TRUE(snapshot->exists()); + EXPECT_THAT(snapshot->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); +} + TEST_F(FirestoreTest, CanReadDocsAfterRestartFirestoreAndCreateNewNamedDatabaseInstance) { // TODO(Mila): Remove the emulator env check and LocateEmulator call after diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index e1eb2427b..54ce75d53 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -43,7 +43,7 @@ struct TestFriend { absl::make_unique(), absl::make_unique()); #else - return new FirestoreInternal(app); + return new FirestoreInternal(app, database_id.c_str()); #endif // !defined(__ANDROID__) } }; diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index 3bd0f43ef..b5524411e 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -443,33 +443,37 @@ TEST_F(ValidationTest, TEST_F(ValidationTest, FirestoreGetInstanceWithNoArgumentsReturnsNonNullInstance) { InitResult result; - EXPECT_NO_THROW(Firestore::GetInstance(&result)); + Firestore* instance = Firestore::GetInstance(&result); EXPECT_EQ(kInitResultSuccess, result); - EXPECT_NE(Firestore::GetInstance(), nullptr); + EXPECT_NE(instance, nullptr); + delete instance; } TEST_F(ValidationTest, FirestoreGetInstanceWithNonNullAppReturnsNonNullInstance) { InitResult result; - EXPECT_NO_THROW(Firestore::GetInstance(app(), &result)); + Firestore* instance = Firestore::GetInstance(app(), &result); EXPECT_EQ(kInitResultSuccess, result); - EXPECT_NE(Firestore::GetInstance(app()), nullptr); + EXPECT_NE(instance, nullptr); + delete instance; } TEST_F(ValidationTest, FirestoreGetInstanceWithAppAndDatabaseNameReturnsNonNullInstance) { InitResult result; - EXPECT_NO_THROW(Firestore::GetInstance(app(), "foo", &result)); + Firestore* instance = Firestore::GetInstance(app(), "foo", &result); EXPECT_EQ(kInitResultSuccess, result); - EXPECT_NE(Firestore::GetInstance(app(), "foo"), nullptr); + EXPECT_NE(instance, nullptr); + delete instance; } TEST_F(ValidationTest, FirestoreGetInstanceWithDatabaseNameReturnsNonNullInstance) { InitResult result; - EXPECT_NO_THROW(Firestore::GetInstance("foo", &result)); + Firestore* instance = Firestore::GetInstance("foo", &result); EXPECT_EQ(kInitResultSuccess, result); - EXPECT_NE(Firestore::GetInstance("foo"), nullptr); + EXPECT_NE(instance, nullptr); + delete instance; } TEST_F(ValidationTest, @@ -479,28 +483,24 @@ TEST_F(ValidationTest, Firestore* instance2 = Firestore::GetInstance(); EXPECT_EQ(instance1, instance2); delete instance1; - delete instance2; } { Firestore* instance1 = Firestore::GetInstance(app()); Firestore* instance2 = Firestore::GetInstance(app()); EXPECT_EQ(instance1, instance2); delete instance1; - delete instance2; } { Firestore* instance1 = Firestore::GetInstance("foo"); Firestore* instance2 = Firestore::GetInstance("foo"); EXPECT_EQ(instance1, instance2); delete instance1; - delete instance2; } { Firestore* instance1 = Firestore::GetInstance(app(), "foo"); Firestore* instance2 = Firestore::GetInstance(app(), "foo"); EXPECT_EQ(instance1, instance2); delete instance1; - delete instance2; } } @@ -515,9 +515,6 @@ TEST_F(ValidationTest, EXPECT_EQ(instance1, instance3); EXPECT_EQ(instance1, instance4); delete instance1; - delete instance2; - delete instance3; - delete instance4; } TEST_F( @@ -527,7 +524,6 @@ TEST_F( Firestore* instance2 = Firestore::GetInstance(app(), "foo"); EXPECT_EQ(instance1, instance2); delete instance1; - delete instance2; } TEST_F( diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index 4c0e2a990..3a625accd 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -114,7 +114,7 @@ Method kGetSettings( "()Lcom/google/firebase/firestore/FirebaseFirestoreSettings;"); StaticMethod kGetInstance( "getInstance", - "(Lcom/google/firebase/FirebaseApp;)" + "(Lcom/google/firebase/FirebaseApp;Ljava/lang/String;)" "Lcom/google/firebase/firestore/FirebaseFirestore;"); StaticMethod kSetLoggingEnabled("setLoggingEnabled", "(Z)V"); StaticMethod kSetClientLanguage("setClientLanguage", @@ -260,14 +260,16 @@ Local CreateLoadBundleTask(Env& env, const char kApiIdentifier[] = "Firestore"; -FirestoreInternal::FirestoreInternal(App* app) { +FirestoreInternal::FirestoreInternal(App* app, const char* database_id) { FIREBASE_ASSERT(app != nullptr); if (!Initialize(app)) return; app_ = app; Env env = GetEnv(); Local platform_app(env.get(), app_->GetPlatformApp()); - Local java_firestore = env.Call(kGetInstance, platform_app); + Local java_database_id = env.NewStringUtf(database_id); + Local java_firestore = + env.Call(kGetInstance, platform_app, java_database_id); FIREBASE_ASSERT(java_firestore.get() != nullptr); obj_ = java_firestore; @@ -285,6 +287,7 @@ FirestoreInternal::FirestoreInternal(App* app) { user_callback_executor_ = java_user_callback_executor; promises_ = std::make_unique>(this); + database_name_ = database_id; } /* static */ diff --git a/firestore/src/android/firestore_android.h b/firestore/src/android/firestore_android.h index d5079c753..b79234055 100644 --- a/firestore/src/android/firestore_android.h +++ b/firestore/src/android/firestore_android.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "app/src/cleanup_notifier.h" @@ -75,7 +76,7 @@ class FirestoreInternal { }; // Note: call `set_firestore_public` immediately after construction. - explicit FirestoreInternal(App* app); + FirestoreInternal(App* app, const char* database_id); ~FirestoreInternal(); App* app() const { return app_; } @@ -135,6 +136,8 @@ class FirestoreInternal { ListenerRegistrationInternal* registration); void ClearListeners(); + const std::string& database_name() const { return database_name_; } + // Bundles Future LoadBundle(const std::string& bundle); Future LoadBundle( @@ -211,6 +214,8 @@ class FirestoreInternal { std::unique_ptr> promises_; CleanupNotifier cleanup_; + + std::string database_name_; }; // Holds a "weak reference" to a `FirestoreInternal` object. diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index cbf469922..0502f4d79 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -180,7 +180,7 @@ Firestore* Firestore::CreateFirestore(App* app, MutexLock lock(*g_firestores_lock); - const char* database_id = internal->database_id().database_id().c_str(); + const char* database_id = internal->database_name().c_str(); Firestore* from_cache = FindFirestoreInCache(app, database_id, init_result_out); SIMPLE_HARD_ASSERT(from_cache == nullptr, @@ -244,7 +244,7 @@ void Firestore::DeleteInternal() { if (!internal_) return; App* my_app = app(); - const std::string database_id = internal_->database_id().database_id(); + const std::string database_id = internal_->database_name(); // Only need to unregister if internal_ is initialized. if (internal_->initialized()) { @@ -383,7 +383,7 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - const std::string& database_id = internal_->database_id().database_id(); + const std::string& database_id = internal_->database_name(); FirestoreMap::key_type key = MakeKey(app(), database_id.c_str()); FirestoreCache()->erase(key); diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 3b1b9714d..7e9090b7a 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -119,6 +119,7 @@ FirestoreInternal::FirestoreInternal( transaction_executor_(absl::ShareUniquePtr(Executor::CreateConcurrent( "com.google.firebase.firestore.transaction", /*threads=*/5))) { ApplyDefaultSettings(); + database_name_ = database_id; #if FIREBASE_PLATFORM_ANDROID App::RegisterLibrary("fire-fst", kFirestoreVersionString, app->GetJNIEnv()); diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index 4e7f164a5..4c0aeb41f 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -21,6 +21,7 @@ #include #include #include // NOLINT(build/c++11) +#include #include #include "Firestore/core/src/api/firestore.h" @@ -107,6 +108,8 @@ class FirestoreInternal { return firestore_core_->database_id(); } + const std::string& database_name() const { return database_name_; } + // Bundles Future LoadBundle(const std::string& bundle); Future LoadBundle( @@ -184,6 +187,7 @@ class FirestoreInternal { std::unordered_set listeners_; std::shared_ptr transaction_executor_; + std::string database_name_; }; } // namespace firestore From 40b05d91d9f1b625fd8a72bd90b11d2a40f0f21c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 16 May 2023 11:22:13 -0400 Subject: [PATCH 05/18] format --- firestore/src/common/firestore.cc | 21 +++++++++------------ firestore/src/include/firebase/firestore.h | 1 - release_build_files/readme.md | 1 + 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 0502f4d79..66ded7751 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -67,8 +67,8 @@ const char* GetPlatform() { // firestore instance in cache. using FirestoreMap = std::map, Firestore*>; -FirestoreMap::key_type MakeKey(App* app, const char* database_id) { - return std::make_pair(app, std::string(database_id)); +FirestoreMap::key_type MakeKey(App* app, std::string database_id) { + return std::make_pair(app, std::move(database_id)); } Mutex* g_firestores_lock = new Mutex(); @@ -167,8 +167,7 @@ Firestore* Firestore::GetInstance(App* app, return from_cache; } - return AddFirestoreToCache(new Firestore(app, db_name), db_name, - init_result_out); + return AddFirestoreToCache(new Firestore(app, db_name), init_result_out); } Firestore* Firestore::CreateFirestore(App* app, @@ -186,12 +185,10 @@ Firestore* Firestore::CreateFirestore(App* app, SIMPLE_HARD_ASSERT(from_cache == nullptr, "Firestore must not be created already"); - return AddFirestoreToCache(new Firestore(internal), database_id, - init_result_out); + return AddFirestoreToCache(new Firestore(internal), init_result_out); } Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, - const char* database_id, InitResult* init_result_out) { InitResult init_result = CheckInitialized(*firestore->internal_); if (init_result_out) { @@ -202,7 +199,8 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return nullptr; } - FirestoreMap::key_type key = MakeKey(firestore->app(), database_id); + FirestoreMap::key_type key = + MakeKey(firestore->app(), firestore->internal_->database_name()); FirestoreCache()->emplace(key, firestore); return firestore; } @@ -244,6 +242,7 @@ void Firestore::DeleteInternal() { if (!internal_) return; App* my_app = app(); + // Store the database id here as it will be deleted const std::string database_id = internal_->database_name(); // Only need to unregister if internal_ is initialized. @@ -269,8 +268,7 @@ void Firestore::DeleteInternal() { internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. - FirestoreMap::key_type key = MakeKey(my_app, database_id.c_str()); - + FirestoreMap::key_type key = MakeKey(my_app, database_id); FirestoreCache()->erase(key); if (g_firestores->empty()) { delete g_firestores; @@ -383,8 +381,7 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - const std::string& database_id = internal_->database_name(); - FirestoreMap::key_type key = MakeKey(app(), database_id.c_str()); + FirestoreMap::key_type key = MakeKey(app(), internal_->database_name()); FirestoreCache()->erase(key); return internal_->Terminate(); diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index 85ffa227e..ce83bdd9b 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -502,7 +502,6 @@ class Firestore { FirestoreInternal* internal, InitResult* init_result_out); static Firestore* AddFirestoreToCache(Firestore* firestore, - const char* database_id, InitResult* init_result_out); static void SetClientLanguage(const std::string& language_token); diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 43bd213df..eb383447b 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -633,6 +633,7 @@ code. home directory contains non-ANSI characters (Unicode above U+00FF). - Storage (Desktop): Fixed a crash on Windows when uploading files from a path containing non-ANSI characters (Unicode above U+00FF). + - Firestore: Added MultiDb support. ### 11.0.1 - Changes From d4d5778a20a35dba2262e19f53d976ed62adecec Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 16 May 2023 11:40:20 -0400 Subject: [PATCH 06/18] Update release_build_files/readme.md Co-authored-by: Tom Andersen --- release_build_files/readme.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index eb383447b..b35ddaf9b 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -633,7 +633,7 @@ code. home directory contains non-ANSI characters (Unicode above U+00FF). - Storage (Desktop): Fixed a crash on Windows when uploading files from a path containing non-ANSI characters (Unicode above U+00FF). - - Firestore: Added MultiDb support. + - Firestore: Added MultiDb support. ([#1321](https://github.com/firebase/firebase-cpp-sdk/pull/1321)). ### 11.0.1 - Changes From 387ab6de65a871246ca6e82e7f0b3be63212f3d2 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 16 May 2023 11:51:13 -0400 Subject: [PATCH 07/18] format --- .../src/firestore_test.cc | 37 +++++++++---------- .../src/util/integration_test_util.cc | 1 - 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index b8e33f768..dec4c6742 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -99,7 +99,7 @@ TEST_F(FirestoreTest, GetInstance) { delete auth; } -TEST_F(FirestoreTest, GetInstanceWithNamedDatabase) { +TEST_F(FirestoreTest, GetInstanceWithDynamicDatabaseId) { App* app = this->app(); EXPECT_NE(nullptr, app); @@ -1404,13 +1404,13 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { delete db1; } -TEST_F(FirestoreTest, CanCreateMultipleDatabases) { +TEST_F(FirestoreTest, CanCreateMultipleFirestoreInstances) { // TODO(Mila): Remove the emulator env check after prod supports multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } - // Create two database instances in the same app. + // Create two Firestore instances in the same app. App* app = App::GetInstance(); Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); @@ -1437,13 +1437,13 @@ TEST_F(FirestoreTest, CanCreateMultipleDatabases) { ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar2")}})); } -TEST_F(FirestoreTest, CanTerminateMultipleDatabases) { +TEST_F(FirestoreTest, CanTerminateMultipleFirestoreInstances) { // TODO(Mila): Remove the emulator env check after prod supports multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } - // Create two database instances in the same app. + // Create two Firestore instances in the same app. App* app = App::GetInstance(); Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); @@ -1460,8 +1460,7 @@ TEST_F(FirestoreTest, CanTerminateMultipleDatabases) { ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); } -TEST_F(FirestoreTest, - CanReadDocsAfterRestartFirestoreAndCreateNewNamedDatabaseInstance) { +TEST_F(FirestoreTest, CanReadDocsAfterRestartFirestoreAndCreateNewInstance) { // TODO(Mila): Remove the emulator env check and LocateEmulator call after // prod supports multiDB. if (!IsUsingFirestoreEmulator()) { @@ -1502,14 +1501,13 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { GTEST_SKIP(); } - // Create two DB instances in the same app. + // Create two Firestore instances in the same app. App* app = App::GetInstance(); Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); - EXPECT_NE(db1, db2); - // Create a document in the first DB instance. + // Create a document in the first Firestore instance. DocumentReference doc1 = db1->Collection("abc").Document(); const std::string doc_path = doc1.path(); EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); @@ -1518,9 +1516,9 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { EXPECT_THAT(snapshot1->GetData(), ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); - // Verify that the previously saved document only exists in the first DB - // instance by verifying that the document does not exist in the second - // instance. + // Verify that the previously saved document only exists in the first + // Firestore instance by verifying that the document does not exist in the + // second instance. DocumentReference doc2 = db2->Document(doc_path); const DocumentSnapshot* snapshot2 = Await(doc2.Get()); EXPECT_FALSE(snapshot2->exists()); @@ -1533,17 +1531,15 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { GTEST_SKIP(); } - // Create two DB instances in the same app. + // Create two Firestore instances in the same app. App* app = App::GetInstance(); Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); - Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); - EXPECT_NE(db1, db2); DisableNetwork(); - // Create a document in the first DB instance. + // Create a document in the first Firestore instance. DocumentReference doc1 = db1->Collection("abc").Document(); const std::string doc_path = doc1.path(); EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); @@ -1551,9 +1547,10 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { EXPECT_TRUE(snapshot1->exists()); EXPECT_THAT(snapshot1->GetData(), ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); - // Verify that the previously saved document only exists in the first DB - // instance by verifying that the document does not exist in the second - // instance. + + // Verify that the previously saved document only exists in the first + // Firestore instance by verifying that the document does not exist in the + // second instance. DocumentReference doc2 = db2->Document(doc_path); const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); EXPECT_FALSE(snapshot2->exists()); diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index 54ce75d53..17a215404 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -15,7 +15,6 @@ */ #include // NOLINT(build/c++11) -#include #include #include // NOLINT(build/c++11) From 45199ba177a800d9e2c853eac7810f623ac6fad6 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 16 May 2023 13:12:10 -0400 Subject: [PATCH 08/18] add the task id to the TODO comments --- .../src/firestore_test.cc | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index dec4c6742..549f9aaab 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1405,7 +1405,8 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { } TEST_F(FirestoreTest, CanCreateMultipleFirestoreInstances) { - // TODO(Mila): Remove the emulator env check after prod supports multiDB. + // TODO(b/282947967): Remove the emulator env check after prod supports + // multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } @@ -1438,7 +1439,8 @@ TEST_F(FirestoreTest, CanCreateMultipleFirestoreInstances) { } TEST_F(FirestoreTest, CanTerminateMultipleFirestoreInstances) { - // TODO(Mila): Remove the emulator env check after prod supports multiDB. + // TODO(b/282947967): Remove the emulator env check after prod supports + // multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } @@ -1461,8 +1463,8 @@ TEST_F(FirestoreTest, CanTerminateMultipleFirestoreInstances) { } TEST_F(FirestoreTest, CanReadDocsAfterRestartFirestoreAndCreateNewInstance) { - // TODO(Mila): Remove the emulator env check and LocateEmulator call after - // prod supports multiDB. + // TODO(b/282947967): Remove the emulator env check and LocateEmulator call + // after prod supports multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } @@ -1496,7 +1498,8 @@ TEST_F(FirestoreTest, CanReadDocsAfterRestartFirestoreAndCreateNewInstance) { } TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { - // TODO(Mila): Remove the emulator env check after prod supports multiDB. + // TODO(b/282947967): Remove the emulator env check after prod supports + // multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } @@ -1526,7 +1529,8 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { } TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { - // TODO(Mila): Remove the emulator env check after prod supports multiDB. + // TODO(b/282947967): Remove the emulator env check after prod supports + // multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } From 4c7692134f0c1d60aa79f3a04815ea2566d1d6fb Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 16 May 2023 15:41:27 -0400 Subject: [PATCH 09/18] refactor --- .idea/.name | 1 + .../src/firestore_test.cc | 2 +- .../src/util/locate_emulator.cc | 4 ++-- .../src/util/locate_emulator.h | 2 +- .../src/validation_test.cc | 16 ++++++++-------- firestore/src/android/firestore_android.cc | 2 +- firestore/src/common/firestore.cc | 10 +++++----- firestore/src/main/firestore_main.cc | 4 ++-- 8 files changed, 21 insertions(+), 20 deletions(-) create mode 100644 .idea/.name diff --git a/.idea/.name b/.idea/.name new file mode 100644 index 000000000..f1e19caaa --- /dev/null +++ b/.idea/.name @@ -0,0 +1 @@ +firebase_testapp \ No newline at end of file diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 549f9aaab..37ef42f3a 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1490,7 +1490,7 @@ TEST_F(FirestoreTest, CanReadDocsAfterRestartFirestoreAndCreateNewInstance) { // Verify that the new instance points to the same database by verifying that // the document created with the old instance exists in the new instance. DocumentReference doc2 = db2->Document(doc_path); - const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); + const DocumentSnapshot* snapshot2 = Await(doc2.Get()); ASSERT_NE(snapshot2, nullptr); EXPECT_TRUE(snapshot2->exists()); EXPECT_THAT(snapshot2->GetData(), diff --git a/firestore/integration_test_internal/src/util/locate_emulator.cc b/firestore/integration_test_internal/src/util/locate_emulator.cc index 4e6fb4ae3..ef2d7c521 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.cc +++ b/firestore/integration_test_internal/src/util/locate_emulator.cc @@ -53,9 +53,9 @@ void LocateEmulator(Firestore* db) { db->set_settings(settings); } -// Check if firestore is using Firestore Emulator via USE_FIRESTORE_EMULATOR. bool IsUsingFirestoreEmulator() { - // Emulator is used if the env variable is set, regardless of its value. + // Emulator is used if `USE_FIRESTORE_EMULATOR` is set, regardless of its + // value. if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { return false; } diff --git a/firestore/integration_test_internal/src/util/locate_emulator.h b/firestore/integration_test_internal/src/util/locate_emulator.h index a19ce284e..4d4d7aa93 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.h +++ b/firestore/integration_test_internal/src/util/locate_emulator.h @@ -24,7 +24,7 @@ namespace firestore { // Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR void LocateEmulator(Firestore* db); -// Check if firestore is using Firestore Emulator via USE_FIRESTORE_EMULATOR. +// Check if firestore is using Firestore Emulator. bool IsUsingFirestoreEmulator(); } // namespace firestore diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index b5524411e..1d3ee2fcd 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -410,15 +410,15 @@ TEST_F(ValidationTest, FirestoreGetInstanceWithNullAppFails) { EXPECT_ERROR(Firestore::GetInstance(/*app=*/(App*)nullptr, /*init_result=*/(InitResult*)nullptr), "firebase::App instance cannot be null. Use other " - "firebase::App::GetInstance() if you'd like to use the default " - "instance."); + "Firestore::GetInstance() if you'd like to use the default " + "app instance."); } TEST_F(ValidationTest, FirestoreGetInstanceWithNullDatabaseNameFails) { EXPECT_ERROR(Firestore::GetInstance(/*db_name=*/(char*)nullptr, /*init_result=*/(InitResult*)nullptr), "Provided database ID must not be null. Use other " - "firebase::App::GetInstance() if you'd like to use the default " + "Firestore::GetInstance() if you'd like to use the default " "database ID."); } @@ -427,8 +427,8 @@ TEST_F(ValidationTest, EXPECT_ERROR(Firestore::GetInstance(/*app=*/(App*)nullptr, "foo", /*init_result=*/(InitResult*)nullptr), "firebase::App instance cannot be null. Use other " - "firebase::App::GetInstance() if you'd like to use the default " - "instance."); + "Firestore::GetInstance() if you'd like to use the default " + "app instance."); } TEST_F(ValidationTest, @@ -436,7 +436,7 @@ TEST_F(ValidationTest, EXPECT_ERROR(Firestore::GetInstance(app(), /*db_name=*/(char*)nullptr, /*init_result=*/(InitResult*)nullptr), "Provided database ID must not be null. Use other " - "firebase::App::GetInstance() if you'd like to use the default " + "Firestore::GetInstance() if you'd like to use the default " "database ID."); } @@ -505,7 +505,7 @@ TEST_F(ValidationTest, } TEST_F(ValidationTest, - DifferentFirestoreGetInstanceCanGetSameDefaultFirestoreInstance) { + DifferentFirestoreGetInstanceMethodCanGetSameDefaultFirestoreInstance) { Firestore* instance1 = Firestore::GetInstance(); Firestore* instance2 = Firestore::GetInstance(app()); Firestore* instance3 = Firestore::GetInstance("(default)"); @@ -531,7 +531,7 @@ TEST_F( DifferentFirestoreGetInstanceWithDifferentDatabaseNameShouldGetDifferentFirestoreInstance) { { Firestore* instance1 = Firestore::GetInstance(); - Firestore* instance2 = Firestore::GetInstance("bar"); + Firestore* instance2 = Firestore::GetInstance("foo"); EXPECT_NE(instance1, instance2); delete instance1; delete instance2; diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index 3a625accd..5cd01417d 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -264,6 +264,7 @@ FirestoreInternal::FirestoreInternal(App* app, const char* database_id) { FIREBASE_ASSERT(app != nullptr); if (!Initialize(app)) return; app_ = app; + database_name_ = database_id; Env env = GetEnv(); Local platform_app(env.get(), app_->GetPlatformApp()); @@ -287,7 +288,6 @@ FirestoreInternal::FirestoreInternal(App* app, const char* database_id) { user_callback_executor_ = java_user_callback_executor; promises_ = std::make_unique>(this); - database_name_ = database_id; } /* static */ diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 66ded7751..ef08c31b7 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -114,8 +114,8 @@ void ValidateApp(App* app) { if (!app) { SimpleThrowInvalidArgument( "firebase::App instance cannot be null. Use other " - "firebase::App::GetInstance() if you'd like to use the default " - "instance."); + "Firestore::GetInstance() if you'd like to use the default " + "app instance."); } } @@ -123,7 +123,7 @@ void ValidateDatabase(const char* database_id) { if (!database_id) { SimpleThrowInvalidArgument( "Provided database ID must not be null. Use other " - "firebase::App::GetInstance() if you'd like to use the default " + "Firestore::GetInstance() if you'd like to use the default " "database ID."); } } @@ -242,8 +242,6 @@ void Firestore::DeleteInternal() { if (!internal_) return; App* my_app = app(); - // Store the database id here as it will be deleted - const std::string database_id = internal_->database_name(); // Only need to unregister if internal_ is initialized. if (internal_->initialized()) { @@ -264,6 +262,8 @@ void Firestore::DeleteInternal() { // Force cleanup to happen first. internal_->cleanup().CleanupAll(); + // Store the database id before deleting the firestore instance. + const std::string database_id = internal_->database_name(); delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 7e9090b7a..39a86a39c 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -117,9 +117,9 @@ FirestoreInternal::FirestoreInternal( std::move(auth_credentials), std::move(app_check_credentials))), transaction_executor_(absl::ShareUniquePtr(Executor::CreateConcurrent( - "com.google.firebase.firestore.transaction", /*threads=*/5))) { + "com.google.firebase.firestore.transaction", /*threads=*/5))), + database_name_(database_id) { ApplyDefaultSettings(); - database_name_ = database_id; #if FIREBASE_PLATFORM_ANDROID App::RegisterLibrary("fire-fst", kFirestoreVersionString, app->GetJNIEnv()); From f2c93237f7604ac685918f2ab6ffd7732d41ef38 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 16 May 2023 16:07:54 -0400 Subject: [PATCH 10/18] Delete .name --- .idea/.name | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .idea/.name diff --git a/.idea/.name b/.idea/.name deleted file mode 100644 index f1e19caaa..000000000 --- a/.idea/.name +++ /dev/null @@ -1 +0,0 @@ -firebase_testapp \ No newline at end of file From 158986f4f863a7fda4fdf4ea12c7b344ac957d1c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 18 May 2023 13:14:48 -0400 Subject: [PATCH 11/18] try using this->databaseId() in android --- firestore/src/android/firestore_android.cc | 1 - firestore/src/android/firestore_android.h | 9 +++++---- firestore/src/common/firestore.cc | 8 ++++---- firestore/src/main/firestore_main.cc | 3 +-- firestore/src/main/firestore_main.h | 3 --- 5 files changed, 10 insertions(+), 14 deletions(-) diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index 5cd01417d..3b723557d 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -264,7 +264,6 @@ FirestoreInternal::FirestoreInternal(App* app, const char* database_id) { FIREBASE_ASSERT(app != nullptr); if (!Initialize(app)) return; app_ = app; - database_name_ = database_id; Env env = GetEnv(); Local platform_app(env.get(), app_->GetPlatformApp()); diff --git a/firestore/src/android/firestore_android.h b/firestore/src/android/firestore_android.h index b79234055..523ef17de 100644 --- a/firestore/src/android/firestore_android.h +++ b/firestore/src/android/firestore_android.h @@ -25,6 +25,7 @@ #include #include +#include "Firestore/core/src/model/database_id.h" #include "app/src/cleanup_notifier.h" #include "app/src/future_manager.h" #include "app/src/include/firebase/app.h" @@ -136,8 +137,6 @@ class FirestoreInternal { ListenerRegistrationInternal* registration); void ClearListeners(); - const std::string& database_name() const { return database_name_; } - // Bundles Future LoadBundle(const std::string& bundle); Future LoadBundle( @@ -167,6 +166,10 @@ class FirestoreInternal { firestore_public_ = firestore_public; } + const model::DatabaseId& database_id() const { + return this->database_id(); + } + Firestore* firestore_public() { return firestore_public_; } const Firestore* firestore_public() const { return firestore_public_; } @@ -214,8 +217,6 @@ class FirestoreInternal { std::unique_ptr> promises_; CleanupNotifier cleanup_; - - std::string database_name_; }; // Holds a "weak reference" to a `FirestoreInternal` object. diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index ef08c31b7..2dd65a566 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -179,7 +179,7 @@ Firestore* Firestore::CreateFirestore(App* app, MutexLock lock(*g_firestores_lock); - const char* database_id = internal->database_name().c_str(); + const char* database_id = internal->database_id().database_id().c_str(); Firestore* from_cache = FindFirestoreInCache(app, database_id, init_result_out); SIMPLE_HARD_ASSERT(from_cache == nullptr, @@ -200,7 +200,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, } FirestoreMap::key_type key = - MakeKey(firestore->app(), firestore->internal_->database_name()); + MakeKey(firestore->app(), firestore->internal_->database_id().database_id()); FirestoreCache()->emplace(key, firestore); return firestore; } @@ -263,7 +263,7 @@ void Firestore::DeleteInternal() { // Force cleanup to happen first. internal_->cleanup().CleanupAll(); // Store the database id before deleting the firestore instance. - const std::string database_id = internal_->database_name(); + const std::string database_id = internal_->database_id().database_id(); delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. @@ -381,7 +381,7 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - FirestoreMap::key_type key = MakeKey(app(), internal_->database_name()); + FirestoreMap::key_type key = MakeKey(app(), internal_->database_id().database_id()); FirestoreCache()->erase(key); return internal_->Terminate(); diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 39a86a39c..3b1b9714d 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -117,8 +117,7 @@ FirestoreInternal::FirestoreInternal( std::move(auth_credentials), std::move(app_check_credentials))), transaction_executor_(absl::ShareUniquePtr(Executor::CreateConcurrent( - "com.google.firebase.firestore.transaction", /*threads=*/5))), - database_name_(database_id) { + "com.google.firebase.firestore.transaction", /*threads=*/5))) { ApplyDefaultSettings(); #if FIREBASE_PLATFORM_ANDROID diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index 4c0aeb41f..fb76c4401 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -108,8 +108,6 @@ class FirestoreInternal { return firestore_core_->database_id(); } - const std::string& database_name() const { return database_name_; } - // Bundles Future LoadBundle(const std::string& bundle); Future LoadBundle( @@ -187,7 +185,6 @@ class FirestoreInternal { std::unordered_set listeners_; std::shared_ptr transaction_executor_; - std::string database_name_; }; } // namespace firestore From 7639a73b431f64c8a3507151f3091c8ad4e323a2 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 18 May 2023 13:25:01 -0400 Subject: [PATCH 12/18] update firestore_test file --- firestore/integration_test_internal/src/firestore_test.cc | 4 ++-- firestore/src/android/firestore_android.h | 4 +--- firestore/src/common/firestore.cc | 7 ++++--- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 37ef42f3a..aad5e8c80 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -67,8 +67,8 @@ using ::testing::HasSubstr; class FirestoreTest : public FirestoreIntegrationTest { protected: - const std::string GetFirestoreDatabaseId(Firestore* firestore) { - return GetInternal(firestore)->database_name(); + const std::string& GetFirestoreDatabaseId(Firestore* firestore) { + return GetInternal(firestore)->database_id().database_id(); } }; diff --git a/firestore/src/android/firestore_android.h b/firestore/src/android/firestore_android.h index 523ef17de..578db6cf0 100644 --- a/firestore/src/android/firestore_android.h +++ b/firestore/src/android/firestore_android.h @@ -166,9 +166,7 @@ class FirestoreInternal { firestore_public_ = firestore_public; } - const model::DatabaseId& database_id() const { - return this->database_id(); - } + const model::DatabaseId& database_id() const { return this->database_id(); } Firestore* firestore_public() { return firestore_public_; } const Firestore* firestore_public() const { return firestore_public_; } diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 2dd65a566..494b5c175 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -199,8 +199,8 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return nullptr; } - FirestoreMap::key_type key = - MakeKey(firestore->app(), firestore->internal_->database_id().database_id()); + FirestoreMap::key_type key = MakeKey( + firestore->app(), firestore->internal_->database_id().database_id()); FirestoreCache()->emplace(key, firestore); return firestore; } @@ -381,7 +381,8 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - FirestoreMap::key_type key = MakeKey(app(), internal_->database_id().database_id()); + FirestoreMap::key_type key = + MakeKey(app(), internal_->database_id().database_id()); FirestoreCache()->erase(key); return internal_->Terminate(); From a3b74954e90770fb907ce8e726144f579ed2da22 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 18 May 2023 14:26:13 -0400 Subject: [PATCH 13/18] Revert "update firestore_test file" This reverts commit 7639a73b431f64c8a3507151f3091c8ad4e323a2. --- firestore/integration_test_internal/src/firestore_test.cc | 4 ++-- firestore/src/android/firestore_android.h | 4 +++- firestore/src/common/firestore.cc | 7 +++---- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index aad5e8c80..37ef42f3a 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -67,8 +67,8 @@ using ::testing::HasSubstr; class FirestoreTest : public FirestoreIntegrationTest { protected: - const std::string& GetFirestoreDatabaseId(Firestore* firestore) { - return GetInternal(firestore)->database_id().database_id(); + const std::string GetFirestoreDatabaseId(Firestore* firestore) { + return GetInternal(firestore)->database_name(); } }; diff --git a/firestore/src/android/firestore_android.h b/firestore/src/android/firestore_android.h index 578db6cf0..523ef17de 100644 --- a/firestore/src/android/firestore_android.h +++ b/firestore/src/android/firestore_android.h @@ -166,7 +166,9 @@ class FirestoreInternal { firestore_public_ = firestore_public; } - const model::DatabaseId& database_id() const { return this->database_id(); } + const model::DatabaseId& database_id() const { + return this->database_id(); + } Firestore* firestore_public() { return firestore_public_; } const Firestore* firestore_public() const { return firestore_public_; } diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 494b5c175..2dd65a566 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -199,8 +199,8 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return nullptr; } - FirestoreMap::key_type key = MakeKey( - firestore->app(), firestore->internal_->database_id().database_id()); + FirestoreMap::key_type key = + MakeKey(firestore->app(), firestore->internal_->database_id().database_id()); FirestoreCache()->emplace(key, firestore); return firestore; } @@ -381,8 +381,7 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - FirestoreMap::key_type key = - MakeKey(app(), internal_->database_id().database_id()); + FirestoreMap::key_type key = MakeKey(app(), internal_->database_id().database_id()); FirestoreCache()->erase(key); return internal_->Terminate(); From 2ccf668fd4e4b8837726d763139eca99168e5796 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 18 May 2023 14:26:22 -0400 Subject: [PATCH 14/18] Revert "try using this->databaseId() in android" This reverts commit 158986f4f863a7fda4fdf4ea12c7b344ac957d1c. --- firestore/src/android/firestore_android.cc | 1 + firestore/src/android/firestore_android.h | 9 ++++----- firestore/src/common/firestore.cc | 8 ++++---- firestore/src/main/firestore_main.cc | 3 ++- firestore/src/main/firestore_main.h | 3 +++ 5 files changed, 14 insertions(+), 10 deletions(-) diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index 3b723557d..5cd01417d 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -264,6 +264,7 @@ FirestoreInternal::FirestoreInternal(App* app, const char* database_id) { FIREBASE_ASSERT(app != nullptr); if (!Initialize(app)) return; app_ = app; + database_name_ = database_id; Env env = GetEnv(); Local platform_app(env.get(), app_->GetPlatformApp()); diff --git a/firestore/src/android/firestore_android.h b/firestore/src/android/firestore_android.h index 523ef17de..b79234055 100644 --- a/firestore/src/android/firestore_android.h +++ b/firestore/src/android/firestore_android.h @@ -25,7 +25,6 @@ #include #include -#include "Firestore/core/src/model/database_id.h" #include "app/src/cleanup_notifier.h" #include "app/src/future_manager.h" #include "app/src/include/firebase/app.h" @@ -137,6 +136,8 @@ class FirestoreInternal { ListenerRegistrationInternal* registration); void ClearListeners(); + const std::string& database_name() const { return database_name_; } + // Bundles Future LoadBundle(const std::string& bundle); Future LoadBundle( @@ -166,10 +167,6 @@ class FirestoreInternal { firestore_public_ = firestore_public; } - const model::DatabaseId& database_id() const { - return this->database_id(); - } - Firestore* firestore_public() { return firestore_public_; } const Firestore* firestore_public() const { return firestore_public_; } @@ -217,6 +214,8 @@ class FirestoreInternal { std::unique_ptr> promises_; CleanupNotifier cleanup_; + + std::string database_name_; }; // Holds a "weak reference" to a `FirestoreInternal` object. diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 2dd65a566..ef08c31b7 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -179,7 +179,7 @@ Firestore* Firestore::CreateFirestore(App* app, MutexLock lock(*g_firestores_lock); - const char* database_id = internal->database_id().database_id().c_str(); + const char* database_id = internal->database_name().c_str(); Firestore* from_cache = FindFirestoreInCache(app, database_id, init_result_out); SIMPLE_HARD_ASSERT(from_cache == nullptr, @@ -200,7 +200,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, } FirestoreMap::key_type key = - MakeKey(firestore->app(), firestore->internal_->database_id().database_id()); + MakeKey(firestore->app(), firestore->internal_->database_name()); FirestoreCache()->emplace(key, firestore); return firestore; } @@ -263,7 +263,7 @@ void Firestore::DeleteInternal() { // Force cleanup to happen first. internal_->cleanup().CleanupAll(); // Store the database id before deleting the firestore instance. - const std::string database_id = internal_->database_id().database_id(); + const std::string database_id = internal_->database_name(); delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. @@ -381,7 +381,7 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - FirestoreMap::key_type key = MakeKey(app(), internal_->database_id().database_id()); + FirestoreMap::key_type key = MakeKey(app(), internal_->database_name()); FirestoreCache()->erase(key); return internal_->Terminate(); diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 3b1b9714d..39a86a39c 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -117,7 +117,8 @@ FirestoreInternal::FirestoreInternal( std::move(auth_credentials), std::move(app_check_credentials))), transaction_executor_(absl::ShareUniquePtr(Executor::CreateConcurrent( - "com.google.firebase.firestore.transaction", /*threads=*/5))) { + "com.google.firebase.firestore.transaction", /*threads=*/5))), + database_name_(database_id) { ApplyDefaultSettings(); #if FIREBASE_PLATFORM_ANDROID diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index fb76c4401..4c0aeb41f 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -108,6 +108,8 @@ class FirestoreInternal { return firestore_core_->database_id(); } + const std::string& database_name() const { return database_name_; } + // Bundles Future LoadBundle(const std::string& bundle); Future LoadBundle( @@ -185,6 +187,7 @@ class FirestoreInternal { std::unordered_set listeners_; std::shared_ptr transaction_executor_; + std::string database_name_; }; } // namespace firestore From 7f122327a2251ba75fbff2cd34e782c0803fff5f Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 18 May 2023 16:38:05 -0400 Subject: [PATCH 15/18] set up global macro to run against emulator --- .../src/firestore_test.cc | 20 +++++-------------- .../src/util/locate_emulator.cc | 8 -------- .../src/util/locate_emulator.h | 3 --- .../src/firebase_test_framework.h | 12 +++++++++++ 4 files changed, 17 insertions(+), 26 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 37ef42f3a..6644dac03 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1407,9 +1407,7 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { TEST_F(FirestoreTest, CanCreateMultipleFirestoreInstances) { // TODO(b/282947967): Remove the emulator env check after prod supports // multiDB. - if (!IsUsingFirestoreEmulator()) { - GTEST_SKIP(); - } + RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR; // Create two Firestore instances in the same app. App* app = App::GetInstance(); @@ -1441,9 +1439,7 @@ TEST_F(FirestoreTest, CanCreateMultipleFirestoreInstances) { TEST_F(FirestoreTest, CanTerminateMultipleFirestoreInstances) { // TODO(b/282947967): Remove the emulator env check after prod supports // multiDB. - if (!IsUsingFirestoreEmulator()) { - GTEST_SKIP(); - } + RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR; // Create two Firestore instances in the same app. App* app = App::GetInstance(); @@ -1465,9 +1461,7 @@ TEST_F(FirestoreTest, CanTerminateMultipleFirestoreInstances) { TEST_F(FirestoreTest, CanReadDocsAfterRestartFirestoreAndCreateNewInstance) { // TODO(b/282947967): Remove the emulator env check and LocateEmulator call // after prod supports multiDB. - if (!IsUsingFirestoreEmulator()) { - GTEST_SKIP(); - } + RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR; App* app = App::GetInstance(); auto db1 = std::unique_ptr(Firestore::GetInstance(app, "test-db")); @@ -1500,9 +1494,7 @@ TEST_F(FirestoreTest, CanReadDocsAfterRestartFirestoreAndCreateNewInstance) { TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { // TODO(b/282947967): Remove the emulator env check after prod supports // multiDB. - if (!IsUsingFirestoreEmulator()) { - GTEST_SKIP(); - } + RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR; // Create two Firestore instances in the same app. App* app = App::GetInstance(); @@ -1531,9 +1523,7 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { // TODO(b/282947967): Remove the emulator env check after prod supports // multiDB. - if (!IsUsingFirestoreEmulator()) { - GTEST_SKIP(); - } + RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR; // Create two Firestore instances in the same app. App* app = App::GetInstance(); diff --git a/firestore/integration_test_internal/src/util/locate_emulator.cc b/firestore/integration_test_internal/src/util/locate_emulator.cc index ef2d7c521..89f7470e1 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.cc +++ b/firestore/integration_test_internal/src/util/locate_emulator.cc @@ -53,13 +53,5 @@ void LocateEmulator(Firestore* db) { db->set_settings(settings); } -bool IsUsingFirestoreEmulator() { - // Emulator is used if `USE_FIRESTORE_EMULATOR` is set, regardless of its - // value. - if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { - return false; - } - return true; -} } // namespace firestore } // namespace firebase diff --git a/firestore/integration_test_internal/src/util/locate_emulator.h b/firestore/integration_test_internal/src/util/locate_emulator.h index 4d4d7aa93..99225985d 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.h +++ b/firestore/integration_test_internal/src/util/locate_emulator.h @@ -24,9 +24,6 @@ namespace firestore { // Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR void LocateEmulator(Firestore* db); -// Check if firestore is using Firestore Emulator. -bool IsUsingFirestoreEmulator(); - } // namespace firestore } // namespace firebase diff --git a/testing/test_framework/src/firebase_test_framework.h b/testing/test_framework/src/firebase_test_framework.h index 8c2ce92d0..f5e08e573 100644 --- a/testing/test_framework/src/firebase_test_framework.h +++ b/testing/test_framework/src/firebase_test_framework.h @@ -58,6 +58,18 @@ namespace firebase_test_framework { return; \ } + +#define RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR \ + { \ + if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { \ + app_framework::LogInfo( \ + "Skipping %s on non firestore emulator environment.", \ + test_info_->name()); \ + GTEST_SKIP(); \ + return; \ + } \ + } + #if TARGET_OS_IPHONE #define TEST_REQUIRES_USER_INTERACTION_ON_IOS TEST_REQUIRES_USER_INTERACTION #define TEST_REQUIRES_USER_INTERACTION_ON_ANDROID ((void)0) From a2112d014d490dc685aaee86486147a638cf2ba5 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 19 May 2023 10:17:52 -0400 Subject: [PATCH 16/18] use const std::string& --- .../src/firestore_test.cc | 57 ++++++++++++++++++- firestore/src/android/firestore_android.cc | 2 +- firestore/src/android/firestore_android.h | 2 +- firestore/src/common/firestore.cc | 6 +- firestore/src/include/firebase/firestore.h | 2 +- firestore/src/main/firestore_main.cc | 6 +- firestore/src/main/firestore_main.h | 6 +- .../src/firebase_test_framework.h | 21 ++++--- 8 files changed, 78 insertions(+), 24 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 6644dac03..2e398cc3b 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1449,7 +1449,6 @@ TEST_F(FirestoreTest, CanTerminateMultipleFirestoreInstances) { // A database can be terminated without affecting other databases. DeleteFirestore(db1); - DocumentReference doc = db2->Collection("abc").Document(); EXPECT_THAT(doc.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); const DocumentSnapshot* snapshot = Await(doc.Get()); @@ -1551,6 +1550,62 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{})); } +TEST_F(FirestoreTest, ComprehensiveTestOnMultiDbCreationAndTermination) { + // TODO(b/282947967): Remove the emulator env check and LocateEmulator call + // after prod supports multiDB. + RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR; + + // Create 2 firestore instances. + App* app = App::GetInstance(); + Firestore* db1 = Firestore::GetInstance(app, "db1"); + Firestore* db2 = Firestore::GetInstance(app, "db2"); + firestore::LocateEmulator(db1); + firestore::LocateEmulator(db2); + EXPECT_NE(db1, db2); + + // Each firestore can store and read documents independently. + DocumentReference doc1 = db1->Collection("abc").Document(); + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar1")}}), + FutureSucceeds()); + DocumentReference doc2 = db2->Collection("abc").Document(); + EXPECT_THAT(doc2.Set({{"foo", FieldValue::String("bar2")}}), + FutureSucceeds()); + const DocumentSnapshot* snapshot1 = Await(doc1.Get()); + EXPECT_TRUE(snapshot1->exists()); + EXPECT_THAT(snapshot1->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar1")}})); + + // Save the doc path in db1 for verification later. + const std::string doc_path = doc1.path(); + + // Terminate `db1` so that it will be removed from the instance cache. + EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + delete db1; + + // Termination of one firestore instance will not affect other instances. + const DocumentSnapshot* snapshot2 = Await(doc2.Get()); + EXPECT_TRUE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar2")}})); + + // GetInstance() returns a new instance since the old instance has been + // terminated. + Firestore* db3 = Firestore::GetInstance(app, "db1"); + firestore::LocateEmulator(db3); + + // The new instance points to the same database by verifying that the document + // created with the old instance exists in the new instance. + DocumentReference doc3 = db3->Document(doc_path); + const DocumentSnapshot* snapshot3 = Await(doc3.Get()); + ASSERT_NE(snapshot3, nullptr); + EXPECT_TRUE(snapshot3->exists()); + EXPECT_THAT(snapshot3->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar1")}})); + + delete db2; + delete db3; +} + TEST_F(FirestoreTest, CanStopListeningAfterTerminate) { auto instance = TestFirestore(); DocumentReference reference = instance->Document("abc/123"); diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index 5cd01417d..e3317b6fe 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -260,7 +260,7 @@ Local CreateLoadBundleTask(Env& env, const char kApiIdentifier[] = "Firestore"; -FirestoreInternal::FirestoreInternal(App* app, const char* database_id) { +FirestoreInternal::FirestoreInternal(App* app, const std::string& database_id) { FIREBASE_ASSERT(app != nullptr); if (!Initialize(app)) return; app_ = app; diff --git a/firestore/src/android/firestore_android.h b/firestore/src/android/firestore_android.h index b79234055..be72945ce 100644 --- a/firestore/src/android/firestore_android.h +++ b/firestore/src/android/firestore_android.h @@ -76,7 +76,7 @@ class FirestoreInternal { }; // Note: call `set_firestore_public` immediately after construction. - FirestoreInternal(App* app, const char* database_id); + FirestoreInternal(App* app, const std::string& database_id); ~FirestoreInternal(); App* app() const { return app_; } diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index ef08c31b7..144d4909c 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -87,7 +87,7 @@ FirestoreMap* FirestoreCache() { // Prerequisite: `g_firestores_lock` must be locked before calling this // function. Firestore* FindFirestoreInCache(App* app, - const char* database_id, + const std::string& database_id, InitResult* init_result_out) { auto* cache = FirestoreCache(); @@ -179,7 +179,7 @@ Firestore* Firestore::CreateFirestore(App* app, MutexLock lock(*g_firestores_lock); - const char* database_id = internal->database_name().c_str(); + const std::string& database_id = internal->database_name(); Firestore* from_cache = FindFirestoreInCache(app, database_id, init_result_out); SIMPLE_HARD_ASSERT(from_cache == nullptr, @@ -205,7 +205,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return firestore; } -Firestore::Firestore(::firebase::App* app, const char* database_id) +Firestore::Firestore(::firebase::App* app, const std::string& database_id) : Firestore{new FirestoreInternal{app, database_id}} {} Firestore::Firestore(FirestoreInternal* internal) diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index ce83bdd9b..7cc027481 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -495,7 +495,7 @@ class Firestore { friend class csharp::TransactionManager; explicit Firestore(::firebase::App* app, - const char* database_id = kDefaultDatabase); + const std::string& database_id = kDefaultDatabase); explicit Firestore(FirestoreInternal* internal); static Firestore* CreateFirestore(::firebase::App* app, diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 39a86a39c..8a512a49e 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -102,13 +102,13 @@ void ValidateDoubleSlash(const char* path) { } // namespace -FirestoreInternal::FirestoreInternal(App* app, const char* database_id) +FirestoreInternal::FirestoreInternal(App* app, const std::string& database_id) : FirestoreInternal{app, database_id, CreateCredentialsProvider(*app), CreateAppCheckCredentialsProvider(*app)} {} FirestoreInternal::FirestoreInternal( App* app, - const char* database_id, + const std::string& database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials) : app_(NOT_NULL(app)), @@ -136,7 +136,7 @@ FirestoreInternal::~FirestoreInternal() { std::shared_ptr FirestoreInternal::CreateFirestore( App* app, - const char* database_id, + const std::string& database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials) { const AppOptions& opt = app->options(); diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index 4c0aeb41f..7920eccc4 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -56,7 +56,7 @@ class Executor; class FirestoreInternal { public: // Note: call `set_firestore_public` immediately after construction. - FirestoreInternal(App* app, const char* database_id); + FirestoreInternal(App* app, const std::string& database_id); ~FirestoreInternal(); FirestoreInternal(const FirestoreInternal&) = delete; @@ -153,14 +153,14 @@ class FirestoreInternal { FirestoreInternal( App* app, - const char* database_id, + const std::string& database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials); std::shared_ptr CreateFirestore( App* app, - const char* database_id, + const std::string& database_id, std::unique_ptr auth_credentials, std::unique_ptr app_check_credentials); diff --git a/testing/test_framework/src/firebase_test_framework.h b/testing/test_framework/src/firebase_test_framework.h index f5e08e573..cff9ef5ae 100644 --- a/testing/test_framework/src/firebase_test_framework.h +++ b/testing/test_framework/src/firebase_test_framework.h @@ -58,18 +58,17 @@ namespace firebase_test_framework { return; \ } - -#define RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR \ - { \ - if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { \ - app_framework::LogInfo( \ - "Skipping %s on non firestore emulator environment.", \ - test_info_->name()); \ - GTEST_SKIP(); \ - return; \ - } \ +#define RUN_TEST_ONLY_AGAINST_FIRESTORE_EMULATOR \ + { \ + if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { \ + app_framework::LogInfo( \ + "Skipping %s on non firestore emulator environment.", \ + test_info_->name()); \ + GTEST_SKIP(); \ + return; \ + } \ } - + #if TARGET_OS_IPHONE #define TEST_REQUIRES_USER_INTERACTION_ON_IOS TEST_REQUIRES_USER_INTERACTION #define TEST_REQUIRES_USER_INTERACTION_ON_ANDROID ((void)0) From 5bc40c84a5eb6a8cb3581a4ad582a90af9233584 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 19 May 2023 10:32:15 -0400 Subject: [PATCH 17/18] Update integration_test_util.cc --- .../src/util/integration_test_util.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index 17a215404..d3a4129ed 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -38,11 +38,11 @@ struct TestFriend { App* app, const std::string& database_id) { #if !defined(__ANDROID__) return new FirestoreInternal( - app, database_id.c_str(), + app, database_id, absl::make_unique(), absl::make_unique()); #else - return new FirestoreInternal(app, database_id.c_str()); + return new FirestoreInternal(app, database_id); #endif // !defined(__ANDROID__) } }; From 6cf1ba706c13a8127957396cd3b114a415af7ef9 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 19 May 2023 11:47:17 -0400 Subject: [PATCH 18/18] use std::move() on key --- firestore/integration_test_internal/src/firestore_test.cc | 2 +- firestore/src/common/firestore.cc | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 2e398cc3b..b82c3aa63 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -67,7 +67,7 @@ using ::testing::HasSubstr; class FirestoreTest : public FirestoreIntegrationTest { protected: - const std::string GetFirestoreDatabaseId(Firestore* firestore) { + const std::string& GetFirestoreDatabaseId(Firestore* firestore) { return GetInternal(firestore)->database_name(); } }; diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 144d4909c..b5484c684 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include "app/meta/move.h" #include "app/src/cleanup_notifier.h" @@ -201,7 +202,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, FirestoreMap::key_type key = MakeKey(firestore->app(), firestore->internal_->database_name()); - FirestoreCache()->emplace(key, firestore); + FirestoreCache()->emplace(std::move(key), firestore); return firestore; } @@ -242,6 +243,8 @@ void Firestore::DeleteInternal() { if (!internal_) return; App* my_app = app(); + // Store the database id before deleting the firestore instance. + const std::string database_id = internal_->database_name(); // Only need to unregister if internal_ is initialized. if (internal_->initialized()) { @@ -262,8 +265,6 @@ void Firestore::DeleteInternal() { // Force cleanup to happen first. internal_->cleanup().CleanupAll(); - // Store the database id before deleting the firestore instance. - const std::string database_id = internal_->database_name(); delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache.