From f58cad4834e11c6f6c4c7c04c3986492793659f3 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 19 Dec 2022 14:52:01 -0500 Subject: [PATCH 01/26] Firestore COUNT API --- .../firebase/firestore/aggregate_query.h | 134 +++++++++++++++++ .../firestore/aggregate_query_snapshot.h | 138 ++++++++++++++++++ .../firebase/firestore/aggregate_source.h | 44 ++++++ .../src/include/firebase/firestore/query.h | 16 ++ 4 files changed, 332 insertions(+) create mode 100644 firestore/src/include/firebase/firestore/aggregate_query.h create mode 100644 firestore/src/include/firebase/firestore/aggregate_query_snapshot.h create mode 100644 firestore/src/include/firebase/firestore/aggregate_source.h diff --git a/firestore/src/include/firebase/firestore/aggregate_query.h b/firestore/src/include/firebase/firestore/aggregate_query.h new file mode 100644 index 0000000000..e34b921a38 --- /dev/null +++ b/firestore/src/include/firebase/firestore/aggregate_query.h @@ -0,0 +1,134 @@ +/* + * Copyright 2022 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_AGGREGATE_QUERY_H_ +#define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_QUERY_H_ + +#include "firebase/firestore/aggregate_source.h" + +namespace firebase { +/// @cond FIREBASE_APP_INTERNAL +template +class Future; +/// @endcond + +namespace firestore { + +class AggregateQuerySnapshot; +class Query; + +/** + * @brief A query that calculates aggregations over an underlying query. + */ +class AggregateQuery { + public: + + /** + * @brief AggregateQuery an invalid Query that has to be reassigned before it can be + * used. + * + * Calling any member function on an invalid AggregateQuery will be a no-op. If the + * function returns a value, it will return a zero, empty, or invalid value, + * depending on the type of the value. + */ + AggregateQuery(); + + /** + * @brief Copy constructor. + * + * `AggregateQuery` is immutable and can be efficiently copied (no deep copy is + * performed). + * + * @param[in] other `AggregateQuery` to copy from. + */ + AggregateQuery(const AggregateQuery& other); + + /** + * @brief Move constructor. + * + * Moving is more efficient than copying for a `AggregateQuery`. After being moved + * from, a `AggregateQuery` is equivalent to its default-constructed state. + * + * @param[in] other `AggregateQuery` to move data from. + */ + AggregateQuery(AggregateQuery&& other); + + virtual ~AggregateQuery(); + + /** + * @brief Copy assignment operator. + * + * `AggregateQuery` is immutable and can be efficiently copied (no deep copy is + * performed). + * + * @param[in] other `AggregateQuery` to copy from. + * + * @return Reference to the destination `AggregateQuery`. + */ + AggregateQuery& operator=(const AggregateQuery& other); + + /** + * @brief Move assignment operator. + * + * Moving is more efficient than copying for a `AggregateQuery`. After being moved + * from, a `AggregateQuery` is equivalent to its default-constructed state. + * + * @param[in] other `AggregateQuery` to move data from. + * + * @return Reference to the destination `AggregateQuery`. + */ + AggregateQuery& operator=(AggregateQuery&& other); + + /** + * @brief Returns the query whose aggregations will be calculated by this object. + */ + virtual Query query() const; + + /** + * @brief Executes this query. + * + * @param[in] source The source from which to acquire the aggregate results. + * + * @return A Future that will be resolved with the results of the AggregateQuery. + */ + virtual Future Get(AggregateSource aggregateSource) const; + + /** + * @brief Returns true if this `AggregateQuery` is valid, false if it is not valid. + * An invalid `AggregateQuery` could be the result of: + * - Creating a `AggregateQuery` using the default constructor. + * - Moving from the `AggregateQuery`. + * - Deleting your Firestore instance, which will invalidate all the + * `AggregateQuery` instances associated with it. + * + * @return true if this `AggregateQuery` is valid, false if this `AggregateQuery` is + * invalid. + */ + bool is_valid() const; +}; + +/** Checks `lhs` and `rhs` for equality. */ +bool operator==(const AggregateQuery& lhs, const AggregateQuery& rhs); + +/** Checks `lhs` and `rhs` for inequality. */ +inline bool operator!=(const AggregateQuery& lhs, const AggregateQuery& rhs) { + return !(lhs == rhs); +} + +} // namespace firestore +} // namespace firebase + +#endif // FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_QUERY_H_ diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h new file mode 100644 index 0000000000..4de65b0881 --- /dev/null +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -0,0 +1,138 @@ +/* + * Copyright 2022 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_AGGREGATE_QUERY_SNAPSHOT_H_ +#define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_QUERY_SNAPSHOT_H_ + +#include + +namespace firebase { +namespace firestore { + +class AggregateQuery; +class SnapshotMetadata; + +/** + * @brief A QuerySnapshot contains zero or more DocumentSnapshot objects. + * + * QuerySnapshot can be iterated using a range-based for loop, and its size can + * be inspected with empty() and size(). + * + * @note Firestore classes are not meant to be subclassed except for use in test + * mocks. Subclassing is not supported in production code and new SDK releases + * may break code that does so. + */ +class AggregateQuerySnapshot { + public: + + /** + * @brief AggregateQuerySnapshot an invalid Query that has to be reassigned before it + * can be used. + * + * Calling any member function on an invalid AggregateQuerySnapshot will be a no-op. + * If the function returns a value, it will return a zero, empty, or invalid value, + * depending on the type of the value. + */ + AggregateQuerySnapshot(); + + /** + * @brief Copy constructor. + * + * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no deep copy + * is performed). + * + * @param[in] other `AggregateQuerySnapshot` to copy from. + */ + AggregateQuerySnapshot(const AggregateQuerySnapshot& other); + + /** + * @brief Move constructor. + * + * Moving is more efficient than copying for a `AggregateQuerySnapshot`. After being + * moved from, a `AggregateQuerySnapshot` is equivalent to its default-constructed + * state. + * + * @param[in] other `AggregateQuerySnapshot` to move data from. + */ + AggregateQuerySnapshot(AggregateQuerySnapshot&& other); + + virtual ~AggregateQuerySnapshot(); + + /** + * @brief Copy assignment operator. + * + * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no deep copy + * is performed). + * + * @param[in] other `AggregateQuerySnapshot` to copy from. + * + * @return Reference to the destination `AggregateQuerySnapshot`. + */ + AggregateQuerySnapshot& operator=(const AggregateQuerySnapshot& other); + + /** + * @brief Move assignment operator. + * + * Moving is more efficient than copying for a `AggregateQuerySnapshot`. After being + * moved from, a `AggregateQuerySnapshot` is equivalent to its default-constructed + * state. + * + * @param[in] other `AggregateQuerySnapshot` to move data from. + * + * @return Reference to the destination `AggregateQuerySnapshot`. + */ + AggregateQuerySnapshot& operator=(AggregateQuerySnapshot&& other); + + /** + * @brief Returns the query that was executed to produce this result. + * + * @return The `AggregateQuery` instance. + */ + virtual AggregateQuery query() const; + + /** + * @brief Returns the number of documents in the result set of the underlying query. + * + * @return The number of documents in the result set of the underlying query. + */ + virtual int64_t count() const; + + /** + * @brief Returns true if this `AggregateQuerySnapshot` is valid, false if it is not + * valid. An invalid `AggregateQuerySnapshot` could be the result of: + * - Creating a `AggregateQuerySnapshot` using the default constructor. + * - Moving from the `AggregateQuery`. + * - Deleting your Firestore instance, which will invalidate all the + * `AggregateQuery` instances associated with it. + * + * @return true if this `AggregateQuery` is valid, false if this + * `AggregateQuerySnapshot` is invalid. + */ + bool is_valid() const; +}; + +/** Checks `lhs` and `rhs` for equality. */ +bool operator==(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs); + +/** Checks `lhs` and `rhs` for inequality. */ +inline bool operator!=(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs) { + return !(lhs == rhs); +} + +} // namespace firestore +} // namespace firebase + +#endif // FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_QUERY_SNAPSHOT_H_ diff --git a/firestore/src/include/firebase/firestore/aggregate_source.h b/firestore/src/include/firebase/firestore/aggregate_source.h new file mode 100644 index 0000000000..93f5e7786f --- /dev/null +++ b/firestore/src/include/firebase/firestore/aggregate_source.h @@ -0,0 +1,44 @@ +/* + * Copyright 2022 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_AGGREGATE_SOURCE_H_ +#define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_SOURCE_H_ + +namespace firebase { +namespace firestore { + +/** + * @brief The sources from which an AggregateQuery::Get can retrieve its results. + */ +enum class AggregateSource { + /** + * Perform the aggregation on the server and download the result. + * + * The result received from the server is presented, unaltered, without considering + * any local state. That is, documents in the local cache are not taken into + * consideration, neither are local modifications not yet synchronized with the + * server. Previously-downloaded results, if any, are not used: every request using + * this source necessarily involves a round trip to the server. + * + * The AggregateQuery will fail if the server cannot be reached, such as if the + * client is offline. + */ + kServer, +}; + +} // namespace firestore +} // namespace firebase +#endif // FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_SOURCE_H_ diff --git a/firestore/src/include/firebase/firestore/query.h b/firestore/src/include/firebase/firestore/query.h index d2e73400ce..16f3ca351a 100644 --- a/firestore/src/include/firebase/firestore/query.h +++ b/firestore/src/include/firebase/firestore/query.h @@ -37,6 +37,7 @@ class Future; namespace firestore { +class AggregateQuery; class DocumentSnapshot; template class EventListener; @@ -143,6 +144,21 @@ class Query { */ virtual Firestore* firestore(); + /** + * @brief Returns a query that counts the documents in the result set of this query. + * + * The returned query, when executed, counts the documents in the result set of this + * query without actually downloading the documents. + * + * Using the returned query to count the documents is efficient because only the + * final count, not the documents' data, is downloaded. The returned query can even + * count the documents if the result set would be prohibitively large to download + * entirely (e.g. thousands of documents). + * + * @return A query that counts the documents in the result set of this query. + */ + virtual AggregateQuery Count() const; + /** * @brief Creates and returns a new Query with the additional filter that * documents must contain the specified field and the value should be equal to From 8eec324a37ad5fc0f2587c056436460cf27a9e7f Mon Sep 17 00:00:00 2001 From: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Date: Tue, 20 Dec 2022 02:51:42 +0800 Subject: [PATCH 02/26] Exempt from go/allstar check (#1173) --- .allstar/binary_artifacts.yaml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .allstar/binary_artifacts.yaml diff --git a/.allstar/binary_artifacts.yaml b/.allstar/binary_artifacts.yaml new file mode 100644 index 0000000000..c2c56dde36 --- /dev/null +++ b/.allstar/binary_artifacts.yaml @@ -0,0 +1,3 @@ +# Ignore reason: Necessary onboarding tool +ignorePaths: +- generate_xml_from_google_services_json.exe From 3c704e032083e2383a5e81ffa3a1d83e5e1df16d Mon Sep 17 00:00:00 2001 From: Matthew Hyndman Date: Tue, 20 Dec 2022 14:52:41 -0500 Subject: [PATCH 03/26] Token-changed listeners for iOS AppCheck (#1172) --- .../integration_test/src/integration_test.cc | 66 ++++++++++++++++++- app_check/src/include/firebase/app_check.h | 1 + app_check/src/ios/app_check_ios.h | 28 ++++++++ app_check/src/ios/app_check_ios.mm | 62 ++++++++++++++++- 4 files changed, 153 insertions(+), 4 deletions(-) diff --git a/app_check/integration_test/src/integration_test.cc b/app_check/integration_test/src/integration_test.cc index ae06ee2109..96f1a51adf 100644 --- a/app_check/integration_test/src/integration_test.cc +++ b/app_check/integration_test/src/integration_test.cc @@ -119,6 +119,22 @@ class FirebaseAppCheckTest : public FirebaseTest { std::vector cleanup_paths_; }; +// Listens for token changed notifications +class TestAppCheckListener : public firebase::app_check::AppCheckListener { + public: + TestAppCheckListener() : num_token_changes_(0) {} + ~TestAppCheckListener() override {} + + void OnAppCheckTokenChanged( + const firebase::app_check::AppCheckToken& token) override { + last_token_ = token; + num_token_changes_ += 1; + } + + int num_token_changes_; + firebase::app_check::AppCheckToken last_token_; +}; + // Initialization flow looks like this: // - For each test: // - Optionally initialize App Check. @@ -133,7 +149,14 @@ void FirebaseAppCheckTest::InitializeAppCheckWithDebug() { firebase::app_check::DebugAppCheckProviderFactory::GetInstance()); } -void FirebaseAppCheckTest::TerminateAppCheck() {} +void FirebaseAppCheckTest::TerminateAppCheck() { + ::firebase::app_check::AppCheck* app_check = + ::firebase::app_check::AppCheck::GetInstance(app_); + if (app_check) { + LogDebug("Shutdown App Check."); + delete app_check; + } +} void FirebaseAppCheckTest::InitializeApp() { LogDebug("Initialize Firebase App."); @@ -178,8 +201,8 @@ void FirebaseAppCheckTest::TearDown() { // Teardown all the products TerminateDatabase(); TerminateAuth(); - TerminateApp(); TerminateAppCheck(); + TerminateApp(); FirebaseTest::TearDown(); } @@ -380,6 +403,45 @@ TEST_F(FirebaseAppCheckTest, TestGetTokenLastResult) { future2.result()->expire_time_millis); } +TEST_F(FirebaseAppCheckTest, TestAddTokenChangedListener) { + InitializeAppCheckWithDebug(); + InitializeApp(); + ::firebase::app_check::AppCheck* app_check = + ::firebase::app_check::AppCheck::GetInstance(app_); + ASSERT_NE(app_check, nullptr); + + // Create and add a token changed listener. + TestAppCheckListener token_changed_listener; + app_check->AddAppCheckListener(&token_changed_listener); + + firebase::Future<::firebase::app_check::AppCheckToken> future = + app_check->GetAppCheckToken(true); + EXPECT_TRUE(WaitForCompletion(future, "GetToken")); + ::firebase::app_check::AppCheckToken token = *future.result(); + + ASSERT_EQ(token_changed_listener.num_token_changes_, 1); + EXPECT_EQ(token_changed_listener.last_token_.token, token.token); +} + +TEST_F(FirebaseAppCheckTest, TestRemoveTokenChangedListener) { + InitializeAppCheckWithDebug(); + InitializeApp(); + ::firebase::app_check::AppCheck* app_check = + ::firebase::app_check::AppCheck::GetInstance(app_); + ASSERT_NE(app_check, nullptr); + + // Create, add, and immediately remove a token changed listener. + TestAppCheckListener token_changed_listener; + app_check->AddAppCheckListener(&token_changed_listener); + app_check->RemoveAppCheckListener(&token_changed_listener); + + firebase::Future<::firebase::app_check::AppCheckToken> future = + app_check->GetAppCheckToken(true); + EXPECT_TRUE(WaitForCompletion(future, "GetToken")); + + ASSERT_EQ(token_changed_listener.num_token_changes_, 0); +} + TEST_F(FirebaseAppCheckTest, TestSignIn) { InitializeAppCheckWithDebug(); InitializeApp(); diff --git a/app_check/src/include/firebase/app_check.h b/app_check/src/include/firebase/app_check.h index 2aa55ccb0d..24a178317e 100644 --- a/app_check/src/include/firebase/app_check.h +++ b/app_check/src/include/firebase/app_check.h @@ -58,6 +58,7 @@ struct AppCheckToken { /// @brief Base class used to receive messages when AppCheck token changes. class AppCheckListener { + public: virtual ~AppCheckListener() = 0; /// This method gets invoked on the UI thread on changes to the token state. /// Does not trigger on token expiry. diff --git a/app_check/src/ios/app_check_ios.h b/app_check/src/ios/app_check_ios.h index 5fe8391ebc..1dd52ba3a6 100644 --- a/app_check/src/ios/app_check_ios.h +++ b/app_check/src/ios/app_check_ios.h @@ -26,10 +26,32 @@ #import "FIRAppCheck.h" #endif // __OBJC__ +#ifdef __OBJC__ +// Interacts with the default notification center. +@interface AppCheckNotificationCenterWrapper : NSObject + +- (id)init; + +- (void)stopListening; + +- (void)addListener:(firebase::app_check::AppCheckListener* _Nonnull)listener; + +- (void)removeListener:(firebase::app_check::AppCheckListener* _Nonnull)listener; + +- (void)appCheckTokenDidChangeNotification:(NSNotification*)notification; + +@end +#endif // __OBJC__ + namespace firebase { namespace app_check { namespace internal { +// This defines the class AppCheckNotificationCenterWrapperPointer, which is a +// C++-compatible wrapper around the AppCheckNotificationCenterWrapper Obj-C +// class. +OBJ_C_PTR_WRAPPER(AppCheckNotificationCenterWrapper); + // This defines the class FIRAppCheckPointer, which is a C++-compatible // wrapper around the FIRAppCheck Obj-C class. OBJ_C_PTR_WRAPPER(FIRAppCheck); @@ -61,10 +83,16 @@ class AppCheckInternal { private: #ifdef __OBJC__ FIRAppCheck* impl() const { return impl_->get(); } + + AppCheckNotificationCenterWrapper* notification_center_wrapper() const { + return notification_center_wrapper_->get(); + } #endif // __OBJC__ UniquePtr impl_; + UniquePtr notification_center_wrapper_; + ::firebase::App* app_; FutureManager future_manager_; diff --git a/app_check/src/ios/app_check_ios.mm b/app_check/src/ios/app_check_ios.mm index 7308bf5321..6a147a1bca 100644 --- a/app_check/src/ios/app_check_ios.mm +++ b/app_check/src/ios/app_check_ios.mm @@ -15,6 +15,7 @@ #include "app_check/src/ios/app_check_ios.h" #import "FIRApp.h" +#import "FIRAppCheck.h" #import "FIRAppCheckProvider.h" #import "FIRAppCheckProviderFactory.h" #import "FIRAppCheckToken.h" @@ -91,6 +92,56 @@ - (id)initWithProviderFactory:(firebase::app_check::AppCheckProviderFactory* _No @end +@implementation AppCheckNotificationCenterWrapper + +// A collection of NSValues containing AppCheckListener* +NSMutableArray* listeners_; + +- (id)init { + self = [super init]; + if (self) { + listeners_ = [[NSMutableArray alloc] init]; + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(appCheckTokenDidChangeNotification:) + name:FIRAppCheckAppCheckTokenDidChangeNotification + object:nil]; + } + return self; +} + +- (void)stopListening { + [[NSNotificationCenter defaultCenter] removeObserver:self + name:FIRAppCheckAppCheckTokenDidChangeNotification + object:nil]; +} + +- (void)addListener:(firebase::app_check::AppCheckListener* _Nonnull)listener { + [listeners_ addObject:[NSValue valueWithPointer:listener]]; +} + +- (void)removeListener:(firebase::app_check::AppCheckListener* _Nonnull)listener { + [listeners_ removeObject:[NSValue valueWithPointer:listener]]; +} + +- (void)appCheckTokenDidChangeNotification:(NSNotification*)notification { + NSDictionary* userInfo = notification.userInfo; + NSString* app_name = (NSString*)userInfo[kFIRAppCheckAppNameNotificationKey]; + NSString* token = (NSString*)userInfo[kFIRAppCheckTokenNotificationKey]; + // TODO(b/263138261): Include expiration time in this app check token. + // Note: The notification contains a token string, but does not currently + // contain expiration time. + firebase::app_check::AppCheckToken cpp_token; + cpp_token.token = firebase::util::NSStringToString(token); + + for (NSValue* listener_value in listeners_) { + firebase::app_check::AppCheckListener* listener = + static_cast(listener_value.pointerValue); + listener->OnAppCheckTokenChanged(cpp_token); + } +} + +@end + namespace firebase { namespace app_check { namespace internal { @@ -98,9 +149,12 @@ - (id)initWithProviderFactory:(firebase::app_check::AppCheckProviderFactory* _No AppCheckInternal::AppCheckInternal(App* app) : app_(app) { future_manager().AllocFutureApi(this, kAppCheckFnCount); impl_ = MakeUnique([FIRAppCheck appCheck]); + notification_center_wrapper_ = MakeUnique( + [[AppCheckNotificationCenterWrapper alloc] init]); } AppCheckInternal::~AppCheckInternal() { + [notification_center_wrapper() stopListening]; future_manager().ReleaseFutureApi(this); app_ = nullptr; } @@ -157,9 +211,13 @@ - (id)initWithProviderFactory:(firebase::app_check::AppCheckProviderFactory* _No future()->LastResult(kAppCheckFnGetAppCheckToken)); } -void AppCheckInternal::AddAppCheckListener(AppCheckListener* listener) {} +void AppCheckInternal::AddAppCheckListener(AppCheckListener* listener) { + [notification_center_wrapper() addListener:listener]; +} -void AppCheckInternal::RemoveAppCheckListener(AppCheckListener* listener) {} +void AppCheckInternal::RemoveAppCheckListener(AppCheckListener* listener) { + [notification_center_wrapper() removeListener:listener]; +} } // namespace internal } // namespace app_check From 2cb8eb20488b14d60cee9e83b233ec73bfd810f3 Mon Sep 17 00:00:00 2001 From: DellaBitta Date: Wed, 21 Dec 2022 11:18:04 -0500 Subject: [PATCH 04/26] [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. --- app/CMakeLists.txt | 4 +++- app/src/app_desktop.cc | 4 ++++ app/src/semaphore.h | 43 ++++++++++++++++++++++++++++------- app/src/util_apple.h | 33 +++++++++++++++++++++++++++ app/src/util_apple.mm | 39 +++++++++++++++++++++++++++++++ release_build_files/readme.md | 6 +++++ 6 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 app/src/util_apple.h create mode 100644 app/src/util_apple.mm diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index a12d42b2c6..063323e707 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -157,6 +157,7 @@ set(app_android_SRCS src/uuid.cc) set(app_ios_SRCS src/app_ios.mm + src/util_apple.mm src/util_ios.mm src/invites/ios/invites_receiver_internal_ios.mm src/invites/ios/invites_ios_startup.mm @@ -220,7 +221,8 @@ else() src/secure/user_secure_darwin_internal.mm src/filesystem_apple.mm src/locale_apple.mm - src/uuid_ios_darwin.mm) + src/uuid_ios_darwin.mm + src/util_apple.mm) else() # Linux requires libsecret. pkg_check_modules(LIBSECRET libsecret-1) diff --git a/app/src/app_desktop.cc b/app/src/app_desktop.cc index ad0de8ed92..0276c163c9 100644 --- a/app/src/app_desktop.cc +++ b/app/src/app_desktop.cc @@ -29,6 +29,7 @@ #include "app/src/include/firebase/internal/common.h" #include "app/src/include/firebase/version.h" #include "app/src/log.h" +#include "app/src/semaphore.h" #include "app/src/util.h" namespace firebase { @@ -131,6 +132,9 @@ App* App::Create(const AppOptions& options, const char* name) { // NOLINT return app; } LogDebug("Creating Firebase App %s for %s", name, kFirebaseVersionString); + LogDebug("Validating semaphore creation."); + { firebase::Semaphore sem_test(0); } + AppOptions options_with_defaults = options; if (options_with_defaults.PopulateRequiredWithDefaults()) { app = new App(); diff --git a/app/src/semaphore.h b/app/src/semaphore.h index 316373d8a0..83a28468a0 100644 --- a/app/src/semaphore.h +++ b/app/src/semaphore.h @@ -19,6 +19,7 @@ #include +#include "app/src/assert.h" #include "app/src/include/firebase/internal/platform.h" #include "app/src/time.h" @@ -39,6 +40,9 @@ #if FIREBASE_PLATFORM_OSX || FIREBASE_PLATFORM_IOS || FIREBASE_PLATFORM_TVOS #include "app/src/include/firebase/internal/mutex.h" #include "app/src/pthread_condvar.h" +#include "app/src/util_apple.h" + +#define FIREBASE_SEMAPHORE_DEFAULT_PREFIX "/firebase-" #endif // FIREBASE_PLATFORM_OSX || FIREBASE_PLATFORM_IOS || // FIREBASE_PLATFORM_TVOS @@ -50,29 +54,51 @@ class Semaphore { #if FIREBASE_PLATFORM_OSX || FIREBASE_PLATFORM_IOS || FIREBASE_PLATFORM_TVOS // MacOS requires named semaphores, and does not support unnamed. // Generate a unique string for the semaphore name: - static const char kPrefix[] = "/firebase-"; - // String length of the name prefix. - static const int kPprefixLen = sizeof(kPrefix); + // String length of the pointer, when printed to a string. static const int kPointerStringLength = 16; // Buffer size. the +1 is for the null terminator. - static const int kBufferSize = kPprefixLen + kPointerStringLength + 1; + static const int kBufferSize = kPointerStringLength + 1; char buffer[kBufferSize]; - snprintf(buffer, kBufferSize, "%s%016llx", kPrefix, + snprintf(buffer, kBufferSize, "%016llx", static_cast( // NOLINT reinterpret_cast(this))); +#if FIREBASE_PLATFORM_OSX + // Append custom semaphore names, if present, to support macOS Sandbox + // mode. + std::string semaphore_name = util::GetSandboxModeSemaphorePrefix(); + if (semaphore_name.empty()) { + semaphore_name = FIREBASE_SEMAPHORE_DEFAULT_PREFIX; + } +#else + std::string semaphore_name = FIREBASE_SEMAPHORE_DEFAULT_PREFIX; +#endif // FIREBASE_PLATFORM_OSX - semaphore_ = sem_open(buffer, + semaphore_name.append(buffer); + semaphore_ = sem_open(semaphore_name.c_str(), O_CREAT | O_EXCL, // Create if it doesn't exist. S_IRUSR | S_IWUSR, // Only the owner can read/write. initial_count); + + // Check for errors. +#if FIREBASE_PLATFORM_OSX + FIREBASE_ASSERT_MESSAGE( + semaphore_ != SEM_FAILED, + "Firebase failed to create semaphore. If running in sandbox mode be " + "sure to configure FBAppGroupEntitlementName in your app's " + "Info.plist."); +#endif // FIREBASE_PLATFORM_OSX + + assert(semaphore_ != SEM_FAILED); + assert(semaphore_ != nullptr); + // Remove the semaphore from the system registry, so other processes can't // grab it. (These are designed to just be passed around internally by // pointer, like unnamed semaphores. Mac OSX targets don't support unnamed // semaphores though, so we have to use named, and just treat them like // unnamed. - bool success = (sem_unlink(buffer) == 0); + bool success = (sem_unlink(semaphore_name.c_str()) == 0); assert(success); (void)success; #elif !FIREBASE_PLATFORM_WINDOWS @@ -81,13 +107,14 @@ class Semaphore { bool success = sem_init(semaphore_, 0, initial_count) == 0; assert(success); (void)success; + assert(semaphore_ != nullptr); #else semaphore_ = CreateSemaphore(nullptr, // default security attributes initial_count, // initial count LONG_MAX, // maximum count nullptr); // unnamed semaphore -#endif assert(semaphore_ != nullptr); +#endif } ~Semaphore() { diff --git a/app/src/util_apple.h b/app/src/util_apple.h new file mode 100644 index 0000000000..2485c3621d --- /dev/null +++ b/app/src/util_apple.h @@ -0,0 +1,33 @@ +/* + * Copyright 2022 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_APP_SRC_UTIL_APPLE_H_ +#define FIREBASE_APP_SRC_UTIL_APPLE_H_ + +#include + +namespace firebase { +namespace util { + +// Attempts to query the custom semaphore prefix from the application's +// Info.plist file. Returns an empty string if a custom semahpore prefix +// wasn't conifgured. +std::string GetSandboxModeSemaphorePrefix(); + +} // namespace util +} // namespace firebase + +#endif // FIREBASE_APP_SRC_UTIL_APPLE_H_ diff --git a/app/src/util_apple.mm b/app/src/util_apple.mm new file mode 100644 index 0000000000..0d226e43f6 --- /dev/null +++ b/app/src/util_apple.mm @@ -0,0 +1,39 @@ +/* + * Copyright 2022 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 "app/src/util_apple.h" + +#import + +namespace firebase { +namespace util { + +std::string GetSandboxModeSemaphorePrefix() { + NSBundle* mainBundle = [NSBundle mainBundle]; + if (mainBundle != nil) { + NSDictionary* dictionary = [mainBundle infoDictionary]; + if (dictionary != nil) { + NSString* customPrefix = [dictionary valueForKey:@"FBAppGroupEntitlementName"]; + if (customPrefix != nil) { + return std::string(customPrefix.UTF8String).append("/"); + } + } + } + return std::string(); +} + +} // namespace util +} // namespace firebase diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 73ffad6a0b..d4ed89a831 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -645,6 +645,12 @@ code. ### Upcoming Release - Changes - Analytics: Add `analytics::SetConsent()` API. + - General (macOS): In order to support sandbox mode, apps can define a + key/value pair for FBAppGroupEntitlementName in Info.plist. The value + associated with this key will be used to prefix semaphore names + created internally by the Firebase C++ SDK so that they conform with + [macOS sandbox + requirements](https://developer.apple.com/library/archive/documentation/Security/Conceptual/AppSandboxDesignGuide/AppSandboxInDepth/AppSandboxInDepth.html#//apple_ref/doc/uid/TP40011183-CH3-SW24). ### 10.3.0 - Changes From b59f275b43e194c9d74e1f11fa74db001f8be216 Mon Sep 17 00:00:00 2001 From: Mou Sun <69009538+sunmou99@users.noreply.github.com> Date: Tue, 27 Dec 2022 15:08:08 -0800 Subject: [PATCH 05/26] Setup Desktop test workflow that detects C++ SDK breakage caused by iOS SDK changes (#1162) --- .github/workflows/integration_tests.yml | 39 ++++++++++++++++++---- scripts/gha/build_desktop.py | 4 +-- scripts/gha/it_workflow.py | 43 ++++++++++++++++++------- 3 files changed, 66 insertions(+), 20 deletions(-) diff --git a/.github/workflows/integration_tests.yml b/.github/workflows/integration_tests.yml index 0d44f9cce9..3ddaea0630 100644 --- a/.github/workflows/integration_tests.yml +++ b/.github/workflows/integration_tests.yml @@ -3,7 +3,8 @@ name: Integration tests on: schedule: - cron: "0 9 * * *" # 9am UTC = 1am PST / 2am PDT. for all testapps except firestore - - cron: "0 10 * * *" # 10am UTC = 2am PST / 3am PDT. for firestore test only + - cron: "0 10 * * *" # 10am UTC = 2am PST / 3am PDT. for firestore test against release iOS/Android SDK + - cron: "0 11 * * *" # 11am UTC = 3am PST / 4am PDT. for firestore desktop test aginst tip-of-tree iOS repo pull_request: types: [ labeled, closed ] @@ -183,17 +184,25 @@ jobs: # To feed input into the job matrix, we first need to convert to a JSON # list. Then we can use fromJson to define the field in the matrix for the tests job. if [[ "${{ github.event.schedule }}" == "0 9 * * *" ]]; then - # at 1am PST / 2am PDT. Running integration tests and generate test report for all testapps except firestore + # at 1am PST/2am PDT. Running integration tests and generate test report for all testapps except firestore apis="admob,analytics,auth,database,dynamic_links,functions,gma,installations,messaging,remote_config,storage" - elif [[ "${{ github.event.schedule }}" == "0 10 * * *" ]]; then - # at 2am PST / 3am PDT. Running integration tests for firestore and generate test report + elif [[ "${{ github.event.schedule }}" == "0 10 * * *" || "${{ github.event.schedule }}" == "0 11 * * *" ]]; then + # at 2am PST/3am PDT and 3am PST/4am PDT. Running integration tests for firestore and generate test report. apis="firestore" else apis=$( python scripts/gha/print_matrix_configuration.py -c -w integration_tests -k apis -o "${{github.event.inputs.apis}}" ${TEST_MATRIX_PARAM} ) fi + if [[ "${{ github.event.schedule }}" == "0 11 * * *" ]]; then + # at 3am PST/4am PDT. Running firestore desktop integration test aginst tip-of-tree ios repo + matrix_platform="Desktop" + matrix_os=$( python scripts/gha/print_matrix_configuration.py -w integration_tests ${TEST_MATRIX_PARAM} -k os -o "ubuntu-20.04,macos-12") + else + matrix_platform=$( python scripts/gha/print_matrix_configuration.py -w integration_tests ${TEST_MATRIX_PARAM} -k platform -o "${{github.event.inputs.platforms}}" --apis ${apis} ) + matrix_os=$( python scripts/gha/print_matrix_configuration.py -w integration_tests ${TEST_MATRIX_PARAM} -k os -o "${{github.event.inputs.operating_systems}}") + fi echo "apis=${apis}" >> $GITHUB_OUTPUT - echo "matrix_platform=$( python scripts/gha/print_matrix_configuration.py -w integration_tests ${TEST_MATRIX_PARAM} -k platform -o "${{github.event.inputs.platforms}}" --apis ${apis} )" >> $GITHUB_OUTPUT - echo "matrix_os=$( python scripts/gha/print_matrix_configuration.py -w integration_tests ${TEST_MATRIX_PARAM} -k os -o "${{github.event.inputs.operating_systems}}")" >> $GITHUB_OUTPUT + echo "matrix_platform=${matrix_platform}" >> $GITHUB_OUTPUT + echo "matrix_os=${matrix_os}" >> $GITHUB_OUTPUT echo "matrix_arch_macos=$( python scripts/gha/print_matrix_configuration.py -w integration_tests ${TEST_MATRIX_PARAM} -k architecture_macos)" >> $GITHUB_OUTPUT echo "matrix_arch_windows_linux=$( python scripts/gha/print_matrix_configuration.py -w integration_tests ${TEST_MATRIX_PARAM} -k architecture_windows_linux)" >> $GITHUB_OUTPUT # Combine architecture_macos and architecture_windows_linux to get a list of all architectures for the build matrix. @@ -318,6 +327,10 @@ jobs: additional_flags+=(--cmake_flag=-DFIREBASE_USE_BORINGSSL=ON) fi fi + if [[ "${{ github.event.schedule }}" == "0 11 * * *" ]]; then + # at 3am PST/4am PDT. Running firestore desktop integration test aginst tip-of-tree ios repo + additional_flags+=(--cmake_flag=-DFIRESTORE_DEP_SOURCE=TIP) + fi python scripts/gha/build_testapps.py --p Desktop \ --t ${{ needs.check_and_prepare.outputs.apis }} \ --output_directory "${{ github.workspace }}" \ @@ -327,6 +340,13 @@ jobs: --gha_build \ --arch ${{ matrix.arch }} \ ${additional_flags[*]} + - name: Upload Desktop Cmake + uses: actions/upload-artifact@v3 + if: ${{ !cancelled() }} + with: + name: cmake-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.ssl_variant }} + path: D:/a/firebase-cpp-sdk/firebase-cpp-sdk/ta/firestore/iti/CMakeFiles/ + retention-days: 1 - name: Prepare results summary artifact if: ${{ !cancelled() }} shell: bash @@ -1361,7 +1381,12 @@ jobs: if [[ "${{ github.event.inputs.test_pull_request }}" == "nightly-packaging" ]]; then additional_flags=(--build_against sdk) else - additional_flags=(--build_against repo) + if [[ "${{ github.event.schedule }}" == "0 11 * * *" ]]; then + # at 3am PST/4am PDT. Running firestore desktop integration test aginst tip-of-tree ios repo + additional_flags=(--build_against tip) + else + additional_flags=(--build_against repo) + fi fi if [[ "${{ needs.check_and_prepare.outputs.apis }}" == "firestore" ]]; then additional_flags+=(--build_apis firestore) diff --git a/scripts/gha/build_desktop.py b/scripts/gha/build_desktop.py index f23f793a8a..8cb5aac08f 100644 --- a/scripts/gha/build_desktop.py +++ b/scripts/gha/build_desktop.py @@ -230,7 +230,7 @@ def cmake_configure(build_dir, arch, msvc_runtime_library='static', linux_abi='l if verbose: cmd.append('-DCMAKE_VERBOSE_MAKEFILE=1') - utils.run_command(cmd) + utils.run_command(cmd, check=True) def main(): args = parse_cmdline_args() @@ -269,7 +269,7 @@ def main(): # Example: cmake --build build -j 8 --target firebase_app firebase_auth cmd.append('--target') cmd.extend(args.target) - utils.run_command(cmd) + utils.run_command(cmd, check=True) def parse_cmdline_args(): diff --git a/scripts/gha/it_workflow.py b/scripts/gha/it_workflow.py index 4a1ff17cfd..1cc9b77c0d 100644 --- a/scripts/gha/it_workflow.py +++ b/scripts/gha/it_workflow.py @@ -66,17 +66,20 @@ _COMMENT_TITLE_FLAKY = "### Integration test with FLAKINESS (succeeded after retry)\n" _COMMENT_TITLE_FAIL = "### ❌  Integration test FAILED\n" _COMMENT_TITLE_SUCCEED = "### ✅  Integration test succeeded!\n" -_COMMENT_TITLE_FLAKY_SDK = "\n***\n### [build against SDK] Integration test with FLAKINESS (succeeded after retry)\n" -_COMMENT_TITLE_FAIL_SDK = "\n***\n### ❌  [build against SDK] Integration test FAILED\n" -_COMMENT_TITLE_SUCCEED_SDK = "\n***\n### ✅  [build against SDK] Integration test succeeded!\n" _COMMENT_TITLE_FLAKY_REPO = "### [build against repo] Integration test with FLAKINESS (succeeded after retry)\n" _COMMENT_TITLE_FAIL_REPO = "### ❌  [build against repo] Integration test FAILED\n" _COMMENT_TITLE_SUCCEED_REPO = "### ✅  [build against repo] Integration test succeeded!\n" +_COMMENT_TITLE_FLAKY_SDK = "\n***\n### [build against SDK] Integration test with FLAKINESS (succeeded after retry)\n" +_COMMENT_TITLE_FAIL_SDK = "\n***\n### ❌  [build against SDK] Integration test FAILED\n" +_COMMENT_TITLE_SUCCEED_SDK = "\n***\n### ✅  [build against SDK] Integration test succeeded!\n" +_COMMENT_TITLE_FLAKY_TIP = "\n***\n### [build against tip] Integration test with FLAKINESS (succeeded after retry)\n" +_COMMENT_TITLE_FAIL_TIP = "\n***\n### ❌  [build against tip] Integration test FAILED\n" +_COMMENT_TITLE_SUCCEED_TIP = "\n***\n### ✅  [build against tip] Integration test succeeded!\n" _COMMENT_FLAKY_TRACKER = "\n\nAdd flaky tests to **[go/fpl-cpp-flake-tracker](http://go/fpl-cpp-flake-tracker)**\n" _COMMENT_IDENTIFIER = "integration-test-status-comment" -_COMMENT_HIDDEN_DIVIDER = f'\n\n' +_COMMENT_HIDDEN_DIVIDER = f'\r\n\r\n' _LOG_ARTIFACT_NAME = "log-artifact" _LOG_OUTPUT_DIR = "test_results" @@ -89,6 +92,7 @@ _BUILD_AGAINST_SDK = "sdk" _BUILD_AGAINST_REPO = "repo" +_BUILD_AGAINST_TIP = "tip" _BUILD_API_ALL = "all" _BUILD_API_FIRESTORE = "firestore" @@ -222,27 +226,44 @@ def test_report(token, actor, commit, run_id, build_against, build_apis): issue_number = _get_issue_number(token, report_title, _REPORT_LABEL) previous_comment = github.get_issue_body(token, issue_number) - [_, previous_comment_repo, previous_comment_sdk] = previous_comment.split(_COMMENT_HIDDEN_DIVIDER) + [_, previous_comment_repo, previous_comment_sdk, previous_comment_tip] = previous_comment.split(_COMMENT_HIDDEN_DIVIDER) success_or_only_flakiness, log_summary = _get_summary_table(token, run_id) if success_or_only_flakiness and not log_summary: # succeeded (without flakiness) - title = _COMMENT_TITLE_SUCCEED_REPO if build_against==_BUILD_AGAINST_REPO else _COMMENT_TITLE_SUCCEED_SDK + if build_against==_BUILD_AGAINST_REPO: + title = _COMMENT_TITLE_SUCCEED_REPO + elif build_against==_BUILD_AGAINST_SDK: + title = _COMMENT_TITLE_SUCCEED_SDK + else: + title = _COMMENT_TITLE_SUCCEED_TIP comment = title + _get_description(actor, commit, run_id) else: if success_or_only_flakiness: # all failures/errors are due to flakiness (succeeded after retry) - title = _COMMENT_TITLE_FLAKY_REPO if build_against==_BUILD_AGAINST_REPO else _COMMENT_TITLE_FLAKY_SDK + if build_against==_BUILD_AGAINST_REPO: + title = _COMMENT_TITLE_FLAKY_REPO + elif build_against==_BUILD_AGAINST_SDK: + title = _COMMENT_TITLE_FLAKY_SDK + else: + title = _COMMENT_TITLE_FLAKY_TIP else: # failures/errors still exist after retry - title = _COMMENT_TITLE_FAIL_REPO if build_against==_BUILD_AGAINST_REPO else _COMMENT_TITLE_FAIL_SDK + if build_against==_BUILD_AGAINST_REPO: + title = _COMMENT_TITLE_FAIL_REPO + elif build_against==_BUILD_AGAINST_SDK: + title = _COMMENT_TITLE_FAIL_SDK + else: + title = _COMMENT_TITLE_FAIL_TIP comment = title + _get_description(actor, commit, run_id) + log_summary + _COMMENT_FLAKY_TRACKER if build_against==_BUILD_AGAINST_REPO: - comment = prefix + _COMMENT_HIDDEN_DIVIDER + comment + _COMMENT_HIDDEN_DIVIDER + previous_comment_sdk + comment = prefix + _COMMENT_HIDDEN_DIVIDER + comment + _COMMENT_HIDDEN_DIVIDER + previous_comment_sdk + _COMMENT_HIDDEN_DIVIDER + previous_comment_tip elif build_against==_BUILD_AGAINST_SDK: - comment = prefix + _COMMENT_HIDDEN_DIVIDER + previous_comment_repo + _COMMENT_HIDDEN_DIVIDER + comment + comment = prefix + _COMMENT_HIDDEN_DIVIDER + previous_comment_repo + _COMMENT_HIDDEN_DIVIDER + comment + _COMMENT_HIDDEN_DIVIDER + previous_comment_tip + else: + comment = prefix + _COMMENT_HIDDEN_DIVIDER + previous_comment_repo + _COMMENT_HIDDEN_DIVIDER + previous_comment_sdk + _COMMENT_HIDDEN_DIVIDER + comment - if (_COMMENT_TITLE_SUCCEED_REPO in comment) and (_COMMENT_TITLE_SUCCEED_SDK in comment): + if (_COMMENT_TITLE_SUCCEED_REPO in comment) and (_COMMENT_TITLE_SUCCEED_SDK in comment) and (build_apis != _BUILD_API_FIRESTORE or _COMMENT_TITLE_SUCCEED_TIP in comment): github.close_issue(token, issue_number) else: github.open_issue(token, issue_number) From 33c1831fc05995990169d74d0ee581206af6f418 Mon Sep 17 00:00:00 2001 From: Mou Sun <69009538+sunmou99@users.noreply.github.com> Date: Tue, 3 Jan 2023 12:02:05 -0800 Subject: [PATCH 06/26] [Bugfx] Fix Nightly Test Report template (#1181) --- scripts/gha/it_workflow.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/gha/it_workflow.py b/scripts/gha/it_workflow.py index 1cc9b77c0d..ee9db0fafe 100644 --- a/scripts/gha/it_workflow.py +++ b/scripts/gha/it_workflow.py @@ -276,7 +276,7 @@ def _get_issue_number(token, title, label): for issue in issues: if issue["title"] == title: return issue["number"] - empty_comment = _COMMENT_HIDDEN_DIVIDER + " " + _COMMENT_HIDDEN_DIVIDER + empty_comment = _COMMENT_HIDDEN_DIVIDER + " " + _COMMENT_HIDDEN_DIVIDER + " " + _COMMENT_HIDDEN_DIVIDER return github.create_issue(token, title, label, empty_comment)["number"] From 40e7025d035d4b772f2ec2b3abcaa6ec6adebd43 Mon Sep 17 00:00:00 2001 From: DellaBitta Date: Tue, 3 Jan 2023 18:16:32 -0500 Subject: [PATCH 07/26] Reduce number of RewardedAd loads on iOS in CI (#1180) The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id. This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green. --- gma/integration_test/src/integration_test.cc | 110 ++++++------------- 1 file changed, 35 insertions(+), 75 deletions(-) diff --git a/gma/integration_test/src/integration_test.cc b/gma/integration_test/src/integration_test.cc index e02015d04e..153170c4b0 100644 --- a/gma/integration_test/src/integration_test.cc +++ b/gma/integration_test/src/integration_test.cc @@ -778,10 +778,6 @@ TEST_F(FirebaseGmaTest, TestAdViewLoadAd) { firebase::Future load_ad_future; const firebase::gma::AdResult* result_ptr = nullptr; - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - load_ad_future = ad_view->LoadAd(GetAdRequest()); WaitForCompletion(load_ad_future, "LoadAd"); @@ -789,8 +785,6 @@ TEST_F(FirebaseGmaTest, TestAdViewLoadAd) { ASSERT_NE(result_ptr, nullptr); EXPECT_TRUE(result_ptr->is_successful()); - FLAKY_TEST_SECTION_END(); - ASSERT_NE(result_ptr, nullptr); EXPECT_FALSE(result_ptr->response_info().adapter_responses().empty()); EXPECT_FALSE( @@ -811,10 +805,6 @@ TEST_F(FirebaseGmaTest, TestInterstitialAdLoad) { SKIP_TEST_ON_DESKTOP; SKIP_TEST_ON_SIMULATOR; - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - firebase::gma::InterstitialAd* interstitial = new firebase::gma::InterstitialAd(); @@ -837,17 +827,14 @@ TEST_F(FirebaseGmaTest, TestInterstitialAdLoad) { load_ad_future.Release(); delete interstitial; - - FLAKY_TEST_SECTION_END(); } TEST_F(FirebaseGmaTest, TestRewardedAdLoad) { SKIP_TEST_ON_DESKTOP; SKIP_TEST_ON_SIMULATOR; - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); + // TODO(@drsanta): remove when GMA whitelists CI devices. + TEST_REQUIRES_USER_INTERACTION_ON_IOS; firebase::gma::RewardedAd* rewarded = new firebase::gma::RewardedAd(); @@ -870,8 +857,6 @@ TEST_F(FirebaseGmaTest, TestRewardedAdLoad) { load_ad_future.Release(); delete rewarded; - - FLAKY_TEST_SECTION_END(); } // Interactive test section. These have been placed up front so that the @@ -1011,6 +996,9 @@ TEST_F(FirebaseGmaUITest, TestRewardedAdLoadAndShow) { SKIP_TEST_ON_DESKTOP; SKIP_TEST_ON_SIMULATOR; + // TODO(@drsanta): remove when GMA whitelists CI devices. + TEST_REQUIRES_USER_INTERACTION_ON_IOS; + firebase::gma::RewardedAd* rewarded = new firebase::gma::RewardedAd(); WaitForCompletion(rewarded->Initialize(app_framework::GetWindowContext()), @@ -1093,18 +1081,12 @@ TEST_F(FirebaseGmaTest, TestAdViewLoadAdEmptyAdRequest) { firebase::Future load_ad_future; const firebase::gma::AdResult* result_ptr = nullptr; - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - load_ad_future = ad_view->LoadAd(request); WaitForCompletion(load_ad_future, "LoadAd"); result_ptr = load_ad_future.result(); ASSERT_NE(result_ptr, nullptr); EXPECT_TRUE(result_ptr->is_successful()); - FLAKY_TEST_SECTION_END(); - EXPECT_FALSE(result_ptr->response_info().adapter_responses().empty()); EXPECT_FALSE( result_ptr->response_info().mediation_adapter_class_name().empty()); @@ -1133,14 +1115,8 @@ TEST_F(FirebaseGmaTest, TestAdViewLoadAdAnchorAdaptiveAd) { kBannerAdUnit, banner_ad_size), "Initialize"); - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - WaitForCompletion(ad_view->LoadAd(GetAdRequest()), "LoadAd"); - FLAKY_TEST_SECTION_END(); - const AdSize ad_size = ad_view->ad_size(); EXPECT_EQ(ad_size.width(), kBannerWidth); EXPECT_NE(ad_size.height(), 0); @@ -1163,14 +1139,8 @@ TEST_F(FirebaseGmaTest, TestAdViewLoadAdInlineAdaptiveAd) { kBannerAdUnit, banner_ad_size), "Initialize"); - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - WaitForCompletion(ad_view->LoadAd(GetAdRequest()), "LoadAd"); - FLAKY_TEST_SECTION_END(); - const AdSize ad_size = ad_view->ad_size(); EXPECT_EQ(ad_size.width(), kBannerWidth); EXPECT_NE(ad_size.height(), 0); @@ -1192,14 +1162,8 @@ TEST_F(FirebaseGmaTest, TestAdViewLoadAdGetInlineAdaptiveBannerMaxHeight) { kBannerAdUnit, banner_ad_size), "Initialize"); - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - WaitForCompletion(ad_view->LoadAd(GetAdRequest()), "LoadAd"); - FLAKY_TEST_SECTION_END(); - const AdSize ad_size = ad_view->ad_size(); EXPECT_EQ(ad_size.width(), kBannerWidth); EXPECT_NE(ad_size.height(), 0); @@ -1219,15 +1183,7 @@ TEST_F(FirebaseGmaTest, TestAdViewLoadAdDestroyNotCalled) { WaitForCompletion(ad_view->Initialize(app_framework::GetWindowContext(), kBannerAdUnit, banner_ad_size), "Initialize"); - - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - WaitForCompletion(ad_view->LoadAd(GetAdRequest()), "LoadAd"); - - FLAKY_TEST_SECTION_END(); - delete ad_view; } @@ -1305,10 +1261,6 @@ TEST_F(FirebaseGmaTest, TestAdView) { SKIP_TEST_ON_DESKTOP; SKIP_TEST_ON_SIMULATOR; - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - const firebase::gma::AdSize banner_ad_size(kBannerWidth, kBannerHeight); firebase::gma::AdView* ad_view = new firebase::gma::AdView(); WaitForCompletion(ad_view->Initialize(app_framework::GetWindowContext(), @@ -1504,8 +1456,6 @@ TEST_F(FirebaseGmaTest, TestAdView) { bounding_box_listener.bounding_box_changes_.back().height == -1); #endif // defined(ANDROID) || TARGET_OS_IPHONE } - - FLAKY_TEST_SECTION_END(); } TEST_F(FirebaseGmaTest, TestAdViewErrorNotInitialized) { @@ -1674,10 +1624,6 @@ TEST_F(FirebaseGmaTest, TestInterstitialAdLoadEmptyRequest) { SKIP_TEST_ON_DESKTOP; SKIP_TEST_ON_SIMULATOR; - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); - firebase::gma::InterstitialAd* interstitial = new firebase::gma::InterstitialAd(); @@ -1701,8 +1647,6 @@ TEST_F(FirebaseGmaTest, TestInterstitialAdLoadEmptyRequest) { EXPECT_FALSE(result_ptr->response_info().ToString().empty()); delete interstitial; - - FLAKY_TEST_SECTION_END(); } TEST_F(FirebaseGmaTest, TestInterstitialAdErrorNotInitialized) { @@ -1851,9 +1795,8 @@ TEST_F(FirebaseGmaTest, TestRewardedAdLoadEmptyRequest) { SKIP_TEST_ON_DESKTOP; SKIP_TEST_ON_SIMULATOR; - // Loading Ads has been deemed flaky as the AdMob Service has a chance to - // return NoFill for valid ad requests if there aren't any ads to serve. - FLAKY_TEST_SECTION_BEGIN(); + // TODO(@drsanta): remove when GMA whitelists CI devices. + TEST_REQUIRES_USER_INTERACTION_ON_IOS; // Note: while showing an ad requires user interaction in another test, // this test is mean as a baseline loadAd functionality test. @@ -1878,8 +1821,6 @@ TEST_F(FirebaseGmaTest, TestRewardedAdLoadEmptyRequest) { EXPECT_FALSE(result_ptr->response_info().ToString().empty()); delete rewarded; - - FLAKY_TEST_SECTION_END(); } TEST_F(FirebaseGmaTest, TestRewardedAdErrorNotInitialized) { @@ -1938,6 +1879,9 @@ TEST_F(FirebaseGmaTest, TesRewardedAdErrorAlreadyInitialized) { TEST_F(FirebaseGmaTest, TestRewardedAdErrorLoadInProgress) { SKIP_TEST_ON_DESKTOP; + // TODO(@drsanta): remove when GMA whitelists CI devices. + TEST_REQUIRES_USER_INTERACTION_ON_IOS; + firebase::gma::RewardedAd* rewarded = new firebase::gma::RewardedAd(); WaitForCompletion(rewarded->Initialize(app_framework::GetWindowContext()), "Initialize"); @@ -2029,10 +1973,16 @@ TEST_F(FirebaseGmaTest, TestAdViewStress) { // Load the AdView ad. firebase::gma::AdRequest request = GetAdRequest(); - WaitForCompletion(ad_view->LoadAd(request), "TestAdViewStress LoadAd"); - EXPECT_EQ(ad_view->ad_size().width(), kBannerWidth); - EXPECT_EQ(ad_view->ad_size().height(), kBannerHeight); - + firebase::Future future = ad_view->LoadAd(request); + WaitForCompletionAnyResult(future, "TestAdViewStress LoadAd"); + // Stress tests may exhaust the ad pool. If so, loadAd will return + // kAdErrorCodeNoFill. + EXPECT_TRUE(future.error() == firebase::gma::kAdErrorCodeNone || + future.error() == firebase::gma::kAdErrorCodeNoFill); + if (future.error() == firebase::gma::kAdErrorCodeNone) { + EXPECT_EQ(ad_view->ad_size().width(), kBannerWidth); + EXPECT_EQ(ad_view->ad_size().height(), kBannerHeight); + } WaitForCompletion(ad_view->Destroy(), "Destroy the AdView"); delete ad_view; } @@ -2055,8 +2005,13 @@ TEST_F(FirebaseGmaTest, TestInterstitialAdStress) { // When the InterstitialAd is initialized, load an ad. firebase::gma::AdRequest request = GetAdRequest(); - WaitForCompletion(interstitial->LoadAd(kInterstitialAdUnit, request), - "TestInterstitialAdStress LoadAd"); + firebase::Future future = + interstitial->LoadAd(kInterstitialAdUnit, request); + WaitForCompletionAnyResult(future, "TestInterstitialAdStress LoadAd"); + // Stress tests may exhaust the ad pool. If so, loadAd will return + // kAdErrorCodeNoFill. + EXPECT_TRUE(future.error() == firebase::gma::kAdErrorCodeNone || + future.error() == firebase::gma::kAdErrorCodeNoFill); delete interstitial; } } @@ -2065,7 +2020,7 @@ TEST_F(FirebaseGmaTest, TestRewardedAdStress) { SKIP_TEST_ON_DESKTOP; SKIP_TEST_ON_EMULATOR; - // TODO(@drsanta): remove when GMA whitelists CI devices + // TODO(@drsanta): remove when GMA whitelists CI devices. TEST_REQUIRES_USER_INTERACTION_ON_IOS; for (int i = 0; i < 10; ++i) { @@ -2076,8 +2031,13 @@ TEST_F(FirebaseGmaTest, TestRewardedAdStress) { // When the RewardedAd is initialized, load an ad. firebase::gma::AdRequest request = GetAdRequest(); - WaitForCompletion(rewarded->LoadAd(kRewardedAdUnit, request), - "TestRewardedAdStress LoadAd"); + firebase::Future future = + rewarded->LoadAd(kRewardedAdUnit, request); + WaitForCompletionAnyResult(future, "TestRewardedAdStress LoadAd"); + // Stress tests may exhaust the ad pool. If so, loadAd will return + // kAdErrorCodeNoFill. + EXPECT_TRUE(future.error() == firebase::gma::kAdErrorCodeNone || + future.error() == firebase::gma::kAdErrorCodeNoFill); delete rewarded; } } From 2810bef97ac5d053e3b1a76ace367545924011ab Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 30 Jan 2023 13:29:41 -0500 Subject: [PATCH 08/26] Firestore COUNT implementation (WIP) --- app/CMakeLists.txt | 5 +- firestore/CMakeLists.txt | 7 +- firestore/src/common/aggregate_query.cc | 107 ++++++++++++++++++ .../src/common/aggregate_query_snapshot.cc | 101 +++++++++++++++++ firestore/src/common/query.cc | 6 + firestore/src/common/type_mapping.h | 12 ++ .../firebase/firestore/aggregate_query.h | 24 +++- .../firestore/aggregate_query_snapshot.h | 19 +++- firestore/src/main/aggregate_query_main.cc | 70 ++++++++++++ firestore/src/main/aggregate_query_main.h | 62 ++++++++++ .../src/main/aggregate_query_snapshot_main.cc | 42 +++++++ .../src/main/aggregate_query_snapshot_main.h | 67 +++++++++++ firestore/src/main/converter_main.h | 15 +++ firestore/src/main/query_main.cc | 6 + firestore/src/main/query_main.h | 2 + release_build_files/readme.md | 1 + 16 files changed, 539 insertions(+), 7 deletions(-) create mode 100644 firestore/src/common/aggregate_query.cc create mode 100644 firestore/src/common/aggregate_query_snapshot.cc create mode 100644 firestore/src/main/aggregate_query_main.cc create mode 100644 firestore/src/main/aggregate_query_main.h create mode 100644 firestore/src/main/aggregate_query_snapshot_main.cc create mode 100644 firestore/src/main/aggregate_query_snapshot_main.h diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index 063323e707..68ea9ea8e7 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -522,8 +522,11 @@ if (IOS) ${FIREBASE_SOURCE_DIR}/dynamic_links/src/include/firebase/dynamic_links/components.h) set(firestore_HDRS ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore.h - ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/collection_reference.h + ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/aggregate_query.h + ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h + ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/aggregate_source.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/document_change.h + ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/collection_reference.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/document_reference.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/document_snapshot.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/field_path.h diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index 1d9c9c5f07..30603b9939 100644 --- a/firestore/CMakeLists.txt +++ b/firestore/CMakeLists.txt @@ -20,6 +20,8 @@ else() endif() set(common_SRCS + src/common/aggregate_query.cc + src/common/aggregate_query_snapshot.cc src/common/cleanup.h src/common/collection_reference.cc src/common/compiler_info.cc @@ -185,7 +187,10 @@ set(android_SRCS # Sources that apply to all non-Android platforms. set(main_SRCS - src/main/collection_reference_main.cc + src/main/aggregate_query_main.cc + src/main/aggregate_query_main.h + src/main/aggregate_query_snapshot_main.cc + src/main/aggregate_query_snapshot_main.h src/main/collection_reference_main.h src/main/converter_main.h src/main/document_change_main.cc diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc new file mode 100644 index 0000000000..c9b947e01b --- /dev/null +++ b/firestore/src/common/aggregate_query.cc @@ -0,0 +1,107 @@ +/* +* Copyright 2022 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/aggregate_query.h" +#include "firestore/src/include/firebase/firestore/aggregate_query_snapshot.h" +#include "firestore/src/common/cleanup.h" +#include "firestore/src/common/futures.h" +#include "firestore/src/common/util.h" + +#if defined(__ANDROID__) +#include "firestore/src/android/aggregate_query_android.h" +#else +#include "firestore/src/main/aggregate_query_main.h" +#endif // defined(__ANDROID__) + +namespace firebase { +namespace firestore { + +using CleanupFnQuery = CleanupFn; + +AggregateQuery::AggregateQuery() {} + +AggregateQuery::AggregateQuery(const AggregateQuery& query) { + if (query.internal_) { + internal_ = new AggregateQueryInternal(*query.internal_); + } + CleanupFnQuery::Register(this, internal_); +} + +AggregateQuery::AggregateQuery(AggregateQuery&& query) { + CleanupFnQuery::Unregister(&query, query.internal_); + std::swap(internal_, query.internal_); + CleanupFnQuery::Register(this, internal_); +} + +AggregateQuery::AggregateQuery(AggregateQueryInternal* internal) : internal_(internal) { + // NOTE: We don't assert internal != nullptr here since internal can be + // nullptr when called by the CollectionReference copy constructor. + CleanupFnQuery::Register(this, internal_); +} + +AggregateQuery::~AggregateQuery() { + CleanupFnQuery::Unregister(this, internal_); + delete internal_; + internal_ = nullptr; +} + +AggregateQuery& AggregateQuery::operator=(const AggregateQuery& query) { + if (this == &query) { + return *this; + } + + CleanupFnQuery::Unregister(this, internal_); + delete internal_; + if (query.internal_) { + internal_ = new AggregateQueryInternal(*query.internal_); + } else { + internal_ = nullptr; + } + CleanupFnQuery::Register(this, internal_); + return *this; +} + +AggregateQuery& AggregateQuery::operator=(AggregateQuery&& query) { + if (this == &query) { + return *this; + } + + CleanupFnQuery::Unregister(&query, query.internal_); + CleanupFnQuery::Unregister(this, internal_); + delete internal_; + internal_ = query.internal_; + query.internal_ = nullptr; + CleanupFnQuery::Register(this, internal_); + return *this; +} + +Query AggregateQuery::query() const { + if (!internal_) return {}; + return internal_->query(); +} + +Future AggregateQuery::Get(AggregateSource aggregateSource) const { + if (!internal_) return FailedFuture(); + return internal_->Get(aggregateSource); +} + +bool operator==(const AggregateQuery& lhs, const AggregateQuery& rhs) { + return EqualityCompare(lhs.internal_, rhs.internal_); +} + + +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc new file mode 100644 index 0000000000..b76b6df79e --- /dev/null +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -0,0 +1,101 @@ +/* +* 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/aggregate_query_snapshot.h" + +#include "firestore/src/common/cleanup.h" +#include "firestore/src/include/firebase/firestore/aggregate_query.h" + +#if defined(__ANDROID__) + +#else +#include "firestore/src/main/aggregate_query_snapshot_main.h" +#endif // defined(__ANDROID__) + +namespace firebase { +namespace firestore { + +using CleanupFnAggregateQuerySnapshot = CleanupFn; + +AggregateQuerySnapshot::AggregateQuerySnapshot() {} + +AggregateQuerySnapshot::AggregateQuerySnapshot(const AggregateQuerySnapshot& query) { + if (query.internal_) { + internal_ = new AggregateQuerySnapshotInternal(*query.internal_); + } + CleanupFnAggregateQuerySnapshot::Register(this, internal_); +} + +AggregateQuerySnapshot::AggregateQuerySnapshot(AggregateQuerySnapshot&& query) { + CleanupFnAggregateQuerySnapshot::Unregister(&query, query.internal_); + std::swap(internal_, query.internal_); + CleanupFnAggregateQuerySnapshot::Register(this, internal_); +} + +AggregateQuerySnapshot::AggregateQuerySnapshot(AggregateQuerySnapshotInternal* internal) : internal_(internal) { + // NOTE: We don't assert internal != nullptr here since internal can be + // nullptr when called by the CollectionReference copy constructor. + CleanupFnAggregateQuerySnapshot::Register(this, internal_); +} + +AggregateQuerySnapshot::~AggregateQuerySnapshot() { + CleanupFnAggregateQuerySnapshot::Unregister(this, internal_); + delete internal_; + internal_ = nullptr; +} + +AggregateQuerySnapshot& AggregateQuerySnapshot::operator=(const AggregateQuerySnapshot& snapshot) { + if (this == &snapshot) { + return *this; + } + + CleanupFnAggregateQuerySnapshot::Unregister(this, internal_); + delete internal_; + if (snapshot.internal_) { + internal_ = new AggregateQuerySnapshotInternal(*snapshot.internal_); + } else { + internal_ = nullptr; + } + CleanupFnAggregateQuerySnapshot::Register(this, internal_); + return *this; +} + +AggregateQuerySnapshot& AggregateQuerySnapshot::operator=(AggregateQuerySnapshot&& snapshot) { + if (this == &snapshot) { + return *this; + } + + CleanupFnAggregateQuerySnapshot::Unregister(&snapshot, snapshot.internal_); + CleanupFnAggregateQuerySnapshot::Unregister(this, internal_); + delete internal_; + internal_ = snapshot.internal_; + snapshot.internal_ = nullptr; + CleanupFnAggregateQuerySnapshot::Register(this, internal_); + return *this; +} + +AggregateQuery AggregateQuerySnapshot::query() const { + if (!internal_) return {}; + return internal_->query(); +} + +int64_t AggregateQuerySnapshot::count() const { + if (!internal_) return 0; + return internal_->count(); +} + +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/common/query.cc b/firestore/src/common/query.cc index de525c52cc..7edbdd7602 100644 --- a/firestore/src/common/query.cc +++ b/firestore/src/common/query.cc @@ -23,6 +23,7 @@ #include "firestore/src/common/futures.h" #include "firestore/src/common/hard_assert_common.h" #include "firestore/src/common/util.h" +#include "firestore/src/include/firebase/firestore/aggregate_query.h" #include "firestore/src/include/firebase/firestore/document_snapshot.h" #include "firestore/src/include/firebase/firestore/field_path.h" #include "firestore/src/include/firebase/firestore/field_value.h" @@ -106,6 +107,11 @@ Firestore* Query::firestore() { return internal_->firestore(); } +AggregateQuery Query::Count() const { + if (!internal_) return {}; + return internal_->Count(); +} + Query Query::WhereEqualTo(const std::string& field, const FieldValue& value) const { return WhereEqualTo(FieldPath::FromDotSeparatedString(field), value); diff --git a/firestore/src/common/type_mapping.h b/firestore/src/common/type_mapping.h index aedee57887..269b92a410 100644 --- a/firestore/src/common/type_mapping.h +++ b/firestore/src/common/type_mapping.h @@ -22,6 +22,10 @@ namespace firebase { namespace firestore { +class AggregateQuery; +class AggregateQueryInternal; +class AggregateQuerySnapshot; +class AggregateQuerySnapshotInternal; class CollectionReference; class CollectionReferenceInternal; class DocumentChange; @@ -54,6 +58,14 @@ class WriteBatchInternal; template struct InternalTypeMap {}; +template <> +struct InternalTypeMap { + using type = AggregateQueryInternal; +}; +template <> +struct InternalTypeMap { + using type = AggregateQuerySnapshotInternal; +}; template <> struct InternalTypeMap { using type = CollectionReferenceInternal; diff --git a/firestore/src/include/firebase/firestore/aggregate_query.h b/firestore/src/include/firebase/firestore/aggregate_query.h index e34b921a38..91d31be6e3 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query.h +++ b/firestore/src/include/firebase/firestore/aggregate_query.h @@ -19,6 +19,8 @@ #include "firebase/firestore/aggregate_source.h" +#include + namespace firebase { /// @cond FIREBASE_APP_INTERNAL template @@ -27,6 +29,7 @@ class Future; namespace firestore { +class AggregateQueryInternal; class AggregateQuerySnapshot; class Query; @@ -35,7 +38,6 @@ class Query; */ class AggregateQuery { public: - /** * @brief AggregateQuery an invalid Query that has to be reassigned before it can be * used. @@ -100,7 +102,7 @@ class AggregateQuery { /** * @brief Executes this query. * - * @param[in] source The source from which to acquire the aggregate results. + * @param[in] aggregateSource The source from which to acquire the aggregate results. * * @return A Future that will be resolved with the results of the AggregateQuery. */ @@ -117,7 +119,23 @@ class AggregateQuery { * @return true if this `AggregateQuery` is valid, false if this `AggregateQuery` is * invalid. */ - bool is_valid() const; + bool is_valid() const { return internal_ != nullptr; } + + private: + std::size_t Hash() const; + + friend class AggregateQueryInternal; + friend struct ConverterImpl; + + friend bool operator==(const AggregateQuery& lhs, const AggregateQuery& rhs); + friend std::size_t QuerySnapshotHash(const AggregateQuery& snapshot); + + template + friend struct CleanupFn; + + explicit AggregateQuery(AggregateQueryInternal* internal); + + mutable AggregateQueryInternal* internal_ = nullptr; }; /** Checks `lhs` and `rhs` for equality. */ diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h index 4de65b0881..df9046ca35 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -17,13 +17,15 @@ #ifndef FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_QUERY_SNAPSHOT_H_ #define FIREBASE_FIRESTORE_SRC_INCLUDE_FIREBASE_FIRESTORE_AGGREGATE_QUERY_SNAPSHOT_H_ +#include #include +#include "firestore/src/main/aggregate_query_snapshot_main.h" + namespace firebase { namespace firestore { class AggregateQuery; -class SnapshotMetadata; /** * @brief A QuerySnapshot contains zero or more DocumentSnapshot objects. @@ -37,7 +39,6 @@ class SnapshotMetadata; */ class AggregateQuerySnapshot { public: - /** * @brief AggregateQuerySnapshot an invalid Query that has to be reassigned before it * can be used. @@ -122,6 +123,20 @@ class AggregateQuerySnapshot { * `AggregateQuerySnapshot` is invalid. */ bool is_valid() const; + + private: + std::size_t Hash() const; + + friend bool operator==(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs); + friend std::size_t AggregateQuerySnapshotHash(const AggregateQuerySnapshot& snapshot); + friend struct ConverterImpl; + + template + friend struct CleanupFn; + + explicit AggregateQuerySnapshot(AggregateQuerySnapshotInternal* internal); + + mutable AggregateQuerySnapshotInternal* internal_ = nullptr; }; /** Checks `lhs` and `rhs` for equality. */ diff --git a/firestore/src/main/aggregate_query_main.cc b/firestore/src/main/aggregate_query_main.cc new file mode 100644 index 0000000000..3a62e0464f --- /dev/null +++ b/firestore/src/main/aggregate_query_main.cc @@ -0,0 +1,70 @@ +/* +* 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/main/aggregate_query_main.h" + +#include "Firestore/core/src/api/aggregate_query.h" +#include "Firestore/core/src/api/query_core.h" +#include "firestore/src/include/firebase/firestore/aggregate_query.h" +#include "firestore/src/include/firebase/firestore/aggregate_query_snapshot.h" +#include "firestore/src/main/aggregate_query_snapshot_main.h" +#include "firestore/src/main/listener_main.h" +#include "firestore/src/main/promise_factory_main.h" +#include "firestore/src/main/util_main.h" + +#include + +namespace firebase { +namespace firestore { + +AggregateQueryInternal::AggregateQueryInternal(api::AggregateQuery&& aggregate_query) + : aggregate_query_{std::move(aggregate_query)}, + promise_factory_{PromiseFactory::Create(this)} {} + +FirestoreInternal* AggregateQueryInternal::firestore_internal() { + return GetFirestoreInternal(&aggregate_query_.query()); +} + +const FirestoreInternal* AggregateQueryInternal::firestore_internal() const { + return GetFirestoreInternal(&aggregate_query_.query()); +} + +Query AggregateQueryInternal::query() { + return MakePublic(api::Query(aggregate_query_.query())); +} + +Future AggregateQueryInternal::Get(AggregateSource source) { + auto promise = promise_factory_.CreatePromise(AsyncApis::kGet); + aggregate_query_.Get([promise](util::StatusOr maybe_value) mutable { + if (maybe_value.ok()) { + int64_t count = maybe_value.ValueOrDie(); + AggregateQuerySnapshotInternal internal = AggregateQuerySnapshotInternal(count); + AggregateQuerySnapshot snapshot = MakePublic(std::move(internal)); + promise.SetValue(std::move(snapshot)); + } else { + promise.SetError(maybe_value.status()); + } + }); + return promise.future(); +} + +bool operator==(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs) { + // TODO - there needs to be equals operator defined on api::AggregateQuery + return lhs.aggregate_query_.query() == rhs.aggregate_query_.query(); +} + +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/main/aggregate_query_main.h b/firestore/src/main/aggregate_query_main.h new file mode 100644 index 0000000000..480def214d --- /dev/null +++ b/firestore/src/main/aggregate_query_main.h @@ -0,0 +1,62 @@ +/* +* Copyright 2022 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_TESTAPP_AGGREGATE_QUERY_MAIN_H +#define FIREBASE_TESTAPP_AGGREGATE_QUERY_MAIN_H + +#include "Firestore/core/src/api/aggregate_query.h" +#include "firestore/src/include/firebase/firestore/aggregate_query.h" +#include "firestore/src/main/firestore_main.h" + +#if defined(__ANDROID__) +#error "This header should not be used on Android." +#endif + +namespace firebase { +namespace firestore { + +class AggregateQueryInternal { + public: + explicit AggregateQueryInternal(api::AggregateQuery&& aggregate_query); + ~AggregateQueryInternal() = default; + + FirestoreInternal* firestore_internal(); + const FirestoreInternal* firestore_internal() const; + + Query query(); + + Future Get(AggregateSource source); + + friend bool operator==(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs); + + private: + enum class AsyncApis { + kGet, + kCount, + }; + + api::AggregateQuery aggregate_query_; + PromiseFactory promise_factory_; +}; + +inline bool operator!=(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs) { + return !(lhs == rhs); +} + +} // namespace firestore +} // namespace firebase + +#endif // FIREBASE_TESTAPP_AGGREGATE_QUERY_MAIN_H diff --git a/firestore/src/main/aggregate_query_snapshot_main.cc b/firestore/src/main/aggregate_query_snapshot_main.cc new file mode 100644 index 0000000000..60eecb0789 --- /dev/null +++ b/firestore/src/main/aggregate_query_snapshot_main.cc @@ -0,0 +1,42 @@ +/* +* 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/main/aggregate_query_snapshot_main.h" + +#include "firestore/src/main/converter_main.h" +#include "firestore/src/main/util_main.h" + +namespace firebase { +namespace firestore { + +FirestoreInternal* AggregateQuerySnapshotInternal::firestore_internal() { + return GetFirestoreInternal(&aggregate_query_.query()); +} + +const FirestoreInternal* AggregateQuerySnapshotInternal::firestore_internal() const { + return GetFirestoreInternal(&aggregate_query_.query()); +} + +AggregateQuery AggregateQuerySnapshotInternal::query() const { + return MakePublic(api::AggregateQuery(aggregate_query_)); +} + +int64_t AggregateQuerySnapshotInternal::count() const { + return count_result; +} + +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/main/aggregate_query_snapshot_main.h b/firestore/src/main/aggregate_query_snapshot_main.h new file mode 100644 index 0000000000..252328e6c4 --- /dev/null +++ b/firestore/src/main/aggregate_query_snapshot_main.h @@ -0,0 +1,67 @@ +/* + * 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_AGGREGATE_QUERY_SNAPSHOT_MAIN_H_ +#define FIREBASE_FIRESTORE_SRC_MAIN_AGGREGATE_QUERY_SNAPSHOT_MAIN_H_ + +#include +#include + +#include "Firestore/core/src/api/aggregate_query.h" +#include "firestore/src/include/firebase/firestore/aggregate_query.h" +//#include "absl/types/optional.h" +//#include "firestore/src/include/firebase/firestore/document_change.h" +//#include "firestore/src/include/firebase/firestore/document_snapshot.h" +//#include "firestore/src/include/firebase/firestore/metadata_changes.h" +//#include "firestore/src/include/firebase/firestore/query.h" +//#include "firestore/src/include/firebase/firestore/query_snapshot.h" +//#include "firestore/src/include/firebase/firestore/snapshot_metadata.h" +#include "firestore/src/main/aggregate_query_snapshot_main.h" +#include "firestore/src/main/firestore_main.h" + +#if defined(__ANDROID__) +#error "This header should not be used on Android." +#endif + +namespace firebase { +namespace firestore { + +class AggregateQuerySnapshotInternal { + public: + explicit AggregateQuerySnapshotInternal(int64_t count); + + FirestoreInternal* firestore_internal(); + const FirestoreInternal* firestore_internal() const; + + AggregateQuery query() const; + int64_t count() const; + + friend bool operator==(const AggregateQuerySnapshotInternal& lhs, + const AggregateQuerySnapshotInternal& rhs); + private: + api::AggregateQuery aggregate_query_; + int64_t count_result; +}; + +inline bool operator!=(const AggregateQuerySnapshotInternal& lhs, + const AggregateQuerySnapshotInternal& rhs) { + return !(lhs == rhs); +} + +} // namespace firestore +} // namespace firebase + +#endif // FIREBASE_FIRESTORE_SRC_MAIN_AGGREGATE_QUERY_SNAPSHOT_MAIN_H_ diff --git a/firestore/src/main/converter_main.h b/firestore/src/main/converter_main.h index 38dd62fb85..7a9b168f02 100644 --- a/firestore/src/main/converter_main.h +++ b/firestore/src/main/converter_main.h @@ -20,6 +20,7 @@ #include #include +#include "Firestore/core/src/api/aggregate_query.h" #include "Firestore/core/src/api/collection_reference.h" #include "Firestore/core/src/api/document_change.h" #include "Firestore/core/src/api/document_reference.h" @@ -30,8 +31,12 @@ #include "Firestore/core/src/core/transaction.h" #include "Firestore/core/src/model/field_path.h" #include "absl/memory/memory.h" +#include "firebase/firestore/aggregate_query.h" +#include "firebase/firestore/aggregate_query_snapshot.h" #include "firestore/src/common/type_mapping.h" #include "firestore/src/include/firebase/firestore.h" +#include "firestore/src/main/aggregate_query_main.h" +#include "firestore/src/main/aggregate_query_snapshot_main.h" #include "firestore/src/main/collection_reference_main.h" #include "firestore/src/main/document_change_main.h" #include "firestore/src/main/document_reference_main.h" @@ -43,6 +48,7 @@ #include "firestore/src/main/transaction_main.h" #include "firestore/src/main/write_batch_main.h" + #if defined(__ANDROID__) #error "This header should not be used on Android." #endif @@ -82,6 +88,11 @@ struct ConverterImpl { // MakePublic +inline AggregateQuery MakePublic(api::AggregateQuery&& from) { + return ConverterImpl::MakePublicFromCore( + std::move(from)); +} + inline CollectionReference MakePublic(api::CollectionReference&& from) { return ConverterImpl::MakePublicFromCore( std::move(from)); @@ -103,6 +114,10 @@ inline FieldValue MakePublic(FieldValueInternal&& from) { return ConverterImpl::MakePublicFromInternal(std::move(from)); } +inline AggregateQuerySnapshot MakePublic(AggregateQuerySnapshotInternal&& from) { + return ConverterImpl::MakePublicFromInternal(std::move(from)); +} + inline ListenerRegistration MakePublic( std::unique_ptr from, FirestoreInternal* firestore) { diff --git a/firestore/src/main/query_main.cc b/firestore/src/main/query_main.cc index 070e113fc1..b2330c04b2 100644 --- a/firestore/src/main/query_main.cc +++ b/firestore/src/main/query_main.cc @@ -18,6 +18,7 @@ #include +#include "Firestore/core/src/api/aggregate_query.h" #include "Firestore/core/src/api/listener_registration.h" #include "Firestore/core/src/core/filter.h" #include "Firestore/core/src/core/listen_options.h" @@ -33,6 +34,7 @@ #include "firestore/src/common/hard_assert_common.h" #include "firestore/src/common/macros.h" #include "firestore/src/include/firebase/firestore.h" +#include "firestore/src/main/aggregate_query_main.h" #include "firestore/src/main/converter_main.h" #include "firestore/src/main/document_snapshot_main.h" #include "firestore/src/main/listener_main.h" @@ -94,6 +96,10 @@ Future QueryInternal::Get(Source source) { return promise.future(); } +AggregateQuery QueryInternal::Count() { + return MakePublic(std::move(query_.Count())); +} + Query QueryInternal::Where(const FieldPath& field_path, Operator op, const FieldValue& value) const { diff --git a/firestore/src/main/query_main.h b/firestore/src/main/query_main.h index 4f3e89016d..958df81f03 100644 --- a/firestore/src/main/query_main.h +++ b/firestore/src/main/query_main.h @@ -57,6 +57,8 @@ class QueryInternal { virtual Future Get(Source source); + AggregateQuery Count(); + ListenerRegistration AddSnapshotListener( MetadataChanges metadata_changes, EventListener* listener); diff --git a/release_build_files/readme.md b/release_build_files/readme.md index d4ed89a831..4fb85c7e44 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -651,6 +651,7 @@ code. created internally by the Firebase C++ SDK so that they conform with [macOS sandbox requirements](https://developer.apple.com/library/archive/documentation/Security/Conceptual/AppSandboxDesignGuide/AppSandboxInDepth/AppSandboxInDepth.html#//apple_ref/doc/uid/TP40011183-CH3-SW24). + - Firestore: Add Count API. ### 10.3.0 - Changes From 371b820cdc17ee34b505d640c968c03a237ecf4f Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 30 Jan 2023 14:35:45 -0500 Subject: [PATCH 09/26] Firestore COUNT implementation (WIP) --- firestore/src/main/aggregate_query_main.cc | 5 +++-- firestore/src/main/aggregate_query_snapshot_main.cc | 4 ++++ firestore/src/main/aggregate_query_snapshot_main.h | 2 +- firestore/src/main/converter_main.h | 1 - 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/firestore/src/main/aggregate_query_main.cc b/firestore/src/main/aggregate_query_main.cc index 3a62e0464f..f04d2bedc4 100644 --- a/firestore/src/main/aggregate_query_main.cc +++ b/firestore/src/main/aggregate_query_main.cc @@ -48,10 +48,11 @@ Query AggregateQueryInternal::query() { Future AggregateQueryInternal::Get(AggregateSource source) { auto promise = promise_factory_.CreatePromise(AsyncApis::kGet); - aggregate_query_.Get([promise](util::StatusOr maybe_value) mutable { + //TODO Is there a better way to pass `aggregate_query_` than through `this` + aggregate_query_.Get([this, promise](util::StatusOr maybe_value) mutable { if (maybe_value.ok()) { int64_t count = maybe_value.ValueOrDie(); - AggregateQuerySnapshotInternal internal = AggregateQuerySnapshotInternal(count); + AggregateQuerySnapshotInternal internal = AggregateQuerySnapshotInternal(aggregate_query_, count); AggregateQuerySnapshot snapshot = MakePublic(std::move(internal)); promise.SetValue(std::move(snapshot)); } else { diff --git a/firestore/src/main/aggregate_query_snapshot_main.cc b/firestore/src/main/aggregate_query_snapshot_main.cc index 60eecb0789..52c128c8c6 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.cc +++ b/firestore/src/main/aggregate_query_snapshot_main.cc @@ -22,6 +22,10 @@ namespace firebase { namespace firestore { +AggregateQuerySnapshotInternal::AggregateQuerySnapshotInternal(api::AggregateQuery aggregate_query, int64_t count_result) + : aggregate_query_(aggregate_query), + count_result(count_result) {} + FirestoreInternal* AggregateQuerySnapshotInternal::firestore_internal() { return GetFirestoreInternal(&aggregate_query_.query()); } diff --git a/firestore/src/main/aggregate_query_snapshot_main.h b/firestore/src/main/aggregate_query_snapshot_main.h index 252328e6c4..7037905092 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.h +++ b/firestore/src/main/aggregate_query_snapshot_main.h @@ -41,7 +41,7 @@ namespace firestore { class AggregateQuerySnapshotInternal { public: - explicit AggregateQuerySnapshotInternal(int64_t count); + explicit AggregateQuerySnapshotInternal(api::AggregateQuery aggregate_query, int64_t count); FirestoreInternal* firestore_internal(); const FirestoreInternal* firestore_internal() const; diff --git a/firestore/src/main/converter_main.h b/firestore/src/main/converter_main.h index 7a9b168f02..7e8cfa5289 100644 --- a/firestore/src/main/converter_main.h +++ b/firestore/src/main/converter_main.h @@ -48,7 +48,6 @@ #include "firestore/src/main/transaction_main.h" #include "firestore/src/main/write_batch_main.h" - #if defined(__ANDROID__) #error "This header should not be used on Android." #endif From 00570d7430b70ecf553dc81b4f76c16be279457f Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 30 Jan 2023 15:30:32 -0500 Subject: [PATCH 10/26] Fix linting --- firestore/src/main/aggregate_query_main.cc | 8 ++++---- firestore/src/main/aggregate_query_main.h | 6 +++--- firestore/src/main/aggregate_query_snapshot_main.h | 8 -------- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/firestore/src/main/aggregate_query_main.cc b/firestore/src/main/aggregate_query_main.cc index f04d2bedc4..b65cbd364d 100644 --- a/firestore/src/main/aggregate_query_main.cc +++ b/firestore/src/main/aggregate_query_main.cc @@ -14,6 +14,8 @@ * limitations under the License. */ +#include + #include "firestore/src/main/aggregate_query_main.h" #include "Firestore/core/src/api/aggregate_query.h" @@ -25,8 +27,6 @@ #include "firestore/src/main/promise_factory_main.h" #include "firestore/src/main/util_main.h" -#include - namespace firebase { namespace firestore { @@ -48,7 +48,7 @@ Query AggregateQueryInternal::query() { Future AggregateQueryInternal::Get(AggregateSource source) { auto promise = promise_factory_.CreatePromise(AsyncApis::kGet); - //TODO Is there a better way to pass `aggregate_query_` than through `this` + //TODO(tomandersen) Is there a better way to pass `aggregate_query_` than through `this` aggregate_query_.Get([this, promise](util::StatusOr maybe_value) mutable { if (maybe_value.ok()) { int64_t count = maybe_value.ValueOrDie(); @@ -63,7 +63,7 @@ Future AggregateQueryInternal::Get(AggregateSource sourc } bool operator==(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs) { - // TODO - there needs to be equals operator defined on api::AggregateQuery + // TODO(tomandersen) - there needs to be equals operator defined on api::AggregateQuery return lhs.aggregate_query_.query() == rhs.aggregate_query_.query(); } diff --git a/firestore/src/main/aggregate_query_main.h b/firestore/src/main/aggregate_query_main.h index 480def214d..21067593f1 100644 --- a/firestore/src/main/aggregate_query_main.h +++ b/firestore/src/main/aggregate_query_main.h @@ -14,8 +14,8 @@ * limitations under the License. */ -#ifndef FIREBASE_TESTAPP_AGGREGATE_QUERY_MAIN_H -#define FIREBASE_TESTAPP_AGGREGATE_QUERY_MAIN_H +#ifndef FIREBASE_FIRESTORE_SRC_MAIN_AGGREGATE_QUERY_MAIN_H_ +#define FIREBASE_FIRESTORE_SRC_MAIN_AGGREGATE_QUERY_MAIN_H_ #include "Firestore/core/src/api/aggregate_query.h" #include "firestore/src/include/firebase/firestore/aggregate_query.h" @@ -59,4 +59,4 @@ inline bool operator!=(const AggregateQueryInternal& lhs, const AggregateQueryIn } // namespace firestore } // namespace firebase -#endif // FIREBASE_TESTAPP_AGGREGATE_QUERY_MAIN_H +#endif // FIREBASE_FIRESTORE_SRC_MAIN_AGGREGATE_QUERY_MAIN_H_ diff --git a/firestore/src/main/aggregate_query_snapshot_main.h b/firestore/src/main/aggregate_query_snapshot_main.h index 7037905092..b96cd30f4b 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.h +++ b/firestore/src/main/aggregate_query_snapshot_main.h @@ -22,14 +22,6 @@ #include "Firestore/core/src/api/aggregate_query.h" #include "firestore/src/include/firebase/firestore/aggregate_query.h" -//#include "absl/types/optional.h" -//#include "firestore/src/include/firebase/firestore/document_change.h" -//#include "firestore/src/include/firebase/firestore/document_snapshot.h" -//#include "firestore/src/include/firebase/firestore/metadata_changes.h" -//#include "firestore/src/include/firebase/firestore/query.h" -//#include "firestore/src/include/firebase/firestore/query_snapshot.h" -//#include "firestore/src/include/firebase/firestore/snapshot_metadata.h" -#include "firestore/src/main/aggregate_query_snapshot_main.h" #include "firestore/src/main/firestore_main.h" #if defined(__ANDROID__) From 1f3f40dd044c500f7910f19dc4cff26053a77418 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 30 Jan 2023 15:33:01 -0500 Subject: [PATCH 11/26] Fix linting --- firestore/src/main/aggregate_query_main.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/src/main/aggregate_query_main.cc b/firestore/src/main/aggregate_query_main.cc index b65cbd364d..ebfd9b199f 100644 --- a/firestore/src/main/aggregate_query_main.cc +++ b/firestore/src/main/aggregate_query_main.cc @@ -48,7 +48,7 @@ Query AggregateQueryInternal::query() { Future AggregateQueryInternal::Get(AggregateSource source) { auto promise = promise_factory_.CreatePromise(AsyncApis::kGet); - //TODO(tomandersen) Is there a better way to pass `aggregate_query_` than through `this` + // TODO(tomandersen) Is there a better way to pass `aggregate_query_` than through `this` aggregate_query_.Get([this, promise](util::StatusOr maybe_value) mutable { if (maybe_value.ok()) { int64_t count = maybe_value.ValueOrDie(); From bed0cea30a3be01924c57ac18ef2a4544d6a53f4 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 31 Jan 2023 10:40:06 -0500 Subject: [PATCH 12/26] Fix linking --- app/CMakeLists.txt | 2 +- firestore/CMakeLists.txt | 1 + firestore/src/common/collection_reference.cc | 2 +- .../src/include/firebase/firestore/aggregate_query_snapshot.h | 3 +-- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index 68ea9ea8e7..50c0162a7d 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -525,8 +525,8 @@ if (IOS) ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/aggregate_query.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/aggregate_source.h - ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/document_change.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/collection_reference.h + ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/document_change.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/document_reference.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/document_snapshot.h ${FIREBASE_SOURCE_DIR}/firestore/src/include/firebase/firestore/field_path.h diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index 30603b9939..b0a3b222c5 100644 --- a/firestore/CMakeLists.txt +++ b/firestore/CMakeLists.txt @@ -191,6 +191,7 @@ set(main_SRCS src/main/aggregate_query_main.h src/main/aggregate_query_snapshot_main.cc src/main/aggregate_query_snapshot_main.h + src/main/collection_reference_main.cc src/main/collection_reference_main.h src/main/converter_main.h src/main/document_change_main.cc diff --git a/firestore/src/common/collection_reference.cc b/firestore/src/common/collection_reference.cc index ff8fc81280..133cda5bf8 100644 --- a/firestore/src/common/collection_reference.cc +++ b/firestore/src/common/collection_reference.cc @@ -23,7 +23,7 @@ #include "firestore/src/common/util.h" #include "firestore/src/include/firebase/firestore/document_reference.h" #if defined(__ANDROID__) -#include "firestore/src/android/collection_reference_android.h" +//#include "firestore/src/android/collection_reference_android.h" #else #include "firestore/src/main/collection_reference_main.h" #endif // defined(__ANDROID__) diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h index df9046ca35..9e2bc76d22 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -20,12 +20,11 @@ #include #include -#include "firestore/src/main/aggregate_query_snapshot_main.h" - namespace firebase { namespace firestore { class AggregateQuery; +class AggregateQuerySnapshotInternal; /** * @brief A QuerySnapshot contains zero or more DocumentSnapshot objects. From e6bb4df3c2ec53064cac5e84704c8c2c25a6bec4 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 31 Jan 2023 10:48:58 -0500 Subject: [PATCH 13/26] Cleanup --- firestore/src/common/aggregate_query.cc | 2 +- firestore/src/common/aggregate_query_snapshot.cc | 2 +- firestore/src/common/collection_reference.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index c9b947e01b..5cb279e170 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -21,7 +21,7 @@ #include "firestore/src/common/util.h" #if defined(__ANDROID__) -#include "firestore/src/android/aggregate_query_android.h" +// TODO(tomandersen) #include "firestore/src/android/aggregate_query_android.h" #else #include "firestore/src/main/aggregate_query_main.h" #endif // defined(__ANDROID__) diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index b76b6df79e..124757aed3 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -20,7 +20,7 @@ #include "firestore/src/include/firebase/firestore/aggregate_query.h" #if defined(__ANDROID__) - +// TODO(tomandersen) #include "firestore/src/android/aggregate_query_snapshot_android.h" #else #include "firestore/src/main/aggregate_query_snapshot_main.h" #endif // defined(__ANDROID__) diff --git a/firestore/src/common/collection_reference.cc b/firestore/src/common/collection_reference.cc index 133cda5bf8..ff8fc81280 100644 --- a/firestore/src/common/collection_reference.cc +++ b/firestore/src/common/collection_reference.cc @@ -23,7 +23,7 @@ #include "firestore/src/common/util.h" #include "firestore/src/include/firebase/firestore/document_reference.h" #if defined(__ANDROID__) -//#include "firestore/src/android/collection_reference_android.h" +#include "firestore/src/android/collection_reference_android.h" #else #include "firestore/src/main/collection_reference_main.h" #endif // defined(__ANDROID__) From 088560d6450d48b46787ed764491b7a1dee658f8 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 31 Jan 2023 10:54:02 -0500 Subject: [PATCH 14/26] Fix release notes --- release_build_files/readme.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 52f1928a7a..bdf1b250e2 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -642,6 +642,9 @@ workflow use only during the development of your app, not for publicly shipping code. ## Release Notes +### Upcoming Release + - Firestore: Add Count API. + ### 10.4.0 - Changes - General (Android): Update to Firebase Android BoM version 31.2.0. @@ -657,13 +660,6 @@ code. - GMA (Android): Updated dependency to play-services-ads version 21.4.0. This new version requires Multidex to be enabled in your Android builds. - GMA (iOS): Updated dependency to Google-Mobile-Ads-SDK version 9.14.0. - - General (macOS): In order to support sandbox mode, apps can define a - key/value pair for FBAppGroupEntitlementName in Info.plist. The value - associated with this key will be used to prefix semaphore names - created internally by the Firebase C++ SDK so that they conform with - [macOS sandbox - requirements](https://developer.apple.com/library/archive/documentation/Security/Conceptual/AppSandboxDesignGuide/AppSandboxInDepth/AppSandboxInDepth.html#//apple_ref/doc/uid/TP40011183-CH3-SW24). - - Firestore: Add Count API. ### 10.3.0 - Changes From d928ae8f5f48947b9f549d8e983c4f35f965b2bc Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 31 Jan 2023 11:00:42 -0500 Subject: [PATCH 15/26] Format --- firestore/src/common/aggregate_query.cc | 39 +++++------ .../src/common/aggregate_query_snapshot.cc | 44 ++++++------ .../firebase/firestore/aggregate_query.h | 50 ++++++++------ .../firestore/aggregate_query_snapshot.h | 49 +++++++------ .../firebase/firestore/aggregate_source.h | 18 ++--- .../src/include/firebase/firestore/query.h | 15 ++-- firestore/src/main/aggregate_query_main.cc | 68 +++++++++++-------- firestore/src/main/aggregate_query_main.h | 34 +++++----- .../src/main/aggregate_query_snapshot_main.cc | 41 ++++++----- .../src/main/aggregate_query_snapshot_main.h | 4 +- firestore/src/main/converter_main.h | 9 +-- 11 files changed, 202 insertions(+), 169 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index 5cb279e170..6a9a6efdaa 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -1,24 +1,24 @@ /* -* Copyright 2022 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. -*/ + * Copyright 2022 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/aggregate_query.h" -#include "firestore/src/include/firebase/firestore/aggregate_query_snapshot.h" #include "firestore/src/common/cleanup.h" #include "firestore/src/common/futures.h" #include "firestore/src/common/util.h" +#include "firestore/src/include/firebase/firestore/aggregate_query_snapshot.h" #if defined(__ANDROID__) // TODO(tomandersen) #include "firestore/src/android/aggregate_query_android.h" @@ -33,7 +33,7 @@ using CleanupFnQuery = CleanupFn; AggregateQuery::AggregateQuery() {} -AggregateQuery::AggregateQuery(const AggregateQuery& query) { +AggregateQuery::AggregateQuery(const AggregateQuery& query) { if (query.internal_) { internal_ = new AggregateQueryInternal(*query.internal_); } @@ -46,7 +46,8 @@ AggregateQuery::AggregateQuery(AggregateQuery&& query) { CleanupFnQuery::Register(this, internal_); } -AggregateQuery::AggregateQuery(AggregateQueryInternal* internal) : internal_(internal) { +AggregateQuery::AggregateQuery(AggregateQueryInternal* internal) + : internal_(internal) { // NOTE: We don't assert internal != nullptr here since internal can be // nullptr when called by the CollectionReference copy constructor. CleanupFnQuery::Register(this, internal_); @@ -93,7 +94,8 @@ Query AggregateQuery::query() const { return internal_->query(); } -Future AggregateQuery::Get(AggregateSource aggregateSource) const { +Future AggregateQuery::Get( + AggregateSource aggregateSource) const { if (!internal_) return FailedFuture(); return internal_->Get(aggregateSource); } @@ -102,6 +104,5 @@ bool operator==(const AggregateQuery& lhs, const AggregateQuery& rhs) { return EqualityCompare(lhs.internal_, rhs.internal_); } - } // namespace firestore } // namespace firebase diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index 124757aed3..bef1e9e923 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -1,18 +1,18 @@ /* -* 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. -*/ + * 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/aggregate_query_snapshot.h" @@ -20,7 +20,8 @@ #include "firestore/src/include/firebase/firestore/aggregate_query.h" #if defined(__ANDROID__) -// TODO(tomandersen) #include "firestore/src/android/aggregate_query_snapshot_android.h" +// TODO(tomandersen) #include +// "firestore/src/android/aggregate_query_snapshot_android.h" #else #include "firestore/src/main/aggregate_query_snapshot_main.h" #endif // defined(__ANDROID__) @@ -32,7 +33,8 @@ using CleanupFnAggregateQuerySnapshot = CleanupFn; AggregateQuerySnapshot::AggregateQuerySnapshot() {} -AggregateQuerySnapshot::AggregateQuerySnapshot(const AggregateQuerySnapshot& query) { +AggregateQuerySnapshot::AggregateQuerySnapshot( + const AggregateQuerySnapshot& query) { if (query.internal_) { internal_ = new AggregateQuerySnapshotInternal(*query.internal_); } @@ -45,7 +47,9 @@ AggregateQuerySnapshot::AggregateQuerySnapshot(AggregateQuerySnapshot&& query) { CleanupFnAggregateQuerySnapshot::Register(this, internal_); } -AggregateQuerySnapshot::AggregateQuerySnapshot(AggregateQuerySnapshotInternal* internal) : internal_(internal) { +AggregateQuerySnapshot::AggregateQuerySnapshot( + AggregateQuerySnapshotInternal* internal) + : internal_(internal) { // NOTE: We don't assert internal != nullptr here since internal can be // nullptr when called by the CollectionReference copy constructor. CleanupFnAggregateQuerySnapshot::Register(this, internal_); @@ -57,7 +61,8 @@ AggregateQuerySnapshot::~AggregateQuerySnapshot() { internal_ = nullptr; } -AggregateQuerySnapshot& AggregateQuerySnapshot::operator=(const AggregateQuerySnapshot& snapshot) { +AggregateQuerySnapshot& AggregateQuerySnapshot::operator=( + const AggregateQuerySnapshot& snapshot) { if (this == &snapshot) { return *this; } @@ -73,7 +78,8 @@ AggregateQuerySnapshot& AggregateQuerySnapshot::operator=(const AggregateQuerySn return *this; } -AggregateQuerySnapshot& AggregateQuerySnapshot::operator=(AggregateQuerySnapshot&& snapshot) { +AggregateQuerySnapshot& AggregateQuerySnapshot::operator=( + AggregateQuerySnapshot&& snapshot) { if (this == &snapshot) { return *this; } diff --git a/firestore/src/include/firebase/firestore/aggregate_query.h b/firestore/src/include/firebase/firestore/aggregate_query.h index 91d31be6e3..98ede58328 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query.h +++ b/firestore/src/include/firebase/firestore/aggregate_query.h @@ -37,22 +37,22 @@ class Query; * @brief A query that calculates aggregations over an underlying query. */ class AggregateQuery { - public: + public: /** - * @brief AggregateQuery an invalid Query that has to be reassigned before it can be - * used. + * @brief AggregateQuery an invalid Query that has to be reassigned before it + * can be used. * - * Calling any member function on an invalid AggregateQuery will be a no-op. If the - * function returns a value, it will return a zero, empty, or invalid value, - * depending on the type of the value. + * Calling any member function on an invalid AggregateQuery will be a no-op. + * If the function returns a value, it will return a zero, empty, or invalid + * value, depending on the type of the value. */ AggregateQuery(); /** * @brief Copy constructor. * - * `AggregateQuery` is immutable and can be efficiently copied (no deep copy is - * performed). + * `AggregateQuery` is immutable and can be efficiently copied (no deep copy + * is performed). * * @param[in] other `AggregateQuery` to copy from. */ @@ -61,8 +61,9 @@ class AggregateQuery { /** * @brief Move constructor. * - * Moving is more efficient than copying for a `AggregateQuery`. After being moved - * from, a `AggregateQuery` is equivalent to its default-constructed state. + * Moving is more efficient than copying for a `AggregateQuery`. After being + * moved from, a `AggregateQuery` is equivalent to its default-constructed + * state. * * @param[in] other `AggregateQuery` to move data from. */ @@ -73,8 +74,8 @@ class AggregateQuery { /** * @brief Copy assignment operator. * - * `AggregateQuery` is immutable and can be efficiently copied (no deep copy is - * performed). + * `AggregateQuery` is immutable and can be efficiently copied (no deep copy + * is performed). * * @param[in] other `AggregateQuery` to copy from. * @@ -85,8 +86,9 @@ class AggregateQuery { /** * @brief Move assignment operator. * - * Moving is more efficient than copying for a `AggregateQuery`. After being moved - * from, a `AggregateQuery` is equivalent to its default-constructed state. + * Moving is more efficient than copying for a `AggregateQuery`. After being + * moved from, a `AggregateQuery` is equivalent to its default-constructed + * state. * * @param[in] other `AggregateQuery` to move data from. * @@ -95,29 +97,33 @@ class AggregateQuery { AggregateQuery& operator=(AggregateQuery&& other); /** - * @brief Returns the query whose aggregations will be calculated by this object. + * @brief Returns the query whose aggregations will be calculated by this + * object. */ virtual Query query() const; /** * @brief Executes this query. * - * @param[in] aggregateSource The source from which to acquire the aggregate results. + * @param[in] aggregateSource The source from which to acquire the aggregate + * results. * - * @return A Future that will be resolved with the results of the AggregateQuery. + * @return A Future that will be resolved with the results of the + * AggregateQuery. */ - virtual Future Get(AggregateSource aggregateSource) const; + virtual Future Get( + AggregateSource aggregateSource) const; /** - * @brief Returns true if this `AggregateQuery` is valid, false if it is not valid. - * An invalid `AggregateQuery` could be the result of: + * @brief Returns true if this `AggregateQuery` is valid, false if it is not + * valid. An invalid `AggregateQuery` could be the result of: * - Creating a `AggregateQuery` using the default constructor. * - Moving from the `AggregateQuery`. * - Deleting your Firestore instance, which will invalidate all the * `AggregateQuery` instances associated with it. * - * @return true if this `AggregateQuery` is valid, false if this `AggregateQuery` is - * invalid. + * @return true if this `AggregateQuery` is valid, false if this + * `AggregateQuery` is invalid. */ bool is_valid() const { return internal_ != nullptr; } diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h index 9e2bc76d22..64f8f343b3 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -39,20 +39,20 @@ class AggregateQuerySnapshotInternal; class AggregateQuerySnapshot { public: /** - * @brief AggregateQuerySnapshot an invalid Query that has to be reassigned before it - * can be used. + * @brief AggregateQuerySnapshot an invalid Query that has to be reassigned + * before it can be used. * - * Calling any member function on an invalid AggregateQuerySnapshot will be a no-op. - * If the function returns a value, it will return a zero, empty, or invalid value, - * depending on the type of the value. + * Calling any member function on an invalid AggregateQuerySnapshot will be a + * no-op. If the function returns a value, it will return a zero, empty, or + * invalid value, depending on the type of the value. */ AggregateQuerySnapshot(); /** * @brief Copy constructor. * - * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no deep copy - * is performed). + * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no + * deep copy is performed). * * @param[in] other `AggregateQuerySnapshot` to copy from. */ @@ -61,9 +61,9 @@ class AggregateQuerySnapshot { /** * @brief Move constructor. * - * Moving is more efficient than copying for a `AggregateQuerySnapshot`. After being - * moved from, a `AggregateQuerySnapshot` is equivalent to its default-constructed - * state. + * Moving is more efficient than copying for a `AggregateQuerySnapshot`. After + * being moved from, a `AggregateQuerySnapshot` is equivalent to its + * default-constructed state. * * @param[in] other `AggregateQuerySnapshot` to move data from. */ @@ -74,8 +74,8 @@ class AggregateQuerySnapshot { /** * @brief Copy assignment operator. * - * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no deep copy - * is performed). + * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no + * deep copy is performed). * * @param[in] other `AggregateQuerySnapshot` to copy from. * @@ -86,9 +86,9 @@ class AggregateQuerySnapshot { /** * @brief Move assignment operator. * - * Moving is more efficient than copying for a `AggregateQuerySnapshot`. After being - * moved from, a `AggregateQuerySnapshot` is equivalent to its default-constructed - * state. + * Moving is more efficient than copying for a `AggregateQuerySnapshot`. After + * being moved from, a `AggregateQuerySnapshot` is equivalent to its + * default-constructed state. * * @param[in] other `AggregateQuerySnapshot` to move data from. * @@ -104,15 +104,16 @@ class AggregateQuerySnapshot { virtual AggregateQuery query() const; /** - * @brief Returns the number of documents in the result set of the underlying query. + * @brief Returns the number of documents in the result set of the underlying + * query. * * @return The number of documents in the result set of the underlying query. */ virtual int64_t count() const; /** - * @brief Returns true if this `AggregateQuerySnapshot` is valid, false if it is not - * valid. An invalid `AggregateQuerySnapshot` could be the result of: + * @brief Returns true if this `AggregateQuerySnapshot` is valid, false if it + * is not valid. An invalid `AggregateQuerySnapshot` could be the result of: * - Creating a `AggregateQuerySnapshot` using the default constructor. * - Moving from the `AggregateQuery`. * - Deleting your Firestore instance, which will invalidate all the @@ -126,8 +127,10 @@ class AggregateQuerySnapshot { private: std::size_t Hash() const; - friend bool operator==(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs); - friend std::size_t AggregateQuerySnapshotHash(const AggregateQuerySnapshot& snapshot); + friend bool operator==(const AggregateQuerySnapshot& lhs, + const AggregateQuerySnapshot& rhs); + friend std::size_t AggregateQuerySnapshotHash( + const AggregateQuerySnapshot& snapshot); friend struct ConverterImpl; template @@ -139,10 +142,12 @@ class AggregateQuerySnapshot { }; /** Checks `lhs` and `rhs` for equality. */ -bool operator==(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs); +bool operator==(const AggregateQuerySnapshot& lhs, + const AggregateQuerySnapshot& rhs); /** Checks `lhs` and `rhs` for inequality. */ -inline bool operator!=(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs) { +inline bool operator!=(const AggregateQuerySnapshot& lhs, + const AggregateQuerySnapshot& rhs) { return !(lhs == rhs); } diff --git a/firestore/src/include/firebase/firestore/aggregate_source.h b/firestore/src/include/firebase/firestore/aggregate_source.h index 93f5e7786f..f0017004de 100644 --- a/firestore/src/include/firebase/firestore/aggregate_source.h +++ b/firestore/src/include/firebase/firestore/aggregate_source.h @@ -21,20 +21,22 @@ namespace firebase { namespace firestore { /** - * @brief The sources from which an AggregateQuery::Get can retrieve its results. + * @brief The sources from which an AggregateQuery::Get can retrieve its + * results. */ enum class AggregateSource { /** * Perform the aggregation on the server and download the result. * - * The result received from the server is presented, unaltered, without considering - * any local state. That is, documents in the local cache are not taken into - * consideration, neither are local modifications not yet synchronized with the - * server. Previously-downloaded results, if any, are not used: every request using - * this source necessarily involves a round trip to the server. + * The result received from the server is presented, unaltered, without + * considering any local state. That is, documents in the local cache are not + * taken into consideration, neither are local modifications not yet + * synchronized with the server. Previously-downloaded results, if any, are + * not used: every request using this source necessarily involves a round trip + * to the server. * - * The AggregateQuery will fail if the server cannot be reached, such as if the - * client is offline. + * The AggregateQuery will fail if the server cannot be reached, such as if + * the client is offline. */ kServer, }; diff --git a/firestore/src/include/firebase/firestore/query.h b/firestore/src/include/firebase/firestore/query.h index 16f3ca351a..1a6c0c398c 100644 --- a/firestore/src/include/firebase/firestore/query.h +++ b/firestore/src/include/firebase/firestore/query.h @@ -145,15 +145,16 @@ class Query { virtual Firestore* firestore(); /** - * @brief Returns a query that counts the documents in the result set of this query. + * @brief Returns a query that counts the documents in the result set of this + * query. * - * The returned query, when executed, counts the documents in the result set of this - * query without actually downloading the documents. + * The returned query, when executed, counts the documents in the result set + * of this query without actually downloading the documents. * - * Using the returned query to count the documents is efficient because only the - * final count, not the documents' data, is downloaded. The returned query can even - * count the documents if the result set would be prohibitively large to download - * entirely (e.g. thousands of documents). + * Using the returned query to count the documents is efficient because only + * the final count, not the documents' data, is downloaded. The returned query + * can even count the documents if the result set would be prohibitively large + * to download entirely (e.g. thousands of documents). * * @return A query that counts the documents in the result set of this query. */ diff --git a/firestore/src/main/aggregate_query_main.cc b/firestore/src/main/aggregate_query_main.cc index ebfd9b199f..5379d978a3 100644 --- a/firestore/src/main/aggregate_query_main.cc +++ b/firestore/src/main/aggregate_query_main.cc @@ -1,18 +1,18 @@ /* -* 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. -*/ + * 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 @@ -30,7 +30,8 @@ namespace firebase { namespace firestore { -AggregateQueryInternal::AggregateQueryInternal(api::AggregateQuery&& aggregate_query) +AggregateQueryInternal::AggregateQueryInternal( + api::AggregateQuery&& aggregate_query) : aggregate_query_{std::move(aggregate_query)}, promise_factory_{PromiseFactory::Create(this)} {} @@ -46,24 +47,31 @@ Query AggregateQueryInternal::query() { return MakePublic(api::Query(aggregate_query_.query())); } -Future AggregateQueryInternal::Get(AggregateSource source) { - auto promise = promise_factory_.CreatePromise(AsyncApis::kGet); - // TODO(tomandersen) Is there a better way to pass `aggregate_query_` than through `this` - aggregate_query_.Get([this, promise](util::StatusOr maybe_value) mutable { - if (maybe_value.ok()) { - int64_t count = maybe_value.ValueOrDie(); - AggregateQuerySnapshotInternal internal = AggregateQuerySnapshotInternal(aggregate_query_, count); - AggregateQuerySnapshot snapshot = MakePublic(std::move(internal)); - promise.SetValue(std::move(snapshot)); - } else { - promise.SetError(maybe_value.status()); - } - }); +Future AggregateQueryInternal::Get( + AggregateSource source) { + auto promise = + promise_factory_.CreatePromise(AsyncApis::kGet); + // TODO(tomandersen) Is there a better way to pass `aggregate_query_` than + // through `this` + aggregate_query_.Get( + [this, promise](util::StatusOr maybe_value) mutable { + if (maybe_value.ok()) { + int64_t count = maybe_value.ValueOrDie(); + AggregateQuerySnapshotInternal internal = + AggregateQuerySnapshotInternal(aggregate_query_, count); + AggregateQuerySnapshot snapshot = MakePublic(std::move(internal)); + promise.SetValue(std::move(snapshot)); + } else { + promise.SetError(maybe_value.status()); + } + }); return promise.future(); } -bool operator==(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs) { - // TODO(tomandersen) - there needs to be equals operator defined on api::AggregateQuery +bool operator==(const AggregateQueryInternal& lhs, + const AggregateQueryInternal& rhs) { + // TODO(tomandersen) - there needs to be equals operator defined on + // api::AggregateQuery return lhs.aggregate_query_.query() == rhs.aggregate_query_.query(); } diff --git a/firestore/src/main/aggregate_query_main.h b/firestore/src/main/aggregate_query_main.h index 21067593f1..1ae9d0616d 100644 --- a/firestore/src/main/aggregate_query_main.h +++ b/firestore/src/main/aggregate_query_main.h @@ -1,18 +1,18 @@ /* -* Copyright 2022 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. -*/ + * Copyright 2022 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_AGGREGATE_QUERY_MAIN_H_ #define FIREBASE_FIRESTORE_SRC_MAIN_AGGREGATE_QUERY_MAIN_H_ @@ -40,7 +40,8 @@ class AggregateQueryInternal { Future Get(AggregateSource source); - friend bool operator==(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs); + friend bool operator==(const AggregateQueryInternal& lhs, + const AggregateQueryInternal& rhs); private: enum class AsyncApis { @@ -52,7 +53,8 @@ class AggregateQueryInternal { PromiseFactory promise_factory_; }; -inline bool operator!=(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs) { +inline bool operator!=(const AggregateQueryInternal& lhs, + const AggregateQueryInternal& rhs) { return !(lhs == rhs); } diff --git a/firestore/src/main/aggregate_query_snapshot_main.cc b/firestore/src/main/aggregate_query_snapshot_main.cc index 52c128c8c6..55ecedc1a6 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.cc +++ b/firestore/src/main/aggregate_query_snapshot_main.cc @@ -1,18 +1,18 @@ /* -* 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. -*/ + * 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/main/aggregate_query_snapshot_main.h" @@ -22,15 +22,16 @@ namespace firebase { namespace firestore { -AggregateQuerySnapshotInternal::AggregateQuerySnapshotInternal(api::AggregateQuery aggregate_query, int64_t count_result) - : aggregate_query_(aggregate_query), - count_result(count_result) {} +AggregateQuerySnapshotInternal::AggregateQuerySnapshotInternal( + api::AggregateQuery aggregate_query, int64_t count_result) + : aggregate_query_(aggregate_query), count_result(count_result) {} FirestoreInternal* AggregateQuerySnapshotInternal::firestore_internal() { return GetFirestoreInternal(&aggregate_query_.query()); } -const FirestoreInternal* AggregateQuerySnapshotInternal::firestore_internal() const { +const FirestoreInternal* AggregateQuerySnapshotInternal::firestore_internal() + const { return GetFirestoreInternal(&aggregate_query_.query()); } @@ -38,9 +39,7 @@ AggregateQuery AggregateQuerySnapshotInternal::query() const { return MakePublic(api::AggregateQuery(aggregate_query_)); } -int64_t AggregateQuerySnapshotInternal::count() const { - return count_result; -} +int64_t AggregateQuerySnapshotInternal::count() const { return count_result; } } // namespace firestore } // namespace firebase diff --git a/firestore/src/main/aggregate_query_snapshot_main.h b/firestore/src/main/aggregate_query_snapshot_main.h index b96cd30f4b..1de57100f8 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.h +++ b/firestore/src/main/aggregate_query_snapshot_main.h @@ -33,7 +33,8 @@ namespace firestore { class AggregateQuerySnapshotInternal { public: - explicit AggregateQuerySnapshotInternal(api::AggregateQuery aggregate_query, int64_t count); + explicit AggregateQuerySnapshotInternal(api::AggregateQuery aggregate_query, + int64_t count); FirestoreInternal* firestore_internal(); const FirestoreInternal* firestore_internal() const; @@ -43,6 +44,7 @@ class AggregateQuerySnapshotInternal { friend bool operator==(const AggregateQuerySnapshotInternal& lhs, const AggregateQuerySnapshotInternal& rhs); + private: api::AggregateQuery aggregate_query_; int64_t count_result; diff --git a/firestore/src/main/converter_main.h b/firestore/src/main/converter_main.h index 7e8cfa5289..14dac38a59 100644 --- a/firestore/src/main/converter_main.h +++ b/firestore/src/main/converter_main.h @@ -88,8 +88,7 @@ struct ConverterImpl { // MakePublic inline AggregateQuery MakePublic(api::AggregateQuery&& from) { - return ConverterImpl::MakePublicFromCore( - std::move(from)); + return ConverterImpl::MakePublicFromCore(std::move(from)); } inline CollectionReference MakePublic(api::CollectionReference&& from) { @@ -113,8 +112,10 @@ inline FieldValue MakePublic(FieldValueInternal&& from) { return ConverterImpl::MakePublicFromInternal(std::move(from)); } -inline AggregateQuerySnapshot MakePublic(AggregateQuerySnapshotInternal&& from) { - return ConverterImpl::MakePublicFromInternal(std::move(from)); +inline AggregateQuerySnapshot MakePublic( + AggregateQuerySnapshotInternal&& from) { + return ConverterImpl::MakePublicFromInternal( + std::move(from)); } inline ListenerRegistration MakePublic( From c08a1babe285d4d3364d038aa87a7c8252c83abe Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 8 Feb 2023 12:12:48 -0500 Subject: [PATCH 16/26] Fixes from review --- firestore/src/common/aggregate_query.cc | 3 +-- firestore/src/common/aggregate_query_snapshot.cc | 5 +++++ .../include/firebase/firestore/aggregate_query.h | 4 ++-- .../firebase/firestore/aggregate_query_snapshot.h | 7 ++----- firestore/src/main/aggregate_query_main.cc | 7 +++---- firestore/src/main/aggregate_query_main.h | 1 - .../src/main/aggregate_query_snapshot_main.cc | 14 +++++++++++--- firestore/src/main/aggregate_query_snapshot_main.h | 4 ++-- release_build_files/readme.md | 4 +++- 9 files changed, 29 insertions(+), 20 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index 6a9a6efdaa..09767911b7 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -48,8 +48,7 @@ AggregateQuery::AggregateQuery(AggregateQuery&& query) { AggregateQuery::AggregateQuery(AggregateQueryInternal* internal) : internal_(internal) { - // NOTE: We don't assert internal != nullptr here since internal can be - // nullptr when called by the CollectionReference copy constructor. + SIMPLE_HARD_ASSERT(internal != nullptr); CleanupFnQuery::Register(this, internal_); } diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index bef1e9e923..6f58e5ceca 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -17,6 +17,7 @@ #include "firestore/src/include/firebase/firestore/aggregate_query_snapshot.h" #include "firestore/src/common/cleanup.h" +#include "firestore/src/common/util.h" #include "firestore/src/include/firebase/firestore/aggregate_query.h" #if defined(__ANDROID__) @@ -103,5 +104,9 @@ int64_t AggregateQuerySnapshot::count() const { return internal_->count(); } +bool operator==(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs) { + return EqualityCompare(lhs.internal_, rhs.internal_); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/include/firebase/firestore/aggregate_query.h b/firestore/src/include/firebase/firestore/aggregate_query.h index 98ede58328..0d8656ef9d 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query.h +++ b/firestore/src/include/firebase/firestore/aggregate_query.h @@ -39,8 +39,8 @@ class Query; class AggregateQuery { public: /** - * @brief AggregateQuery an invalid Query that has to be reassigned before it - * can be used. + * @brief Creates an invalid AggregateQuery that has to be reassigned before + * it can be used. * * Calling any member function on an invalid AggregateQuery will be a no-op. * If the function returns a value, it will return a zero, empty, or invalid diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h index 64f8f343b3..4fa2f6c7d5 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -27,10 +27,7 @@ class AggregateQuery; class AggregateQuerySnapshotInternal; /** - * @brief A QuerySnapshot contains zero or more DocumentSnapshot objects. - * - * QuerySnapshot can be iterated using a range-based for loop, and its size can - * be inspected with empty() and size(). + * @brief The results of executing an AggregateQuery. * * @note Firestore classes are not meant to be subclassed except for use in test * mocks. Subclassing is not supported in production code and new SDK releases @@ -39,7 +36,7 @@ class AggregateQuerySnapshotInternal; class AggregateQuerySnapshot { public: /** - * @brief AggregateQuerySnapshot an invalid Query that has to be reassigned + * @brief Creates an invalid AggregateQuerySnapshot that has to be reassigned * before it can be used. * * Calling any member function on an invalid AggregateQuerySnapshot will be a diff --git a/firestore/src/main/aggregate_query_main.cc b/firestore/src/main/aggregate_query_main.cc index 5379d978a3..4a4a60c633 100644 --- a/firestore/src/main/aggregate_query_main.cc +++ b/firestore/src/main/aggregate_query_main.cc @@ -49,16 +49,15 @@ Query AggregateQueryInternal::query() { Future AggregateQueryInternal::Get( AggregateSource source) { + auto aggregate_query = aggregate_query_; auto promise = promise_factory_.CreatePromise(AsyncApis::kGet); - // TODO(tomandersen) Is there a better way to pass `aggregate_query_` than - // through `this` aggregate_query_.Get( - [this, promise](util::StatusOr maybe_value) mutable { + [aggregate_query, promise](util::StatusOr maybe_value) mutable { if (maybe_value.ok()) { int64_t count = maybe_value.ValueOrDie(); AggregateQuerySnapshotInternal internal = - AggregateQuerySnapshotInternal(aggregate_query_, count); + AggregateQuerySnapshotInternal(aggregate_query, count); AggregateQuerySnapshot snapshot = MakePublic(std::move(internal)); promise.SetValue(std::move(snapshot)); } else { diff --git a/firestore/src/main/aggregate_query_main.h b/firestore/src/main/aggregate_query_main.h index 1ae9d0616d..51584d4b06 100644 --- a/firestore/src/main/aggregate_query_main.h +++ b/firestore/src/main/aggregate_query_main.h @@ -31,7 +31,6 @@ namespace firestore { class AggregateQueryInternal { public: explicit AggregateQueryInternal(api::AggregateQuery&& aggregate_query); - ~AggregateQueryInternal() = default; FirestoreInternal* firestore_internal(); const FirestoreInternal* firestore_internal() const; diff --git a/firestore/src/main/aggregate_query_snapshot_main.cc b/firestore/src/main/aggregate_query_snapshot_main.cc index 55ecedc1a6..5571799076 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.cc +++ b/firestore/src/main/aggregate_query_snapshot_main.cc @@ -23,8 +23,9 @@ namespace firebase { namespace firestore { AggregateQuerySnapshotInternal::AggregateQuerySnapshotInternal( - api::AggregateQuery aggregate_query, int64_t count_result) - : aggregate_query_(aggregate_query), count_result(count_result) {} + api::AggregateQuery&& aggregate_query, int64_t count_result) + : aggregate_query_(std::move(aggregate_query)), + count_result_(count_result) {} FirestoreInternal* AggregateQuerySnapshotInternal::firestore_internal() { return GetFirestoreInternal(&aggregate_query_.query()); @@ -39,7 +40,14 @@ AggregateQuery AggregateQuerySnapshotInternal::query() const { return MakePublic(api::AggregateQuery(aggregate_query_)); } -int64_t AggregateQuerySnapshotInternal::count() const { return count_result; } +int64_t AggregateQuerySnapshotInternal::count() const { return count_result_; } + +bool operator==(const AggregateQuerySnapshotInternal& lhs, const AggregateQuerySnapshotInternal& rhs) { + // TODO(tomandersen) - there needs to be equals operator defined on + // api::AggregateQuery + return lhs.aggregate_query_.query() == rhs.aggregate_query_.query() + && lhs.count_result_ == rhs.count_result_; +} } // namespace firestore } // namespace firebase diff --git a/firestore/src/main/aggregate_query_snapshot_main.h b/firestore/src/main/aggregate_query_snapshot_main.h index 1de57100f8..3bb91df14d 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.h +++ b/firestore/src/main/aggregate_query_snapshot_main.h @@ -33,7 +33,7 @@ namespace firestore { class AggregateQuerySnapshotInternal { public: - explicit AggregateQuerySnapshotInternal(api::AggregateQuery aggregate_query, + explicit AggregateQuerySnapshotInternal(api::AggregateQuery&& aggregate_query, int64_t count); FirestoreInternal* firestore_internal(); @@ -47,7 +47,7 @@ class AggregateQuerySnapshotInternal { private: api::AggregateQuery aggregate_query_; - int64_t count_result; + int64_t count_result_; }; inline bool operator!=(const AggregateQuerySnapshotInternal& lhs, diff --git a/release_build_files/readme.md b/release_build_files/readme.md index bdf1b250e2..0a0faaecfa 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -643,7 +643,9 @@ code. ## Release Notes ### Upcoming Release - - Firestore: Add Count API. +- Changes + - Added `Query::Count()`, which fetches the number of documents in the result + set without actually downloading the documents ([#1174](https://github.com/firebase/firebase-cpp-sdk/pull/1174)). ### 10.4.0 - Changes From dfa4248bc7310152db5dc92c5c973b4700d7b6d7 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 8 Feb 2023 12:22:25 -0500 Subject: [PATCH 17/26] Formatting --- firestore/src/common/aggregate_query_snapshot.cc | 3 ++- firestore/src/main/aggregate_query_snapshot_main.cc | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index 6f58e5ceca..f0a9145d66 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -104,7 +104,8 @@ int64_t AggregateQuerySnapshot::count() const { return internal_->count(); } -bool operator==(const AggregateQuerySnapshot& lhs, const AggregateQuerySnapshot& rhs) { +bool operator==(const AggregateQuerySnapshot& lhs, + const AggregateQuerySnapshot& rhs) { return EqualityCompare(lhs.internal_, rhs.internal_); } diff --git a/firestore/src/main/aggregate_query_snapshot_main.cc b/firestore/src/main/aggregate_query_snapshot_main.cc index 5571799076..d68455135a 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.cc +++ b/firestore/src/main/aggregate_query_snapshot_main.cc @@ -42,11 +42,12 @@ AggregateQuery AggregateQuerySnapshotInternal::query() const { int64_t AggregateQuerySnapshotInternal::count() const { return count_result_; } -bool operator==(const AggregateQuerySnapshotInternal& lhs, const AggregateQuerySnapshotInternal& rhs) { +bool operator==(const AggregateQuerySnapshotInternal& lhs, + const AggregateQuerySnapshotInternal& rhs) { // TODO(tomandersen) - there needs to be equals operator defined on // api::AggregateQuery - return lhs.aggregate_query_.query() == rhs.aggregate_query_.query() - && lhs.count_result_ == rhs.count_result_; + return lhs.aggregate_query_.query() == rhs.aggregate_query_.query() && + lhs.count_result_ == rhs.count_result_; } } // namespace firestore From 94fcdefc6648ffaf0652d6eb0d091da5ef22f1f3 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 9 Feb 2023 10:24:19 -0500 Subject: [PATCH 18/26] Responding to PR comments. --- firestore/CONTRIBUTING.md | 2 +- firestore/src/common/aggregate_query.cc | 35 ++++++++++--------- .../src/common/aggregate_query_snapshot.cc | 16 ++++----- .../firebase/firestore/aggregate_query.h | 6 ++-- .../firestore/aggregate_query_snapshot.h | 8 ++--- firestore/src/main/aggregate_query_main.cc | 8 +++-- .../src/main/aggregate_query_snapshot_main.cc | 1 + firestore/src/main/query_main.cc | 4 +-- release_build_files/readme.md | 2 +- 9 files changed, 42 insertions(+), 40 deletions(-) diff --git a/firestore/CONTRIBUTING.md b/firestore/CONTRIBUTING.md index 51cc290734..d7ad3455ea 100644 --- a/firestore/CONTRIBUTING.md +++ b/firestore/CONTRIBUTING.md @@ -19,7 +19,7 @@ a Python3 installation. # Architecture -It is easier to work this Firestore CPP SDK by keeping a high level archetecture in mind: +It is easier to work this Firestore CPP SDK by keeping a high level architecture in mind: ![architecture.png](architecture.png) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index 09767911b7..d6969b9884 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -33,16 +33,16 @@ using CleanupFnQuery = CleanupFn; AggregateQuery::AggregateQuery() {} -AggregateQuery::AggregateQuery(const AggregateQuery& query) { - if (query.internal_) { - internal_ = new AggregateQueryInternal(*query.internal_); +AggregateQuery::AggregateQuery(const AggregateQuery& aggregate_query) { + if (aggregate_query.internal_) { + internal_ = new AggregateQueryInternal(*aggregate_query.internal_); } CleanupFnQuery::Register(this, internal_); } -AggregateQuery::AggregateQuery(AggregateQuery&& query) { - CleanupFnQuery::Unregister(&query, query.internal_); - std::swap(internal_, query.internal_); +AggregateQuery::AggregateQuery(AggregateQuery&& aggregate_query) { + CleanupFnQuery::Unregister(&aggregate_query, aggregate_query.internal_); + std::swap(internal_, aggregate_query.internal_); CleanupFnQuery::Register(this, internal_); } @@ -58,15 +58,16 @@ AggregateQuery::~AggregateQuery() { internal_ = nullptr; } -AggregateQuery& AggregateQuery::operator=(const AggregateQuery& query) { - if (this == &query) { +AggregateQuery& AggregateQuery::operator=( + const AggregateQuery& aggregate_query) { + if (this == &aggregate_query) { return *this; } CleanupFnQuery::Unregister(this, internal_); delete internal_; - if (query.internal_) { - internal_ = new AggregateQueryInternal(*query.internal_); + if (aggregate_query.internal_) { + internal_ = new AggregateQueryInternal(*aggregate_query.internal_); } else { internal_ = nullptr; } @@ -74,16 +75,16 @@ AggregateQuery& AggregateQuery::operator=(const AggregateQuery& query) { return *this; } -AggregateQuery& AggregateQuery::operator=(AggregateQuery&& query) { - if (this == &query) { +AggregateQuery& AggregateQuery::operator=(AggregateQuery&& aggregate_query) { + if (this == &aggregate_query) { return *this; } - CleanupFnQuery::Unregister(&query, query.internal_); + CleanupFnQuery::Unregister(&aggregate_query, aggregate_query.internal_); CleanupFnQuery::Unregister(this, internal_); delete internal_; - internal_ = query.internal_; - query.internal_ = nullptr; + internal_ = aggregate_query.internal_; + aggregate_query.internal_ = nullptr; CleanupFnQuery::Register(this, internal_); return *this; } @@ -94,9 +95,9 @@ Query AggregateQuery::query() const { } Future AggregateQuery::Get( - AggregateSource aggregateSource) const { + AggregateSource aggregate_source) const { if (!internal_) return FailedFuture(); - return internal_->Get(aggregateSource); + return internal_->Get(aggregate_source); } bool operator==(const AggregateQuery& lhs, const AggregateQuery& rhs) { diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index f0a9145d66..b29744b100 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -35,24 +35,24 @@ using CleanupFnAggregateQuerySnapshot = CleanupFn; AggregateQuerySnapshot::AggregateQuerySnapshot() {} AggregateQuerySnapshot::AggregateQuerySnapshot( - const AggregateQuerySnapshot& query) { - if (query.internal_) { - internal_ = new AggregateQuerySnapshotInternal(*query.internal_); + const AggregateQuerySnapshot& snapshot) { + if (snapshot.internal_) { + internal_ = new AggregateQuerySnapshotInternal(*snapshot.internal_); } CleanupFnAggregateQuerySnapshot::Register(this, internal_); } -AggregateQuerySnapshot::AggregateQuerySnapshot(AggregateQuerySnapshot&& query) { - CleanupFnAggregateQuerySnapshot::Unregister(&query, query.internal_); - std::swap(internal_, query.internal_); +AggregateQuerySnapshot::AggregateQuerySnapshot( + AggregateQuerySnapshot&& snapshot) { + CleanupFnAggregateQuerySnapshot::Unregister(&snapshot, snapshot.internal_); + std::swap(internal_, snapshot.internal_); CleanupFnAggregateQuerySnapshot::Register(this, internal_); } AggregateQuerySnapshot::AggregateQuerySnapshot( AggregateQuerySnapshotInternal* internal) : internal_(internal) { - // NOTE: We don't assert internal != nullptr here since internal can be - // nullptr when called by the CollectionReference copy constructor. + SIMPLE_HARD_ASSERT(internal != nullptr); CleanupFnAggregateQuerySnapshot::Register(this, internal_); } diff --git a/firestore/src/include/firebase/firestore/aggregate_query.h b/firestore/src/include/firebase/firestore/aggregate_query.h index 0d8656ef9d..a976ba811b 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query.h +++ b/firestore/src/include/firebase/firestore/aggregate_query.h @@ -105,14 +105,14 @@ class AggregateQuery { /** * @brief Executes this query. * - * @param[in] aggregateSource The source from which to acquire the aggregate + * @param[in] aggregate_source The source from which to acquire the aggregate * results. * * @return A Future that will be resolved with the results of the * AggregateQuery. */ virtual Future Get( - AggregateSource aggregateSource) const; + AggregateSource aggregate_source) const; /** * @brief Returns true if this `AggregateQuery` is valid, false if it is not @@ -134,7 +134,7 @@ class AggregateQuery { friend struct ConverterImpl; friend bool operator==(const AggregateQuery& lhs, const AggregateQuery& rhs); - friend std::size_t QuerySnapshotHash(const AggregateQuery& snapshot); + friend std::size_t AggregateQueryHash(const AggregateQuery& aggregate_query); template friend struct CleanupFn; diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h index 4fa2f6c7d5..f2735de630 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -51,9 +51,9 @@ class AggregateQuerySnapshot { * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no * deep copy is performed). * - * @param[in] other `AggregateQuerySnapshot` to copy from. + * @param[in] snapshot `AggregateQuerySnapshot` to copy from. */ - AggregateQuerySnapshot(const AggregateQuerySnapshot& other); + AggregateQuerySnapshot(const AggregateQuerySnapshot& snapshot); /** * @brief Move constructor. @@ -62,9 +62,9 @@ class AggregateQuerySnapshot { * being moved from, a `AggregateQuerySnapshot` is equivalent to its * default-constructed state. * - * @param[in] other `AggregateQuerySnapshot` to move data from. + * @param[in] snapshot `AggregateQuerySnapshot` to move data from. */ - AggregateQuerySnapshot(AggregateQuerySnapshot&& other); + AggregateQuerySnapshot(AggregateQuerySnapshot&& snapshot); virtual ~AggregateQuerySnapshot(); diff --git a/firestore/src/main/aggregate_query_main.cc b/firestore/src/main/aggregate_query_main.cc index 4a4a60c633..607db0774b 100644 --- a/firestore/src/main/aggregate_query_main.cc +++ b/firestore/src/main/aggregate_query_main.cc @@ -49,15 +49,17 @@ Query AggregateQueryInternal::query() { Future AggregateQueryInternal::Get( AggregateSource source) { - auto aggregate_query = aggregate_query_; + api::AggregateQuery aggregate_query = aggregate_query_; auto promise = promise_factory_.CreatePromise(AsyncApis::kGet); + // TODO(C++14) + // https://en.wikipedia.org/wiki/C%2B%2B14#Lambda_capture_expressions aggregate_query_.Get( [aggregate_query, promise](util::StatusOr maybe_value) mutable { if (maybe_value.ok()) { int64_t count = maybe_value.ValueOrDie(); - AggregateQuerySnapshotInternal internal = - AggregateQuerySnapshotInternal(aggregate_query, count); + AggregateQuerySnapshotInternal internal{std::move(aggregate_query), + count}; AggregateQuerySnapshot snapshot = MakePublic(std::move(internal)); promise.SetValue(std::move(snapshot)); } else { diff --git a/firestore/src/main/aggregate_query_snapshot_main.cc b/firestore/src/main/aggregate_query_snapshot_main.cc index d68455135a..3cb427f243 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.cc +++ b/firestore/src/main/aggregate_query_snapshot_main.cc @@ -16,6 +16,7 @@ #include "firestore/src/main/aggregate_query_snapshot_main.h" +#include #include "firestore/src/main/converter_main.h" #include "firestore/src/main/util_main.h" diff --git a/firestore/src/main/query_main.cc b/firestore/src/main/query_main.cc index b2330c04b2..ffed058c9e 100644 --- a/firestore/src/main/query_main.cc +++ b/firestore/src/main/query_main.cc @@ -96,9 +96,7 @@ Future QueryInternal::Get(Source source) { return promise.future(); } -AggregateQuery QueryInternal::Count() { - return MakePublic(std::move(query_.Count())); -} +AggregateQuery QueryInternal::Count() { return MakePublic(query_.Count()); } Query QueryInternal::Where(const FieldPath& field_path, Operator op, diff --git a/release_build_files/readme.md b/release_build_files/readme.md index 0a0faaecfa..b0ab9d13c0 100644 --- a/release_build_files/readme.md +++ b/release_build_files/readme.md @@ -644,7 +644,7 @@ code. ## Release Notes ### Upcoming Release - Changes - - Added `Query::Count()`, which fetches the number of documents in the result + - Firestore: Added `Query::Count()`, which fetches the number of documents in the result set without actually downloading the documents ([#1174](https://github.com/firebase/firebase-cpp-sdk/pull/1174)). ### 10.4.0 From e9502a3f106badac45818efa565a96eddfab54ff Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 10:13:14 -0500 Subject: [PATCH 19/26] Add Hash --- firestore/src/common/aggregate_query_snapshot.cc | 5 +++++ firestore/src/include/firebase/firestore.h | 3 +++ firestore/src/main/aggregate_query_snapshot_main.h | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index b29744b100..75791264fd 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -109,5 +109,10 @@ bool operator==(const AggregateQuerySnapshot& lhs, return EqualityCompare(lhs.internal_, rhs.internal_); } +size_t AggregateQuerySnapshot::Hash() const { + if (!internal_) return {}; + return internal_->Hash(); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index c35de11b4d..1749a4e3b2 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -27,6 +27,9 @@ #include "firebase/log.h" // Include *all* the public headers to make sure including just "firestore.h" is // sufficient for users. +#include "firebase/firestore/aggregate_query.h" +#include "firebase/firestore/aggregate_query_snapshot.h" +#include "firebase/firestore/aggregate_source.h" #include "firebase/firestore/collection_reference.h" #include "firebase/firestore/document_change.h" #include "firebase/firestore/document_reference.h" diff --git a/firestore/src/main/aggregate_query_snapshot_main.h b/firestore/src/main/aggregate_query_snapshot_main.h index 3bb91df14d..336903d1ec 100644 --- a/firestore/src/main/aggregate_query_snapshot_main.h +++ b/firestore/src/main/aggregate_query_snapshot_main.h @@ -42,6 +42,10 @@ class AggregateQuerySnapshotInternal { AggregateQuery query() const; int64_t count() const; + std::size_t Hash() const { + return util::Hash(aggregate_query_.query().Hash(), count_result_); + } + friend bool operator==(const AggregateQuerySnapshotInternal& lhs, const AggregateQuerySnapshotInternal& rhs); From a4d88ebdfc9e362fc0dfdaa4e1f0582014a144e8 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 11:52:54 -0500 Subject: [PATCH 20/26] Add Hash --- firestore/src/common/aggregate_query.cc | 5 +++++ firestore/src/main/aggregate_query_main.h | 2 ++ 2 files changed, 7 insertions(+) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index d6969b9884..3437bfeec7 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -100,6 +100,11 @@ Future AggregateQuery::Get( return internal_->Get(aggregate_source); } +size_t AggregateQuery::Hash() const { + if (!internal_) return {}; + return internal_->Hash(); +} + bool operator==(const AggregateQuery& lhs, const AggregateQuery& rhs) { return EqualityCompare(lhs.internal_, rhs.internal_); } diff --git a/firestore/src/main/aggregate_query_main.h b/firestore/src/main/aggregate_query_main.h index 51584d4b06..793b221170 100644 --- a/firestore/src/main/aggregate_query_main.h +++ b/firestore/src/main/aggregate_query_main.h @@ -39,6 +39,8 @@ class AggregateQueryInternal { Future Get(AggregateSource source); + size_t Hash() const { return aggregate_query_.query().Hash(); } + friend bool operator==(const AggregateQueryInternal& lhs, const AggregateQueryInternal& rhs); From d2d6d36a5434dc6adcae8f5190f484cd309198e6 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 11:54:45 -0500 Subject: [PATCH 21/26] Fix copyright year --- firestore/src/common/aggregate_query.cc | 2 +- firestore/src/include/firebase/firestore/aggregate_query.h | 2 +- .../src/include/firebase/firestore/aggregate_query_snapshot.h | 2 +- firestore/src/include/firebase/firestore/aggregate_source.h | 2 +- firestore/src/main/aggregate_query_main.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index 3437bfeec7..d1046256c2 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -1,5 +1,5 @@ /* - * Copyright 2022 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. diff --git a/firestore/src/include/firebase/firestore/aggregate_query.h b/firestore/src/include/firebase/firestore/aggregate_query.h index a976ba811b..035d923af7 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query.h +++ b/firestore/src/include/firebase/firestore/aggregate_query.h @@ -1,5 +1,5 @@ /* - * Copyright 2022 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. diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h index f2735de630..6f69dc7589 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -1,5 +1,5 @@ /* - * Copyright 2022 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. diff --git a/firestore/src/include/firebase/firestore/aggregate_source.h b/firestore/src/include/firebase/firestore/aggregate_source.h index f0017004de..f6d88d9279 100644 --- a/firestore/src/include/firebase/firestore/aggregate_source.h +++ b/firestore/src/include/firebase/firestore/aggregate_source.h @@ -1,5 +1,5 @@ /* - * Copyright 2022 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. diff --git a/firestore/src/main/aggregate_query_main.h b/firestore/src/main/aggregate_query_main.h index 793b221170..defe03a06f 100644 --- a/firestore/src/main/aggregate_query_main.h +++ b/firestore/src/main/aggregate_query_main.h @@ -1,5 +1,5 @@ /* - * Copyright 2022 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. From cedc0e2dc0a381377aa8e46ba02bd94032856b4e Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 11:57:40 -0500 Subject: [PATCH 22/26] Rename --- firestore/src/common/aggregate_query.cc | 22 +++++++++++----------- firestore/src/common/query.cc | 22 +++++++++++----------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index d1046256c2..1275ee7e7b 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -29,7 +29,7 @@ namespace firebase { namespace firestore { -using CleanupFnQuery = CleanupFn; +using CleanupFnAggregateQuery = CleanupFn; AggregateQuery::AggregateQuery() {} @@ -37,23 +37,23 @@ AggregateQuery::AggregateQuery(const AggregateQuery& aggregate_query) { if (aggregate_query.internal_) { internal_ = new AggregateQueryInternal(*aggregate_query.internal_); } - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); } AggregateQuery::AggregateQuery(AggregateQuery&& aggregate_query) { - CleanupFnQuery::Unregister(&aggregate_query, aggregate_query.internal_); + CleanupFnAggregateQuery::Unregister(&aggregate_query, aggregate_query.internal_); std::swap(internal_, aggregate_query.internal_); - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); } AggregateQuery::AggregateQuery(AggregateQueryInternal* internal) : internal_(internal) { SIMPLE_HARD_ASSERT(internal != nullptr); - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); } AggregateQuery::~AggregateQuery() { - CleanupFnQuery::Unregister(this, internal_); + CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; internal_ = nullptr; } @@ -64,14 +64,14 @@ AggregateQuery& AggregateQuery::operator=( return *this; } - CleanupFnQuery::Unregister(this, internal_); + CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; if (aggregate_query.internal_) { internal_ = new AggregateQueryInternal(*aggregate_query.internal_); } else { internal_ = nullptr; } - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); return *this; } @@ -80,12 +80,12 @@ AggregateQuery& AggregateQuery::operator=(AggregateQuery&& aggregate_query) { return *this; } - CleanupFnQuery::Unregister(&aggregate_query, aggregate_query.internal_); - CleanupFnQuery::Unregister(this, internal_); + CleanupFnAggregateQuery::Unregister(&aggregate_query, aggregate_query.internal_); + CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; internal_ = aggregate_query.internal_; aggregate_query.internal_ = nullptr; - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); return *this; } diff --git a/firestore/src/common/query.cc b/firestore/src/common/query.cc index 7edbdd7602..cbc948d212 100644 --- a/firestore/src/common/query.cc +++ b/firestore/src/common/query.cc @@ -38,7 +38,7 @@ namespace firebase { namespace firestore { -using CleanupFnQuery = CleanupFn; +using CleanupFnAggregateQuery = CleanupFn; Query::Query() {} @@ -46,23 +46,23 @@ Query::Query(const Query& query) { if (query.internal_) { internal_ = new QueryInternal(*query.internal_); } - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); } Query::Query(Query&& query) { - CleanupFnQuery::Unregister(&query, query.internal_); + CleanupFnAggregateQuery::Unregister(&query, query.internal_); std::swap(internal_, query.internal_); - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); } Query::Query(QueryInternal* internal) : internal_(internal) { // NOTE: We don't assert internal != nullptr here since internal can be // nullptr when called by the CollectionReference copy constructor. - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); } Query::~Query() { - CleanupFnQuery::Unregister(this, internal_); + CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; internal_ = nullptr; } @@ -72,14 +72,14 @@ Query& Query::operator=(const Query& query) { return *this; } - CleanupFnQuery::Unregister(this, internal_); + CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; if (query.internal_) { internal_ = new QueryInternal(*query.internal_); } else { internal_ = nullptr; } - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); return *this; } @@ -88,12 +88,12 @@ Query& Query::operator=(Query&& query) { return *this; } - CleanupFnQuery::Unregister(&query, query.internal_); - CleanupFnQuery::Unregister(this, internal_); + CleanupFnAggregateQuery::Unregister(&query, query.internal_); + CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; internal_ = query.internal_; query.internal_ = nullptr; - CleanupFnQuery::Register(this, internal_); + CleanupFnAggregateQuery::Register(this, internal_); return *this; } From 04eca2ec1d825693c492bf79679a93154645d0e5 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 11:58:07 -0500 Subject: [PATCH 23/26] Format --- firestore/src/common/aggregate_query.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index 1275ee7e7b..8c31a8821d 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -41,7 +41,8 @@ AggregateQuery::AggregateQuery(const AggregateQuery& aggregate_query) { } AggregateQuery::AggregateQuery(AggregateQuery&& aggregate_query) { - CleanupFnAggregateQuery::Unregister(&aggregate_query, aggregate_query.internal_); + CleanupFnAggregateQuery::Unregister(&aggregate_query, + aggregate_query.internal_); std::swap(internal_, aggregate_query.internal_); CleanupFnAggregateQuery::Register(this, internal_); } @@ -80,7 +81,8 @@ AggregateQuery& AggregateQuery::operator=(AggregateQuery&& aggregate_query) { return *this; } - CleanupFnAggregateQuery::Unregister(&aggregate_query, aggregate_query.internal_); + CleanupFnAggregateQuery::Unregister(&aggregate_query, + aggregate_query.internal_); CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; internal_ = aggregate_query.internal_; From dc6e00c166b95de7f737f775cd0a0cb33cec0c5b Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 12:01:04 -0500 Subject: [PATCH 24/26] Rename --- firestore/src/common/query.cc | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/firestore/src/common/query.cc b/firestore/src/common/query.cc index cbc948d212..7edbdd7602 100644 --- a/firestore/src/common/query.cc +++ b/firestore/src/common/query.cc @@ -38,7 +38,7 @@ namespace firebase { namespace firestore { -using CleanupFnAggregateQuery = CleanupFn; +using CleanupFnQuery = CleanupFn; Query::Query() {} @@ -46,23 +46,23 @@ Query::Query(const Query& query) { if (query.internal_) { internal_ = new QueryInternal(*query.internal_); } - CleanupFnAggregateQuery::Register(this, internal_); + CleanupFnQuery::Register(this, internal_); } Query::Query(Query&& query) { - CleanupFnAggregateQuery::Unregister(&query, query.internal_); + CleanupFnQuery::Unregister(&query, query.internal_); std::swap(internal_, query.internal_); - CleanupFnAggregateQuery::Register(this, internal_); + CleanupFnQuery::Register(this, internal_); } Query::Query(QueryInternal* internal) : internal_(internal) { // NOTE: We don't assert internal != nullptr here since internal can be // nullptr when called by the CollectionReference copy constructor. - CleanupFnAggregateQuery::Register(this, internal_); + CleanupFnQuery::Register(this, internal_); } Query::~Query() { - CleanupFnAggregateQuery::Unregister(this, internal_); + CleanupFnQuery::Unregister(this, internal_); delete internal_; internal_ = nullptr; } @@ -72,14 +72,14 @@ Query& Query::operator=(const Query& query) { return *this; } - CleanupFnAggregateQuery::Unregister(this, internal_); + CleanupFnQuery::Unregister(this, internal_); delete internal_; if (query.internal_) { internal_ = new QueryInternal(*query.internal_); } else { internal_ = nullptr; } - CleanupFnAggregateQuery::Register(this, internal_); + CleanupFnQuery::Register(this, internal_); return *this; } @@ -88,12 +88,12 @@ Query& Query::operator=(Query&& query) { return *this; } - CleanupFnAggregateQuery::Unregister(&query, query.internal_); - CleanupFnAggregateQuery::Unregister(this, internal_); + CleanupFnQuery::Unregister(&query, query.internal_); + CleanupFnQuery::Unregister(this, internal_); delete internal_; internal_ = query.internal_; query.internal_ = nullptr; - CleanupFnAggregateQuery::Register(this, internal_); + CleanupFnQuery::Register(this, internal_); return *this; } From 281d2b52067f6018ca2a08cf35b8d1f05460f7db Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 12:52:02 -0500 Subject: [PATCH 25/26] Fixup constructor/assignment parameter naming. --- firestore/src/common/aggregate_query.cc | 32 +++++++++---------- .../src/common/aggregate_query_snapshot.cc | 30 ++++++++--------- .../firestore/aggregate_query_snapshot.h | 8 ++--- 3 files changed, 34 insertions(+), 36 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index 8c31a8821d..ea9ebedc29 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -33,17 +33,16 @@ using CleanupFnAggregateQuery = CleanupFn; AggregateQuery::AggregateQuery() {} -AggregateQuery::AggregateQuery(const AggregateQuery& aggregate_query) { - if (aggregate_query.internal_) { - internal_ = new AggregateQueryInternal(*aggregate_query.internal_); +AggregateQuery::AggregateQuery(const AggregateQuery& other) { + if (other.internal_) { + internal_ = new AggregateQueryInternal(*other.internal_); } CleanupFnAggregateQuery::Register(this, internal_); } -AggregateQuery::AggregateQuery(AggregateQuery&& aggregate_query) { - CleanupFnAggregateQuery::Unregister(&aggregate_query, - aggregate_query.internal_); - std::swap(internal_, aggregate_query.internal_); +AggregateQuery::AggregateQuery(AggregateQuery&& other) { + CleanupFnAggregateQuery::Unregister(&other, other.internal_); + std::swap(internal_, other.internal_); CleanupFnAggregateQuery::Register(this, internal_); } @@ -60,15 +59,15 @@ AggregateQuery::~AggregateQuery() { } AggregateQuery& AggregateQuery::operator=( - const AggregateQuery& aggregate_query) { - if (this == &aggregate_query) { + const AggregateQuery& other) { + if (this == &other) { return *this; } CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; - if (aggregate_query.internal_) { - internal_ = new AggregateQueryInternal(*aggregate_query.internal_); + if (other.internal_) { + internal_ = new AggregateQueryInternal(*other.internal_); } else { internal_ = nullptr; } @@ -76,17 +75,16 @@ AggregateQuery& AggregateQuery::operator=( return *this; } -AggregateQuery& AggregateQuery::operator=(AggregateQuery&& aggregate_query) { - if (this == &aggregate_query) { +AggregateQuery& AggregateQuery::operator=(AggregateQuery&& other) { + if (this == &other) { return *this; } - CleanupFnAggregateQuery::Unregister(&aggregate_query, - aggregate_query.internal_); + CleanupFnAggregateQuery::Unregister(&other, other.internal_); CleanupFnAggregateQuery::Unregister(this, internal_); delete internal_; - internal_ = aggregate_query.internal_; - aggregate_query.internal_ = nullptr; + internal_ = other.internal_; + other.internal_ = nullptr; CleanupFnAggregateQuery::Register(this, internal_); return *this; } diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index 75791264fd..38903be3fe 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -35,17 +35,17 @@ using CleanupFnAggregateQuerySnapshot = CleanupFn; AggregateQuerySnapshot::AggregateQuerySnapshot() {} AggregateQuerySnapshot::AggregateQuerySnapshot( - const AggregateQuerySnapshot& snapshot) { - if (snapshot.internal_) { - internal_ = new AggregateQuerySnapshotInternal(*snapshot.internal_); + const AggregateQuerySnapshot& other) { + if (other.internal_) { + internal_ = new AggregateQuerySnapshotInternal(*other.internal_); } CleanupFnAggregateQuerySnapshot::Register(this, internal_); } AggregateQuerySnapshot::AggregateQuerySnapshot( - AggregateQuerySnapshot&& snapshot) { - CleanupFnAggregateQuerySnapshot::Unregister(&snapshot, snapshot.internal_); - std::swap(internal_, snapshot.internal_); + AggregateQuerySnapshot&& other) { + CleanupFnAggregateQuerySnapshot::Unregister(&other, other.internal_); + std::swap(internal_, other.internal_); CleanupFnAggregateQuerySnapshot::Register(this, internal_); } @@ -63,15 +63,15 @@ AggregateQuerySnapshot::~AggregateQuerySnapshot() { } AggregateQuerySnapshot& AggregateQuerySnapshot::operator=( - const AggregateQuerySnapshot& snapshot) { - if (this == &snapshot) { + const AggregateQuerySnapshot& other) { + if (this == &other) { return *this; } CleanupFnAggregateQuerySnapshot::Unregister(this, internal_); delete internal_; - if (snapshot.internal_) { - internal_ = new AggregateQuerySnapshotInternal(*snapshot.internal_); + if (other.internal_) { + internal_ = new AggregateQuerySnapshotInternal(*other.internal_); } else { internal_ = nullptr; } @@ -80,16 +80,16 @@ AggregateQuerySnapshot& AggregateQuerySnapshot::operator=( } AggregateQuerySnapshot& AggregateQuerySnapshot::operator=( - AggregateQuerySnapshot&& snapshot) { - if (this == &snapshot) { + AggregateQuerySnapshot&& other) { + if (this == &other) { return *this; } - CleanupFnAggregateQuerySnapshot::Unregister(&snapshot, snapshot.internal_); + CleanupFnAggregateQuerySnapshot::Unregister(&other, other.internal_); CleanupFnAggregateQuerySnapshot::Unregister(this, internal_); delete internal_; - internal_ = snapshot.internal_; - snapshot.internal_ = nullptr; + internal_ = other.internal_; + other.internal_ = nullptr; CleanupFnAggregateQuerySnapshot::Register(this, internal_); return *this; } diff --git a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h index 6f69dc7589..103d9c431d 100644 --- a/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h +++ b/firestore/src/include/firebase/firestore/aggregate_query_snapshot.h @@ -51,9 +51,9 @@ class AggregateQuerySnapshot { * `AggregateQuerySnapshot` is immutable and can be efficiently copied (no * deep copy is performed). * - * @param[in] snapshot `AggregateQuerySnapshot` to copy from. + * @param[in] other `AggregateQuerySnapshot` to copy from. */ - AggregateQuerySnapshot(const AggregateQuerySnapshot& snapshot); + AggregateQuerySnapshot(const AggregateQuerySnapshot& other); /** * @brief Move constructor. @@ -62,9 +62,9 @@ class AggregateQuerySnapshot { * being moved from, a `AggregateQuerySnapshot` is equivalent to its * default-constructed state. * - * @param[in] snapshot `AggregateQuerySnapshot` to move data from. + * @param[in] other `AggregateQuerySnapshot` to move data from. */ - AggregateQuerySnapshot(AggregateQuerySnapshot&& snapshot); + AggregateQuerySnapshot(AggregateQuerySnapshot&& other); virtual ~AggregateQuerySnapshot(); From 398a262984ecf56bb5d860106f165ee14613ee2c Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 10 Feb 2023 12:52:33 -0500 Subject: [PATCH 26/26] Format --- firestore/src/common/aggregate_query.cc | 3 +-- firestore/src/common/aggregate_query_snapshot.cc | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/firestore/src/common/aggregate_query.cc b/firestore/src/common/aggregate_query.cc index ea9ebedc29..cb20323775 100644 --- a/firestore/src/common/aggregate_query.cc +++ b/firestore/src/common/aggregate_query.cc @@ -58,8 +58,7 @@ AggregateQuery::~AggregateQuery() { internal_ = nullptr; } -AggregateQuery& AggregateQuery::operator=( - const AggregateQuery& other) { +AggregateQuery& AggregateQuery::operator=(const AggregateQuery& other) { if (this == &other) { return *this; } diff --git a/firestore/src/common/aggregate_query_snapshot.cc b/firestore/src/common/aggregate_query_snapshot.cc index 38903be3fe..d86dd44863 100644 --- a/firestore/src/common/aggregate_query_snapshot.cc +++ b/firestore/src/common/aggregate_query_snapshot.cc @@ -42,8 +42,7 @@ AggregateQuerySnapshot::AggregateQuerySnapshot( CleanupFnAggregateQuerySnapshot::Register(this, internal_); } -AggregateQuerySnapshot::AggregateQuerySnapshot( - AggregateQuerySnapshot&& other) { +AggregateQuerySnapshot::AggregateQuerySnapshot(AggregateQuerySnapshot&& other) { CleanupFnAggregateQuerySnapshot::Unregister(&other, other.internal_); std::swap(internal_, other.internal_); CleanupFnAggregateQuerySnapshot::Register(this, internal_);