Minor cleanup: removed the database's SafeRef, since the repo already has one, and the repo can wrap things in a safe ref lock itself. Having two ways to lock things is redundant when everything is being routed through Repo anyway.

PiperOrigin-RevId: 256103848

Author: amablue

app/rest/transport_mock.cc

@@ -17,7 +17,7 @@
 #include "app/rest/transport_mock.h"
 #include <string>
 #include "app/rest/util.h"
-#include "firebase/testing/cppsdk/config_desktop.h"
+#include "testing/config_desktop.h"
 
 namespace firebase {
 namespace rest {

database/src/desktop/core/repo.cc

@@ -296,25 +296,25 @@ void Repo::PurgeOutstandingWrites() {
 // triggered.
 class SetValueResponse : public connection::Response {
  public:
-  SetValueResponse(const DatabaseInternal::ThisRef& database, const Path& path,
+  SetValueResponse(const Repo::ThisRef& repo, const Path& path,
                    WriteId write_id, ReferenceCountedFutureImpl* api,
                    SafeFutureHandle<void> handle, ResponseCallback callback)
       : connection::Response(callback),
-        database_ref_(database),
+        repo_ref_(repo),
         path_(path),
         write_id_(write_id),
         api_(api),
         handle_(handle) {}
 
-  DatabaseInternal::ThisRef& database_ref() { return database_ref_; }
+  Repo::ThisRef& repo_ref() { return repo_ref_; }
   const Path& path() { return path_; }
   WriteId write_id() { return write_id_; }
   ReferenceCountedFutureImpl* api() { return api_; }
   SafeFutureHandle<void> handle() { return handle_; }
 
  private:
-  // Database reference
-  DatabaseInternal::ThisRef database_ref_;
+  // Repo reference
+  Repo::ThisRef repo_ref_;
 
   // Database path for this write request
   Path path_;
@@ -339,22 +339,21 @@ void Repo::SetValue(const Path& path, const Variant& new_data_unresolved,
       kPersist);
   PostEvents(events);
 
-  connection_->Put(
-      path, new_data_unresolved,
-      MakeShared<SetValueResponse>(
-          DatabaseInternal::ThisRef(database_), path, write_id, api, handle,
-          [](const connection::ResponsePtr& ptr) {
-            auto* response = static_cast<SetValueResponse*>(ptr.get());
-            DatabaseInternal::ThisRefLock lock(&response->database_ref());
-            DatabaseInternal* database = lock.GetReference();
-            Repo* repo = database->repo();
-            repo->AckWriteAndRerunTransactions(response->write_id(),
-                                               response->path(),
-                                               response->GetErrorCode());
-            response->api()->Complete(
-                response->handle(), response->GetErrorCode(),
-                GetErrorMessage(response->GetErrorCode()));
-          }));
+  connection_->Put(path, new_data_unresolved,
+                   MakeShared<SetValueResponse>(
+                       Repo::ThisRef(this), path, write_id, api, handle,
+                       [](const connection::ResponsePtr& ptr) {
+                         auto* response =
+                             static_cast<SetValueResponse*>(ptr.get());
+                         Repo::ThisRefLock lock(&response->repo_ref());
+                         Repo* repo = lock.GetReference();
+                         repo->AckWriteAndRerunTransactions(
+                             response->write_id(), response->path(),
+                             response->GetErrorCode());
+                         response->api()->Complete(
+                             response->handle(), response->GetErrorCode(),
+                             GetErrorMessage(response->GetErrorCode()));
+                       }));
 
   Path affected_path = AbortTransactions(path, kErrorOverriddenBySet);
   RerunTransactions(affected_path);
@@ -379,22 +378,21 @@ void Repo::UpdateChildren(const Path& path, const Variant& data,
       path, updates, resolved, write_id, kPersist);
   PostEvents(events);
 
-  connection_->Merge(
-      path, data,
-      MakeShared<SetValueResponse>(
-          DatabaseInternal::ThisRef(database_), path, write_id, api, handle,
-          [](const connection::ResponsePtr& ptr) {
-            auto* response = static_cast<SetValueResponse*>(ptr.get());
-            DatabaseInternal::ThisRefLock lock(&response->database_ref());
-            DatabaseInternal* database = lock.GetReference();
-            Repo* repo = database->repo();
-            repo->AckWriteAndRerunTransactions(response->write_id(),
-                                               response->path(),
-                                               response->GetErrorCode());
-            response->api()->Complete(
-                response->handle(), response->GetErrorCode(),
-                GetErrorMessage(response->GetErrorCode()));
-          }));
+  connection_->Merge(path, data,
+                     MakeShared<SetValueResponse>(
+                         Repo::ThisRef(this), path, write_id, api, handle,
+                         [](const connection::ResponsePtr& ptr) {
+                           auto* response =
+                               static_cast<SetValueResponse*>(ptr.get());
+                           Repo::ThisRefLock lock(&response->repo_ref());
+                           Repo* repo = lock.GetReference();
+                           repo->AckWriteAndRerunTransactions(
+                               response->write_id(), response->path(),
+                               response->GetErrorCode());
+                           response->api()->Complete(
+                               response->handle(), response->GetErrorCode(),
+                               GetErrorMessage(response->GetErrorCode()));
+                         }));
 
   updates.write_tree().CallOnEach(
       Path(), [this](const Path& path_from_root, const Variant& variant) {

database/src/desktop/database_desktop.cc

@@ -16,6 +16,7 @@
 
 #include <queue>
 #include <stack>
+
 #include "app/memory/shared_ptr.h"
 #include "app/src/assert.h"
 #include "app/src/function_registry.h"
@@ -67,7 +68,6 @@ DatabaseInternal::DatabaseInternal(App* app, const char* url)
     : app_(app),
       future_manager_(),
       cleanup_(),
-      safe_this_(this),
       constructor_url_(url),
       repo_(app, this, url),
       log_level_(kLogLevelInfo) {
@@ -81,9 +81,6 @@ DatabaseInternal::DatabaseInternal(App* app, const char* url)
 
 DatabaseInternal::~DatabaseInternal() {
   cleanup_.CleanupAll();
-  // Clear safe reference immediately so that scheduled callback can skip
-  // executing code which requires reference to this.
-  safe_this_.ClearReference();
 
   // If initialization failed, there is nothing to clean up.
   if (app_ == nullptr) return;
@@ -142,35 +139,35 @@ DatabaseReference DatabaseInternal::GetReferenceFromUrl(const char* url) const {
 
 void DatabaseInternal::GoOffline() {
   Repo::scheduler().Schedule(NewCallback(
-      [](ThisRef ref) {
-        ThisRefLock lock(&ref);
+      [](Repo::ThisRef ref) {
+        Repo::ThisRefLock lock(&ref);
         if (lock.GetReference() != nullptr) {
-          lock.GetReference()->repo_.connection()->Interrupt();
+          lock.GetReference()->connection()->Interrupt();
         }
       },
-      safe_this_));
+      repo_.this_ref()));
 }
 
 void DatabaseInternal::GoOnline() {
   Repo::scheduler().Schedule(NewCallback(
-      [](ThisRef ref) {
-        ThisRefLock lock(&ref);
+      [](Repo::ThisRef ref) {
+        Repo::ThisRefLock lock(&ref);
         if (lock.GetReference() != nullptr) {
-          lock.GetReference()->repo_.connection()->Resume();
+          lock.GetReference()->connection()->Resume();
         }
       },
-      safe_this_));
+      repo_.this_ref()));
 }
 
 void DatabaseInternal::PurgeOutstandingWrites() {
   Repo::scheduler().Schedule(NewCallback(
-      [](ThisRef ref) {
-        ThisRefLock lock(&ref);
+      [](Repo::ThisRef ref) {
+        Repo::ThisRefLock lock(&ref);
         if (lock.GetReference() != nullptr) {
-          lock.GetReference()->repo_.PurgeOutstandingWrites();
+          lock.GetReference()->PurgeOutstandingWrites();
         }
       },
-      safe_this_));
+      repo_.this_ref()));
 }
 
 static std::string* g_sdk_version = nullptr;

database/src/desktop/database_desktop.h

@@ -185,10 +185,6 @@ class DatabaseInternal {
   // Needed to generate names that are guarenteed to be unique.
   PushChildNameGenerator name_generator_;
 
-  // Safe reference to this.  Set in constructor and cleared in destructor
-  // Should be safe to be copied to any thread.
-  ThisRef safe_this_;
-
   // The url passed to the constructor (or "" if none was passed).
   // We keep it so that we can find the database in our cache.
   std::string constructor_url_;

testing/TickerExample.java

@@ -0,0 +1,22 @@
+package com.google.firebase.testing.cppsdk;
+
+/** An example subclass of {@link TickerObserver} for unit-testing the ticker mechanism. */
+public final class TickerExample implements TickerObserver {
+  // When the callback should happen.
+  private final long delay;
+
+  public TickerExample(long delay) {
+    this.delay = delay;
+    TickerAndroid.register(this);
+  }
+
+  @Override
+  public void elapse() {
+    // We pass in ticker directly to save a few JNI calls. A fake should implement the logic right
+    // here without using native function. We use native function here in order to consolidate logic
+    // in the unit test .cc file.
+    nativeFunction(TickerAndroid.now(), delay);
+  }
+
+  private native void nativeFunction(long ticker, long delay);
+}

testing/config.cc

@@ -0,0 +1,30 @@
+#include "testing/config.h"
+
+#include "base/logging.h"
+#include "testing/testdata_config_resource.h"
+#include "flatbuffers/idl.h"
+
+namespace firebase {
+namespace testing {
+namespace cppsdk {
+
+const char* kSchemaFilePath = "";
+
+// Replace the current test data.
+void ConfigSet(const char* test_data_in_json) {
+  flatbuffers::Parser parser;
+  const char* schema_str =
+      reinterpret_cast<const char*>(testdata_config_resource_data);
+  CHECK(parser.Parse(schema_str)) << "Failed to parse schema:" << parser.error_;
+  CHECK(parser.Parse(test_data_in_json)) << "Invalid JSON:" << parser.error_;
+
+  // Assign
+  internal::ConfigSetImpl(parser.builder_.GetBufferPointer(),
+                          parser.builder_.GetSize());
+}
+
+void ConfigReset() { internal::ConfigSetImpl(nullptr, 0); }
+
+}  // namespace cppsdk
+}  // namespace testing
+}  // namespace firebase

testing/config.h

@@ -0,0 +1,30 @@
+// Provide a way for C++ unit test to send test data to fakes. The fakes will
+// behave as specified in order to mimic different real world scenarios. See
+// goto/firebasecppunittest for more details.
+#ifndef FIREBASE_TESTING_CPPSDK_CONFIG_H_
+#define FIREBASE_TESTING_CPPSDK_CONFIG_H_
+
+#include <cstdint>
+#include "testing/testdata_config_generated.h"
+
+namespace firebase {
+namespace testing {
+namespace cppsdk {
+
+// Replace the current test data.
+void ConfigSet(const char* test_data_in_json);
+
+// Reset (free up) the current test data.
+void ConfigReset();
+
+namespace internal {
+// Platform-specific function to send the test data.
+void ConfigSetImpl(const uint8_t* test_data_binary,
+                   flatbuffers::uoffset_t size);
+}  // namespace internal
+
+}  // namespace cppsdk
+}  // namespace testing
+}  // namespace firebase
+
+#endif  // FIREBASE_TESTING_CPPSDK_CONFIG_H_

testing/config_android.cc

@@ -0,0 +1,35 @@
+#include <jni.h>
+
+#include "base/logging.h"
+#include "testing/config.h"
+#include "testing/run_all_tests.h"
+#include "testing/testdata_config_generated.h"
+#include "testing/util_android.h"
+
+namespace firebase {
+namespace testing {
+namespace cppsdk {
+namespace internal {
+void ConfigSetImpl(const uint8_t* test_data_binary,
+                   flatbuffers::uoffset_t size) {
+  JNIEnv* android_jni_env = GetTestJniEnv();
+  jbyteArray j_test_data = nullptr;
+  if (test_data_binary != nullptr && size > 0) {
+    j_test_data = android_jni_env->NewByteArray(size);
+    android_jni_env->SetByteArrayRegion(
+        j_test_data, 0, size,
+        reinterpret_cast<jbyte*>(const_cast<uint8_t*>(test_data_binary)));
+  }
+  jclass cls = android_jni_env->FindClass(
+      "com/google/testing/ConfigAndroid");
+  android_jni_env->CallStaticVoidMethod(
+      cls, android_jni_env->GetStaticMethodID(cls, "setImpl", "([B)V"),
+      j_test_data);
+  util::CheckAndClearException(android_jni_env);
+  android_jni_env->DeleteLocalRef(j_test_data);
+  android_jni_env->DeleteLocalRef(cls);
+}
+}  // namespace internal
+}  // namespace cppsdk
+}  // namespace testing
+}  // namespace firebase

testing/config_desktop.cc

@@ -0,0 +1,58 @@
+#include "testing/config_desktop.h"
+#include "testing/config.h"
+
+#include <cstdint>
+
+#include "base/logging.h"
+#include "app/src/mutex.h"
+#include "testing/testdata_config_generated.h"
+#include "flatbuffers/idl.h"
+
+namespace firebase {
+namespace testing {
+namespace cppsdk {
+
+// We keep this raw data in order to be able to return it back for merge.
+static uint8_t* g_test_data_config = nullptr;
+// Mutex to make sure we don't delete the pointer while someone else is reading.
+static Mutex testing_mutex;  // NOLINT
+// List of all the test data we've used so far, so we can clean it up later.
+// (we can't delete it when it's replaced because old threads might still
+// be looking at it.)
+static std::vector<uint8_t*> g_all_test_data;  // NOLINT
+
+const ConfigRow* ConfigGet(const char* fake) {
+  MutexLock lock(testing_mutex);
+  CHECK(g_test_data_config != nullptr) << "No test data at all";
+  const TestDataConfig* config = GetTestDataConfig(g_test_data_config);
+  // LookupByKey() does not work because the data passed in may not conform. So
+  // we just iterate over the test data.
+  for (const ConfigRow* row : *(config->config())) {
+    if (strcmp(row->fake()->c_str(), fake) == 0) {
+      return row;
+    }
+  }
+  return nullptr;
+}
+
+namespace internal {
+void ConfigSetImpl(const uint8_t* test_data_binary,
+                   flatbuffers::uoffset_t size) {
+  MutexLock lock(testing_mutex);
+  if (test_data_binary != nullptr && size > 0) {
+    g_test_data_config = new uint8_t[size];
+    memcpy(g_test_data_config, test_data_binary, size);
+    g_all_test_data.push_back(g_test_data_config);
+  } else {
+    g_test_data_config = nullptr;
+    for (int i = 0; i < g_all_test_data.size(); i++) {
+      delete[] g_all_test_data[i];
+    }
+    g_all_test_data.clear();
+  }
+}
+}  // namespace internal
+
+}  // namespace cppsdk
+}  // namespace testing
+}  // namespace firebase

testing/config_desktop.h

@@ -0,0 +1,17 @@
+#ifndef FIREBASE_TESTING_CPPSDK_CONFIG_DESKTOP_H_
+#define FIREBASE_TESTING_CPPSDK_CONFIG_DESKTOP_H_
+
+#include "testing/testdata_config_generated.h"
+
+namespace firebase {
+namespace testing {
+namespace cppsdk {
+
+// Returns the test data for the specific fake.
+const ConfigRow* ConfigGet(const char* fake);
+
+}  // namespace cppsdk
+}  // namespace testing
+}  // namespace firebase
+
+#endif  // FIREBASE_TESTING_CPPSDK_CONFIG_DESKTOP_H_