fix: index-spec matching for optional values and vectorConfig. (#9003)

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

The few new optional values for indexes were added recently via
#8939.

When comparing the local index with the server index, the default value of optional index configurations should be taken into account depending on the database edition (standard or enterprise).

I also found that the matching logic for vectorConfig was not working correctly, so I fixed that as well.

Fixes #8859.

* Fix typos.

* Address feedback.

* Address feedback.

* add CHANGELOG

Author: ehsannas

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 utils from "../utils";
 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 { firestoreOrigin } from "../api";
 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" });
@@ -74,8 +76,10 @@ export class FirestoreApi {
       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 @@ export class FirestoreApi {
     }
 
     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 @@ export class FirestoreApi {
       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 @@ export class FirestoreApi {
     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 @@ export class FirestoreApi {
       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 @@ export class FirestoreApi {
         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;
       }