Make FirestoreInternal store a pointer to the corresponding public Firestore instance.

The existing setup instead stores a pointer to `App` and uses it to retrieve the corresponding `Firestore` from the "main" cache of Firestore instances. This is unnecessarily tricky and falls apart in tests, which use their own cache of Firestore instances not synchronized with the "main" cache. What happens is that a call to e.g. `DocumentReference::firestore()` tries to retrieve an instance by App from the main cache, but because the main cache is empty, it leads to creation of a new Firestore instance.

PiperOrigin-RevId: 295244529

Author: var-const

firestore/src/android/document_reference_android.cc

@@ -52,11 +52,8 @@ METHOD_LOOKUP_DEFINITION(document_reference,
                          DOCUMENT_REFERENCE_METHODS)
 
 Firestore* DocumentReferenceInternal::firestore() {
-  App* app = firestore_->app();
-  FIREBASE_ASSERT(app != nullptr);
-  Firestore* firestore = Firestore::GetInstance(app);
-  FIREBASE_ASSERT(firestore != nullptr);
-  return firestore;
+  FIREBASE_ASSERT(firestore_->firestore_public() != nullptr);
+  return firestore_->firestore_public();
 }
 
 const std::string& DocumentReferenceInternal::id() const {

firestore/src/android/document_snapshot_android.cc

@@ -44,7 +44,8 @@ METHOD_LOOKUP_DEFINITION(document_snapshot,
                          DOCUMENT_SNAPSHOT_METHODS)
 
 Firestore* DocumentSnapshotInternal::firestore() const {
-  return Firestore::GetInstance(firestore_->app());
+  FIREBASE_ASSERT(firestore_->firestore_public() != nullptr);
+  return firestore_->firestore_public();
 }
 
 const std::string& DocumentSnapshotInternal::id() const {

firestore/src/android/firestore_android.h

@@ -22,10 +22,11 @@
 namespace firebase {
 namespace firestore {
 
+class Firestore;
+class ListenerRegistrationInternal;
 class Transaction;
 class TransactionFunction;
 class WriteBatch;
-class ListenerRegistrationInternal;
 
 // Used for registering global callbacks. See
 // firebase::util::RegisterCallbackOnTask for context.
@@ -52,6 +53,7 @@ class FirestoreInternal {
  public:
   using ApiType = Firestore;
 
+  // Note: call `set_firestore_public` immediately after construction.
   explicit FirestoreInternal(App* app);
   ~FirestoreInternal();
 
@@ -142,6 +144,13 @@ class FirestoreInternal {
     return static_cast<InternalType*>(value.internal_);
   }
 
+  void set_firestore_public(Firestore* firestore_public) {
+    firestore_public_ = firestore_public;
+  }
+
+  Firestore* firestore_public() { return firestore_public_; }
+  const Firestore* firestore_public() const { return firestore_public_; }
+
  private:
   // Gets the reference-counted Future implementation of this instance, which
   // can be used to create a Future.
@@ -165,6 +174,7 @@ class FirestoreInternal {
   static int initialize_count_;
 
   App* app_ = nullptr;
+  Firestore* firestore_public_ = nullptr;
   // Java Firestore global ref.
   jobject obj_;
 

firestore/src/android/query_android.cc

@@ -24,7 +24,8 @@ METHOD_LOOKUP_DEFINITION(query,
                          QUERY_METHODS)
 
 Firestore* QueryInternal::firestore() {
-  return Firestore::GetInstance(firestore_->app());
+  FIREBASE_ASSERT(firestore_->firestore_public() != nullptr);
+  return firestore_->firestore_public();
 }
 
 Query QueryInternal::OrderBy(const FieldPath& field,

firestore/src/common/firestore.cc

@@ -121,6 +121,8 @@ Firestore::Firestore(::firebase::App* app)
 Firestore::Firestore(FirestoreInternal* internal)
     // TODO(wuandy): use make_unique once it is supported for our build here.
     : internal_(internal) {
+  internal_->set_firestore_public(this);
+
   if (internal_->initialized()) {
     CleanupNotifier* app_notifier = CleanupNotifier::FindByOwner(app());
     assert(app_notifier);

firestore/src/ios/firestore_ios.cc

@@ -52,7 +52,7 @@ FirestoreInternal::FirestoreInternal(App* app)
 FirestoreInternal::FirestoreInternal(
     App* app, std::unique_ptr<CredentialsProvider> credentials)
     : app_(NOT_NULL(app)),
-      firestore_(CreateFirestore(app, std::move(credentials))) {
+      firestore_core_(CreateFirestore(app, std::move(credentials))) {
   ApplyDefaultSettings();
 }
 
@@ -68,25 +68,25 @@ std::shared_ptr<api::Firestore> FirestoreInternal::CreateFirestore(
 
 CollectionReference FirestoreInternal::Collection(
     const char* collection_path) const {
-  auto result = firestore_->GetCollection(collection_path);
+  auto result = firestore_core_->GetCollection(collection_path);
   return MakePublic(std::move(result));
 }
 
 DocumentReference FirestoreInternal::Document(const char* document_path) const {
-  auto result = firestore_->GetDocument(document_path);
+  auto result = firestore_core_->GetDocument(document_path);
   return MakePublic(std::move(result));
 }
 
 Query FirestoreInternal::CollectionGroup(const char* collection_id) const {
-  core::Query core_query = firestore_->GetCollectionGroup(collection_id);
-  api::Query api_query(std::move(core_query), firestore_);
+  core::Query core_query = firestore_core_->GetCollectionGroup(collection_id);
+  api::Query api_query(std::move(core_query), firestore_core_);
   return MakePublic(std::move(api_query));
 }
 
 Settings FirestoreInternal::settings() const {
   Settings result;
 
-  const api::Settings& from = firestore_->settings();
+  const api::Settings& from = firestore_core_->settings();
   result.set_host(from.host());
   result.set_ssl_enabled(from.ssl_enabled());
   result.set_persistence_enabled(from.persistence_enabled());
@@ -105,14 +105,14 @@ void FirestoreInternal::set_settings(const Settings& from) {
   // TODO(varconst): implement `cache_size_bytes` in public `Settings` and
   // uncomment.
   // settings.set_cache_size_bytes(from.cache_size_bytes());
-  firestore_->set_settings(settings);
+  firestore_core_->set_settings(settings);
 
   std::unique_ptr<Executor> user_executor = from.CreateExecutor();
-  firestore_->set_user_executor(std::move(user_executor));
+  firestore_core_->set_user_executor(std::move(user_executor));
 }
 
 WriteBatch FirestoreInternal::batch() const {
-  return MakePublic(firestore_->GetBatch());
+  return MakePublic(firestore_core_->GetBatch());
 }
 
 Future<void> FirestoreInternal::RunTransaction(TransactionFunction* update) {
@@ -170,8 +170,8 @@ Future<void> FirestoreInternal::RunTransaction(
     }
   };
 
-  firestore_->RunTransaction(std::move(update_callback),
-                             std::move(final_result_callback));
+  firestore_core_->RunTransaction(std::move(update_callback),
+                                  std::move(final_result_callback));
 
   return promise.future();
 }
@@ -183,7 +183,7 @@ Future<void> FirestoreInternal::RunTransactionLastResult() {
 Future<void> FirestoreInternal::DisableNetwork() {
   auto promise =
       promise_factory_.CreatePromise<void>(AsyncApi::kDisableNetwork);
-  firestore_->DisableNetwork(StatusCallbackWithPromise(promise));
+  firestore_core_->DisableNetwork(StatusCallbackWithPromise(promise));
   return promise.future();
 }
 
@@ -193,7 +193,7 @@ Future<void> FirestoreInternal::DisableNetworkLastResult() {
 
 Future<void> FirestoreInternal::EnableNetwork() {
   auto promise = promise_factory_.CreatePromise<void>(AsyncApi::kEnableNetwork);
-  firestore_->EnableNetwork(StatusCallbackWithPromise(promise));
+  firestore_core_->EnableNetwork(StatusCallbackWithPromise(promise));
   return promise.future();
 }
 
@@ -204,7 +204,7 @@ Future<void> FirestoreInternal::EnableNetworkLastResult() {
 Future<void> FirestoreInternal::Terminate() {
   auto promise = promise_factory_.CreatePromise<void>(AsyncApi::kTerminate);
   ClearListeners();
-  firestore_->Terminate(StatusCallbackWithPromise(promise));
+  firestore_core_->Terminate(StatusCallbackWithPromise(promise));
   return promise.future();
 }
 
@@ -215,7 +215,7 @@ Future<void> FirestoreInternal::TerminateLastResult() {
 Future<void> FirestoreInternal::WaitForPendingWrites() {
   auto promise =
       promise_factory_.CreatePromise<void>(AsyncApi::kWaitForPendingWrites);
-  firestore_->WaitForPendingWrites(StatusCallbackWithPromise(promise));
+  firestore_core_->WaitForPendingWrites(StatusCallbackWithPromise(promise));
   return promise.future();
 }
 
@@ -226,7 +226,7 @@ Future<void> FirestoreInternal::WaitForPendingWritesLastResult() {
 Future<void> FirestoreInternal::ClearPersistence() {
   auto promise =
       promise_factory_.CreatePromise<void>(AsyncApi::kClearPersistence);
-  firestore_->ClearPersistence(StatusCallbackWithPromise(promise));
+  firestore_core_->ClearPersistence(StatusCallbackWithPromise(promise));
   return promise.future();
 }
 
@@ -247,14 +247,14 @@ ListenerRegistration FirestoreInternal::AddSnapshotsInSyncListener(
   std::function<void()> listener_function = [listener] {
     listener->OnEvent(Error::Ok);
   };
-  auto result = firestore_->AddSnapshotsInSyncListener(
+  auto result = firestore_core_->AddSnapshotsInSyncListener(
       ListenerWithCallback(std::move(listener_function)));
   return MakePublic(std::move(result), this);
 }
 
 ListenerRegistration FirestoreInternal::AddSnapshotsInSyncListener(
     std::function<void()> callback) {
-  auto result = firestore_->AddSnapshotsInSyncListener(
+  auto result = firestore_core_->AddSnapshotsInSyncListener(
       ListenerWithCallback(std::move(callback)));
   return MakePublic(std::move(result), this);
 }

firestore/src/ios/firestore_ios.h

@@ -21,13 +21,15 @@
 namespace firebase {
 namespace firestore {
 
+class Firestore;
 class ListenerRegistrationInternal;
 class Transaction;
 class TransactionFunction;
 class WriteBatch;
 
 class FirestoreInternal {
  public:
+  // Note: call `set_firestore_public` immediately after construction.
   explicit FirestoreInternal(App* app);
   ~FirestoreInternal();
 
@@ -83,18 +85,23 @@ class FirestoreInternal {
       std::function<void()> callback);
 
   const model::DatabaseId& database_id() const {
-    return firestore_->database_id();
+    return firestore_core_->database_id();
   }
 
   // Manages the ListenerRegistrationInternal objects.
   void RegisterListenerRegistration(ListenerRegistrationInternal* registration);
   void UnregisterListenerRegistration(
       ListenerRegistrationInternal* registration);
 
-  // TODO(varconst): this method should become unnecessary once
-  // `api::Transaction::Lookup()` starts returning C++ documents.
+  void set_firestore_public(Firestore* firestore_public) {
+    firestore_public_ = firestore_public;
+  }
+
+  Firestore* firestore_public() { return firestore_public_; }
+  const Firestore* firestore_public() const { return firestore_public_; }
+
   const std::shared_ptr<api::Firestore>& firestore_core() const {
-    return firestore_;
+    return firestore_core_;
   }
 
  private:
@@ -127,7 +134,8 @@ class FirestoreInternal {
   void ApplyDefaultSettings();
 
   App* app_ = nullptr;
-  std::shared_ptr<api::Firestore> firestore_;
+  Firestore* firestore_public_ = nullptr;
+  std::shared_ptr<api::Firestore> firestore_core_;
 
   CleanupNotifier cleanup_;
 

firestore/src/ios/util_ios.h

@@ -16,8 +16,7 @@ FirestoreInternal* GetFirestoreInternal(T* object) {
 
 template <typename T>
 Firestore* GetFirestore(T* object) {
-  FirestoreInternal* internal = GetFirestoreInternal(object);
-  return Firestore::GetInstance(internal->app());
+  return GetFirestoreInternal(object)->firestore_public();
 }
 
 }  // namespace firestore

firestore/src/stub/firestore_stub.h

@@ -120,6 +120,8 @@ class FirestoreInternal {
     return static_cast<InternalType*>(value.internal_);
   }
 
+  void set_firestore_public(Firestore*) {}
+
  private:
   CleanupNotifier cleanup_;
   App* app_;