Public Auto Persistent Index Creation API (#11757)

cherylEnkidu · ncooke3 · web-flow · commit 065a5bbc7408 · 2023-09-05T17:49:43.000-04:00
Co-authored-by: Nick Cooke &lt;36927374+ncooke3@users.noreply.github.com&gt;
diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md
@@ -1,3 +1,7 @@
+# Unreleased
+- [feature] Add the option to allow the SDK to create cache indexes automatically to
+  improve query execution locally. (#11596)
+
 # 10.12.0
 - [feature] Implemented an optimization in the local cache synchronization logic
   that reduces the number of billed document reads when documents were deleted
diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj
diff --git a/Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm b/Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm
@@ -1844,7 +1844,7 @@ - (void)testGetValidPersistentCacheIndexManager {
   XCTAssertNotNil(db2.persistentCacheIndexManager);
 
   // Disable persistent disk cache
-  FIRFirestore *db3 = [FIRFirestore firestoreForDatabase:@"PersistentCacheIndexManagerDB3"];
+  FIRFirestore *db3 = [FIRFirestore firestoreForDatabase:@"MemoryCacheIndexManagerDB1"];
   FIRFirestoreSettings *settings3 = [db3 settings];
   [settings3 setCacheSettings:[[FIRMemoryCacheSettings alloc] init]];
   [db3 setSettings:settings3];
diff --git a/Firestore/Example/Tests/Integration/API/FIRIndexingTests.mm b/Firestore/Example/Tests/Integration/API/FIRIndexingTests.mm
@@ -14,15 +14,17 @@
  * limitations under the License.
  */
 
+// TODO(csi): Delete this once setIndexConfigurationFromJSON and setIndexConfigurationFromStream
+//  are removed.
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+
 #import <FirebaseFirestore/FirebaseFirestore.h>
 
 #import <XCTest/XCTest.h>
 
-#import "Firestore/Source/API/FIRFirestore+Internal.h"
-#import "Firestore/Source/API/FIRPersistentCacheIndexManager+Internal.h"
-
 #import "Firestore/Example/Tests/Util/FSTHelpers.h"
 #import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"
+#import "Firestore/Source/Public/FirebaseFirestore/FIRPersistentCacheIndexManager.h"
 
 @interface FIRIndexingTests : FSTIntegrationTestCase
 @end
@@ -194,13 +196,13 @@ - (void)testAutoIndexCreationSetSuccessfullyUsingDefault {
 - (void)testAutoIndexCreationAfterFailsTermination {
   [self terminateFirestore:self.db];
 
-  FSTAssertThrows([self.db.persistentCacheIndexManager enableIndexAutoCreation],
+  XCTAssertThrows([self.db.persistentCacheIndexManager enableIndexAutoCreation],
                   @"The client has already been terminated.");
 
-  FSTAssertThrows([self.db.persistentCacheIndexManager disableIndexAutoCreation],
+  XCTAssertThrows([self.db.persistentCacheIndexManager disableIndexAutoCreation],
                   @"The client has already been terminated.");
 
-  FSTAssertThrows([self.db.persistentCacheIndexManager deleteAllIndexes],
+  XCTAssertThrows([self.db.persistentCacheIndexManager deleteAllIndexes],
                   @"The client has already been terminated.");
 }
 
diff --git a/Firestore/Example/Tests/Util/FSTExceptionCatcher.h b/Firestore/Example/Tests/Util/FSTExceptionCatcher.h
@@ -0,0 +1,27 @@
+// 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.
+
+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface FSTExceptionCatcher : NSObject
+
+typedef void (^ThrowingBlock)(void);
+
++ (BOOL)catchException:(ThrowingBlock)block error:(__autoreleasing NSError **)error;
+
+@end
+
+NS_ASSUME_NONNULL_END
diff --git a/Firestore/Example/Tests/Util/FSTExceptionCatcher.m b/Firestore/Example/Tests/Util/FSTExceptionCatcher.m
@@ -0,0 +1,40 @@
+// 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.
+
+#import "Firestore/Example/Tests/Util/FSTExceptionCatcher.h"
+
+@implementation FSTExceptionCatcher
+
++ (BOOL)catchException:(ThrowingBlock)block error:(__autoreleasing NSError **)error {
+  @try {
+    block();
+    return YES;
+  } @catch (NSException *exception) {
+    NSMutableDictionary *info = [NSMutableDictionary dictionary];
+    [info setValue:exception.name forKey:@"ExceptionName"];
+    [info setValue:exception.reason forKey:@"ExceptionReason"];
+    [info setValue:exception.callStackReturnAddresses forKey:@"ExceptionCallStackReturnAddresses"];
+    [info setValue:exception.callStackSymbols forKey:@"ExceptionCallStackSymbols"];
+    [info setValue:exception.userInfo forKey:@"ExceptionUserInfo"];
+
+    // Just using error code `FIRErrorCodeConfigFailed` for now
+    NSInteger FIRErrorCodeConfigFailed = -114;
+    *error = [[NSError alloc] initWithDomain:exception.name
+                                        code:FIRErrorCodeConfigFailed
+                                    userInfo:info];
+    return NO;
+  }
+}
+
+@end
diff --git a/Firestore/Source/API/FIRFirestore+Internal.h b/Firestore/Source/API/FIRFirestore+Internal.h
@@ -79,9 +79,6 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (const std::shared_ptr<firebase::firestore::util::AsyncQueue> &)workerQueue;
 
-// TODO(csi): make this function public
-@property(nonatomic, readonly) FIRPersistentCacheIndexManager *persistentCacheIndexManager;
-
 @property(nonatomic, assign, readonly) std::shared_ptr<api::Firestore> wrapped;
 
 @property(nonatomic, assign, readonly) const model::DatabaseId &databaseID;
diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm
@@ -14,6 +14,10 @@
  * limitations under the License.
  */
 
+// TODO(csi): Delete this once setIndexConfigurationFromJSON and setIndexConfigurationFromStream
+//  are removed.
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+
 #import "FIRFirestore+Internal.h"
 
 #include <memory>
@@ -535,12 +539,14 @@ @implementation FIRFirestore (Internal)
   return _firestore->worker_queue();
 }
 
-- (FIRPersistentCacheIndexManager *)persistentCacheIndexManager {
+- (nullable FIRPersistentCacheIndexManager *)persistentCacheIndexManager {
   if (!_indexManager) {
     auto index_manager = _firestore->persistent_cache_index_manager();
     if (index_manager) {
       _indexManager = [[FIRPersistentCacheIndexManager alloc]
           initWithPersistentCacheIndexManager:index_manager];
+    } else {
+      return nil;
     }
   }
   return _indexManager;
diff --git a/Firestore/Source/API/FIRPersistentCacheIndexManager+Internal.h b/Firestore/Source/API/FIRPersistentCacheIndexManager+Internal.h
@@ -13,51 +13,15 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+#import "FIRPersistentCacheIndexManager.h"
 
 #import <Foundation/Foundation.h>
 
 #include <memory>
-
 #include "Firestore/core/src/api/persistent_cache_index_manager.h"
 
 NS_ASSUME_NONNULL_BEGIN
 
-// TODO(sum/avg) move the contents of this category to
-// ../Public/FirebaseFirestore/FIRPersistentCacheIndexManager.h
-/**
- * A PersistentCacheIndexManager which you can config persistent cache indexes used for
- * local query execution.
- */
-NS_SWIFT_NAME(PersistentCacheIndexManager)
-@interface FIRPersistentCacheIndexManager : NSObject
-
-/** :nodoc: */
-- (instancetype)init
-    __attribute__((unavailable("FIRPersistentCacheIndexManager cannot be created directly.")));
-
-/**
- * Enables SDK to create persistent cache indexes automatically for local query execution when SDK
- * believes cache indexes can help improves performance.
- *
- * This feature is disabled by default.
- */
-- (void)enableIndexAutoCreation NS_SWIFT_NAME(enableIndexAutoCreation());
-
-/**
- * Stops creating persistent cache indexes automatically for local query execution. The indexes
- * which have been created by calling `enableIndexAutoCreation` still take effect.
- */
-- (void)disableIndexAutoCreation NS_SWIFT_NAME(disableIndexAutoCreation());
-
-/**
- * Removes all persistent cache indexes. Please note this function will also deletes indexes
- * generated by [[FIRFirestore firestore] setIndexConfigurationFromJSON] and [[FIRFirestore
- * firestore] setIndexConfigurationFromStream], which are deprecated.
- */
-- (void)deleteAllIndexes NS_SWIFT_NAME(deleteAllIndexes());
-
-@end
-
 @interface FIRPersistentCacheIndexManager (/* Init */)
 
 - (instancetype)initWithPersistentCacheIndexManager:
diff --git a/Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h b/Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h
@@ -28,6 +28,7 @@
 @class FIRTransaction;
 @class FIRTransactionOptions;
 @class FIRWriteBatch;
+@class FIRPersistentCacheIndexManager;
 
 NS_ASSUME_NONNULL_BEGIN
 
@@ -103,7 +104,16 @@ NS_SWIFT_NAME(Firestore)
 #pragma mark - Configure FieldIndexes
 
 /**
- * This method is in preview. API signature and functionality are subject to change.
+ * A PersistentCacheIndexManager which you can config persistent cache indexes used for
+ * local query execution.
+ */
+@property(nonatomic, readonly, nullable)
+    FIRPersistentCacheIndexManager *persistentCacheIndexManager;
+
+/**
+ * NOTE: This preview method will be deprecated in a future major release. Consider using
+ * `PersistentCacheIndexManager.enableIndexAutoCreation()` to let the SDK decide whether to create
+ * cache indexes for queries running locally.
  *
  * Configures indexing for local query execution. Any previous index configuration is overridden.
  *
@@ -121,10 +131,15 @@ NS_SWIFT_NAME(Firestore)
  */
 - (void)setIndexConfigurationFromJSON:(NSString *)json
                            completion:(nullable void (^)(NSError *_Nullable error))completion
-    NS_SWIFT_NAME(setIndexConfiguration(_:completion:));
+    NS_SWIFT_NAME(setIndexConfiguration(_:completion:)) DEPRECATED_MSG_ATTRIBUTE(
+        "Instead of creating cache indexes manually, consider using "
+        "`PersistentCacheIndexManager.enableIndexAutoCreation()` to let the SDK decide whether to "
+        "create cache indexes for queries running locally.");
 
 /**
- * This method is in preview. API signature and functionality are subject to change.
+ * NOTE: This preview method will be deprecated in a future major release. Consider using
+ * `PersistentCacheIndexManager.enableIndexAutoCreation()` to let the SDK decide whether to create
+ * cache indexes for queries running locally.
  *
  * Configures indexing for local query execution. Any previous index configuration is overridden.
  *
@@ -145,7 +160,10 @@ NS_SWIFT_NAME(Firestore)
  */
 - (void)setIndexConfigurationFromStream:(NSInputStream *)stream
                              completion:(nullable void (^)(NSError *_Nullable error))completion
-    NS_SWIFT_NAME(setIndexConfiguration(_:completion:));
+    NS_SWIFT_NAME(setIndexConfiguration(_:completion:)) DEPRECATED_MSG_ATTRIBUTE(
+        "Instead of creating cache indexes manually, consider using "
+        "`PersistentCacheIndexManager.enableIndexAutoCreation()` to let the SDK decide whether to "
+        "create cache indexes for queries running locally.");
 
 #pragma mark - Collections and Documents
 
diff --git a/Firestore/Source/Public/FirebaseFirestore/FIRPersistentCacheIndexManager.h b/Firestore/Source/Public/FirebaseFirestore/FIRPersistentCacheIndexManager.h
@@ -0,0 +1,55 @@
+/*
+ * 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.
+ */
+
+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+/**
+ * A PersistentCacheIndexManager, for configuring persistent cache indexes used for local query
+ * execution.
+ */
+NS_SWIFT_NAME(PersistentCacheIndexManager)
+@interface FIRPersistentCacheIndexManager : NSObject
+
+/** :nodoc: */
+- (instancetype)init
+    __attribute__((unavailable("FIRPersistentCacheIndexManager cannot be created directly.")));
+
+/**
+ * Enables the SDK to create persistent cache indexes automatically for local query execution when
+ * the SDK believes cache indexes can improve performance.
+ *
+ * This feature is disabled by default.
+ */
+- (void)enableIndexAutoCreation NS_SWIFT_NAME(enableIndexAutoCreation());
+
+/**
+ * Stops creating persistent cache indexes automatically for local query execution. The indexes
+ * which have been created by calling `enableIndexAutoCreation` still take effect.
+ */
+- (void)disableIndexAutoCreation NS_SWIFT_NAME(disableIndexAutoCreation());
+
+/**
+ * Removes all persistent cache indexes. Please note this function also deletes indexes generated by
+ * [[FIRFirestore firestore] setIndexConfigurationFromJSON] and [[FIRFirestore firestore]
+ * setIndexConfigurationFromStream], which are deprecated.
+ */
+- (void)deleteAllIndexes NS_SWIFT_NAME(deleteAllIndexes());
+
+@end
+
+NS_ASSUME_NONNULL_END
diff --git a/Firestore/Swift/Tests/BridgingHeader.h b/Firestore/Swift/Tests/BridgingHeader.h
@@ -18,6 +18,7 @@
 #define FIRESTORE_SWIFT_TESTS_BRIDGINGHEADER_H_
 
 #import "Firestore/Example/Tests/API/FSTAPIHelpers.h"
+#import "Firestore/Example/Tests/Util/FSTExceptionCatcher.h"
 #import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"
 
 #endif /* FIRESTORE_SWIFT_TESTS_BRIDGINGHEADER_H_ */
diff --git a/Firestore/Swift/Tests/Integration/AsyncAwaitIntegrationTests.swift b/Firestore/Swift/Tests/Integration/AsyncAwaitIntegrationTests.swift
@@ -93,5 +93,33 @@ let emptyBundle = """
       XCTAssertEqual(FIRQuerySnapshotGetIDs(snapshot),
                      ["doc1", "doc2", "doc4", "doc5"])
     }
+
+    func testAutoIndexCreationAfterFailsTermination() async throws {
+      try await db.terminate()
+
+      let enableIndexAutoCreation = {
+        try FSTExceptionCatcher.catchException {
+          self.db.persistentCacheIndexManager?.enableIndexAutoCreation()
+        }
+      }
+      XCTAssertThrowsError(try enableIndexAutoCreation(), "The client has already been terminated.")
+
+      let disableIndexAutoCreation = {
+        try FSTExceptionCatcher.catchException {
+          self.db.persistentCacheIndexManager?.disableIndexAutoCreation()
+        }
+      }
+      XCTAssertThrowsError(
+        try disableIndexAutoCreation(),
+        "The client has already been terminated."
+      )
+
+      let deleteAllIndexes = {
+        try FSTExceptionCatcher.catchException {
+          self.db.persistentCacheIndexManager?.deleteAllIndexes()
+        }
+      }
+      XCTAssertThrowsError(try deleteAllIndexes(), "The client has already been terminated.")
+    }
   }
 #endif
diff --git a/Firestore/Swift/Tests/Integration/DatabaseTests.swift b/Firestore/Swift/Tests/Integration/DatabaseTests.swift
diff --git a/Firestore/Swift/Tests/Integration/IndexingTests.swift b/Firestore/Swift/Tests/Integration/IndexingTests.swift
