fix: index-spec matching for optional values and vectorConfig.

CHANGELOG.md

@@ -1,3 +1,4 @@
+- Fixed a bug when deploying firestore indexes failed due to broken index comparison logic (#8859)
 - Added prefix support for multi-instance Cloud Functions extension parameters. (#8911)
 - Fixed a bug when `firebase deploy --only dataconnect` doesn't include GQL in nested folders (#8981)
 - Make it possible to init a dataconnect project in non interactive mode (#8993)

src/firestore/api.ts

@@ -5,6 +5,7 @@
 import * as validator from "./validator";
 
 import * as types from "./api-types";
+import { DatabaseEdition, Density } from "./api-types";
 import * as Spec from "./api-spec";
 import * as sort from "./api-sort";
 import * as util from "./util";
@@ -13,6 +14,7 @@
 import { FirebaseError } from "../error";
 import { Client } from "../apiv2";
 import { PrettyPrint } from "./pretty-print";
+import { optionalValueMatches } from "../functional";
 
 export class FirestoreApi {
   apiClient = new Client({ urlPrefix: firestoreOrigin(), apiVersion: "v1" });
@@ -22,7 +24,7 @@
    * Process indexes by filtering out implicit __name__ fields with ASCENDING order.
    * Keeps explicit __name__ fields with DESCENDING order.
    * @param indexes Array of indexes to process
    * @returns Processed array of indexes with filtered fields
    */
   public static processIndexes(indexes: types.Index[]): types.Index[] {
     return indexes.map((index: types.Index): types.Index => {
@@ -45,7 +47,7 @@
 
   /**
    * Deploy an index specification to the specified project.
    * @param options the CLI options.
    * @param indexes an array of objects, each will be validated and then converted
    * to an {@link Spec.Index}.
    * @param fieldOverrides an array of objects, each will be validated and then
@@ -53,11 +55,11 @@
    */
   async deploy(
     options: { project: string; nonInteractive: boolean; force: boolean },
     indexes: any[],
     fieldOverrides: any[],
     databaseId = "(default)",
   ): Promise<void> {
     const spec = this.upgradeOldSpec({
       indexes,
       fieldOverrides,
     });
@@ -65,8 +67,8 @@
     this.validateSpec(spec);
 
     // Now that the spec is validated we can safely assert these types.
     const indexesToDeploy: Spec.Index[] = spec.indexes;
     const fieldOverridesToDeploy: Spec.FieldOverride[] = spec.fieldOverrides;
 
     const existingIndexes: types.Index[] = await this.listIndexes(options.project, databaseId);
     const existingFieldOverrides: types.Field[] = await this.listFieldOverrides(
@@ -74,8 +76,10 @@
       databaseId,
     );
 
+    const database = await this.getDatabase(options.project, databaseId);
+    const edition = database.databaseEdition ?? DatabaseEdition.STANDARD;
     const indexesToDelete = existingIndexes.filter((index) => {
-      return !indexesToDeploy.some((spec) => this.indexMatchesSpec(index, spec));
+      return !indexesToDeploy.some((spec) => this.indexMatchesSpec(index, spec, edition));
     });
 
     // We only want to delete fields where there is nothing in the local file with the same
@@ -127,7 +131,7 @@
     }
 
     for (const index of indexesToDeploy) {
-      const exists = existingIndexes.some((x) => this.indexMatchesSpec(x, index));
+      const exists = existingIndexes.some((x) => this.indexMatchesSpec(x, index, edition));
       if (exists) {
         logger.debug(`Skipping existing index: ${JSON.stringify(index)}`);
       } else {
@@ -325,8 +329,11 @@
       validator.assertType("multikey", index.multikey, "boolean");
     }
 
-    if (index.unique) {
+    if (index.unique !== undefined) {
       validator.assertType("unique", index.unique, "boolean");
+      // TODO(b/439901837): Remove this check and update indexMatchesSpec once
+      //  unique index configuration is supported.
+      throw new FirebaseError("The `unique` index configuration is not supported yet.");
     }
 
     validator.assertHas(index, "fields");
@@ -479,10 +486,51 @@
     return this.apiClient.delete(`/${url}`);
   }
 
+  /**
+   * Returns true if the given ApiScope values match.
+   * If either one is undefined, the default value is used for comparison.
+   * @param lhs the first ApiScope value.
+   * @param rhs the second ApiScope value.
+   */
+  optionalApiScopeMatches(
+    lhs: types.ApiScope | undefined,
+    rhs: types.ApiScope | undefined,
+  ): boolean {
+    return optionalValueMatches<types.ApiScope>(lhs, rhs, types.ApiScope.ANY_API);
+  }
+
+  /**
+   * Returns true if the given Density values match.
+   * If either one is undefined, the default value is used for comparison based on Database Edition.
+   * @param lhs the first Density value.
+   * @param rhs the second Density value.
+   * @param edition the database edition used to determine the default value.
+   */
+  optionalDensityMatches(
+    lhs: Density | undefined,
+    rhs: Density | undefined,
+    edition: types.DatabaseEdition,
+  ): boolean {
+    const defaultValue =
+      edition === DatabaseEdition.STANDARD ? types.Density.SPARSE_ALL : types.Density.DENSE;
+    return optionalValueMatches<types.Density>(lhs, rhs, defaultValue);
+  }
+
+  /**
+   * Returns true if the given Multikey values match.
+   * If either one is undefined, the default value is used for comparison.
+   * @param lhs the first Multikey value.
+   * @param rhs the second Multikey value.
+   */
+  optionalMultikeyMatches(lhs: boolean | undefined, rhs: boolean | undefined): boolean {
+    const defaultValue = false;
+    return optionalValueMatches<boolean>(lhs, rhs, defaultValue);
+  }
+
   /**
    * Determine if an API Index and a Spec Index are functionally equivalent.
    */
-  indexMatchesSpec(index: types.Index, spec: Spec.Index): boolean {
+  indexMatchesSpec(index: types.Index, spec: Spec.Index, edition: types.DatabaseEdition): boolean {
     const collection = util.parseIndexName(index.name).collectionGroupId;
     if (collection !== spec.collectionGroup) {
       return false;
@@ -492,21 +540,24 @@
       return false;
     }
 
-    if (index.apiScope !== spec.apiScope) {
+    // apiScope is an optional value and may be missing in firestore.indexes.json,
+    // and may also be missing from the server value (when default is picked).
+    if (!this.optionalApiScopeMatches(index.apiScope, spec.apiScope)) {
       return false;
     }
 
-    if (index.density !== spec.density) {
+    // density is an optional value and may be missing in firestore.indexes.json,
+    // and may also be missing from the server value (when default is picked).
+    if (!this.optionalDensityMatches(index.density, spec.density, edition)) {
       return false;
     }
-
-    if (index.multikey !== spec.multikey) {
+    // multikey is an optional value and may be missing in firestore.indexes.json,
+    // and may also be missing from the server value (when default is picked).
+    if (!this.optionalMultikeyMatches(index.multikey, spec.multikey)) {
       return false;
     }
 
-    if (index.unique !== spec.unique) {
-      return false;
-    }
+    // TODO(b/439901837): Compare `unique` index configuration when it's supported.
 
     if (index.fields.length !== spec.fields.length) {
       return false;
@@ -529,7 +580,8 @@
         return false;
       }
 
-      if (iField.vectorConfig !== sField.vectorConfig) {
+      // Note: vectorConfig is an object, and using '!==' should not be used.
+      if (!utils.deepEqual(iField.vectorConfig, sField.vectorConfig)) {
         return false;
       }