From ef27076e4a10d931b8afa0a25bdcdff7e375292e Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 3 May 2023 11:03:17 -0400 Subject: [PATCH 01/22] creating new settings files --- firestore/src/common/local_cache_settings.cc | 65 ++++++++++++++++++ .../firebase/firestore/local_cache_settings.h | 67 +++++++++++++++++++ .../src/main/local_cache_settings_main.h | 62 +++++++++++++++++ 3 files changed, 194 insertions(+) create mode 100644 firestore/src/common/local_cache_settings.cc create mode 100644 firestore/src/include/firebase/firestore/local_cache_settings.h create mode 100644 firestore/src/main/local_cache_settings_main.h diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc new file mode 100644 index 0000000000..d7a82b6696 --- /dev/null +++ b/firestore/src/common/local_cache_settings.cc @@ -0,0 +1,65 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "firestore/src/include/firebase/firestore/local_cache_settings.h" +#include + +#include "firestore/src/common/hard_assert_common.h" +#if defined(__ANDROID__) +#else +#include "firestore/src/main/local_cache_settings_main.h" +#endif // defined(__ANDROID__) + +namespace firebase { +namespace firestore { + +PersistentCacheSettings PersistentCacheSettings::Create() { return {}; } + +PersistentCacheSettings::PersistentCacheSettings() { + settings_internal_ = std::make_unique( + CorePersistentSettings{}); +} + +PersistentCacheSettings::~PersistentCacheSettings() { + settings_internal_.reset(); +} + +PersistentCacheSettings::PersistentCacheSettings( + const PersistentCacheSettingsInternal& other) { + settings_internal_ = std::make_unique(other); +} + +PersistentCacheSettings PersistentCacheSettings::WithSizeBytes( + int64_t size) const { + return {PersistentCacheSettingsInternal{ + settings_internal_->core_settings().WithSizeBytes(size)}}; +} + +MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } + +MemoryCacheSettings::MemoryCacheSettings() { + settings_internal_ = + std::make_unique(CoreMemorySettings{}); +} + +MemoryCacheSettings::~MemoryCacheSettings() { settings_internal_.reset(); } + +MemoryCacheSettings::MemoryCacheSettings( + const MemoryCacheSettingsInternal& other) { + settings_internal_ = std::make_unique(other); +} +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h new file mode 100644 index 0000000000..4a48607742 --- /dev/null +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -0,0 +1,67 @@ +/* + * Copyright 2018 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_LOCAL_CACHE_SETTINGS_H_ +#define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_LOCAL_CACHE_SETTINGS_H_ + +#include +#include + +#include "Firestore/core/src/api/settings.h" + +namespace firebase { +namespace firestore { + +using CoreMemorySettings = api::MemoryCacheSettings; +using CorePersistentSettings = api::PersistentCacheSettings; + +class PersistentCacheSettingsInternal; +class MemoryCacheSettingsInternal; + +class LocalCacheSettings { + virtual ~LocalCacheSettings() = 0; +}; + +class PersistentCacheSettings : public LocalCacheSettings { + public: + static PersistentCacheSettings Create(); + + PersistentCacheSettings WithSizeBytes(int64_t size) const; + + private: + PersistentCacheSettings(); + PersistentCacheSettings(const PersistentCacheSettingsInternal& other); + ~PersistentCacheSettings(); + + std::unique_ptr settings_internal_; +}; + +class MemoryCacheSettings : public LocalCacheSettings { + public: + static MemoryCacheSettings Create(); + + private: + MemoryCacheSettings(); + MemoryCacheSettings(const MemoryCacheSettingsInternal& other); + ~MemoryCacheSettings(); + + std::unique_ptr settings_internal_; +}; + +} // namespace firestore +} // namespace firebase + +#endif // FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_LOCAL_CACHE_SETTINGS_H_ diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h new file mode 100644 index 0000000000..4d602001a0 --- /dev/null +++ b/firestore/src/main/local_cache_settings_main.h @@ -0,0 +1,62 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETINGS_MAIN_H_ +#define FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETINGS_MAIN_H_ + +#include + +#include "Firestore/core/src/api/settings.h" + +namespace firebase { +namespace firestore { + +using CoreMemorySettings = api::MemoryCacheSettings; +using CorePersistentSettings = api::PersistentCacheSettings; + +class LocalCacheSettingsInternal {}; + +class PersistentCacheSettingsInternal : public LocalCacheSettingsInternal { + public: + explicit PersistentCacheSettingsInternal( + const CorePersistentSettings& core_settings) + : settings_(std::move(core_settings)) {} + const CorePersistentSettings& core_settings() { return settings_; } + void set_core_settings(const CorePersistentSettings& settings) { + settings_ = settings; + } + + private: + CorePersistentSettings settings_; +}; + +class MemoryCacheSettingsInternal : public LocalCacheSettingsInternal { + public: + explicit MemoryCacheSettingsInternal(const CoreMemorySettings& core_settings) + : settings_(std::move(core_settings)) {} + const CoreMemorySettings& core_settings() { return settings_; } + void set_core_settings(const CoreMemorySettings& settings) { + settings_ = settings; + } + + private: + CoreMemorySettings settings_; +}; + +} // namespace firestore +} // namespace firebase + +#endif // FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETINGS_MAIN_H_ From 2021a270d2c582f607a5b972de0b2b80ca0b73a7 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Thu, 4 May 2023 11:04:32 -0400 Subject: [PATCH 02/22] Compiling..pheww --- firestore/CMakeLists.txt | 1 + firestore/src/common/local_cache_settings.cc | 9 +++++ firestore/src/common/settings.cc | 38 +++++++++++++++++++ .../firebase/firestore/local_cache_settings.h | 38 +++++++++++++++---- .../src/include/firebase/firestore/settings.h | 9 +++++ firestore/src/main/firestore_main.cc | 10 ++++- firestore/src/main/firestore_main.h | 1 + .../src/main/local_cache_settings_main.h | 5 ++- 8 files changed, 100 insertions(+), 11 deletions(-) diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index 762d217554..cf5c239f88 100644 --- a/firestore/CMakeLists.txt +++ b/firestore/CMakeLists.txt @@ -41,6 +41,7 @@ set(common_SRCS src/common/hard_assert_common.h src/common/listener_registration.cc src/common/load_bundle_task_progress.cc + src/common/local_cache_settings.cc src/common/macros.h src/common/query.cc src/common/query_snapshot.cc diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index d7a82b6696..cde2ab2ab9 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -48,6 +48,10 @@ PersistentCacheSettings PersistentCacheSettings::WithSizeBytes( settings_internal_->core_settings().WithSizeBytes(size)}}; } +const CoreCacheSettings& PersistentCacheSettings::core_cache_settings() const { + return settings_internal_->core_settings(); +} + MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } MemoryCacheSettings::MemoryCacheSettings() { @@ -61,5 +65,10 @@ MemoryCacheSettings::MemoryCacheSettings( const MemoryCacheSettingsInternal& other) { settings_internal_ = std::make_unique(other); } + +const CoreCacheSettings& MemoryCacheSettings::core_cache_settings() const { + return settings_internal_->core_settings(); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 3e1bfeebfb..283f3c66ed 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -16,10 +16,14 @@ #include "firestore/src/include/firebase/firestore/settings.h" +#include #include #include +#include "Firestore/core/src/api/settings.h" +#include "Firestore/core/src/util/hard_assert.h" #include "app/meta/move.h" +#include "firebase/firestore/local_cache_settings.h" #if !defined(__ANDROID__) #include "Firestore/core/src/util/executor.h" @@ -51,12 +55,46 @@ void Settings::set_host(std::string host) { host_ = firebase::Move(host); } void Settings::set_ssl_enabled(bool enabled) { ssl_enabled_ = enabled; } +std::shared_ptr Settings::local_cache_settings() const { + if (used_legacy_cache_settings_) { + if (is_persistence_enabled()) { + return std::make_shared( + *PersistentCacheSettings::Create() + .WithSizeBytes(cache_size_bytes()) + .settings_internal_); + } else { + return std::make_shared( + *MemoryCacheSettings::Create().settings_internal_); + } + } else if (local_cache_settings_ != nullptr) { + return local_cache_settings_; + } + + return std::make_shared( + *PersistentCacheSettings::Create().settings_internal_); +} + +void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { + HARD_ASSERT(!used_legacy_cache_settings_, ""); + if (cache.kind() == api::LocalCacheSettings::Kind::kPersistent) { + local_cache_settings_ = std::make_shared( + *static_cast(cache).settings_internal_); + } else { + local_cache_settings_ = std::make_shared( + *static_cast(cache).settings_internal_); + } +} + void Settings::set_persistence_enabled(bool enabled) { + HARD_ASSERT(local_cache_settings() == nullptr, ""); persistence_enabled_ = enabled; + used_legacy_cache_settings_ = true; } void Settings::set_cache_size_bytes(int64_t value) { + HARD_ASSERT(local_cache_settings() == nullptr, ""); cache_size_bytes_ = value; + used_legacy_cache_settings_ = true; } std::string Settings::ToString() const { diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 4a48607742..7a032fd2e3 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -21,42 +21,66 @@ #include #include "Firestore/core/src/api/settings.h" +#include "firestore/src/main/firestore_main.h" namespace firebase { namespace firestore { +using CoreCacheSettings = api::LocalCacheSettings; using CoreMemorySettings = api::MemoryCacheSettings; using CorePersistentSettings = api::PersistentCacheSettings; +class Settings; class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; class LocalCacheSettings { - virtual ~LocalCacheSettings() = 0; + public: + virtual api::LocalCacheSettings::Kind kind() const = 0; + virtual ~LocalCacheSettings() = default; + + private: + friend class FirestoreInternal; + + virtual const CoreCacheSettings& core_cache_settings() const = 0; }; -class PersistentCacheSettings : public LocalCacheSettings { +class PersistentCacheSettings final : public LocalCacheSettings { public: static PersistentCacheSettings Create(); + ~PersistentCacheSettings(); + PersistentCacheSettings(const PersistentCacheSettingsInternal& other); PersistentCacheSettings WithSizeBytes(int64_t size) const; + const CoreCacheSettings& core_cache_settings() const override; + api::LocalCacheSettings::Kind kind() const override { + return api::LocalCacheSettings::Kind::kPersistent; + } private: + friend class Settings; + PersistentCacheSettings(); - PersistentCacheSettings(const PersistentCacheSettingsInternal& other); - ~PersistentCacheSettings(); std::unique_ptr settings_internal_; }; -class MemoryCacheSettings : public LocalCacheSettings { +class MemoryCacheSettings final : public LocalCacheSettings { public: static MemoryCacheSettings Create(); + ~MemoryCacheSettings(); + + const CoreCacheSettings& core_cache_settings() const override; + api::LocalCacheSettings::Kind kind() const override { + return api::LocalCacheSettings::Kind::kMemory; + } + + MemoryCacheSettings(const MemoryCacheSettingsInternal& other); private: + friend class Settings; + MemoryCacheSettings(); - MemoryCacheSettings(const MemoryCacheSettingsInternal& other); - ~MemoryCacheSettings(); std::unique_ptr settings_internal_; }; diff --git a/firestore/src/include/firebase/firestore/settings.h b/firestore/src/include/firebase/firestore/settings.h index 8169d9f594..60afcba1ed 100644 --- a/firestore/src/include/firebase/firestore/settings.h +++ b/firestore/src/include/firebase/firestore/settings.h @@ -17,6 +17,7 @@ #ifndef FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_SETTINGS_H_ #define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_SETTINGS_H_ +#include "firebase/firestore/local_cache_settings.h" #if defined(__OBJC__) #include #endif @@ -47,6 +48,7 @@ class Executor; #endif class FirestoreInternal; +class LocalCacheSettings; /** Settings used to configure a Firestore instance. */ class Settings final { @@ -136,6 +138,9 @@ class Settings final { */ void set_ssl_enabled(bool enabled); + std::shared_ptr local_cache_settings() const; + void set_local_cache_settings(const LocalCacheSettings& cache); + /** * Enables or disables local persistent storage. * @@ -213,6 +218,10 @@ class Settings final { std::string host_; bool ssl_enabled_ = true; + + std::shared_ptr local_cache_settings_ = nullptr; + + bool used_legacy_cache_settings_ = false; bool persistence_enabled_ = true; int64_t cache_size_bytes_ = kDefaultCacheSizeBytes; diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 1616e34534..7d4c3728cc 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -23,6 +23,7 @@ #include "Firestore/core/src/api/document_reference.h" #include "Firestore/core/src/api/query_core.h" +#include "Firestore/core/src/api/settings.h" #include "Firestore/core/src/credentials/empty_credentials_provider.h" #include "Firestore/core/src/model/database_id.h" #include "Firestore/core/src/model/resource_path.h" @@ -198,8 +199,13 @@ void FirestoreInternal::set_settings(Settings from) { api::Settings settings; settings.set_host(std::move(from.host())); settings.set_ssl_enabled(from.is_ssl_enabled()); - settings.set_persistence_enabled(from.is_persistence_enabled()); - settings.set_cache_size_bytes(from.cache_size_bytes()); + if (!from.used_legacy_cache_settings_) { + settings.set_local_cache_settings( + from.local_cache_settings()->core_cache_settings()); + } else { + settings.set_persistence_enabled(from.is_persistence_enabled()); + settings.set_cache_size_bytes(from.cache_size_bytes()); + } firestore_core_->set_settings(settings); std::unique_ptr user_executor = from.CreateExecutor(); diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index 49b3779547..9cafb4502d 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -47,6 +47,7 @@ class Firestore; class ListenerRegistrationInternal; class Transaction; class WriteBatch; +class Settings; namespace util { class Executor; diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index 4d602001a0..b24a3a8ee9 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -29,7 +29,8 @@ using CorePersistentSettings = api::PersistentCacheSettings; class LocalCacheSettingsInternal {}; -class PersistentCacheSettingsInternal : public LocalCacheSettingsInternal { +class PersistentCacheSettingsInternal final + : public LocalCacheSettingsInternal { public: explicit PersistentCacheSettingsInternal( const CorePersistentSettings& core_settings) @@ -43,7 +44,7 @@ class PersistentCacheSettingsInternal : public LocalCacheSettingsInternal { CorePersistentSettings settings_; }; -class MemoryCacheSettingsInternal : public LocalCacheSettingsInternal { +class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { public: explicit MemoryCacheSettingsInternal(const CoreMemorySettings& core_settings) : settings_(std::move(core_settings)) {} From 12086e0a2d21e8e51a50637676ea768c65847fa8 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Thu, 4 May 2023 16:32:42 -0400 Subject: [PATCH 03/22] main tests pass. --- .../src/firestore_test.cc | 89 +++++++++++++++++++ firestore/src/common/settings.cc | 22 ++++- .../firebase/firestore/local_cache_settings.h | 5 +- firestore/src/main/firestore_main.cc | 9 +- 4 files changed, 119 insertions(+), 6 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 0569448f7d..4968ba533e 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -20,6 +20,11 @@ #include #include #include +#include "firebase/firestore/document_snapshot.h" +#include "firebase/firestore/field_value.h" +#include "firebase/firestore/local_cache_settings.h" +#include "firebase/firestore/map_field_value.h" +#include "firebase/firestore/source.h" #if defined(__ANDROID__) #include "android/firestore_integration_test_android.h" @@ -1499,6 +1504,90 @@ TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) { EXPECT_EQ(await_clear_persistence.error(), Error::kErrorFailedPrecondition); } +TEST_F(FirestoreIntegrationTest, LegacyCacheConfigForMemoryCacheWorks) { + auto* db = TestFirestore("legacy_memory_cache"); + auto settings = db->settings(); + settings.set_persistence_enabled(false); + db->set_settings(std::move(settings)); + + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); + EXPECT_FALSE(snapshot->is_valid()); + snapshot->id(); +} + +TEST_F(FirestoreIntegrationTest, LegacyCacheConfigForPersistenceCacheWorks) { + auto* db = TestFirestore("legacy_persistent_cache"); + auto settings = db->settings(); + settings.set_persistence_enabled(true); + db->set_settings(std::move(settings)); + + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); + EXPECT_TRUE(snapshot->is_valid()); + EXPECT_THAT(snapshot->GetData(), + ContainerEq(MapFieldValue{{"desc", FieldValue::String("eros")}})); +} + +TEST_F(FirestoreIntegrationTest, NewCacheConfigForMemoryCacheWorks) { + auto* db = TestFirestore("new_memory_cache"); + auto settings = db->settings(); + settings.set_local_cache_settings(MemoryCacheSettings::Create()); + db->set_settings(std::move(settings)); + + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); + EXPECT_FALSE(snapshot->is_valid()); +} + +TEST_F(FirestoreIntegrationTest, NewCacheConfigForPersistenceCacheWorks) { + auto* db = TestFirestore("new_persistent_cache"); + auto settings = db->settings(); + settings.set_local_cache_settings( + PersistentCacheSettings::Create().WithSizeBytes(50 * 1024 * 1024)); + db->set_settings(std::move(settings)); + + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); + EXPECT_TRUE(snapshot->is_valid()); + EXPECT_THAT(snapshot->GetData(), + ContainerEq(MapFieldValue{{"desc", FieldValue::String("eros")}})); +} + +TEST_F(FirestoreIntegrationTest, CannotMixNewAndLegacyCacheConfig) { + { + auto* db = TestFirestore("mixing_1"); + auto settings = db->settings(); + settings.set_local_cache_settings( + PersistentCacheSettings::Create().WithSizeBytes(50 * 1024 * 1024)); + EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error); + } + { + auto* db = TestFirestore("mixing_2"); + auto settings = db->settings(); + settings.set_persistence_enabled(false); + EXPECT_THROW( + settings.set_local_cache_settings(MemoryCacheSettings::Create()), + std::logic_error); + } +} + // Note: this test only exists in C++. TEST_F(FirestoreIntegrationTest, DomainObjectsReferToSameFirestoreInstance) { EXPECT_EQ(TestFirestore(), TestFirestore()->Document("foo/bar").firestore()); diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 283f3c66ed..7548bc4913 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -24,6 +24,7 @@ #include "Firestore/core/src/util/hard_assert.h" #include "app/meta/move.h" #include "firebase/firestore/local_cache_settings.h" +#include "firestore/src/common/exception_common.h" #if !defined(__ANDROID__) #include "Firestore/core/src/util/executor.h" @@ -75,7 +76,12 @@ std::shared_ptr Settings::local_cache_settings() const { } void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { - HARD_ASSERT(!used_legacy_cache_settings_, ""); + if (used_legacy_cache_settings_) { + SimpleThrowIllegalState( + "Cannot mix set_local_cache_settings() with legacy cache api like " + "set_persistence_enabled() or set_cache_size_bytes()"); + } + if (cache.kind() == api::LocalCacheSettings::Kind::kPersistent) { local_cache_settings_ = std::make_shared( *static_cast(cache).settings_internal_); @@ -86,13 +92,23 @@ void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { } void Settings::set_persistence_enabled(bool enabled) { - HARD_ASSERT(local_cache_settings() == nullptr, ""); + if (local_cache_settings_ != nullptr) { + SimpleThrowIllegalState( + "Cannot mix legacy cache api set_persistence_enabled() with new cache " + "api set_local_cache_settings()"); + } + persistence_enabled_ = enabled; used_legacy_cache_settings_ = true; } void Settings::set_cache_size_bytes(int64_t value) { - HARD_ASSERT(local_cache_settings() == nullptr, ""); + if (local_cache_settings_ != nullptr) { + SimpleThrowIllegalState( + "Cannot mix legacy cache api set_cache_size_bytes() with new cache api " + "set_local_cache_settings()"); + } + cache_size_bytes_ = value; used_legacy_cache_settings_ = true; } diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 7a032fd2e3..68f5690ab9 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -52,7 +52,6 @@ class PersistentCacheSettings final : public LocalCacheSettings { PersistentCacheSettings(const PersistentCacheSettingsInternal& other); PersistentCacheSettings WithSizeBytes(int64_t size) const; - const CoreCacheSettings& core_cache_settings() const override; api::LocalCacheSettings::Kind kind() const override { return api::LocalCacheSettings::Kind::kPersistent; } @@ -62,6 +61,8 @@ class PersistentCacheSettings final : public LocalCacheSettings { PersistentCacheSettings(); + const CoreCacheSettings& core_cache_settings() const override; + std::unique_ptr settings_internal_; }; @@ -70,7 +71,6 @@ class MemoryCacheSettings final : public LocalCacheSettings { static MemoryCacheSettings Create(); ~MemoryCacheSettings(); - const CoreCacheSettings& core_cache_settings() const override; api::LocalCacheSettings::Kind kind() const override { return api::LocalCacheSettings::Kind::kMemory; } @@ -81,6 +81,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { friend class Settings; MemoryCacheSettings(); + const CoreCacheSettings& core_cache_settings() const override; std::unique_ptr settings_internal_; }; diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 7d4c3728cc..30c37b921d 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -191,6 +191,9 @@ Settings FirestoreInternal::settings() const { result.set_ssl_enabled(from.ssl_enabled()); result.set_persistence_enabled(from.persistence_enabled()); result.set_cache_size_bytes(from.cache_size_bytes()); + // TODO(wuandy): This line should be deleted when legacy cache config is + // removed. + result.used_legacy_cache_settings_ = false; return result; } @@ -199,7 +202,11 @@ void FirestoreInternal::set_settings(Settings from) { api::Settings settings; settings.set_host(std::move(from.host())); settings.set_ssl_enabled(from.is_ssl_enabled()); - if (!from.used_legacy_cache_settings_) { + // TODO(wuandy): Checking `from.local_cache_settings_` is required, because + // FirestoreInternal::settings() overrides used_legacy_cache_settings_. All + // this special logic should go away when legacy cache config is removed. + if (!from.used_legacy_cache_settings_ && + from.local_cache_settings_ != nullptr) { settings.set_local_cache_settings( from.local_cache_settings()->core_cache_settings()); } else { From 26e9896a2df2ea6f94e9a436353c0aae6295be9e Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 8 May 2023 12:04:30 -0400 Subject: [PATCH 04/22] LRU GC compiles --- firestore/src/common/local_cache_settings.cc | 49 ++++++++++++--- firestore/src/common/settings.cc | 24 ++++--- .../firebase/firestore/local_cache_settings.h | 63 ++++++++++++++----- .../src/include/firebase/firestore/settings.h | 3 +- firestore/src/main/firestore_main.cc | 1 + .../src/main/local_cache_settings_main.h | 34 ++++++++++ 6 files changed, 136 insertions(+), 38 deletions(-) diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index cde2ab2ab9..4b6983cd6c 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -38,20 +38,51 @@ PersistentCacheSettings::~PersistentCacheSettings() { } PersistentCacheSettings::PersistentCacheSettings( - const PersistentCacheSettingsInternal& other) { - settings_internal_ = std::make_unique(other); + const PersistentCacheSettings& other) { + settings_internal_ = std::make_unique( + *other.settings_internal_); } PersistentCacheSettings PersistentCacheSettings::WithSizeBytes( int64_t size) const { - return {PersistentCacheSettingsInternal{ - settings_internal_->core_settings().WithSizeBytes(size)}}; + PersistentCacheSettings new_settings{*this}; + new_settings.settings_internal_->set_core_settings( + new_settings.settings_internal_->core_settings().WithSizeBytes(size)); + return new_settings; } const CoreCacheSettings& PersistentCacheSettings::core_cache_settings() const { return settings_internal_->core_settings(); } +MemoryEagerGCSettings MemoryEagerGCSettings::Create() { return {}; } + +MemoryEagerGCSettings::MemoryEagerGCSettings() { + settings_internal_ = std::make_unique( + CoreMemoryEagerGcSettings{}); +} + +MemoryEagerGCSettings::~MemoryEagerGCSettings() { settings_internal_.reset(); } + +MemoryLruGCSettings MemoryLruGCSettings::Create() { return {}; } + +MemoryLruGCSettings::MemoryLruGCSettings() { + settings_internal_ = + std::make_unique(CoreMemoryLruGcSettings{}); +} + +MemoryLruGCSettings::MemoryLruGCSettings( + const MemoryLruGCSettingsInternal& other) { + settings_internal_ = std::make_unique(other); +} + +MemoryLruGCSettings::~MemoryLruGCSettings() { settings_internal_.reset(); } + +MemoryLruGCSettings MemoryLruGCSettings::WithSizeBytes(int64_t size) { + return {MemoryLruGCSettingsInternal{ + settings_internal_->core_settings().WithSizeBytes(size)}}; +} + MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } MemoryCacheSettings::MemoryCacheSettings() { @@ -59,13 +90,13 @@ MemoryCacheSettings::MemoryCacheSettings() { std::make_unique(CoreMemorySettings{}); } -MemoryCacheSettings::~MemoryCacheSettings() { settings_internal_.reset(); } - -MemoryCacheSettings::MemoryCacheSettings( - const MemoryCacheSettingsInternal& other) { - settings_internal_ = std::make_unique(other); +MemoryCacheSettings::MemoryCacheSettings(const MemoryCacheSettings& other) { + settings_internal_ = + std::make_unique(*other.settings_internal_); } +MemoryCacheSettings::~MemoryCacheSettings() { settings_internal_.reset(); } + const CoreCacheSettings& MemoryCacheSettings::core_cache_settings() const { return settings_internal_->core_settings(); } diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 7548bc4913..45c3a4dac9 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -56,23 +56,21 @@ void Settings::set_host(std::string host) { host_ = firebase::Move(host); } void Settings::set_ssl_enabled(bool enabled) { ssl_enabled_ = enabled; } -std::shared_ptr Settings::local_cache_settings() const { +std::shared_ptr Settings::local_cache_settings() { if (used_legacy_cache_settings_) { if (is_persistence_enabled()) { - return std::make_shared( - *PersistentCacheSettings::Create() - .WithSizeBytes(cache_size_bytes()) - .settings_internal_); + local_cache_settings_ = std::make_shared( + PersistentCacheSettings::Create().WithSizeBytes(cache_size_bytes())); } else { - return std::make_shared( - *MemoryCacheSettings::Create().settings_internal_); + local_cache_settings_ = + std::make_shared(MemoryCacheSettings::Create()); } - } else if (local_cache_settings_ != nullptr) { - return local_cache_settings_; + } else if (local_cache_settings_ == nullptr) { + local_cache_settings_ = std::make_shared( + PersistentCacheSettings::Create()); } - return std::make_shared( - *PersistentCacheSettings::Create().settings_internal_); + return local_cache_settings_; } void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { @@ -84,10 +82,10 @@ void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { if (cache.kind() == api::LocalCacheSettings::Kind::kPersistent) { local_cache_settings_ = std::make_shared( - *static_cast(cache).settings_internal_); + static_cast(cache)); } else { local_cache_settings_ = std::make_shared( - *static_cast(cache).settings_internal_); + static_cast(cache)); } } diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 68f5690ab9..6ac894b717 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -21,66 +21,101 @@ #include #include "Firestore/core/src/api/settings.h" -#include "firestore/src/main/firestore_main.h" +#include "firestore/src/include/firebase/firestore/settings.h" +#include "firestore/src/main/local_cache_settings_main.h" namespace firebase { namespace firestore { using CoreCacheSettings = api::LocalCacheSettings; -using CoreMemorySettings = api::MemoryCacheSettings; -using CorePersistentSettings = api::PersistentCacheSettings; -class Settings; class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; class LocalCacheSettings { public: - virtual api::LocalCacheSettings::Kind kind() const = 0; virtual ~LocalCacheSettings() = default; private: friend class FirestoreInternal; + friend class Settings; + virtual api::LocalCacheSettings::Kind kind() const = 0; virtual const CoreCacheSettings& core_cache_settings() const = 0; }; class PersistentCacheSettings final : public LocalCacheSettings { public: static PersistentCacheSettings Create(); + + PersistentCacheSettings(const PersistentCacheSettings& other); ~PersistentCacheSettings(); - PersistentCacheSettings(const PersistentCacheSettingsInternal& other); PersistentCacheSettings WithSizeBytes(int64_t size) const; - api::LocalCacheSettings::Kind kind() const override { - return api::LocalCacheSettings::Kind::kPersistent; - } private: friend class Settings; PersistentCacheSettings(); + PersistentCacheSettings(const PersistentCacheSettingsInternal& other); + + api::LocalCacheSettings::Kind kind() const override { + return api::LocalCacheSettings::Kind::kPersistent; + } const CoreCacheSettings& core_cache_settings() const override; std::unique_ptr settings_internal_; }; +class MemoryGarbageCollectorSettings { + public: + virtual ~MemoryGarbageCollectorSettings() = default; + + private: +}; + +class MemoryEagerGCSettings final : MemoryGarbageCollectorSettings { + static MemoryEagerGCSettings Create(); + ~MemoryEagerGCSettings(); + + private: + MemoryEagerGCSettings(); + + std::unique_ptr settings_internal_; +}; + +class MemoryLruGCSettings final : MemoryGarbageCollectorSettings { + static MemoryLruGCSettings Create(); + ~MemoryLruGCSettings(); + MemoryLruGCSettings WithSizeBytes(int64_t size); + + private: + MemoryLruGCSettings(); + MemoryLruGCSettings(const MemoryLruGCSettingsInternal& other); + + std::unique_ptr settings_internal_; +}; + class MemoryCacheSettings final : public LocalCacheSettings { public: static MemoryCacheSettings Create(); + MemoryCacheSettings(const MemoryCacheSettings& other); ~MemoryCacheSettings(); - api::LocalCacheSettings::Kind kind() const override { - return api::LocalCacheSettings::Kind::kMemory; - } - - MemoryCacheSettings(const MemoryCacheSettingsInternal& other); + MemoryCacheSettings WithGarbageCollectorSettings( + const MemoryGarbageCollectorSettings& settings); private: friend class Settings; MemoryCacheSettings(); + MemoryCacheSettings(const MemoryCacheSettingsInternal& other); + + api::LocalCacheSettings::Kind kind() const override { + return api::LocalCacheSettings::Kind::kMemory; + } + const CoreCacheSettings& core_cache_settings() const override; std::unique_ptr settings_internal_; diff --git a/firestore/src/include/firebase/firestore/settings.h b/firestore/src/include/firebase/firestore/settings.h index 60afcba1ed..63028f68b0 100644 --- a/firestore/src/include/firebase/firestore/settings.h +++ b/firestore/src/include/firebase/firestore/settings.h @@ -17,7 +17,6 @@ #ifndef FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_SETTINGS_H_ #define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_SETTINGS_H_ -#include "firebase/firestore/local_cache_settings.h" #if defined(__OBJC__) #include #endif @@ -138,7 +137,7 @@ class Settings final { */ void set_ssl_enabled(bool enabled); - std::shared_ptr local_cache_settings() const; + std::shared_ptr local_cache_settings(); void set_local_cache_settings(const LocalCacheSettings& cache); /** diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 30c37b921d..09ec274f2f 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -43,6 +43,7 @@ #include "firestore/src/common/macros.h" #include "firestore/src/common/util.h" #include "firestore/src/include/firebase/firestore.h" +#include "firestore/src/include/firebase/firestore/local_cache_settings.h" #include "firestore/src/main/converter_main.h" #include "firestore/src/main/create_app_check_credentials_provider.h" #include "firestore/src/main/create_credentials_provider.h" diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index b24a3a8ee9..522651a025 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -24,6 +24,8 @@ namespace firebase { namespace firestore { +using CoreMemoryEagerGcSettings = api::MemoryEagerGcSettings; +using CoreMemoryLruGcSettings = api::MemoryLruGcSettings; using CoreMemorySettings = api::MemoryCacheSettings; using CorePersistentSettings = api::PersistentCacheSettings; @@ -44,6 +46,38 @@ class PersistentCacheSettingsInternal final CorePersistentSettings settings_; }; +class MemoryGarbageCollectorSettingsInternal {}; + +class MemoryEagerGCSettingsInternal final + : public MemoryGarbageCollectorSettingsInternal { + public: + explicit MemoryEagerGCSettingsInternal( + const CoreMemoryEagerGcSettings& core_settings) + : settings_(std::move(core_settings)) {} + const CoreMemoryEagerGcSettings& core_settings() { return settings_; } + void set_core_settings(const CoreMemoryEagerGcSettings& settings) { + settings_ = settings; + } + + private: + CoreMemoryEagerGcSettings settings_; +}; + +class MemoryLruGCSettingsInternal final + : public MemoryGarbageCollectorSettingsInternal { + public: + explicit MemoryLruGCSettingsInternal( + const CoreMemoryLruGcSettings& core_settings) + : settings_(std::move(core_settings)) {} + const CoreMemoryLruGcSettings& core_settings() { return settings_; } + void set_core_settings(const CoreMemoryLruGcSettings& settings) { + settings_ = settings; + } + + private: + CoreMemoryLruGcSettings settings_; +}; + class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { public: explicit MemoryCacheSettingsInternal(const CoreMemorySettings& core_settings) From e7a0f70291807dc1853c01a34b6b232678e4a2f6 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 8 May 2023 12:48:41 -0400 Subject: [PATCH 05/22] LRU tests pass --- .../src/firestore_test.cc | 36 ++++++++++ firestore/src/common/local_cache_settings.cc | 10 +++ .../firebase/firestore/local_cache_settings.h | 68 ++++++++++++------- 3 files changed, 88 insertions(+), 26 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 4968ba533e..b764ebcd27 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1588,6 +1588,42 @@ TEST_F(FirestoreIntegrationTest, CannotMixNewAndLegacyCacheConfig) { } } +TEST_F(FirestoreIntegrationTest, CanGetDocumentFromCacheWithMemoryLruGC) { + auto* db = TestFirestore("new_persistent_cache"); + auto settings = db->settings(); + settings.set_local_cache_settings( + MemoryCacheSettings::Create().WithGarbageCollectorSettings( + MemoryLruGCSettings::Create())); + db->set_settings(std::move(settings)); + + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); + EXPECT_TRUE(snapshot->is_valid()); + EXPECT_THAT(snapshot->GetData(), + ContainerEq(MapFieldValue{{"desc", FieldValue::String("eros")}})); +} + +TEST_F(FirestoreIntegrationTest, CannotGetDocumentFromCacheFromMemoryEagerGC) { + auto* db = TestFirestore("new_persistent_cache"); + auto settings = db->settings(); + settings.set_local_cache_settings( + MemoryCacheSettings::Create().WithGarbageCollectorSettings( + MemoryEagerGCSettings::Create())); + db->set_settings(std::move(settings)); + + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); + EXPECT_FALSE(snapshot->is_valid()); +} + // Note: this test only exists in C++. TEST_F(FirestoreIntegrationTest, DomainObjectsReferToSameFirestoreInstance) { EXPECT_EQ(TestFirestore(), TestFirestore()->Document("foo/bar").firestore()); diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index 4b6983cd6c..1babeb2257 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -97,6 +97,16 @@ MemoryCacheSettings::MemoryCacheSettings(const MemoryCacheSettings& other) { MemoryCacheSettings::~MemoryCacheSettings() { settings_internal_.reset(); } +MemoryCacheSettings MemoryCacheSettings::WithGarbageCollectorSettings( + const MemoryGarbageCollectorSettings& settings) const { + MemoryCacheSettings result{*this}; + CoreMemorySettings core_settings = result.settings_internal_->core_settings(); + result.settings_internal_->set_core_settings( + core_settings.WithMemoryGarbageCollectorSettings( + settings.core_gc_settings())); + return result; +} + const CoreCacheSettings& MemoryCacheSettings::core_cache_settings() const { return settings_internal_->core_settings(); } diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 6ac894b717..293aa1aa05 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -28,6 +28,7 @@ namespace firebase { namespace firestore { using CoreCacheSettings = api::LocalCacheSettings; +using CoreMemoryGarbageCollectorSettings = api::MemoryGargabeCollectorSettings; class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; @@ -57,7 +58,6 @@ class PersistentCacheSettings final : public LocalCacheSettings { friend class Settings; PersistentCacheSettings(); - PersistentCacheSettings(const PersistentCacheSettingsInternal& other); api::LocalCacheSettings::Kind kind() const override { return api::LocalCacheSettings::Kind::kPersistent; @@ -68,57 +68,73 @@ class PersistentCacheSettings final : public LocalCacheSettings { std::unique_ptr settings_internal_; }; +class MemoryGarbageCollectorSettings; + +class MemoryCacheSettings final : public LocalCacheSettings { + public: + static MemoryCacheSettings Create(); + MemoryCacheSettings(const MemoryCacheSettings& other); + ~MemoryCacheSettings(); + + MemoryCacheSettings WithGarbageCollectorSettings( + const MemoryGarbageCollectorSettings& settings) const; + + private: + friend class Settings; + + MemoryCacheSettings(); + + api::LocalCacheSettings::Kind kind() const override { + return api::LocalCacheSettings::Kind::kMemory; + } + + const CoreCacheSettings& core_cache_settings() const override; + + std::unique_ptr settings_internal_; +}; + class MemoryGarbageCollectorSettings { public: virtual ~MemoryGarbageCollectorSettings() = default; private: + friend class MemoryCacheSettings; + virtual const CoreMemoryGarbageCollectorSettings& core_gc_settings() + const = 0; }; -class MemoryEagerGCSettings final : MemoryGarbageCollectorSettings { +class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { + public: static MemoryEagerGCSettings Create(); ~MemoryEagerGCSettings(); private: + friend class MemoryCacheSettings; MemoryEagerGCSettings(); + const CoreMemoryGarbageCollectorSettings& core_gc_settings() const override { + return settings_internal_->core_settings(); + } + std::unique_ptr settings_internal_; }; -class MemoryLruGCSettings final : MemoryGarbageCollectorSettings { +class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { + public: static MemoryLruGCSettings Create(); ~MemoryLruGCSettings(); MemoryLruGCSettings WithSizeBytes(int64_t size); private: + friend class MemoryCacheSettings; MemoryLruGCSettings(); MemoryLruGCSettings(const MemoryLruGCSettingsInternal& other); - std::unique_ptr settings_internal_; -}; - -class MemoryCacheSettings final : public LocalCacheSettings { - public: - static MemoryCacheSettings Create(); - MemoryCacheSettings(const MemoryCacheSettings& other); - ~MemoryCacheSettings(); - - MemoryCacheSettings WithGarbageCollectorSettings( - const MemoryGarbageCollectorSettings& settings); - - private: - friend class Settings; - - MemoryCacheSettings(); - MemoryCacheSettings(const MemoryCacheSettingsInternal& other); - - api::LocalCacheSettings::Kind kind() const override { - return api::LocalCacheSettings::Kind::kMemory; + const CoreMemoryGarbageCollectorSettings& core_gc_settings() const override { + return settings_internal_->core_settings(); } - const CoreCacheSettings& core_cache_settings() const override; - - std::unique_ptr settings_internal_; + std::unique_ptr settings_internal_; }; } // namespace firestore From b93e93360446d52c951e426a66bf2b64d3091e4a Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Tue, 9 May 2023 11:43:52 -0400 Subject: [PATCH 06/22] Feedback --- .../src/bundle_test.cc | 3 +- .../src/firestore_test.cc | 10 +- .../src/settings_test.cc | 25 ++-- firestore/src/common/local_cache_settings.cc | 16 +++ .../firebase/firestore/local_cache_settings.h | 113 +++++++++++++++++- .../src/include/firebase/firestore/settings.h | 26 +++- .../src/main/local_cache_settings_main.h | 7 +- 7 files changed, 177 insertions(+), 23 deletions(-) diff --git a/firestore/integration_test_internal/src/bundle_test.cc b/firestore/integration_test_internal/src/bundle_test.cc index 3ac9760a9f..486a5e3058 100644 --- a/firestore/integration_test_internal/src/bundle_test.cc +++ b/firestore/integration_test_internal/src/bundle_test.cc @@ -19,6 +19,7 @@ #include #include "firebase/firestore.h" +#include "firebase/firestore/local_cache_settings.h" #include "firebase_test_framework.h" #include "firestore_integration_test.h" #include "gmock/gmock.h" @@ -327,7 +328,7 @@ TEST_F(BundleTest, LoadedDocumentsShouldNotBeGarbageCollectedRightAway) { // This test really only makes sense with memory persistence, as disk // persistence only ever lazily deletes data. auto new_settings = db->settings(); - new_settings.set_persistence_enabled(false); + new_settings.set_local_cache_settings(MemoryCacheSettings::Create()); db->set_settings(new_settings); auto bundle = CreateTestBundle(db); diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index b764ebcd27..b359f5e60b 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1507,7 +1507,7 @@ TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) { TEST_F(FirestoreIntegrationTest, LegacyCacheConfigForMemoryCacheWorks) { auto* db = TestFirestore("legacy_memory_cache"); auto settings = db->settings(); - settings.set_persistence_enabled(false); + WITH_DEPRECATED_API(settings.set_persistence_enabled(false)); db->set_settings(std::move(settings)); Await(db->Document("rooms/eros") @@ -1523,7 +1523,7 @@ TEST_F(FirestoreIntegrationTest, LegacyCacheConfigForMemoryCacheWorks) { TEST_F(FirestoreIntegrationTest, LegacyCacheConfigForPersistenceCacheWorks) { auto* db = TestFirestore("legacy_persistent_cache"); auto settings = db->settings(); - settings.set_persistence_enabled(true); + WITH_DEPRECATED_API(settings.set_persistence_enabled(true)); db->set_settings(std::move(settings)); Await(db->Document("rooms/eros") @@ -1576,12 +1576,14 @@ TEST_F(FirestoreIntegrationTest, CannotMixNewAndLegacyCacheConfig) { auto settings = db->settings(); settings.set_local_cache_settings( PersistentCacheSettings::Create().WithSizeBytes(50 * 1024 * 1024)); - EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error); + + WITH_DEPRECATED_API(EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error)); } + { auto* db = TestFirestore("mixing_2"); auto settings = db->settings(); - settings.set_persistence_enabled(false); + WITH_DEPRECATED_API(settings.set_persistence_enabled(false)); EXPECT_THROW( settings.set_local_cache_settings(MemoryCacheSettings::Create()), std::logic_error); diff --git a/firestore/integration_test_internal/src/settings_test.cc b/firestore/integration_test_internal/src/settings_test.cc index ca62e2a74e..6fb7d9dfba 100644 --- a/firestore/integration_test_internal/src/settings_test.cc +++ b/firestore/integration_test_internal/src/settings_test.cc @@ -17,6 +17,7 @@ #include #include "firebase/firestore.h" +#include "firebase_test_framework.h" #include "gtest/gtest.h" @@ -31,39 +32,39 @@ TEST(SettingsTest, Equality) { Settings settings1; settings1.set_host("foo"); settings1.set_ssl_enabled(true); - settings1.set_persistence_enabled(true); - settings1.set_cache_size_bytes(kFiveMb); + WITH_DEPRECATED_API(settings1.set_persistence_enabled(true)); + WITH_DEPRECATED_API(settings1.set_cache_size_bytes(kFiveMb)); Settings settings2; settings2.set_host("bar"); settings2.set_ssl_enabled(true); - settings2.set_persistence_enabled(true); - settings2.set_cache_size_bytes(kFiveMb); + WITH_DEPRECATED_API(settings2.set_persistence_enabled(true)); + WITH_DEPRECATED_API(settings2.set_cache_size_bytes(kFiveMb)); Settings settings3; settings3.set_host("foo"); settings3.set_ssl_enabled(false); - settings3.set_persistence_enabled(true); - settings3.set_cache_size_bytes(kFiveMb); + WITH_DEPRECATED_API(settings3.set_persistence_enabled(true)); + WITH_DEPRECATED_API(settings3.set_cache_size_bytes(kFiveMb)); Settings settings4; settings4.set_host("foo"); settings4.set_ssl_enabled(true); - settings4.set_persistence_enabled(false); - settings4.set_cache_size_bytes(kFiveMb); + WITH_DEPRECATED_API(settings4.set_persistence_enabled(false)); + WITH_DEPRECATED_API(settings4.set_cache_size_bytes(kFiveMb)); Settings settings5; settings5.set_host("foo"); settings5.set_ssl_enabled(true); - settings5.set_persistence_enabled(true); - settings5.set_cache_size_bytes(kSixMb); + WITH_DEPRECATED_API(settings5.set_persistence_enabled(true)); + WITH_DEPRECATED_API(settings5.set_cache_size_bytes(kSixMb)); // This is the same as settings4. Settings settings6; settings6.set_host("foo"); settings6.set_ssl_enabled(true); - settings6.set_persistence_enabled(false); - settings6.set_cache_size_bytes(kFiveMb); + WITH_DEPRECATED_API(settings6.set_persistence_enabled(false)); + WITH_DEPRECATED_API(settings6.set_cache_size_bytes(kFiveMb)); EXPECT_TRUE(settings1 == settings1); EXPECT_TRUE(settings6 == settings4); diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index 1babeb2257..2dcfb2c221 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -28,6 +28,14 @@ namespace firestore { PersistentCacheSettings PersistentCacheSettings::Create() { return {}; } +PersistentCacheSettings PersistentCacheSettings::CreateFromCoreSettings( + const CorePersistentSettings& core_settings) { + auto result = PersistentCacheSettings{}; + result.settings_internal_ = + std::make_unique(core_settings); + return result; +} + PersistentCacheSettings::PersistentCacheSettings() { settings_internal_ = std::make_unique( CorePersistentSettings{}); @@ -85,6 +93,14 @@ MemoryLruGCSettings MemoryLruGCSettings::WithSizeBytes(int64_t size) { MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } +MemoryCacheSettings MemoryCacheSettings::CreateFromCoreSettings( + const CoreMemorySettings& core_settings) { + auto result = MemoryCacheSettings{}; + result.settings_internal_ = + std::make_unique(core_settings); + return result; +} + MemoryCacheSettings::MemoryCacheSettings() { settings_internal_ = std::make_unique(CoreMemorySettings{}); diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 293aa1aa05..1bd8edea4a 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -1,5 +1,5 @@ /* - * Copyright 2018 Google LLC + * Copyright 2023 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,17 +22,26 @@ #include "Firestore/core/src/api/settings.h" #include "firestore/src/include/firebase/firestore/settings.h" +#include "firestore/src/main/firestore_main.h" #include "firestore/src/main/local_cache_settings_main.h" namespace firebase { namespace firestore { +namespace { using CoreCacheSettings = api::LocalCacheSettings; using CoreMemoryGarbageCollectorSettings = api::MemoryGargabeCollectorSettings; +} // namespace class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; +/** + * Abstract class implemented by all supported cache settings. + * + * `PersistentCacheSettings` and `MemoryCacheSettings` are the only cache types + * supported by the SDK. Custom implementation is not supported. + */ class LocalCacheSettings { public: virtual ~LocalCacheSettings() = default; @@ -45,24 +54,53 @@ class LocalCacheSettings { virtual const CoreCacheSettings& core_cache_settings() const = 0; }; +/** + * Configures the SDK to use a persistent cache. Firestore documents and + * mutations are persisted across App restart. + * + * This is the default cache type unless explicitly specified otherwise. + * + * To use, create an instance using `PersistentCacheSettings::Create`, then + * pass it to an instance of `Settings` via `set_local_cache_settings()`, and + * use the `Settings` instance to configure the Firestore SDK. + */ class PersistentCacheSettings final : public LocalCacheSettings { public: + /** Create a default instance `PersistenceCacheSettings`. */ static PersistentCacheSettings Create(); + /** Copy constructor. */ PersistentCacheSettings(const PersistentCacheSettings& other); ~PersistentCacheSettings(); + /** + * Copies this settings instance, with the approximate cache size threshold + * for the on-disk data set to the given number in term of number of bytes, + * and return the new setting instance. + * + * If the cache grows beyond this size, Firestore SDK will start removing data + * that hasn't been recently used. The SDK does not guarantee that the cache + * will stay below that size, only that if the cache exceeds the given size, + * cleanup will be attempted. + * + * By default, persistence cache is enabled with a cache size of 100 MB. The + * minimum value is 1 MB. + */ PersistentCacheSettings WithSizeBytes(int64_t size) const; private: friend class Settings; + friend class FirestoreInternal; + static PersistentCacheSettings CreateFromCoreSettings( + const CorePersistentSettings& core_settings); PersistentCacheSettings(); api::LocalCacheSettings::Kind kind() const override { return api::LocalCacheSettings::Kind::kPersistent; } + // Get the corresponding settings object from the core sdk. const CoreCacheSettings& core_cache_settings() const override; std::unique_ptr settings_internal_; @@ -70,29 +108,55 @@ class PersistentCacheSettings final : public LocalCacheSettings { class MemoryGarbageCollectorSettings; +/** + * Configures the SDK to use a memory cache. Firestore documents and mutations + * are NOT persisted across App restart. + * + * To use, create an instance using `MemoryCacheSettings::Create`, then + * pass it to an instance of `Settings` via `set_local_cache_settings()`, and + * use the `Settings` instance to configure the Firestore SDK. + */ class MemoryCacheSettings final : public LocalCacheSettings { public: + /** Create a default instance `MemoryCacheSettings`. */ static MemoryCacheSettings Create(); + + /** Copy constructor. */ MemoryCacheSettings(const MemoryCacheSettings& other); ~MemoryCacheSettings(); + /** + * Copies this settings instance, with its `MemoryGarbageCollectorSettins` set + * the the given parameter, and returns the new settings instance. + */ MemoryCacheSettings WithGarbageCollectorSettings( const MemoryGarbageCollectorSettings& settings) const; private: friend class Settings; + friend class FirestoreInternal; + static MemoryCacheSettings CreateFromCoreSettings( + const CoreMemorySettings& core_settings); MemoryCacheSettings(); api::LocalCacheSettings::Kind kind() const override { return api::LocalCacheSettings::Kind::kMemory; } + // Get the corresponding settings object from the core sdk. const CoreCacheSettings& core_cache_settings() const override; std::unique_ptr settings_internal_; }; +/** + * Abstract class implemented by all supported memory garbage collector. + * + * `MemoryEagerGCSettings` and `MemoryLruGCSettings` are the only memory + * garbage collectors supported by the SDK. Custom implementation is not + * supported. + */ class MemoryGarbageCollectorSettings { public: virtual ~MemoryGarbageCollectorSettings() = default; @@ -103,8 +167,24 @@ class MemoryGarbageCollectorSettings { const = 0; }; +/** + * Configures the memory cache to use a garbage collector with an eager + * strategy. + * + * An eager garbage collector deletes documents whenever they are not part of + * any active queries, and have no local mutations attached to them. + * + * This collector tries to ensure lowest memory footprints from the SDK, + * at the risk of documents not being cached for offline queries or for + * direct queries to the cache. + * + * To use, pass an instance of `MemoryEagerGCSettings` to + * `MemoryCacheSettings::WithGarbageCollectorSettings()` to get a new instance + * of `MemoryCacheSettings`, which can be used to configure the SDK. + */ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { public: + /** Create a default instance `MemoryEagerGCSettings`. */ static MemoryEagerGCSettings Create(); ~MemoryEagerGCSettings(); @@ -119,10 +199,41 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { std::unique_ptr settings_internal_; }; +/** + * Configures the memory cache to use a garbage collector with an + * least-recently-used strategy. + * + * A LRU garbage collector deletes Least-Recently-Used documents in multiple + * batches. + * + * This collector is configured with a target size, and will only perform + * collection when the cached documents exceed the target size. It avoids + * querying backend repeated for the same query or document, at the risk + * of having a larger memory footprint. + * + * To use, pass an instance of `MemoryLRUGCSettings` to + * `MemoryCacheSettings::WithGarbageCollectorSettings()` to get a new instance + * of `MemoryCacheSettings`, which can be used to configure the SDK. + */ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { public: + /** Create a default instance `MemoryLruGCSettings`. */ static MemoryLruGCSettings Create(); ~MemoryLruGCSettings(); + + /** + * Copies this settings instance, with the approximate cache size threshold + * for the memory data set to the given number in term of number of bytes, and + * return the new setting instance. + * + * If the cache grows beyond this size, Firestore SDK will start removing data + * that hasn't been recently used. The SDK does not guarantee that the cache + * will stay below that size, only that if the cache exceeds the given size, + * cleanup will be attempted. + * + * By default, memory LRU cache is enabled with a cache size of 100 MB. The + * minimum value is 1 MB. + */ MemoryLruGCSettings WithSizeBytes(int64_t size); private: diff --git a/firestore/src/include/firebase/firestore/settings.h b/firestore/src/include/firebase/firestore/settings.h index 63028f68b0..1868a77678 100644 --- a/firestore/src/include/firebase/firestore/settings.h +++ b/firestore/src/include/firebase/firestore/settings.h @@ -17,6 +17,7 @@ #ifndef FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_SETTINGS_H_ #define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_SETTINGS_H_ +#include "firebase/internal/common.h" #if defined(__OBJC__) #include #endif @@ -137,17 +138,38 @@ class Settings final { */ void set_ssl_enabled(bool enabled); + /** + * Returns a shared pointer to the `LocalCacheSettings` instance + * used to configure this SDK. + */ std::shared_ptr local_cache_settings(); + + /** + * Configures the SDK with the given `LocalCacheSettings` instance. + * + * By default, persistence cache is enabled, with a cache size of 100 MB. + * + * See documentation of `PersistentCacheSettings` to under the default + * settings. + * + * @param cache_settings Settings object to configue this SDK. + */ void set_local_cache_settings(const LocalCacheSettings& cache); /** + * NOTE: This method is deprecated in favor of `set_local_cache_settings()`. + * It will be deleted in a future major release. + * * Enables or disables local persistent storage. * * @param enabled Set true to enable local persistent storage. */ - void set_persistence_enabled(bool enabled); + FIREBASE_DEPRECATED void set_persistence_enabled(bool enabled); /** + * NOTE: This method is deprecated in favor of `set_local_cache_settings()`. + * It will be deleted in a future major release. + * * Sets an approximate cache size threshold for the on-disk data. If the cache * grows beyond this size, Cloud Firestore will start removing data that * hasn't been recently used. The size is not a guarantee that the cache will @@ -157,7 +179,7 @@ class Settings final { * By default, collection is enabled with a cache size of 100 MB. The minimum * value is 1 MB. */ - void set_cache_size_bytes(int64_t value); + FIREBASE_DEPRECATED void set_cache_size_bytes(int64_t value); #if defined(__OBJC__) || defined(DOXYGEN) /** diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index 522651a025..9d8cdd15f7 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -14,10 +14,11 @@ * limitations under the License. */ -#ifndef FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETINGS_MAIN_H_ -#define FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETINGS_MAIN_H_ +#ifndef FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETTINGS_MAIN_H_ +#define FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETTINGS_MAIN_H_ #include +#include #include "Firestore/core/src/api/settings.h" @@ -94,4 +95,4 @@ class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { } // namespace firestore } // namespace firebase -#endif // FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETINGS_MAIN_H_ +#endif // FIREBASE_FIRESTORE_SRC_MAIN_LOCAL_CACHE_SETTINGS_MAIN_H_ From d65416d6d30add9c3c73ba18ea69ad03169acb51 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Tue, 9 May 2023 11:44:04 -0400 Subject: [PATCH 07/22] Using new cache api, for future reference. --- firestore/src/main/firestore_main.cc | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 09ec274f2f..d35df85b0e 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -51,6 +51,7 @@ #include "firestore/src/main/document_reference_main.h" #include "firestore/src/main/document_snapshot_main.h" #include "firestore/src/main/listener_main.h" +#include "firestore/src/main/local_cache_settings_main.h" namespace firebase { namespace firestore { @@ -190,11 +191,24 @@ Settings FirestoreInternal::settings() const { 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()); - result.set_cache_size_bytes(from.cache_size_bytes()); - // TODO(wuandy): This line should be deleted when legacy cache config is - // removed. - result.used_legacy_cache_settings_ = false; + + if(from.local_cache_settings() == nullptr) { + if(from.persistence_enabled()) { + result.set_local_cache_settings(PersistentCacheSettings::Create().WithSizeBytes(from.cache_size_bytes())); + } else { + result.set_local_cache_settings(MemoryCacheSettings::Create()); + } + } + else if (from.local_cache_settings()->kind() == + api::LocalCacheSettings::Kind::kMemory) { + result.set_local_cache_settings(MemoryCacheSettings::CreateFromCoreSettings( + dynamic_cast(*from.local_cache_settings()))); + } else { + result.set_local_cache_settings( + PersistentCacheSettings::CreateFromCoreSettings( + dynamic_cast( + *from.local_cache_settings()))); + } return result; } From 47f464706fd0ab3a3e582c4a3829ad09200c4315 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Tue, 9 May 2023 11:50:27 -0400 Subject: [PATCH 08/22] fix firestore_main to use legacy API for now --- .../src/firestore_test.cc | 5 ++-- firestore/src/main/firestore_main.cc | 27 +++++++------------ 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index b359f5e60b..f6220f922c 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1577,13 +1577,14 @@ TEST_F(FirestoreIntegrationTest, CannotMixNewAndLegacyCacheConfig) { settings.set_local_cache_settings( PersistentCacheSettings::Create().WithSizeBytes(50 * 1024 * 1024)); - WITH_DEPRECATED_API(EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error)); + WITH_DEPRECATED_API( + EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error)); } { auto* db = TestFirestore("mixing_2"); auto settings = db->settings(); - WITH_DEPRECATED_API(settings.set_persistence_enabled(false)); + WITH_DEPRECATED_API(settings.set_persistence_enabled(false)); EXPECT_THROW( settings.set_local_cache_settings(MemoryCacheSettings::Create()), std::logic_error); diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index d35df85b0e..b6a057daa1 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -192,23 +192,16 @@ Settings FirestoreInternal::settings() const { result.set_host(from.host()); result.set_ssl_enabled(from.ssl_enabled()); - if(from.local_cache_settings() == nullptr) { - if(from.persistence_enabled()) { - result.set_local_cache_settings(PersistentCacheSettings::Create().WithSizeBytes(from.cache_size_bytes())); - } else { - result.set_local_cache_settings(MemoryCacheSettings::Create()); - } - } - else if (from.local_cache_settings()->kind() == - api::LocalCacheSettings::Kind::kMemory) { - result.set_local_cache_settings(MemoryCacheSettings::CreateFromCoreSettings( - dynamic_cast(*from.local_cache_settings()))); - } else { - result.set_local_cache_settings( - PersistentCacheSettings::CreateFromCoreSettings( - dynamic_cast( - *from.local_cache_settings()))); - } + // TODO(wuandy): We use the deprecated API for default settings, but mark + // `used_legacy_cache_settings_` as false such that new settings API is not + // rejected by runtime checks. This should be removed when legacy API is + // removed. +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + result.set_persistence_enabled(from.persistence_enabled()); + result.set_cache_size_bytes(from.cache_size_bytes()); +#pragma clang diagnostic pop + result.used_legacy_cache_settings_ = false; return result; } From bfa379f0ff164873776298f0c97e946bfa4ae0e5 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 10 May 2023 10:04:25 -0400 Subject: [PATCH 09/22] More polishing and documentation. --- firestore/CMakeLists.txt | 2 + firestore/src/common/local_cache_settings.cc | 61 +++++++++++++++++++ .../firebase/firestore/local_cache_settings.h | 57 +++++++++++++++++ .../src/main/local_cache_settings_main.cc | 46 ++++++++++++++ .../src/main/local_cache_settings_main.h | 39 +++++++++++- 5 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 firestore/src/main/local_cache_settings_main.cc diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index cf5c239f88..760517b25a 100644 --- a/firestore/CMakeLists.txt +++ b/firestore/CMakeLists.txt @@ -214,6 +214,8 @@ set(main_SRCS src/main/listener_main.h src/main/listener_registration_main.cc src/main/listener_registration_main.h + src/main/local_cache_settings_main.cc + src/main/local_cache_settings_main.h src/main/promise_factory_main.h src/main/promise_main.h src/main/query_main.cc diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index 2dcfb2c221..a39ce7514d 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -51,6 +51,16 @@ PersistentCacheSettings::PersistentCacheSettings( *other.settings_internal_); } +PersistentCacheSettings& PersistentCacheSettings::operator=( + const PersistentCacheSettings& other) { + if (this == &other) { + return *this; + } + settings_internal_ = std::make_unique( + *other.settings_internal_); + return *this; +} + PersistentCacheSettings PersistentCacheSettings::WithSizeBytes( int64_t size) const { PersistentCacheSettings new_settings{*this}; @@ -111,6 +121,17 @@ MemoryCacheSettings::MemoryCacheSettings(const MemoryCacheSettings& other) { std::make_unique(*other.settings_internal_); } +MemoryCacheSettings& MemoryCacheSettings::operator=( + const MemoryCacheSettings& other) { + if (this == &other) { + return *this; + } + + settings_internal_ = + std::make_unique(*other.settings_internal_); + return *this; +} + MemoryCacheSettings::~MemoryCacheSettings() { settings_internal_.reset(); } MemoryCacheSettings MemoryCacheSettings::WithGarbageCollectorSettings( @@ -127,5 +148,45 @@ const CoreCacheSettings& MemoryCacheSettings::core_cache_settings() const { return settings_internal_->core_settings(); } +bool operator==(const MemoryCacheSettings& lhs, + const MemoryCacheSettings& rhs) { + return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); +} + +bool operator!=(const MemoryCacheSettings& lhs, + const MemoryCacheSettings& rhs) { + return !(lhs == rhs); +} + +bool operator==(const PersistentCacheSettings& lhs, + const PersistentCacheSettings& rhs) { + return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); +} + +bool operator!=(const PersistentCacheSettings& lhs, + const PersistentCacheSettings& rhs) { + return !(lhs == rhs); +} + +bool operator==(const MemoryEagerGCSettings& lhs, + const MemoryEagerGCSettings& rhs) { + return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); +} + +bool operator!=(const MemoryEagerGCSettings& lhs, + const MemoryEagerGCSettings& rhs) { + return !(lhs == rhs); +} + +bool operator==(const MemoryLruGCSettings& lhs, + const MemoryLruGCSettings& rhs) { + return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); +} + +bool operator!=(const MemoryLruGCSettings& lhs, + const MemoryLruGCSettings& rhs) { + return !(lhs == rhs); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 1bd8edea4a..308a8467d4 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -71,8 +71,15 @@ class PersistentCacheSettings final : public LocalCacheSettings { /** Copy constructor. */ PersistentCacheSettings(const PersistentCacheSettings& other); + + /** Copy assignment. */ + PersistentCacheSettings& operator=(const PersistentCacheSettings& other); + ~PersistentCacheSettings(); + friend bool operator==(const PersistentCacheSettings& lhs, + const PersistentCacheSettings& rhs); + /** * Copies this settings instance, with the approximate cache size threshold * for the on-disk data set to the given number in term of number of bytes, @@ -88,6 +95,14 @@ class PersistentCacheSettings final : public LocalCacheSettings { */ PersistentCacheSettings WithSizeBytes(int64_t size) const; + /** + * Returns the approximate cache size threshold configured. Garbage collection + * kicks in once the cache size exceeds this threshold. + */ + int64_t size_bytes() const { + return settings_internal_->core_settings().size_bytes(); + } + private: friend class Settings; friend class FirestoreInternal; @@ -123,8 +138,15 @@ class MemoryCacheSettings final : public LocalCacheSettings { /** Copy constructor. */ MemoryCacheSettings(const MemoryCacheSettings& other); + + /** Copy assignment. */ + MemoryCacheSettings& operator=(const MemoryCacheSettings& other); + ~MemoryCacheSettings(); + friend bool operator==(const MemoryCacheSettings& lhs, + const MemoryCacheSettings& rhs); + /** * Copies this settings instance, with its `MemoryGarbageCollectorSettins` set * the the given parameter, and returns the new settings instance. @@ -186,8 +208,12 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { public: /** Create a default instance `MemoryEagerGCSettings`. */ static MemoryEagerGCSettings Create(); + ~MemoryEagerGCSettings(); + friend bool operator==(const MemoryEagerGCSettings& lhs, + const MemoryEagerGCSettings& rhs); + private: friend class MemoryCacheSettings; MemoryEagerGCSettings(); @@ -221,6 +247,9 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { static MemoryLruGCSettings Create(); ~MemoryLruGCSettings(); + friend bool operator==(const MemoryLruGCSettings& lhs, + const MemoryLruGCSettings& rhs); + /** * Copies this settings instance, with the approximate cache size threshold * for the memory data set to the given number in term of number of bytes, and @@ -236,6 +265,14 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { */ MemoryLruGCSettings WithSizeBytes(int64_t size); + /** + * Returns the approximate cache size threshold configured. Garbage collection + * kicks in once the cache size exceeds this threshold. + */ + int64_t size_bytes() const { + return settings_internal_->core_settings().size_bytes(); + } + private: friend class MemoryCacheSettings; MemoryLruGCSettings(); @@ -248,6 +285,26 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { std::unique_ptr settings_internal_; }; +bool operator==(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs); + +bool operator!=(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs); + +bool operator==(const PersistentCacheSettings& lhs, + const PersistentCacheSettings& rhs); + +bool operator!=(const PersistentCacheSettings& lhs, + const PersistentCacheSettings& rhs); + +bool operator==(const MemoryEagerGCSettings& lhs, + const MemoryEagerGCSettings& rhs); + +bool operator!=(const MemoryEagerGCSettings& lhs, + const MemoryEagerGCSettings& rhs); + +bool operator==(const MemoryLruGCSettings& lhs, const MemoryLruGCSettings& rhs); + +bool operator!=(const MemoryLruGCSettings& lhs, const MemoryLruGCSettings& rhs); + } // namespace firestore } // namespace firebase diff --git a/firestore/src/main/local_cache_settings_main.cc b/firestore/src/main/local_cache_settings_main.cc new file mode 100644 index 0000000000..e5d1a3e8e6 --- /dev/null +++ b/firestore/src/main/local_cache_settings_main.cc @@ -0,0 +1,46 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +#include "firestore/src/main/local_cache_settings_main.h" + +namespace firebase { +namespace firestore { + +bool operator!=(const MemoryCacheSettingsInternal& lhs, + const MemoryCacheSettingsInternal& rhs) { + return !(lhs == rhs); +} + +bool operator!=(const MemoryLruGCSettingsInternal& lhs, + const MemoryLruGCSettingsInternal& rhs) { + return !(lhs == rhs); +} + +bool operator!=(const MemoryEagerGCSettingsInternal& lhs, + const MemoryEagerGCSettingsInternal& rhs) { + return !(lhs == rhs); +} + +bool operator!=(const PersistentCacheSettingsInternal& lhs, + const PersistentCacheSettingsInternal& rhs) { + return !(lhs == rhs); +} + +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index 9d8cdd15f7..5e7afe4d2d 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -25,11 +25,12 @@ namespace firebase { namespace firestore { +namespace { using CoreMemoryEagerGcSettings = api::MemoryEagerGcSettings; using CoreMemoryLruGcSettings = api::MemoryLruGcSettings; using CoreMemorySettings = api::MemoryCacheSettings; using CorePersistentSettings = api::PersistentCacheSettings; - +} // namespace class LocalCacheSettingsInternal {}; class PersistentCacheSettingsInternal final @@ -38,6 +39,12 @@ class PersistentCacheSettingsInternal final explicit PersistentCacheSettingsInternal( const CorePersistentSettings& core_settings) : settings_(std::move(core_settings)) {} + + friend bool operator==(const PersistentCacheSettingsInternal& lhs, + const PersistentCacheSettingsInternal& rhs) { + return &lhs == &rhs || lhs.settings_ == rhs.settings_; + } + const CorePersistentSettings& core_settings() { return settings_; } void set_core_settings(const CorePersistentSettings& settings) { settings_ = settings; @@ -55,6 +62,12 @@ class MemoryEagerGCSettingsInternal final explicit MemoryEagerGCSettingsInternal( const CoreMemoryEagerGcSettings& core_settings) : settings_(std::move(core_settings)) {} + + friend bool operator==(const MemoryEagerGCSettingsInternal& lhs, + const MemoryEagerGCSettingsInternal& rhs) { + return &lhs == &rhs || lhs.settings_ == rhs.settings_; + } + const CoreMemoryEagerGcSettings& core_settings() { return settings_; } void set_core_settings(const CoreMemoryEagerGcSettings& settings) { settings_ = settings; @@ -70,6 +83,12 @@ class MemoryLruGCSettingsInternal final explicit MemoryLruGCSettingsInternal( const CoreMemoryLruGcSettings& core_settings) : settings_(std::move(core_settings)) {} + + friend bool operator==(const MemoryLruGCSettingsInternal& lhs, + const MemoryLruGCSettingsInternal& rhs) { + return &lhs == &rhs || lhs.settings_ == rhs.settings_; + } + const CoreMemoryLruGcSettings& core_settings() { return settings_; } void set_core_settings(const CoreMemoryLruGcSettings& settings) { settings_ = settings; @@ -83,6 +102,12 @@ class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { public: explicit MemoryCacheSettingsInternal(const CoreMemorySettings& core_settings) : settings_(std::move(core_settings)) {} + + friend bool operator==(const MemoryCacheSettingsInternal& lhs, + const MemoryCacheSettingsInternal& rhs) { + return &lhs == &rhs || lhs.settings_ == rhs.settings_; + } + const CoreMemorySettings& core_settings() { return settings_; } void set_core_settings(const CoreMemorySettings& settings) { settings_ = settings; @@ -92,6 +117,18 @@ class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { CoreMemorySettings settings_; }; +bool operator!=(const MemoryCacheSettingsInternal& lhs, + const MemoryCacheSettingsInternal& rhs); + +bool operator!=(const MemoryLruGCSettingsInternal& lhs, + const MemoryLruGCSettingsInternal& rhs); + +bool operator!=(const MemoryEagerGCSettingsInternal& lhs, + const MemoryEagerGCSettingsInternal& rhs); + +bool operator!=(const PersistentCacheSettingsInternal& lhs, + const PersistentCacheSettingsInternal& rhs); + } // namespace firestore } // namespace firebase From 16927381c34b114a852038cba6e1edd4702bdff7 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 10 May 2023 10:41:50 -0400 Subject: [PATCH 10/22] add equality tests --- firestore/CMakeLists.txt | 2 - .../src/settings_test.cc | 98 +++++++++++++++++++ firestore/src/common/local_cache_settings.cc | 5 + firestore/src/common/settings.cc | 19 ++++ .../firebase/firestore/local_cache_settings.h | 3 + .../src/include/firebase/firestore/settings.h | 11 +-- .../src/main/local_cache_settings_main.cc | 46 --------- .../src/main/local_cache_settings_main.h | 30 +++--- 8 files changed, 147 insertions(+), 67 deletions(-) delete mode 100644 firestore/src/main/local_cache_settings_main.cc diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index 760517b25a..cf5c239f88 100644 --- a/firestore/CMakeLists.txt +++ b/firestore/CMakeLists.txt @@ -214,8 +214,6 @@ set(main_SRCS src/main/listener_main.h src/main/listener_registration_main.cc src/main/listener_registration_main.h - src/main/local_cache_settings_main.cc - src/main/local_cache_settings_main.h src/main/promise_factory_main.h src/main/promise_main.h src/main/query_main.cc diff --git a/firestore/integration_test_internal/src/settings_test.cc b/firestore/integration_test_internal/src/settings_test.cc index 6fb7d9dfba..a22e922f19 100644 --- a/firestore/integration_test_internal/src/settings_test.cc +++ b/firestore/integration_test_internal/src/settings_test.cc @@ -17,6 +17,7 @@ #include #include "firebase/firestore.h" +#include "firebase/firestore/local_cache_settings.h" #include "firebase_test_framework.h" #include "gtest/gtest.h" @@ -95,6 +96,103 @@ TEST(SettingsTest, Equality) { EXPECT_TRUE(settings4 != settings5); } +TEST(SettingsTest, EqualityWithLocalCacheSettings) { + constexpr int64_t kFiveMb = 5 * 1024 * 1024; + constexpr int64_t kSixMb = 6 * 1024 * 1024; + + Settings settings1; + settings1.set_host("foo"); + settings1.set_ssl_enabled(true); + settings1.set_local_cache_settings( + PersistentCacheSettings::Create().WithSizeBytes(kFiveMb)); + + Settings settings2; + settings2.set_host("bar"); + settings2.set_ssl_enabled(true); + settings2.set_local_cache_settings( + PersistentCacheSettings::Create().WithSizeBytes(kFiveMb)); + + Settings settings3; + settings3.set_host("foo"); + settings3.set_ssl_enabled(false); + settings3.set_local_cache_settings( + PersistentCacheSettings::Create().WithSizeBytes(kFiveMb)); + + Settings settings4; + settings4.set_host("foo"); + settings4.set_ssl_enabled(true); + settings4.set_local_cache_settings(MemoryCacheSettings::Create()); + + Settings settings5; + settings5.set_host("foo"); + settings5.set_ssl_enabled(true); + settings5.set_local_cache_settings( + PersistentCacheSettings::Create().WithSizeBytes(kSixMb)); + + Settings settings6; + settings6.set_host("foo"); + settings6.set_ssl_enabled(true); + settings6.set_local_cache_settings( + MemoryCacheSettings::Create().WithGarbageCollectorSettings( + MemoryEagerGCSettings::Create())); + + Settings settings7; + settings7.set_host("foo"); + settings7.set_ssl_enabled(true); + settings7.set_local_cache_settings( + MemoryCacheSettings::Create().WithGarbageCollectorSettings( + MemoryLruGCSettings::Create().WithSizeBytes(kFiveMb))); + + Settings settings8; + settings8.set_host("foo"); + settings8.set_ssl_enabled(true); + settings8.set_local_cache_settings( + MemoryCacheSettings::Create().WithGarbageCollectorSettings( + MemoryLruGCSettings::Create().WithSizeBytes(kSixMb))); + + // Same as settings7 + Settings settings9; + settings9.set_host("foo"); + settings9.set_ssl_enabled(true); + settings9.set_local_cache_settings( + MemoryCacheSettings::Create().WithGarbageCollectorSettings( + MemoryLruGCSettings::Create().WithSizeBytes(kFiveMb))); + + EXPECT_TRUE(settings1 == settings1); + EXPECT_TRUE(settings6 == settings4); + EXPECT_TRUE(settings7 == settings9); + + EXPECT_FALSE(settings1 == settings2); + EXPECT_FALSE(settings1 == settings3); + EXPECT_FALSE(settings1 == settings4); + EXPECT_FALSE(settings1 == settings5); + EXPECT_FALSE(settings2 == settings3); + EXPECT_FALSE(settings2 == settings4); + EXPECT_FALSE(settings2 == settings5); + EXPECT_FALSE(settings3 == settings4); + EXPECT_FALSE(settings3 == settings5); + EXPECT_FALSE(settings4 == settings5); + EXPECT_FALSE(settings6 == settings7); + EXPECT_FALSE(settings7 == settings8); + + EXPECT_FALSE(settings1 != settings1); + EXPECT_FALSE(settings6 != settings4); + EXPECT_FALSE(settings7 != settings9); + + EXPECT_TRUE(settings1 != settings2); + EXPECT_TRUE(settings1 != settings3); + EXPECT_TRUE(settings1 != settings4); + EXPECT_TRUE(settings1 != settings5); + EXPECT_TRUE(settings2 != settings3); + EXPECT_TRUE(settings2 != settings4); + EXPECT_TRUE(settings2 != settings5); + EXPECT_TRUE(settings3 != settings4); + EXPECT_TRUE(settings3 != settings5); + EXPECT_TRUE(settings4 != settings5); + EXPECT_TRUE(settings6 != settings7); + EXPECT_TRUE(settings7 != settings8); +} + } // namespace } // namespace firestore } // namespace firebase diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index a39ce7514d..ac2907c88a 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -26,6 +26,11 @@ namespace firebase { namespace firestore { +bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { + return lhs.kind() == rhs.kind() && + lhs.core_cache_settings() == rhs.core_cache_settings(); +} + PersistentCacheSettings PersistentCacheSettings::Create() { return {}; } PersistentCacheSettings PersistentCacheSettings::CreateFromCoreSettings( diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 45c3a4dac9..29553377cb 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -122,6 +122,25 @@ std::ostream& operator<<(std::ostream& out, const Settings& settings) { return out << settings.ToString(); } +bool operator==(const Settings& lhs, const Settings& rhs) { + bool eq = lhs.host() == rhs.host() && + lhs.is_ssl_enabled() == rhs.is_ssl_enabled() && + lhs.is_persistence_enabled() == rhs.is_persistence_enabled() && + lhs.cache_size_bytes() == rhs.cache_size_bytes(); + + if (eq) { + if (lhs.local_cache_settings_ != rhs.local_cache_settings_) { + if (lhs.local_cache_settings_ != nullptr && + rhs.local_cache_settings_ != nullptr) { + eq = (*lhs.local_cache_settings_ == *rhs.local_cache_settings_); + } else { + eq = false; + } + } + } + return eq; +} + // Apple uses a different mechanism, defined in `settings_apple.mm`. #if !defined(__APPLE__) && !defined(__ANDROID__) std::unique_ptr Settings::CreateExecutor() const { diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 308a8467d4..b8421e5d4c 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -46,6 +46,9 @@ class LocalCacheSettings { public: virtual ~LocalCacheSettings() = default; + friend bool operator==(const LocalCacheSettings& lhs, + const LocalCacheSettings& rhs); + private: friend class FirestoreInternal; friend class Settings; diff --git a/firestore/src/include/firebase/firestore/settings.h b/firestore/src/include/firebase/firestore/settings.h index 1868a77678..8280790cae 100644 --- a/firestore/src/include/firebase/firestore/settings.h +++ b/firestore/src/include/firebase/firestore/settings.h @@ -234,6 +234,9 @@ class Settings final { */ friend std::ostream& operator<<(std::ostream& out, const Settings& settings); + /** Checks `lhs` and `rhs` for equality. */ + friend bool operator==(const Settings& lhs, const Settings& rhs); + private: static constexpr int64_t kDefaultCacheSizeBytes = 100 * 1024 * 1024; @@ -258,14 +261,6 @@ class Settings final { #endif }; -/** Checks `lhs` and `rhs` for equality. */ -inline bool operator==(const Settings& lhs, const Settings& rhs) { - return lhs.host() == rhs.host() && - lhs.is_ssl_enabled() == rhs.is_ssl_enabled() && - lhs.is_persistence_enabled() == rhs.is_persistence_enabled() && - lhs.cache_size_bytes() == rhs.cache_size_bytes(); -} - /** Checks `lhs` and `rhs` for inequality. */ inline bool operator!=(const Settings& lhs, const Settings& rhs) { return !(lhs == rhs); diff --git a/firestore/src/main/local_cache_settings_main.cc b/firestore/src/main/local_cache_settings_main.cc deleted file mode 100644 index e5d1a3e8e6..0000000000 --- a/firestore/src/main/local_cache_settings_main.cc +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright 2023 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include -#include - -#include "firestore/src/main/local_cache_settings_main.h" - -namespace firebase { -namespace firestore { - -bool operator!=(const MemoryCacheSettingsInternal& lhs, - const MemoryCacheSettingsInternal& rhs) { - return !(lhs == rhs); -} - -bool operator!=(const MemoryLruGCSettingsInternal& lhs, - const MemoryLruGCSettingsInternal& rhs) { - return !(lhs == rhs); -} - -bool operator!=(const MemoryEagerGCSettingsInternal& lhs, - const MemoryEagerGCSettingsInternal& rhs) { - return !(lhs == rhs); -} - -bool operator!=(const PersistentCacheSettingsInternal& lhs, - const PersistentCacheSettingsInternal& rhs) { - return !(lhs == rhs); -} - -} // namespace firestore -} // namespace firebase diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index 5e7afe4d2d..d209454ec8 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -117,17 +117,25 @@ class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { CoreMemorySettings settings_; }; -bool operator!=(const MemoryCacheSettingsInternal& lhs, - const MemoryCacheSettingsInternal& rhs); - -bool operator!=(const MemoryLruGCSettingsInternal& lhs, - const MemoryLruGCSettingsInternal& rhs); - -bool operator!=(const MemoryEagerGCSettingsInternal& lhs, - const MemoryEagerGCSettingsInternal& rhs); - -bool operator!=(const PersistentCacheSettingsInternal& lhs, - const PersistentCacheSettingsInternal& rhs); +inline bool operator!=(const MemoryCacheSettingsInternal& lhs, + const MemoryCacheSettingsInternal& rhs) { + return !(lhs == rhs); +} + +inline bool operator!=(const MemoryLruGCSettingsInternal& lhs, + const MemoryLruGCSettingsInternal& rhs) { + return !(lhs == rhs); +} + +inline bool operator!=(const MemoryEagerGCSettingsInternal& lhs, + const MemoryEagerGCSettingsInternal& rhs) { + return !(lhs == rhs); +} + +inline bool operator!=(const PersistentCacheSettingsInternal& lhs, + const PersistentCacheSettingsInternal& rhs) { + return !(lhs == rhs); +} } // namespace firestore } // namespace firebase From fa90de08511ab3118c9f6b1b9be1cb5c518f5694 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 10 May 2023 11:06:59 -0400 Subject: [PATCH 11/22] Address lint warnings --- firestore/src/common/local_cache_settings.cc | 10 +++++ .../firebase/firestore/local_cache_settings.h | 40 ++++++++----------- .../src/include/firebase/firestore/settings.h | 2 +- .../src/main/local_cache_settings_main.h | 39 ++++++++---------- 4 files changed, 45 insertions(+), 46 deletions(-) diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index ac2907c88a..3f39a52107 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -17,6 +17,7 @@ #include "firestore/src/include/firebase/firestore/local_cache_settings.h" #include +#include "Firestore/core/src/api/settings.h" #include "firestore/src/common/hard_assert_common.h" #if defined(__ANDROID__) #else @@ -26,6 +27,15 @@ namespace firebase { namespace firestore { +namespace { +using CoreCacheSettings = api::LocalCacheSettings; +using CorePersistentSettings = api::PersistentCacheSettings; +using CoreMemorySettings = api::MemoryCacheSettings; +using CoreMemoryGarbageCollectorSettings = api::MemoryGargabeCollectorSettings; +using CoreMemoryEagerGcSettings = api::MemoryEagerGcSettings; +using CoreMemoryLruGcSettings = api::MemoryLruGcSettings; +} // namespace + bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { return lhs.kind() == rhs.kind() && lhs.core_cache_settings() == rhs.core_cache_settings(); diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index b8421e5d4c..9cc506ab9e 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -28,11 +28,6 @@ namespace firebase { namespace firestore { -namespace { -using CoreCacheSettings = api::LocalCacheSettings; -using CoreMemoryGarbageCollectorSettings = api::MemoryGargabeCollectorSettings; -} // namespace - class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; @@ -46,6 +41,7 @@ class LocalCacheSettings { public: virtual ~LocalCacheSettings() = default; + /** Equality function. */ friend bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs); @@ -54,7 +50,7 @@ class LocalCacheSettings { friend class Settings; virtual api::LocalCacheSettings::Kind kind() const = 0; - virtual const CoreCacheSettings& core_cache_settings() const = 0; + virtual const api::LocalCacheSettings& core_cache_settings() const = 0; }; /** @@ -80,6 +76,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { ~PersistentCacheSettings(); + /** Equality function. */ friend bool operator==(const PersistentCacheSettings& lhs, const PersistentCacheSettings& rhs); @@ -111,7 +108,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { friend class FirestoreInternal; static PersistentCacheSettings CreateFromCoreSettings( - const CorePersistentSettings& core_settings); + const api::PersistentCacheSettings& core_settings); PersistentCacheSettings(); api::LocalCacheSettings::Kind kind() const override { @@ -119,7 +116,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { } // Get the corresponding settings object from the core sdk. - const CoreCacheSettings& core_cache_settings() const override; + const api::LocalCacheSettings& core_cache_settings() const override; std::unique_ptr settings_internal_; }; @@ -147,6 +144,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { ~MemoryCacheSettings(); + /** Equality function. */ friend bool operator==(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs); @@ -162,7 +160,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { friend class FirestoreInternal; static MemoryCacheSettings CreateFromCoreSettings( - const CoreMemorySettings& core_settings); + const api::MemoryCacheSettings& core_settings); MemoryCacheSettings(); api::LocalCacheSettings::Kind kind() const override { @@ -170,7 +168,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { } // Get the corresponding settings object from the core sdk. - const CoreCacheSettings& core_cache_settings() const override; + const api::LocalCacheSettings& core_cache_settings() const override; std::unique_ptr settings_internal_; }; @@ -188,7 +186,7 @@ class MemoryGarbageCollectorSettings { private: friend class MemoryCacheSettings; - virtual const CoreMemoryGarbageCollectorSettings& core_gc_settings() + virtual const api::MemoryGargabeCollectorSettings& core_gc_settings() const = 0; }; @@ -214,6 +212,7 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { ~MemoryEagerGCSettings(); + /** Equality function. */ friend bool operator==(const MemoryEagerGCSettings& lhs, const MemoryEagerGCSettings& rhs); @@ -221,7 +220,7 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { friend class MemoryCacheSettings; MemoryEagerGCSettings(); - const CoreMemoryGarbageCollectorSettings& core_gc_settings() const override { + const api::MemoryGargabeCollectorSettings& core_gc_settings() const override { return settings_internal_->core_settings(); } @@ -250,6 +249,7 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { static MemoryLruGCSettings Create(); ~MemoryLruGCSettings(); + /** Equality function. */ friend bool operator==(const MemoryLruGCSettings& lhs, const MemoryLruGCSettings& rhs); @@ -281,31 +281,25 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { MemoryLruGCSettings(); MemoryLruGCSettings(const MemoryLruGCSettingsInternal& other); - const CoreMemoryGarbageCollectorSettings& core_gc_settings() const override { + const api::MemoryGargabeCollectorSettings& core_gc_settings() const override { return settings_internal_->core_settings(); } std::unique_ptr settings_internal_; }; -bool operator==(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs); - +/** Inequality function. */ bool operator!=(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs); -bool operator==(const PersistentCacheSettings& lhs, - const PersistentCacheSettings& rhs); - +/** Inequality function. */ bool operator!=(const PersistentCacheSettings& lhs, const PersistentCacheSettings& rhs); -bool operator==(const MemoryEagerGCSettings& lhs, - const MemoryEagerGCSettings& rhs); - +/** Inequality function. */ bool operator!=(const MemoryEagerGCSettings& lhs, const MemoryEagerGCSettings& rhs); -bool operator==(const MemoryLruGCSettings& lhs, const MemoryLruGCSettings& rhs); - +/** Inequality function. */ bool operator!=(const MemoryLruGCSettings& lhs, const MemoryLruGCSettings& rhs); } // namespace firestore diff --git a/firestore/src/include/firebase/firestore/settings.h b/firestore/src/include/firebase/firestore/settings.h index 8280790cae..ce1b0348bf 100644 --- a/firestore/src/include/firebase/firestore/settings.h +++ b/firestore/src/include/firebase/firestore/settings.h @@ -154,7 +154,7 @@ class Settings final { * * @param cache_settings Settings object to configue this SDK. */ - void set_local_cache_settings(const LocalCacheSettings& cache); + void set_local_cache_settings(const LocalCacheSettings& cache_settings); /** * NOTE: This method is deprecated in favor of `set_local_cache_settings()`. diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index d209454ec8..1e5fc85a3b 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -25,19 +25,13 @@ namespace firebase { namespace firestore { -namespace { -using CoreMemoryEagerGcSettings = api::MemoryEagerGcSettings; -using CoreMemoryLruGcSettings = api::MemoryLruGcSettings; -using CoreMemorySettings = api::MemoryCacheSettings; -using CorePersistentSettings = api::PersistentCacheSettings; -} // namespace class LocalCacheSettingsInternal {}; class PersistentCacheSettingsInternal final : public LocalCacheSettingsInternal { public: explicit PersistentCacheSettingsInternal( - const CorePersistentSettings& core_settings) + const api::PersistentCacheSettings& core_settings) : settings_(std::move(core_settings)) {} friend bool operator==(const PersistentCacheSettingsInternal& lhs, @@ -45,13 +39,13 @@ class PersistentCacheSettingsInternal final return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const CorePersistentSettings& core_settings() { return settings_; } - void set_core_settings(const CorePersistentSettings& settings) { + const api::PersistentCacheSettings& core_settings() { return settings_; } + void set_core_settings(const api::PersistentCacheSettings& settings) { settings_ = settings; } private: - CorePersistentSettings settings_; + api::PersistentCacheSettings settings_; }; class MemoryGarbageCollectorSettingsInternal {}; @@ -60,7 +54,7 @@ class MemoryEagerGCSettingsInternal final : public MemoryGarbageCollectorSettingsInternal { public: explicit MemoryEagerGCSettingsInternal( - const CoreMemoryEagerGcSettings& core_settings) + const api::MemoryEagerGcSettings& core_settings) : settings_(std::move(core_settings)) {} friend bool operator==(const MemoryEagerGCSettingsInternal& lhs, @@ -68,20 +62,20 @@ class MemoryEagerGCSettingsInternal final return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const CoreMemoryEagerGcSettings& core_settings() { return settings_; } - void set_core_settings(const CoreMemoryEagerGcSettings& settings) { + const api::MemoryEagerGcSettings& core_settings() { return settings_; } + void set_core_settings(const api::MemoryEagerGcSettings& settings) { settings_ = settings; } private: - CoreMemoryEagerGcSettings settings_; + api::MemoryEagerGcSettings settings_; }; class MemoryLruGCSettingsInternal final : public MemoryGarbageCollectorSettingsInternal { public: explicit MemoryLruGCSettingsInternal( - const CoreMemoryLruGcSettings& core_settings) + const api::MemoryLruGcSettings& core_settings) : settings_(std::move(core_settings)) {} friend bool operator==(const MemoryLruGCSettingsInternal& lhs, @@ -89,18 +83,19 @@ class MemoryLruGCSettingsInternal final return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const CoreMemoryLruGcSettings& core_settings() { return settings_; } - void set_core_settings(const CoreMemoryLruGcSettings& settings) { + const api::MemoryLruGcSettings& core_settings() { return settings_; } + void set_core_settings(const api::MemoryLruGcSettings& settings) { settings_ = settings; } private: - CoreMemoryLruGcSettings settings_; + api::MemoryLruGcSettings settings_; }; class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { public: - explicit MemoryCacheSettingsInternal(const CoreMemorySettings& core_settings) + explicit MemoryCacheSettingsInternal( + const api::MemoryCacheSettings& core_settings) : settings_(std::move(core_settings)) {} friend bool operator==(const MemoryCacheSettingsInternal& lhs, @@ -108,13 +103,13 @@ class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const CoreMemorySettings& core_settings() { return settings_; } - void set_core_settings(const CoreMemorySettings& settings) { + const api::MemoryCacheSettings& core_settings() { return settings_; } + void set_core_settings(const api::MemoryCacheSettings& settings) { settings_ = settings; } private: - CoreMemorySettings settings_; + api::MemoryCacheSettings settings_; }; inline bool operator!=(const MemoryCacheSettingsInternal& lhs, From 45a3d66568367b44be1cd18a8bb9e1962769c6d5 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 15 May 2023 10:24:37 -0400 Subject: [PATCH 12/22] feedback --- .../src/firestore_test.cc | 95 ++++++++----------- firestore/src/common/local_cache_settings.cc | 20 ---- firestore/src/common/settings.cc | 2 +- .../firebase/firestore/local_cache_settings.h | 51 ++++++---- firestore/src/main/firestore_main.cc | 6 +- 5 files changed, 78 insertions(+), 96 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index f6220f922c..be0d2df0a2 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1504,73 +1504,70 @@ TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) { EXPECT_EQ(await_clear_persistence.error(), Error::kErrorFailedPrecondition); } -TEST_F(FirestoreIntegrationTest, LegacyCacheConfigForMemoryCacheWorks) { +class FirestoreCacheConfigTest : public FirestoreIntegrationTest { + protected: + void VerifyCachedDocumentDeletedImmediately(Firestore* db) { + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + ASSERT_NE(snapshot, nullptr); + ASSERT_FALSE(snapshot->is_valid()); + } + + void VerifyCachedDocumentStaysAround(Firestore* db) { + Await(db->Document("rooms/eros") + .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); + + auto get_future = db->Document("rooms/eros").Get(Source::kCache); + const DocumentSnapshot* snapshot = Await(get_future); + ASSERT_NE(snapshot, nullptr); + ASSERT_TRUE(snapshot->is_valid()); + ASSERT_THAT( + snapshot->GetData(), + ContainerEq(MapFieldValue{{"desc", FieldValue::String("eros")}})); + } +}; + +TEST_F(FirestoreCacheConfigTest, LegacyCacheConfigForMemoryCacheWorks) { auto* db = TestFirestore("legacy_memory_cache"); auto settings = db->settings(); WITH_DEPRECATED_API(settings.set_persistence_enabled(false)); db->set_settings(std::move(settings)); - Await(db->Document("rooms/eros") - .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); - - auto get_future = db->Document("rooms/eros").Get(Source::kCache); - const DocumentSnapshot* snapshot = Await(get_future); - EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); - EXPECT_FALSE(snapshot->is_valid()); - snapshot->id(); + VerifyCachedDocumentDeletedImmediately(db); } -TEST_F(FirestoreIntegrationTest, LegacyCacheConfigForPersistenceCacheWorks) { +TEST_F(FirestoreCacheConfigTest, LegacyCacheConfigForPersistenceCacheWorks) { auto* db = TestFirestore("legacy_persistent_cache"); auto settings = db->settings(); WITH_DEPRECATED_API(settings.set_persistence_enabled(true)); db->set_settings(std::move(settings)); - Await(db->Document("rooms/eros") - .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); - - auto get_future = db->Document("rooms/eros").Get(Source::kCache); - const DocumentSnapshot* snapshot = Await(get_future); - EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); - EXPECT_TRUE(snapshot->is_valid()); - EXPECT_THAT(snapshot->GetData(), - ContainerEq(MapFieldValue{{"desc", FieldValue::String("eros")}})); + VerifyCachedDocumentStaysAround(db); } -TEST_F(FirestoreIntegrationTest, NewCacheConfigForMemoryCacheWorks) { +TEST_F(FirestoreCacheConfigTest, NewCacheConfigForMemoryCacheWorks) { auto* db = TestFirestore("new_memory_cache"); auto settings = db->settings(); settings.set_local_cache_settings(MemoryCacheSettings::Create()); db->set_settings(std::move(settings)); - Await(db->Document("rooms/eros") - .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); - - auto get_future = db->Document("rooms/eros").Get(Source::kCache); - const DocumentSnapshot* snapshot = Await(get_future); - EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); - EXPECT_FALSE(snapshot->is_valid()); + VerifyCachedDocumentDeletedImmediately(db); } -TEST_F(FirestoreIntegrationTest, NewCacheConfigForPersistenceCacheWorks) { +TEST_F(FirestoreCacheConfigTest, NewCacheConfigForPersistenceCacheWorks) { auto* db = TestFirestore("new_persistent_cache"); auto settings = db->settings(); settings.set_local_cache_settings( PersistentCacheSettings::Create().WithSizeBytes(50 * 1024 * 1024)); db->set_settings(std::move(settings)); - Await(db->Document("rooms/eros") - .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); - - auto get_future = db->Document("rooms/eros").Get(Source::kCache); - const DocumentSnapshot* snapshot = Await(get_future); - EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); - EXPECT_TRUE(snapshot->is_valid()); - EXPECT_THAT(snapshot->GetData(), - ContainerEq(MapFieldValue{{"desc", FieldValue::String("eros")}})); + VerifyCachedDocumentStaysAround(db); } -TEST_F(FirestoreIntegrationTest, CannotMixNewAndLegacyCacheConfig) { +TEST_F(FirestoreCacheConfigTest, CannotMixNewAndLegacyCacheConfig) { { auto* db = TestFirestore("mixing_1"); auto settings = db->settings(); @@ -1591,7 +1588,7 @@ TEST_F(FirestoreIntegrationTest, CannotMixNewAndLegacyCacheConfig) { } } -TEST_F(FirestoreIntegrationTest, CanGetDocumentFromCacheWithMemoryLruGC) { +TEST_F(FirestoreCacheConfigTest, CanGetDocumentFromCacheWithMemoryLruGC) { auto* db = TestFirestore("new_persistent_cache"); auto settings = db->settings(); settings.set_local_cache_settings( @@ -1599,18 +1596,10 @@ TEST_F(FirestoreIntegrationTest, CanGetDocumentFromCacheWithMemoryLruGC) { MemoryLruGCSettings::Create())); db->set_settings(std::move(settings)); - Await(db->Document("rooms/eros") - .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); - - auto get_future = db->Document("rooms/eros").Get(Source::kCache); - const DocumentSnapshot* snapshot = Await(get_future); - EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); - EXPECT_TRUE(snapshot->is_valid()); - EXPECT_THAT(snapshot->GetData(), - ContainerEq(MapFieldValue{{"desc", FieldValue::String("eros")}})); + VerifyCachedDocumentStaysAround(db); } -TEST_F(FirestoreIntegrationTest, CannotGetDocumentFromCacheFromMemoryEagerGC) { +TEST_F(FirestoreCacheConfigTest, CannotGetDocumentFromCacheFromMemoryEagerGC) { auto* db = TestFirestore("new_persistent_cache"); auto settings = db->settings(); settings.set_local_cache_settings( @@ -1618,13 +1607,7 @@ TEST_F(FirestoreIntegrationTest, CannotGetDocumentFromCacheFromMemoryEagerGC) { MemoryEagerGCSettings::Create())); db->set_settings(std::move(settings)); - Await(db->Document("rooms/eros") - .Set(MapFieldValue{{"desc", FieldValue::String("eros")}})); - - auto get_future = db->Document("rooms/eros").Get(Source::kCache); - const DocumentSnapshot* snapshot = Await(get_future); - EXPECT_EQ(get_future.status(), FutureStatus::kFutureStatusComplete); - EXPECT_FALSE(snapshot->is_valid()); + VerifyCachedDocumentDeletedImmediately(db); } // Note: this test only exists in C++. diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index 3f39a52107..736144b4ca 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -168,40 +168,20 @@ bool operator==(const MemoryCacheSettings& lhs, return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); } -bool operator!=(const MemoryCacheSettings& lhs, - const MemoryCacheSettings& rhs) { - return !(lhs == rhs); -} - bool operator==(const PersistentCacheSettings& lhs, const PersistentCacheSettings& rhs) { return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); } -bool operator!=(const PersistentCacheSettings& lhs, - const PersistentCacheSettings& rhs) { - return !(lhs == rhs); -} - bool operator==(const MemoryEagerGCSettings& lhs, const MemoryEagerGCSettings& rhs) { return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); } -bool operator!=(const MemoryEagerGCSettings& lhs, - const MemoryEagerGCSettings& rhs) { - return !(lhs == rhs); -} - bool operator==(const MemoryLruGCSettings& lhs, const MemoryLruGCSettings& rhs) { return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); } -bool operator!=(const MemoryLruGCSettings& lhs, - const MemoryLruGCSettings& rhs) { - return !(lhs == rhs); -} - } // namespace firestore } // namespace firebase diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 29553377cb..3cd671f6b2 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -80,7 +80,7 @@ void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { "set_persistence_enabled() or set_cache_size_bytes()"); } - if (cache.kind() == api::LocalCacheSettings::Kind::kPersistent) { + if (cache.kind() == LocalCacheSettings::Kind::kPersistent) { local_cache_settings_ = std::make_shared( static_cast(cache)); } else { diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 9cc506ab9e..9b78c0e153 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -20,7 +20,6 @@ #include #include -#include "Firestore/core/src/api/settings.h" #include "firestore/src/include/firebase/firestore/settings.h" #include "firestore/src/main/firestore_main.h" #include "firestore/src/main/local_cache_settings_main.h" @@ -45,11 +44,14 @@ class LocalCacheSettings { friend bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs); + protected: + enum class Kind { kMemory, kPersistent }; + private: friend class FirestoreInternal; friend class Settings; - virtual api::LocalCacheSettings::Kind kind() const = 0; + virtual Kind kind() const = 0; virtual const api::LocalCacheSettings& core_cache_settings() const = 0; }; @@ -74,7 +76,13 @@ class PersistentCacheSettings final : public LocalCacheSettings { /** Copy assignment. */ PersistentCacheSettings& operator=(const PersistentCacheSettings& other); - ~PersistentCacheSettings(); + /** Move constructor. */ + PersistentCacheSettings(PersistentCacheSettings&& other) = default; + + /** Move assignment. */ + PersistentCacheSettings& operator=(PersistentCacheSettings&& other) = default; + + ~PersistentCacheSettings() override; /** Equality function. */ friend bool operator==(const PersistentCacheSettings& lhs, @@ -109,10 +117,11 @@ class PersistentCacheSettings final : public LocalCacheSettings { static PersistentCacheSettings CreateFromCoreSettings( const api::PersistentCacheSettings& core_settings); + PersistentCacheSettings(); - api::LocalCacheSettings::Kind kind() const override { - return api::LocalCacheSettings::Kind::kPersistent; + LocalCacheSettings::Kind kind() const override { + return LocalCacheSettings::Kind::kPersistent; } // Get the corresponding settings object from the core sdk. @@ -142,7 +151,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { /** Copy assignment. */ MemoryCacheSettings& operator=(const MemoryCacheSettings& other); - ~MemoryCacheSettings(); + ~MemoryCacheSettings() override; /** Equality function. */ friend bool operator==(const MemoryCacheSettings& lhs, @@ -163,8 +172,8 @@ class MemoryCacheSettings final : public LocalCacheSettings { const api::MemoryCacheSettings& core_settings); MemoryCacheSettings(); - api::LocalCacheSettings::Kind kind() const override { - return api::LocalCacheSettings::Kind::kMemory; + LocalCacheSettings::Kind kind() const override { + return LocalCacheSettings::Kind::kMemory; } // Get the corresponding settings object from the core sdk. @@ -210,7 +219,7 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { /** Create a default instance `MemoryEagerGCSettings`. */ static MemoryEagerGCSettings Create(); - ~MemoryEagerGCSettings(); + ~MemoryEagerGCSettings() override; /** Equality function. */ friend bool operator==(const MemoryEagerGCSettings& lhs, @@ -247,7 +256,7 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { public: /** Create a default instance `MemoryLruGCSettings`. */ static MemoryLruGCSettings Create(); - ~MemoryLruGCSettings(); + ~MemoryLruGCSettings() override; /** Equality function. */ friend bool operator==(const MemoryLruGCSettings& lhs, @@ -289,18 +298,28 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { }; /** Inequality function. */ -bool operator!=(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs); +inline bool operator!=(const MemoryCacheSettings& lhs, + const MemoryCacheSettings& rhs) { + return !(lhs == rhs); +} /** Inequality function. */ -bool operator!=(const PersistentCacheSettings& lhs, - const PersistentCacheSettings& rhs); +inline bool operator!=(const PersistentCacheSettings& lhs, + const PersistentCacheSettings& rhs) { + return !(lhs == rhs); +} /** Inequality function. */ -bool operator!=(const MemoryEagerGCSettings& lhs, - const MemoryEagerGCSettings& rhs); +inline bool operator!=(const MemoryEagerGCSettings& lhs, + const MemoryEagerGCSettings& rhs) { + return !(lhs == rhs); +} /** Inequality function. */ -bool operator!=(const MemoryLruGCSettings& lhs, const MemoryLruGCSettings& rhs); +inline bool operator!=(const MemoryLruGCSettings& lhs, + const MemoryLruGCSettings& rhs) { + return !(lhs == rhs); +} } // namespace firestore } // namespace firebase diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index b6a057daa1..8d5d2fbdce 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -33,6 +33,7 @@ #include "Firestore/core/src/util/executor.h" #include "Firestore/core/src/util/log.h" #include "Firestore/core/src/util/status.h" +#include "Firestore/core/src/util/warnings.h" #include "absl/memory/memory.h" #include "absl/types/any.h" #include "app/src/include/firebase/future.h" @@ -196,11 +197,10 @@ Settings FirestoreInternal::settings() const { // `used_legacy_cache_settings_` as false such that new settings API is not // rejected by runtime checks. This should be removed when legacy API is // removed. -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wdeprecated-declarations" + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN() result.set_persistence_enabled(from.persistence_enabled()); result.set_cache_size_bytes(from.cache_size_bytes()); -#pragma clang diagnostic pop + SUPPRESS_END() result.used_legacy_cache_settings_ = false; return result; From 5be24a403e8f7ea43e03fea7a47f8863f2a115b8 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Tue, 23 May 2023 14:48:38 -0400 Subject: [PATCH 13/22] Use original deprecation suppress macro --- .../src/firestore_test.cc | 18 ++++++--- .../src/settings_test.cc | 37 +++++++++++++------ 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 94464ec001..ce7bc44425 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -47,6 +47,7 @@ #include "util/future_test_util.h" #if !defined(__ANDROID__) #include "Firestore/core/src/util/autoid.h" +#include "Firestore/core/src/util/warnings.h" #include "firestore/src/main/converter_main.h" #include "firestore/src/main/firestore_main.h" #else @@ -1791,7 +1792,9 @@ class FirestoreCacheConfigTest : public FirestoreIntegrationTest { TEST_F(FirestoreCacheConfigTest, LegacyCacheConfigForMemoryCacheWorks) { auto* db = TestFirestore("legacy_memory_cache"); auto settings = db->settings(); - WITH_DEPRECATED_API(settings.set_persistence_enabled(false)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings.set_persistence_enabled(false); +SUPPRESS_END(); db->set_settings(std::move(settings)); VerifyCachedDocumentDeletedImmediately(db); @@ -1800,7 +1803,9 @@ TEST_F(FirestoreCacheConfigTest, LegacyCacheConfigForMemoryCacheWorks) { TEST_F(FirestoreCacheConfigTest, LegacyCacheConfigForPersistenceCacheWorks) { auto* db = TestFirestore("legacy_persistent_cache"); auto settings = db->settings(); - WITH_DEPRECATED_API(settings.set_persistence_enabled(true)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings.set_persistence_enabled(true); +SUPPRESS_END(); db->set_settings(std::move(settings)); VerifyCachedDocumentStaysAround(db); @@ -1832,14 +1837,17 @@ TEST_F(FirestoreCacheConfigTest, CannotMixNewAndLegacyCacheConfig) { settings.set_local_cache_settings( PersistentCacheSettings::Create().WithSizeBytes(50 * 1024 * 1024)); - WITH_DEPRECATED_API( - EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error); +SUPPRESS_END(); } { auto* db = TestFirestore("mixing_2"); auto settings = db->settings(); - WITH_DEPRECATED_API(settings.set_persistence_enabled(false)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings.set_persistence_enabled(false); +SUPPRESS_END(); EXPECT_THROW( settings.set_local_cache_settings(MemoryCacheSettings::Create()), std::logic_error); diff --git a/firestore/integration_test_internal/src/settings_test.cc b/firestore/integration_test_internal/src/settings_test.cc index a22e922f19..4da9bd71f1 100644 --- a/firestore/integration_test_internal/src/settings_test.cc +++ b/firestore/integration_test_internal/src/settings_test.cc @@ -19,6 +19,7 @@ #include "firebase/firestore.h" #include "firebase/firestore/local_cache_settings.h" #include "firebase_test_framework.h" +#include "Firestore/core/src/util/warnings.h" #include "gtest/gtest.h" @@ -33,39 +34,51 @@ TEST(SettingsTest, Equality) { Settings settings1; settings1.set_host("foo"); settings1.set_ssl_enabled(true); - WITH_DEPRECATED_API(settings1.set_persistence_enabled(true)); - WITH_DEPRECATED_API(settings1.set_cache_size_bytes(kFiveMb)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings1.set_persistence_enabled(true); + settings1.set_cache_size_bytes(kFiveMb); +SUPPRESS_END(); Settings settings2; settings2.set_host("bar"); settings2.set_ssl_enabled(true); - WITH_DEPRECATED_API(settings2.set_persistence_enabled(true)); - WITH_DEPRECATED_API(settings2.set_cache_size_bytes(kFiveMb)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings2.set_persistence_enabled(true); + settings2.set_cache_size_bytes(kFiveMb); +SUPPRESS_END(); Settings settings3; settings3.set_host("foo"); settings3.set_ssl_enabled(false); - WITH_DEPRECATED_API(settings3.set_persistence_enabled(true)); - WITH_DEPRECATED_API(settings3.set_cache_size_bytes(kFiveMb)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings3.set_persistence_enabled(true); + settings3.set_cache_size_bytes(kFiveMb); +SUPPRESS_END(); Settings settings4; settings4.set_host("foo"); settings4.set_ssl_enabled(true); - WITH_DEPRECATED_API(settings4.set_persistence_enabled(false)); - WITH_DEPRECATED_API(settings4.set_cache_size_bytes(kFiveMb)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings4.set_persistence_enabled(false); + settings4.set_cache_size_bytes(kFiveMb); +SUPPRESS_END(); Settings settings5; settings5.set_host("foo"); settings5.set_ssl_enabled(true); - WITH_DEPRECATED_API(settings5.set_persistence_enabled(true)); - WITH_DEPRECATED_API(settings5.set_cache_size_bytes(kSixMb)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings5.set_persistence_enabled(true); + settings5.set_cache_size_bytes(kSixMb); +SUPPRESS_END(); // This is the same as settings4. Settings settings6; settings6.set_host("foo"); settings6.set_ssl_enabled(true); - WITH_DEPRECATED_API(settings6.set_persistence_enabled(false)); - WITH_DEPRECATED_API(settings6.set_cache_size_bytes(kFiveMb)); +SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + settings6.set_persistence_enabled(false); + settings6.set_cache_size_bytes(kFiveMb); +SUPPRESS_END(); EXPECT_TRUE(settings1 == settings1); EXPECT_TRUE(settings6 == settings4); From a8db1403f2cb17f2c78ec7c44069d93ca179a691 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Tue, 23 May 2023 14:57:15 -0400 Subject: [PATCH 14/22] Use refs instead of shared_ptr in api and remove dead code --- firestore/src/common/local_cache_settings.cc | 16 ---------------- firestore/src/common/settings.cc | 4 ++-- .../firebase/firestore/local_cache_settings.h | 5 ----- .../src/include/firebase/firestore/settings.h | 2 +- firestore/src/main/firestore_main.cc | 2 +- 5 files changed, 4 insertions(+), 25 deletions(-) diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index 736144b4ca..668ebb6821 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -43,14 +43,6 @@ bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { PersistentCacheSettings PersistentCacheSettings::Create() { return {}; } -PersistentCacheSettings PersistentCacheSettings::CreateFromCoreSettings( - const CorePersistentSettings& core_settings) { - auto result = PersistentCacheSettings{}; - result.settings_internal_ = - std::make_unique(core_settings); - return result; -} - PersistentCacheSettings::PersistentCacheSettings() { settings_internal_ = std::make_unique( CorePersistentSettings{}); @@ -118,14 +110,6 @@ MemoryLruGCSettings MemoryLruGCSettings::WithSizeBytes(int64_t size) { MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } -MemoryCacheSettings MemoryCacheSettings::CreateFromCoreSettings( - const CoreMemorySettings& core_settings) { - auto result = MemoryCacheSettings{}; - result.settings_internal_ = - std::make_unique(core_settings); - return result; -} - MemoryCacheSettings::MemoryCacheSettings() { settings_internal_ = std::make_unique(CoreMemorySettings{}); diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 3cd671f6b2..523d37c7a9 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -56,7 +56,7 @@ void Settings::set_host(std::string host) { host_ = firebase::Move(host); } void Settings::set_ssl_enabled(bool enabled) { ssl_enabled_ = enabled; } -std::shared_ptr Settings::local_cache_settings() { +const LocalCacheSettings& Settings::local_cache_settings() { if (used_legacy_cache_settings_) { if (is_persistence_enabled()) { local_cache_settings_ = std::make_shared( @@ -70,7 +70,7 @@ std::shared_ptr Settings::local_cache_settings() { PersistentCacheSettings::Create()); } - return local_cache_settings_; + return *local_cache_settings_; } void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 9b78c0e153..e84d40a9f6 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -115,9 +115,6 @@ class PersistentCacheSettings final : public LocalCacheSettings { friend class Settings; friend class FirestoreInternal; - static PersistentCacheSettings CreateFromCoreSettings( - const api::PersistentCacheSettings& core_settings); - PersistentCacheSettings(); LocalCacheSettings::Kind kind() const override { @@ -168,8 +165,6 @@ class MemoryCacheSettings final : public LocalCacheSettings { friend class Settings; friend class FirestoreInternal; - static MemoryCacheSettings CreateFromCoreSettings( - const api::MemoryCacheSettings& core_settings); MemoryCacheSettings(); LocalCacheSettings::Kind kind() const override { diff --git a/firestore/src/include/firebase/firestore/settings.h b/firestore/src/include/firebase/firestore/settings.h index ce1b0348bf..cf1b8b7e86 100644 --- a/firestore/src/include/firebase/firestore/settings.h +++ b/firestore/src/include/firebase/firestore/settings.h @@ -142,7 +142,7 @@ class Settings final { * Returns a shared pointer to the `LocalCacheSettings` instance * used to configure this SDK. */ - std::shared_ptr local_cache_settings(); + const LocalCacheSettings& local_cache_settings(); /** * Configures the SDK with the given `LocalCacheSettings` instance. diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 87e00a3bc6..82dece9642 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -221,7 +221,7 @@ void FirestoreInternal::set_settings(Settings from) { if (!from.used_legacy_cache_settings_ && from.local_cache_settings_ != nullptr) { settings.set_local_cache_settings( - from.local_cache_settings()->core_cache_settings()); + from.local_cache_settings().core_cache_settings()); } else { settings.set_persistence_enabled(from.is_persistence_enabled()); settings.set_cache_size_bytes(from.cache_size_bytes()); From d56a2d445fa49d45f52fc736f346c6735ee60b20 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 29 May 2023 14:23:42 -0400 Subject: [PATCH 15/22] Better cache config source management + others --- .../src/firestore_test.cc | 18 ++--- .../src/settings_test.cc | 26 ++++---- firestore/src/common/settings.cc | 66 ++++++++++++------- firestore/src/common/settings_apple.mm | 5 +- .../firebase/firestore/local_cache_settings.h | 6 ++ .../src/include/firebase/firestore/settings.h | 7 +- firestore/src/main/firestore_main.cc | 8 +-- 7 files changed, 81 insertions(+), 55 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index ce7bc44425..9ca9f57cc1 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1792,9 +1792,9 @@ class FirestoreCacheConfigTest : public FirestoreIntegrationTest { TEST_F(FirestoreCacheConfigTest, LegacyCacheConfigForMemoryCacheWorks) { auto* db = TestFirestore("legacy_memory_cache"); auto settings = db->settings(); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings.set_persistence_enabled(false); -SUPPRESS_END(); + SUPPRESS_END(); db->set_settings(std::move(settings)); VerifyCachedDocumentDeletedImmediately(db); @@ -1803,9 +1803,9 @@ SUPPRESS_END(); TEST_F(FirestoreCacheConfigTest, LegacyCacheConfigForPersistenceCacheWorks) { auto* db = TestFirestore("legacy_persistent_cache"); auto settings = db->settings(); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings.set_persistence_enabled(true); -SUPPRESS_END(); + SUPPRESS_END(); db->set_settings(std::move(settings)); VerifyCachedDocumentStaysAround(db); @@ -1837,17 +1837,17 @@ TEST_F(FirestoreCacheConfigTest, CannotMixNewAndLegacyCacheConfig) { settings.set_local_cache_settings( PersistentCacheSettings::Create().WithSizeBytes(50 * 1024 * 1024)); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); - EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error); -SUPPRESS_END(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + EXPECT_THROW(settings.set_cache_size_bytes(0), std::logic_error); + SUPPRESS_END(); } { auto* db = TestFirestore("mixing_2"); auto settings = db->settings(); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings.set_persistence_enabled(false); -SUPPRESS_END(); + SUPPRESS_END(); EXPECT_THROW( settings.set_local_cache_settings(MemoryCacheSettings::Create()), std::logic_error); diff --git a/firestore/integration_test_internal/src/settings_test.cc b/firestore/integration_test_internal/src/settings_test.cc index 4da9bd71f1..19e52269ca 100644 --- a/firestore/integration_test_internal/src/settings_test.cc +++ b/firestore/integration_test_internal/src/settings_test.cc @@ -16,10 +16,10 @@ #include +#include "Firestore/core/src/util/warnings.h" #include "firebase/firestore.h" #include "firebase/firestore/local_cache_settings.h" #include "firebase_test_framework.h" -#include "Firestore/core/src/util/warnings.h" #include "gtest/gtest.h" @@ -34,51 +34,51 @@ TEST(SettingsTest, Equality) { Settings settings1; settings1.set_host("foo"); settings1.set_ssl_enabled(true); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings1.set_persistence_enabled(true); settings1.set_cache_size_bytes(kFiveMb); -SUPPRESS_END(); + SUPPRESS_END(); Settings settings2; settings2.set_host("bar"); settings2.set_ssl_enabled(true); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings2.set_persistence_enabled(true); settings2.set_cache_size_bytes(kFiveMb); -SUPPRESS_END(); + SUPPRESS_END(); Settings settings3; settings3.set_host("foo"); settings3.set_ssl_enabled(false); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings3.set_persistence_enabled(true); settings3.set_cache_size_bytes(kFiveMb); -SUPPRESS_END(); + SUPPRESS_END(); Settings settings4; settings4.set_host("foo"); settings4.set_ssl_enabled(true); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings4.set_persistence_enabled(false); settings4.set_cache_size_bytes(kFiveMb); -SUPPRESS_END(); + SUPPRESS_END(); Settings settings5; settings5.set_host("foo"); settings5.set_ssl_enabled(true); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings5.set_persistence_enabled(true); settings5.set_cache_size_bytes(kSixMb); -SUPPRESS_END(); + SUPPRESS_END(); // This is the same as settings4. Settings settings6; settings6.set_host("foo"); settings6.set_ssl_enabled(true); -SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); + SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN(); settings6.set_persistence_enabled(false); settings6.set_cache_size_bytes(kFiveMb); -SUPPRESS_END(); + SUPPRESS_END(); EXPECT_TRUE(settings1 == settings1); EXPECT_TRUE(settings6 == settings4); diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 523d37c7a9..ea2ba0f62e 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -49,7 +49,10 @@ std::string ToStr(int64_t v) { } // namespace #if !defined(__APPLE__) -Settings::Settings() : host_(kDefaultHost) {} +Settings::Settings() + : host_(kDefaultHost), + local_cache_settings_(std::make_shared( + PersistentCacheSettings::Create())) {} #endif void Settings::set_host(std::string host) { host_ = firebase::Move(host); } @@ -57,7 +60,7 @@ void Settings::set_host(std::string host) { host_ = firebase::Move(host); } void Settings::set_ssl_enabled(bool enabled) { ssl_enabled_ = enabled; } const LocalCacheSettings& Settings::local_cache_settings() { - if (used_legacy_cache_settings_) { + if (cache_settings_source_ == CacheSettingsSource::kOld) { if (is_persistence_enabled()) { local_cache_settings_ = std::make_shared( PersistentCacheSettings::Create().WithSizeBytes(cache_size_bytes())); @@ -65,21 +68,19 @@ const LocalCacheSettings& Settings::local_cache_settings() { local_cache_settings_ = std::make_shared(MemoryCacheSettings::Create()); } - } else if (local_cache_settings_ == nullptr) { - local_cache_settings_ = std::make_shared( - PersistentCacheSettings::Create()); } return *local_cache_settings_; } void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { - if (used_legacy_cache_settings_) { + if (cache_settings_source_ == CacheSettingsSource::kOld) { SimpleThrowIllegalState( "Cannot mix set_local_cache_settings() with legacy cache api like " "set_persistence_enabled() or set_cache_size_bytes()"); } + cache_settings_source_ = CacheSettingsSource::kNew; if (cache.kind() == LocalCacheSettings::Kind::kPersistent) { local_cache_settings_ = std::make_shared( static_cast(cache)); @@ -90,25 +91,25 @@ void Settings::set_local_cache_settings(const LocalCacheSettings& cache) { } void Settings::set_persistence_enabled(bool enabled) { - if (local_cache_settings_ != nullptr) { + if (cache_settings_source_ == CacheSettingsSource::kNew) { SimpleThrowIllegalState( "Cannot mix legacy cache api set_persistence_enabled() with new cache " "api set_local_cache_settings()"); } + cache_settings_source_ = CacheSettingsSource::kOld; persistence_enabled_ = enabled; - used_legacy_cache_settings_ = true; } void Settings::set_cache_size_bytes(int64_t value) { - if (local_cache_settings_ != nullptr) { + if (cache_settings_source_ == CacheSettingsSource::kNew) { SimpleThrowIllegalState( "Cannot mix legacy cache api set_cache_size_bytes() with new cache api " "set_local_cache_settings()"); } + cache_settings_source_ = CacheSettingsSource::kOld; cache_size_bytes_ = value; - used_legacy_cache_settings_ = true; } std::string Settings::ToString() const { @@ -123,22 +124,37 @@ std::ostream& operator<<(std::ostream& out, const Settings& settings) { } bool operator==(const Settings& lhs, const Settings& rhs) { - bool eq = lhs.host() == rhs.host() && - lhs.is_ssl_enabled() == rhs.is_ssl_enabled() && - lhs.is_persistence_enabled() == rhs.is_persistence_enabled() && - lhs.cache_size_bytes() == rhs.cache_size_bytes(); - - if (eq) { - if (lhs.local_cache_settings_ != rhs.local_cache_settings_) { - if (lhs.local_cache_settings_ != nullptr && - rhs.local_cache_settings_ != nullptr) { - eq = (*lhs.local_cache_settings_ == *rhs.local_cache_settings_); - } else { - eq = false; - } - } + if (lhs.host() != rhs.host()) { + return false; + } + + if (lhs.is_ssl_enabled() != rhs.is_ssl_enabled()) { + return false; + } + + if (lhs.cache_settings_source_ != rhs.cache_settings_source_) { + return false; + } + + if (*lhs.local_cache_settings_ != *rhs.local_cache_settings_) { + return false; } - return eq; + + if (lhs.is_persistence_enabled() != rhs.is_persistence_enabled()) { + return false; + } + + if (lhs.cache_size_bytes() != rhs.cache_size_bytes()) { + return false; + } + +#if defined(__OBJC__) + if (lhs.dispatch_queue() != rhs.dispatch_queue()) { + return false; + } +#endif // defined(__OBJC__) + + return true; } // Apple uses a different mechanism, defined in `settings_apple.mm`. diff --git a/firestore/src/common/settings_apple.mm b/firestore/src/common/settings_apple.mm index 5ff4b325df..eba499a857 100644 --- a/firestore/src/common/settings_apple.mm +++ b/firestore/src/common/settings_apple.mm @@ -20,6 +20,7 @@ #include #include "Firestore/core/src/util/executor_libdispatch.h" +#include "firebase/firestore/local_cache_settings.h" namespace firebase { namespace firestore { @@ -36,7 +37,9 @@ Settings::Settings() : host_(kDefaultHost), executor_( - Executor::CreateSerial("com.google.firebase.firestore.callback")) {} + Executor::CreateSerial("com.google.firebase.firestore.callback")), + local_cache_settings_(std::make_unique( + PersistentCacheSettings::Create())) {} std::unique_ptr Settings::CreateExecutor() const { return std::make_unique(dispatch_queue()); diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index e84d40a9f6..2cecf2a9e3 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -292,6 +292,12 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { std::unique_ptr settings_internal_; }; +/** Inequality function. */ +inline bool operator!=(const LocalCacheSettings& lhs, + const LocalCacheSettings& rhs) { + return !(lhs == rhs); +} + /** Inequality function. */ inline bool operator!=(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs) { diff --git a/firestore/src/include/firebase/firestore/settings.h b/firestore/src/include/firebase/firestore/settings.h index cf1b8b7e86..c83b2409d7 100644 --- a/firestore/src/include/firebase/firestore/settings.h +++ b/firestore/src/include/firebase/firestore/settings.h @@ -139,7 +139,7 @@ class Settings final { void set_ssl_enabled(bool enabled); /** - * Returns a shared pointer to the `LocalCacheSettings` instance + * Returns a reference to the `LocalCacheSettings` instance * used to configure this SDK. */ const LocalCacheSettings& local_cache_settings(); @@ -239,13 +239,14 @@ class Settings final { private: static constexpr int64_t kDefaultCacheSizeBytes = 100 * 1024 * 1024; + enum class CacheSettingsSource { kNone, kNew, kOld }; std::string host_; bool ssl_enabled_ = true; - std::shared_ptr local_cache_settings_ = nullptr; + CacheSettingsSource cache_settings_source_{CacheSettingsSource::kNone}; - bool used_legacy_cache_settings_ = false; + std::shared_ptr local_cache_settings_; bool persistence_enabled_ = true; int64_t cache_size_bytes_ = kDefaultCacheSizeBytes; diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 82dece9642..5a03609669 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -39,6 +39,7 @@ #include "app/src/include/firebase/future.h" #include "app/src/reference_counted_future_impl.h" #include "firebase/firestore/firestore_version.h" +#include "firebase/firestore/settings.h" #include "firestore/src/common/exception_common.h" #include "firestore/src/common/hard_assert_common.h" #include "firestore/src/common/macros.h" @@ -199,14 +200,14 @@ Settings FirestoreInternal::settings() const { result.set_ssl_enabled(from.ssl_enabled()); // TODO(wuandy): We use the deprecated API for default settings, but mark - // `used_legacy_cache_settings_` as false such that new settings API is not + // `cache_settings_source_` as `kNew` such that new settings API is not // rejected by runtime checks. This should be removed when legacy API is // removed. SUPPRESS_DEPRECATED_DECLARATIONS_BEGIN() result.set_persistence_enabled(from.persistence_enabled()); result.set_cache_size_bytes(from.cache_size_bytes()); SUPPRESS_END() - result.used_legacy_cache_settings_ = false; + result.cache_settings_source_ = Settings::CacheSettingsSource::kNone; return result; } @@ -218,8 +219,7 @@ void FirestoreInternal::set_settings(Settings from) { // TODO(wuandy): Checking `from.local_cache_settings_` is required, because // FirestoreInternal::settings() overrides used_legacy_cache_settings_. All // this special logic should go away when legacy cache config is removed. - if (!from.used_legacy_cache_settings_ && - from.local_cache_settings_ != nullptr) { + if (from.cache_settings_source_ == Settings::CacheSettingsSource::kNew) { settings.set_local_cache_settings( from.local_cache_settings().core_cache_settings()); } else { From d56072187317e13f3e39ca1ce53e24a25b19dbcf Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 7 Jun 2023 10:50:36 -0400 Subject: [PATCH 16/22] use forward decl for internal classes --- firestore/src/common/local_cache_settings.cc | 69 +++++++------------ .../firebase/firestore/local_cache_settings.h | 66 +++++++----------- 2 files changed, 48 insertions(+), 87 deletions(-) diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index 668ebb6821..f417bdf374 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -44,30 +44,10 @@ bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { PersistentCacheSettings PersistentCacheSettings::Create() { return {}; } PersistentCacheSettings::PersistentCacheSettings() { - settings_internal_ = std::make_unique( + settings_internal_ = std::make_shared( CorePersistentSettings{}); } -PersistentCacheSettings::~PersistentCacheSettings() { - settings_internal_.reset(); -} - -PersistentCacheSettings::PersistentCacheSettings( - const PersistentCacheSettings& other) { - settings_internal_ = std::make_unique( - *other.settings_internal_); -} - -PersistentCacheSettings& PersistentCacheSettings::operator=( - const PersistentCacheSettings& other) { - if (this == &other) { - return *this; - } - settings_internal_ = std::make_unique( - *other.settings_internal_); - return *this; -} - PersistentCacheSettings PersistentCacheSettings::WithSizeBytes( int64_t size) const { PersistentCacheSettings new_settings{*this}; @@ -80,59 +60,55 @@ const CoreCacheSettings& PersistentCacheSettings::core_cache_settings() const { return settings_internal_->core_settings(); } +int64_t PersistentCacheSettings::size_bytes() const { + return settings_internal_->core_settings().size_bytes(); +} + MemoryEagerGCSettings MemoryEagerGCSettings::Create() { return {}; } MemoryEagerGCSettings::MemoryEagerGCSettings() { - settings_internal_ = std::make_unique( + settings_internal_ = std::make_shared( CoreMemoryEagerGcSettings{}); } -MemoryEagerGCSettings::~MemoryEagerGCSettings() { settings_internal_.reset(); } +const api::MemoryGargabeCollectorSettings& +MemoryEagerGCSettings::core_gc_settings() const { + return settings_internal_->core_settings(); +} MemoryLruGCSettings MemoryLruGCSettings::Create() { return {}; } MemoryLruGCSettings::MemoryLruGCSettings() { settings_internal_ = - std::make_unique(CoreMemoryLruGcSettings{}); + std::make_shared(CoreMemoryLruGcSettings{}); } MemoryLruGCSettings::MemoryLruGCSettings( const MemoryLruGCSettingsInternal& other) { - settings_internal_ = std::make_unique(other); + settings_internal_ = std::make_shared(other); } -MemoryLruGCSettings::~MemoryLruGCSettings() { settings_internal_.reset(); } - MemoryLruGCSettings MemoryLruGCSettings::WithSizeBytes(int64_t size) { return {MemoryLruGCSettingsInternal{ settings_internal_->core_settings().WithSizeBytes(size)}}; } -MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } - -MemoryCacheSettings::MemoryCacheSettings() { - settings_internal_ = - std::make_unique(CoreMemorySettings{}); +int64_t MemoryLruGCSettings::size_bytes() const { + return settings_internal_->core_settings().size_bytes(); } -MemoryCacheSettings::MemoryCacheSettings(const MemoryCacheSettings& other) { - settings_internal_ = - std::make_unique(*other.settings_internal_); +const api::MemoryGargabeCollectorSettings& +MemoryLruGCSettings::core_gc_settings() const { + return settings_internal_->core_settings(); } -MemoryCacheSettings& MemoryCacheSettings::operator=( - const MemoryCacheSettings& other) { - if (this == &other) { - return *this; - } +MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } +MemoryCacheSettings::MemoryCacheSettings() { settings_internal_ = - std::make_unique(*other.settings_internal_); - return *this; + std::make_shared(CoreMemorySettings{}); } -MemoryCacheSettings::~MemoryCacheSettings() { settings_internal_.reset(); } - MemoryCacheSettings MemoryCacheSettings::WithGarbageCollectorSettings( const MemoryGarbageCollectorSettings& settings) const { MemoryCacheSettings result{*this}; @@ -157,6 +133,11 @@ bool operator==(const PersistentCacheSettings& lhs, return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); } +bool operator==(const MemoryGarbageCollectorSettings& lhs, + const MemoryGarbageCollectorSettings& rhs) { + return lhs.core_gc_settings() == rhs.core_gc_settings(); +} + bool operator==(const MemoryEagerGCSettings& lhs, const MemoryEagerGCSettings& rhs) { return &lhs == &rhs || (*lhs.settings_internal_ == *rhs.settings_internal_); diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 2cecf2a9e3..4f9fa61468 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -21,14 +21,18 @@ #include #include "firestore/src/include/firebase/firestore/settings.h" -#include "firestore/src/main/firestore_main.h" -#include "firestore/src/main/local_cache_settings_main.h" namespace firebase { namespace firestore { +namespace api { +class LocalCacheSettings; +class MemoryGargabeCollectorSettings; +} // namespace api class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; +class MemoryLruGCSettingsInternal; +class MemoryEagerGCSettingsInternal; /** * Abstract class implemented by all supported cache settings. @@ -70,20 +74,6 @@ class PersistentCacheSettings final : public LocalCacheSettings { /** Create a default instance `PersistenceCacheSettings`. */ static PersistentCacheSettings Create(); - /** Copy constructor. */ - PersistentCacheSettings(const PersistentCacheSettings& other); - - /** Copy assignment. */ - PersistentCacheSettings& operator=(const PersistentCacheSettings& other); - - /** Move constructor. */ - PersistentCacheSettings(PersistentCacheSettings&& other) = default; - - /** Move assignment. */ - PersistentCacheSettings& operator=(PersistentCacheSettings&& other) = default; - - ~PersistentCacheSettings() override; - /** Equality function. */ friend bool operator==(const PersistentCacheSettings& lhs, const PersistentCacheSettings& rhs); @@ -107,9 +97,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { * Returns the approximate cache size threshold configured. Garbage collection * kicks in once the cache size exceeds this threshold. */ - int64_t size_bytes() const { - return settings_internal_->core_settings().size_bytes(); - } + int64_t size_bytes() const; private: friend class Settings; @@ -124,7 +112,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { // Get the corresponding settings object from the core sdk. const api::LocalCacheSettings& core_cache_settings() const override; - std::unique_ptr settings_internal_; + std::shared_ptr settings_internal_; }; class MemoryGarbageCollectorSettings; @@ -142,14 +130,6 @@ class MemoryCacheSettings final : public LocalCacheSettings { /** Create a default instance `MemoryCacheSettings`. */ static MemoryCacheSettings Create(); - /** Copy constructor. */ - MemoryCacheSettings(const MemoryCacheSettings& other); - - /** Copy assignment. */ - MemoryCacheSettings& operator=(const MemoryCacheSettings& other); - - ~MemoryCacheSettings() override; - /** Equality function. */ friend bool operator==(const MemoryCacheSettings& lhs, const MemoryCacheSettings& rhs); @@ -174,7 +154,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { // Get the corresponding settings object from the core sdk. const api::LocalCacheSettings& core_cache_settings() const override; - std::unique_ptr settings_internal_; + std::shared_ptr settings_internal_; }; /** @@ -187,6 +167,9 @@ class MemoryCacheSettings final : public LocalCacheSettings { class MemoryGarbageCollectorSettings { public: virtual ~MemoryGarbageCollectorSettings() = default; + /** Equality function. */ + friend bool operator==(const MemoryGarbageCollectorSettings& lhs, + const MemoryGarbageCollectorSettings& rhs); private: friend class MemoryCacheSettings; @@ -214,8 +197,6 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { /** Create a default instance `MemoryEagerGCSettings`. */ static MemoryEagerGCSettings Create(); - ~MemoryEagerGCSettings() override; - /** Equality function. */ friend bool operator==(const MemoryEagerGCSettings& lhs, const MemoryEagerGCSettings& rhs); @@ -224,11 +205,9 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { friend class MemoryCacheSettings; MemoryEagerGCSettings(); - const api::MemoryGargabeCollectorSettings& core_gc_settings() const override { - return settings_internal_->core_settings(); - } + const api::MemoryGargabeCollectorSettings& core_gc_settings() const override; - std::unique_ptr settings_internal_; + std::shared_ptr settings_internal_; }; /** @@ -251,7 +230,6 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { public: /** Create a default instance `MemoryLruGCSettings`. */ static MemoryLruGCSettings Create(); - ~MemoryLruGCSettings() override; /** Equality function. */ friend bool operator==(const MemoryLruGCSettings& lhs, @@ -276,20 +254,16 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { * Returns the approximate cache size threshold configured. Garbage collection * kicks in once the cache size exceeds this threshold. */ - int64_t size_bytes() const { - return settings_internal_->core_settings().size_bytes(); - } + int64_t size_bytes() const; private: friend class MemoryCacheSettings; MemoryLruGCSettings(); MemoryLruGCSettings(const MemoryLruGCSettingsInternal& other); - const api::MemoryGargabeCollectorSettings& core_gc_settings() const override { - return settings_internal_->core_settings(); - } + const api::MemoryGargabeCollectorSettings& core_gc_settings() const override; - std::unique_ptr settings_internal_; + std::shared_ptr settings_internal_; }; /** Inequality function. */ @@ -310,6 +284,12 @@ inline bool operator!=(const PersistentCacheSettings& lhs, return !(lhs == rhs); } +/** Inequality function. */ +inline bool operator!=(const MemoryGarbageCollectorSettings& lhs, + const MemoryGarbageCollectorSettings& rhs) { + return !(lhs == rhs); +} + /** Inequality function. */ inline bool operator!=(const MemoryEagerGCSettings& lhs, const MemoryEagerGCSettings& rhs) { From b328d026fd9bf86ae128e10c5cb577ab79cf62aa Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 7 Jun 2023 14:58:00 -0400 Subject: [PATCH 17/22] remove main/api references --- firestore/src/common/local_cache_settings.cc | 28 ++++--------------- .../firebase/firestore/local_cache_settings.h | 28 +++++++++++-------- firestore/src/main/firestore_main.cc | 2 +- .../src/main/local_cache_settings_main.h | 26 +++++++++++++---- 4 files changed, 42 insertions(+), 42 deletions(-) diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index f417bdf374..79a2668e04 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -36,11 +36,6 @@ using CoreMemoryEagerGcSettings = api::MemoryEagerGcSettings; using CoreMemoryLruGcSettings = api::MemoryLruGcSettings; } // namespace -bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { - return lhs.kind() == rhs.kind() && - lhs.core_cache_settings() == rhs.core_cache_settings(); -} - PersistentCacheSettings PersistentCacheSettings::Create() { return {}; } PersistentCacheSettings::PersistentCacheSettings() { @@ -56,10 +51,6 @@ PersistentCacheSettings PersistentCacheSettings::WithSizeBytes( return new_settings; } -const CoreCacheSettings& PersistentCacheSettings::core_cache_settings() const { - return settings_internal_->core_settings(); -} - int64_t PersistentCacheSettings::size_bytes() const { return settings_internal_->core_settings().size_bytes(); } @@ -71,11 +62,6 @@ MemoryEagerGCSettings::MemoryEagerGCSettings() { CoreMemoryEagerGcSettings{}); } -const api::MemoryGargabeCollectorSettings& -MemoryEagerGCSettings::core_gc_settings() const { - return settings_internal_->core_settings(); -} - MemoryLruGCSettings MemoryLruGCSettings::Create() { return {}; } MemoryLruGCSettings::MemoryLruGCSettings() { @@ -97,11 +83,6 @@ int64_t MemoryLruGCSettings::size_bytes() const { return settings_internal_->core_settings().size_bytes(); } -const api::MemoryGargabeCollectorSettings& -MemoryLruGCSettings::core_gc_settings() const { - return settings_internal_->core_settings(); -} - MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } MemoryCacheSettings::MemoryCacheSettings() { @@ -115,12 +96,13 @@ MemoryCacheSettings MemoryCacheSettings::WithGarbageCollectorSettings( CoreMemorySettings core_settings = result.settings_internal_->core_settings(); result.settings_internal_->set_core_settings( core_settings.WithMemoryGarbageCollectorSettings( - settings.core_gc_settings())); + settings.internal().core_settings())); return result; } -const CoreCacheSettings& MemoryCacheSettings::core_cache_settings() const { - return settings_internal_->core_settings(); +bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { + return lhs.kind() == rhs.kind() && + lhs.internal().core_settings() == rhs.internal().core_settings(); } bool operator==(const MemoryCacheSettings& lhs, @@ -135,7 +117,7 @@ bool operator==(const PersistentCacheSettings& lhs, bool operator==(const MemoryGarbageCollectorSettings& lhs, const MemoryGarbageCollectorSettings& rhs) { - return lhs.core_gc_settings() == rhs.core_gc_settings(); + return lhs.internal().core_settings() == rhs.internal().core_settings(); } bool operator==(const MemoryEagerGCSettings& lhs, diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 4f9fa61468..0f14cee88d 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -21,14 +21,12 @@ #include #include "firestore/src/include/firebase/firestore/settings.h" +#include "firestore/src/main/local_cache_settings_main.h" namespace firebase { namespace firestore { -namespace api { -class LocalCacheSettings; -class MemoryGargabeCollectorSettings; -} // namespace api +class LocalCacheSettingsInternal; class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; class MemoryLruGCSettingsInternal; @@ -56,7 +54,7 @@ class LocalCacheSettings { friend class Settings; virtual Kind kind() const = 0; - virtual const api::LocalCacheSettings& core_cache_settings() const = 0; + virtual const LocalCacheSettingsInternal& internal() const = 0; }; /** @@ -110,7 +108,9 @@ class PersistentCacheSettings final : public LocalCacheSettings { } // Get the corresponding settings object from the core sdk. - const api::LocalCacheSettings& core_cache_settings() const override; + const PersistentCacheSettingsInternal& internal() const override { + return *settings_internal_; + } std::shared_ptr settings_internal_; }; @@ -152,7 +152,9 @@ class MemoryCacheSettings final : public LocalCacheSettings { } // Get the corresponding settings object from the core sdk. - const api::LocalCacheSettings& core_cache_settings() const override; + const MemoryCacheSettingsInternal& internal() const override { + return *settings_internal_; + } std::shared_ptr settings_internal_; }; @@ -173,8 +175,7 @@ class MemoryGarbageCollectorSettings { private: friend class MemoryCacheSettings; - virtual const api::MemoryGargabeCollectorSettings& core_gc_settings() - const = 0; + virtual const MemoryGarbageCollectorSettingsInternal& internal() const = 0; }; /** @@ -204,8 +205,9 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { private: friend class MemoryCacheSettings; MemoryEagerGCSettings(); - - const api::MemoryGargabeCollectorSettings& core_gc_settings() const override; + const MemoryEagerGCSettingsInternal& internal() const override { + return *settings_internal_; + } std::shared_ptr settings_internal_; }; @@ -261,7 +263,9 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { MemoryLruGCSettings(); MemoryLruGCSettings(const MemoryLruGCSettingsInternal& other); - const api::MemoryGargabeCollectorSettings& core_gc_settings() const override; + const MemoryLruGCSettingsInternal& internal() const override { + return *settings_internal_; + } std::shared_ptr settings_internal_; }; diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index 5a03609669..d40469e9e6 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -221,7 +221,7 @@ void FirestoreInternal::set_settings(Settings from) { // this special logic should go away when legacy cache config is removed. if (from.cache_settings_source_ == Settings::CacheSettingsSource::kNew) { settings.set_local_cache_settings( - from.local_cache_settings().core_cache_settings()); + from.local_cache_settings().internal().core_settings()); } else { settings.set_persistence_enabled(from.is_persistence_enabled()); settings.set_cache_size_bytes(from.cache_size_bytes()); diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index 1e5fc85a3b..88817f6b8b 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -25,7 +25,10 @@ namespace firebase { namespace firestore { -class LocalCacheSettingsInternal {}; +class LocalCacheSettingsInternal { + public: + virtual const api::LocalCacheSettings& core_settings() const = 0; +}; class PersistentCacheSettingsInternal final : public LocalCacheSettingsInternal { @@ -39,7 +42,9 @@ class PersistentCacheSettingsInternal final return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const api::PersistentCacheSettings& core_settings() { return settings_; } + const api::PersistentCacheSettings& core_settings() const override { + return settings_; + } void set_core_settings(const api::PersistentCacheSettings& settings) { settings_ = settings; } @@ -48,7 +53,10 @@ class PersistentCacheSettingsInternal final api::PersistentCacheSettings settings_; }; -class MemoryGarbageCollectorSettingsInternal {}; +class MemoryGarbageCollectorSettingsInternal { + public: + virtual const api::MemoryGargabeCollectorSettings& core_settings() const = 0; +}; class MemoryEagerGCSettingsInternal final : public MemoryGarbageCollectorSettingsInternal { @@ -62,7 +70,9 @@ class MemoryEagerGCSettingsInternal final return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const api::MemoryEagerGcSettings& core_settings() { return settings_; } + const api::MemoryEagerGcSettings& core_settings() const override { + return settings_; + } void set_core_settings(const api::MemoryEagerGcSettings& settings) { settings_ = settings; } @@ -83,7 +93,9 @@ class MemoryLruGCSettingsInternal final return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const api::MemoryLruGcSettings& core_settings() { return settings_; } + const api::MemoryLruGcSettings& core_settings() const override { + return settings_; + } void set_core_settings(const api::MemoryLruGcSettings& settings) { settings_ = settings; } @@ -103,7 +115,9 @@ class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { return &lhs == &rhs || lhs.settings_ == rhs.settings_; } - const api::MemoryCacheSettings& core_settings() { return settings_; } + const api::MemoryCacheSettings& core_settings() const override { + return settings_; + } void set_core_settings(const api::MemoryCacheSettings& settings) { settings_ = settings; } From c4264fd24e2006137325a3e9f7de9e67c6833162 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 7 Jun 2023 15:17:19 -0400 Subject: [PATCH 18/22] No subclassing --- .../src/include/firebase/firestore/local_cache_settings.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 0f14cee88d..ef82e282dd 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -52,6 +52,10 @@ class LocalCacheSettings { private: friend class FirestoreInternal; friend class Settings; + friend class PersistentCacheSettings; + friend class MemoryCacheSettings; + + LocalCacheSettings() = default; virtual Kind kind() const = 0; virtual const LocalCacheSettingsInternal& internal() const = 0; @@ -87,7 +91,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { * cleanup will be attempted. * * By default, persistence cache is enabled with a cache size of 100 MB. The - * minimum value is 1 MB. + * minimum value is 1 MB (1 * 1024 * 1024 bytes). */ PersistentCacheSettings WithSizeBytes(int64_t size) const; @@ -248,7 +252,7 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { * cleanup will be attempted. * * By default, memory LRU cache is enabled with a cache size of 100 MB. The - * minimum value is 1 MB. + * minimum value is 1 MB (1 * 1024 * 1024 bytes). */ MemoryLruGCSettings WithSizeBytes(int64_t size); From d9d5ecdbd5c16e235024f182c3e3ef14e5b7d1f5 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 7 Jun 2023 15:21:07 -0400 Subject: [PATCH 19/22] No subclassing --- .../src/include/firebase/firestore/local_cache_settings.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index ef82e282dd..4efdc52dc7 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -179,6 +179,11 @@ class MemoryGarbageCollectorSettings { private: friend class MemoryCacheSettings; + friend class MemoryEagerGCSettings; + friend class MemoryLruGCSettings; + + MemoryGarbageCollectorSettings() = default; + virtual const MemoryGarbageCollectorSettingsInternal& internal() const = 0; }; From b757139232f00272114c6ec29f290f6f3fb167b2 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 5 Jul 2023 10:50:34 -0400 Subject: [PATCH 20/22] delete includes from main --- firestore/src/common/local_cache_settings.cc | 25 ++++++++++++++--- .../firebase/firestore/local_cache_settings.h | 27 +++++++------------ firestore/src/main/firestore_main.cc | 2 +- 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index 79a2668e04..c1b897b865 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -55,6 +55,10 @@ int64_t PersistentCacheSettings::size_bytes() const { return settings_internal_->core_settings().size_bytes(); } +const CoreCacheSettings& PersistentCacheSettings::core_settings() const { + return settings_internal_->core_settings(); +} + MemoryEagerGCSettings MemoryEagerGCSettings::Create() { return {}; } MemoryEagerGCSettings::MemoryEagerGCSettings() { @@ -62,6 +66,11 @@ MemoryEagerGCSettings::MemoryEagerGCSettings() { CoreMemoryEagerGcSettings{}); } +const CoreMemoryGarbageCollectorSettings& MemoryEagerGCSettings::core_settings() + const { + return settings_internal_->core_settings(); +} + MemoryLruGCSettings MemoryLruGCSettings::Create() { return {}; } MemoryLruGCSettings::MemoryLruGCSettings() { @@ -83,6 +92,11 @@ int64_t MemoryLruGCSettings::size_bytes() const { return settings_internal_->core_settings().size_bytes(); } +const CoreMemoryGarbageCollectorSettings& MemoryLruGCSettings::core_settings() + const { + return settings_internal_->core_settings(); +} + MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } MemoryCacheSettings::MemoryCacheSettings() { @@ -96,13 +110,16 @@ MemoryCacheSettings MemoryCacheSettings::WithGarbageCollectorSettings( CoreMemorySettings core_settings = result.settings_internal_->core_settings(); result.settings_internal_->set_core_settings( core_settings.WithMemoryGarbageCollectorSettings( - settings.internal().core_settings())); + settings.core_settings())); return result; } +const CoreCacheSettings& MemoryCacheSettings::core_settings() const { + return settings_internal_->core_settings(); +} + bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { - return lhs.kind() == rhs.kind() && - lhs.internal().core_settings() == rhs.internal().core_settings(); + return lhs.kind() == rhs.kind() && lhs.core_settings() == rhs.core_settings(); } bool operator==(const MemoryCacheSettings& lhs, @@ -117,7 +134,7 @@ bool operator==(const PersistentCacheSettings& lhs, bool operator==(const MemoryGarbageCollectorSettings& lhs, const MemoryGarbageCollectorSettings& rhs) { - return lhs.internal().core_settings() == rhs.internal().core_settings(); + return lhs.core_settings() == rhs.core_settings(); } bool operator==(const MemoryEagerGCSettings& lhs, diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 4efdc52dc7..5a5b86e04f 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -21,12 +21,14 @@ #include #include "firestore/src/include/firebase/firestore/settings.h" -#include "firestore/src/main/local_cache_settings_main.h" namespace firebase { namespace firestore { +namespace api { +class LocalCacheSettings; +class MemoryGargabeCollectorSettings; +} // namespace api -class LocalCacheSettingsInternal; class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; class MemoryLruGCSettingsInternal; @@ -58,7 +60,7 @@ class LocalCacheSettings { LocalCacheSettings() = default; virtual Kind kind() const = 0; - virtual const LocalCacheSettingsInternal& internal() const = 0; + virtual const api::LocalCacheSettings& core_settings() const = 0; }; /** @@ -112,9 +114,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { } // Get the corresponding settings object from the core sdk. - const PersistentCacheSettingsInternal& internal() const override { - return *settings_internal_; - } + const api::LocalCacheSettings& core_settings() const override; std::shared_ptr settings_internal_; }; @@ -155,10 +155,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { return LocalCacheSettings::Kind::kMemory; } - // Get the corresponding settings object from the core sdk. - const MemoryCacheSettingsInternal& internal() const override { - return *settings_internal_; - } + const api::LocalCacheSettings& core_settings() const override; std::shared_ptr settings_internal_; }; @@ -184,7 +181,7 @@ class MemoryGarbageCollectorSettings { MemoryGarbageCollectorSettings() = default; - virtual const MemoryGarbageCollectorSettingsInternal& internal() const = 0; + virtual const api::MemoryGargabeCollectorSettings& core_settings() const = 0; }; /** @@ -214,9 +211,7 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { private: friend class MemoryCacheSettings; MemoryEagerGCSettings(); - const MemoryEagerGCSettingsInternal& internal() const override { - return *settings_internal_; - } + const api::MemoryGargabeCollectorSettings& core_settings() const override; std::shared_ptr settings_internal_; }; @@ -272,9 +267,7 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { MemoryLruGCSettings(); MemoryLruGCSettings(const MemoryLruGCSettingsInternal& other); - const MemoryLruGCSettingsInternal& internal() const override { - return *settings_internal_; - } + const api::MemoryGargabeCollectorSettings& core_settings() const override; std::shared_ptr settings_internal_; }; diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index d40469e9e6..dd49b80c03 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -221,7 +221,7 @@ void FirestoreInternal::set_settings(Settings from) { // this special logic should go away when legacy cache config is removed. if (from.cache_settings_source_ == Settings::CacheSettingsSource::kNew) { settings.set_local_cache_settings( - from.local_cache_settings().internal().core_settings()); + from.local_cache_settings().core_settings()); } else { settings.set_persistence_enabled(from.is_persistence_enabled()); settings.set_cache_size_bytes(from.cache_size_bytes()); From ded9c2eb36b2e8176e1e8cd7fc468cd99eb73024 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Tue, 11 Jul 2023 10:52:45 -0400 Subject: [PATCH 21/22] No more core from common --- firestore/src/common/local_cache_settings.cc | 67 ++++++++----------- firestore/src/common/settings.cc | 1 - .../firebase/firestore/local_cache_settings.h | 27 ++++---- firestore/src/main/firestore_main.cc | 2 +- .../src/main/local_cache_settings_main.h | 55 ++++++++++----- 5 files changed, 81 insertions(+), 71 deletions(-) diff --git a/firestore/src/common/local_cache_settings.cc b/firestore/src/common/local_cache_settings.cc index c1b897b865..b5ea995ed1 100644 --- a/firestore/src/common/local_cache_settings.cc +++ b/firestore/src/common/local_cache_settings.cc @@ -17,7 +17,6 @@ #include "firestore/src/include/firebase/firestore/local_cache_settings.h" #include -#include "Firestore/core/src/api/settings.h" #include "firestore/src/common/hard_assert_common.h" #if defined(__ANDROID__) #else @@ -27,55 +26,44 @@ namespace firebase { namespace firestore { -namespace { -using CoreCacheSettings = api::LocalCacheSettings; -using CorePersistentSettings = api::PersistentCacheSettings; -using CoreMemorySettings = api::MemoryCacheSettings; -using CoreMemoryGarbageCollectorSettings = api::MemoryGargabeCollectorSettings; -using CoreMemoryEagerGcSettings = api::MemoryEagerGcSettings; -using CoreMemoryLruGcSettings = api::MemoryLruGcSettings; -} // namespace - PersistentCacheSettings PersistentCacheSettings::Create() { return {}; } PersistentCacheSettings::PersistentCacheSettings() { - settings_internal_ = std::make_shared( - CorePersistentSettings{}); + settings_internal_ = std::make_shared(); } PersistentCacheSettings PersistentCacheSettings::WithSizeBytes( int64_t size) const { - PersistentCacheSettings new_settings{*this}; - new_settings.settings_internal_->set_core_settings( - new_settings.settings_internal_->core_settings().WithSizeBytes(size)); + PersistentCacheSettings new_settings; + new_settings.settings_internal_ = + std::make_shared( + this->settings_internal_->WithSizeBytes(size)); return new_settings; } int64_t PersistentCacheSettings::size_bytes() const { - return settings_internal_->core_settings().size_bytes(); + return settings_internal_->size_bytes(); } -const CoreCacheSettings& PersistentCacheSettings::core_settings() const { - return settings_internal_->core_settings(); +const LocalCacheSettingsInternal& PersistentCacheSettings::internal() const { + return *settings_internal_; } MemoryEagerGCSettings MemoryEagerGCSettings::Create() { return {}; } MemoryEagerGCSettings::MemoryEagerGCSettings() { - settings_internal_ = std::make_shared( - CoreMemoryEagerGcSettings{}); + settings_internal_ = std::make_shared(); } -const CoreMemoryGarbageCollectorSettings& MemoryEagerGCSettings::core_settings() +const MemoryGarbageCollectorSettingsInternal& MemoryEagerGCSettings::internal() const { - return settings_internal_->core_settings(); + return *settings_internal_; } MemoryLruGCSettings MemoryLruGCSettings::Create() { return {}; } MemoryLruGCSettings::MemoryLruGCSettings() { - settings_internal_ = - std::make_shared(CoreMemoryLruGcSettings{}); + settings_internal_ = std::make_shared(); } MemoryLruGCSettings::MemoryLruGCSettings( @@ -84,42 +72,41 @@ MemoryLruGCSettings::MemoryLruGCSettings( } MemoryLruGCSettings MemoryLruGCSettings::WithSizeBytes(int64_t size) { - return {MemoryLruGCSettingsInternal{ - settings_internal_->core_settings().WithSizeBytes(size)}}; + MemoryLruGCSettings result; + result.settings_internal_ = std::make_shared( + this->settings_internal_->WithSizeBytes(size)); + return result; } int64_t MemoryLruGCSettings::size_bytes() const { - return settings_internal_->core_settings().size_bytes(); + return settings_internal_->size_bytes(); } -const CoreMemoryGarbageCollectorSettings& MemoryLruGCSettings::core_settings() +const MemoryGarbageCollectorSettingsInternal& MemoryLruGCSettings::internal() const { - return settings_internal_->core_settings(); + return *settings_internal_; } MemoryCacheSettings MemoryCacheSettings::Create() { return {}; } MemoryCacheSettings::MemoryCacheSettings() { - settings_internal_ = - std::make_shared(CoreMemorySettings{}); + settings_internal_ = std::make_shared(); } MemoryCacheSettings MemoryCacheSettings::WithGarbageCollectorSettings( const MemoryGarbageCollectorSettings& settings) const { - MemoryCacheSettings result{*this}; - CoreMemorySettings core_settings = result.settings_internal_->core_settings(); - result.settings_internal_->set_core_settings( - core_settings.WithMemoryGarbageCollectorSettings( - settings.core_settings())); + MemoryCacheSettings result; + result.settings_internal_ = std::make_shared( + this->settings_internal_->WithGarbageCollectorSettings(settings)); return result; } -const CoreCacheSettings& MemoryCacheSettings::core_settings() const { - return settings_internal_->core_settings(); +const LocalCacheSettingsInternal& MemoryCacheSettings::internal() const { + return *settings_internal_; } bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs) { - return lhs.kind() == rhs.kind() && lhs.core_settings() == rhs.core_settings(); + return lhs.kind() == rhs.kind() && lhs.internal() == rhs.internal(); } bool operator==(const MemoryCacheSettings& lhs, @@ -134,7 +121,7 @@ bool operator==(const PersistentCacheSettings& lhs, bool operator==(const MemoryGarbageCollectorSettings& lhs, const MemoryGarbageCollectorSettings& rhs) { - return lhs.core_settings() == rhs.core_settings(); + return lhs.internal() == rhs.internal(); } bool operator==(const MemoryEagerGCSettings& lhs, diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index ea2ba0f62e..06edbc57d0 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -20,7 +20,6 @@ #include #include -#include "Firestore/core/src/api/settings.h" #include "Firestore/core/src/util/hard_assert.h" #include "app/meta/move.h" #include "firebase/firestore/local_cache_settings.h" diff --git a/firestore/src/include/firebase/firestore/local_cache_settings.h b/firestore/src/include/firebase/firestore/local_cache_settings.h index 5a5b86e04f..b7d4a5eaa8 100644 --- a/firestore/src/include/firebase/firestore/local_cache_settings.h +++ b/firestore/src/include/firebase/firestore/local_cache_settings.h @@ -24,15 +24,13 @@ namespace firebase { namespace firestore { -namespace api { -class LocalCacheSettings; -class MemoryGargabeCollectorSettings; -} // namespace api +class LocalCacheSettingsInternal; class PersistentCacheSettingsInternal; class MemoryCacheSettingsInternal; class MemoryLruGCSettingsInternal; class MemoryEagerGCSettingsInternal; +class MemoryGarbageCollectorSettingsInternal; /** * Abstract class implemented by all supported cache settings. @@ -42,15 +40,16 @@ class MemoryEagerGCSettingsInternal; */ class LocalCacheSettings { public: + enum class Kind { kMemory, kPersistent }; + virtual ~LocalCacheSettings() = default; + virtual Kind kind() const = 0; + /** Equality function. */ friend bool operator==(const LocalCacheSettings& lhs, const LocalCacheSettings& rhs); - protected: - enum class Kind { kMemory, kPersistent }; - private: friend class FirestoreInternal; friend class Settings; @@ -59,8 +58,7 @@ class LocalCacheSettings { LocalCacheSettings() = default; - virtual Kind kind() const = 0; - virtual const api::LocalCacheSettings& core_settings() const = 0; + virtual const LocalCacheSettingsInternal& internal() const = 0; }; /** @@ -114,7 +112,7 @@ class PersistentCacheSettings final : public LocalCacheSettings { } // Get the corresponding settings object from the core sdk. - const api::LocalCacheSettings& core_settings() const override; + const LocalCacheSettingsInternal& internal() const override; std::shared_ptr settings_internal_; }; @@ -155,7 +153,7 @@ class MemoryCacheSettings final : public LocalCacheSettings { return LocalCacheSettings::Kind::kMemory; } - const api::LocalCacheSettings& core_settings() const override; + const LocalCacheSettingsInternal& internal() const override; std::shared_ptr settings_internal_; }; @@ -178,10 +176,11 @@ class MemoryGarbageCollectorSettings { friend class MemoryCacheSettings; friend class MemoryEagerGCSettings; friend class MemoryLruGCSettings; + friend class MemoryCacheSettingsInternal; MemoryGarbageCollectorSettings() = default; - virtual const api::MemoryGargabeCollectorSettings& core_settings() const = 0; + virtual const MemoryGarbageCollectorSettingsInternal& internal() const = 0; }; /** @@ -211,7 +210,7 @@ class MemoryEagerGCSettings final : public MemoryGarbageCollectorSettings { private: friend class MemoryCacheSettings; MemoryEagerGCSettings(); - const api::MemoryGargabeCollectorSettings& core_settings() const override; + const MemoryGarbageCollectorSettingsInternal& internal() const override; std::shared_ptr settings_internal_; }; @@ -267,7 +266,7 @@ class MemoryLruGCSettings final : public MemoryGarbageCollectorSettings { MemoryLruGCSettings(); MemoryLruGCSettings(const MemoryLruGCSettingsInternal& other); - const api::MemoryGargabeCollectorSettings& core_settings() const override; + const MemoryGarbageCollectorSettingsInternal& internal() const override; std::shared_ptr settings_internal_; }; diff --git a/firestore/src/main/firestore_main.cc b/firestore/src/main/firestore_main.cc index dd49b80c03..d40469e9e6 100644 --- a/firestore/src/main/firestore_main.cc +++ b/firestore/src/main/firestore_main.cc @@ -221,7 +221,7 @@ void FirestoreInternal::set_settings(Settings from) { // this special logic should go away when legacy cache config is removed. if (from.cache_settings_source_ == Settings::CacheSettingsSource::kNew) { settings.set_local_cache_settings( - from.local_cache_settings().core_settings()); + from.local_cache_settings().internal().core_settings()); } else { settings.set_persistence_enabled(from.is_persistence_enabled()); settings.set_cache_size_bytes(from.cache_size_bytes()); diff --git a/firestore/src/main/local_cache_settings_main.h b/firestore/src/main/local_cache_settings_main.h index 88817f6b8b..7d23ad5068 100644 --- a/firestore/src/main/local_cache_settings_main.h +++ b/firestore/src/main/local_cache_settings_main.h @@ -21,27 +21,39 @@ #include #include "Firestore/core/src/api/settings.h" +#include "firebase/firestore/local_cache_settings.h" namespace firebase { namespace firestore { class LocalCacheSettingsInternal { public: + friend bool operator==(const LocalCacheSettingsInternal& lhs, + const LocalCacheSettingsInternal& rhs) { + return &lhs == &rhs || lhs.core_settings() == rhs.core_settings(); + } + virtual const api::LocalCacheSettings& core_settings() const = 0; }; class PersistentCacheSettingsInternal final : public LocalCacheSettingsInternal { public: - explicit PersistentCacheSettingsInternal( - const api::PersistentCacheSettings& core_settings) - : settings_(std::move(core_settings)) {} + explicit PersistentCacheSettingsInternal() = default; friend bool operator==(const PersistentCacheSettingsInternal& lhs, const PersistentCacheSettingsInternal& rhs) { return &lhs == &rhs || lhs.settings_ == rhs.settings_; } + PersistentCacheSettingsInternal WithSizeBytes(int64_t size) { + PersistentCacheSettingsInternal result; + result.set_core_settings(this->core_settings().WithSizeBytes(size)); + return result; + } + + int64_t size_bytes() const { return settings_.size_bytes(); } + const api::PersistentCacheSettings& core_settings() const override { return settings_; } @@ -55,15 +67,18 @@ class PersistentCacheSettingsInternal final class MemoryGarbageCollectorSettingsInternal { public: + friend bool operator==(const MemoryGarbageCollectorSettingsInternal& lhs, + const MemoryGarbageCollectorSettingsInternal& rhs) { + return &lhs == &rhs || lhs.core_settings() == rhs.core_settings(); + } + virtual const api::MemoryGargabeCollectorSettings& core_settings() const = 0; }; class MemoryEagerGCSettingsInternal final : public MemoryGarbageCollectorSettingsInternal { public: - explicit MemoryEagerGCSettingsInternal( - const api::MemoryEagerGcSettings& core_settings) - : settings_(std::move(core_settings)) {} + explicit MemoryEagerGCSettingsInternal() = default; friend bool operator==(const MemoryEagerGCSettingsInternal& lhs, const MemoryEagerGCSettingsInternal& rhs) { @@ -84,15 +99,21 @@ class MemoryEagerGCSettingsInternal final class MemoryLruGCSettingsInternal final : public MemoryGarbageCollectorSettingsInternal { public: - explicit MemoryLruGCSettingsInternal( - const api::MemoryLruGcSettings& core_settings) - : settings_(std::move(core_settings)) {} + explicit MemoryLruGCSettingsInternal() = default; friend bool operator==(const MemoryLruGCSettingsInternal& lhs, const MemoryLruGCSettingsInternal& rhs) { return &lhs == &rhs || lhs.settings_ == rhs.settings_; } + MemoryLruGCSettingsInternal WithSizeBytes(int64_t size) { + MemoryLruGCSettingsInternal result; + result.set_core_settings(this->core_settings().WithSizeBytes(size)); + return result; + } + + int64_t size_bytes() const { return settings_.size_bytes(); } + const api::MemoryLruGcSettings& core_settings() const override { return settings_; } @@ -106,23 +127,27 @@ class MemoryLruGCSettingsInternal final class MemoryCacheSettingsInternal final : public LocalCacheSettingsInternal { public: - explicit MemoryCacheSettingsInternal( - const api::MemoryCacheSettings& core_settings) - : settings_(std::move(core_settings)) {} + explicit MemoryCacheSettingsInternal() = default; friend bool operator==(const MemoryCacheSettingsInternal& lhs, const MemoryCacheSettingsInternal& rhs) { return &lhs == &rhs || lhs.settings_ == rhs.settings_; } + MemoryCacheSettingsInternal WithGarbageCollectorSettings( + const MemoryGarbageCollectorSettings& gc_settings) { + return MemoryCacheSettingsInternal( + api::MemoryCacheSettings().WithMemoryGarbageCollectorSettings( + gc_settings.internal().core_settings())); + } + const api::MemoryCacheSettings& core_settings() const override { return settings_; } - void set_core_settings(const api::MemoryCacheSettings& settings) { - settings_ = settings; - } private: + explicit MemoryCacheSettingsInternal(const api::MemoryCacheSettings& settings) + : settings_(settings) {} api::MemoryCacheSettings settings_; }; From 570e8c0edb37039e7cd94b6e2f135d19123c4c2b Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 12 Jul 2023 11:24:32 -0400 Subject: [PATCH 22/22] remove unnecessary include --- firestore/src/common/settings.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/firestore/src/common/settings.cc b/firestore/src/common/settings.cc index 2b1ce89a5e..3be7d0c9a1 100644 --- a/firestore/src/common/settings.cc +++ b/firestore/src/common/settings.cc @@ -21,7 +21,6 @@ #include #include -#include "Firestore/core/src/util/hard_assert.h" #include "firebase/firestore/local_cache_settings.h" #include "firestore/src/common/exception_common.h"