Model Index Bugfixes (#31048) (#31049) (#31094)

* [Model Indexes] Use model metadata to determine indexability (#31048)
* [Model Indexes] Allow Updating Metadata at the same time as adding an index (#31049)
* [Model Indexes] Fix Model Index Front End Cache Invalidation (#31094)

e2e/test/scenarios/models/model-indexes.cy.spec.js

@@ -20,6 +20,7 @@ describe("scenarios > model indexes", () => {
     cy.intercept("POST", "/api/dataset").as("dataset");
     cy.intercept("POST", "/api/model-index").as("modelIndexCreate");
     cy.intercept("DELETE", "/api/model-index/*").as("modelIndexDelete");
+    cy.intercept("PUT", "/api/card/*").as("cardUpdate");
 
     cy.createQuestion({
       name: "Products Model",
@@ -28,7 +29,7 @@ describe("scenarios > model indexes", () => {
     });
   });
 
-  it("should create and delete a model index on product titles", () => {
+  it("should create, delete, and re-create a model index on product titles", () => {
     cy.visit(`/model/${modelId}`);
     cy.wait("@dataset");
 
@@ -64,6 +65,60 @@ describe("scenarios > model indexes", () => {
       expect(request.url).to.include("/api/model-index/1");
       expect(response.statusCode).to.equal(200);
     });
+
+    cy.wait("@dataset");
+
+    editTitleMetadata();
+
+    sidebar()
+      .findByLabelText(/surface individual records/i)
+      .click();
+
+    cy.findByTestId("dataset-edit-bar").within(() => {
+      cy.button("Save changes").click();
+    });
+
+    // this tests redux cache invalidation (#31407)
+    cy.wait("@modelIndexCreate").then(({ request, response }) => {
+      expect(request.body.model_id).to.equal(modelId);
+
+      // this will likely change when this becomes an async process
+      expect(response.body.state).to.equal("indexed");
+      expect(response.body.id).to.equal(2);
+    });
+  });
+
+  it("should not allow indexing when a primary key has been unassigned", () => {
+    cy.visit(`/model/${modelId}`);
+    cy.wait("@dataset");
+
+    editTitleMetadata();
+
+    sidebar()
+      .findByLabelText(/surface individual records/i)
+      .click();
+
+    openColumnOptions("ID");
+
+    // change the entity key to a foreign key so no key exists
+    sidebar()
+      .findByText(/entity key/i)
+      .click();
+
+    popover()
+      .findByText(/foreign key/i)
+      .click();
+
+    cy.findByTestId("dataset-edit-bar").button("Save changes").click();
+
+    cy.wait("@cardUpdate");
+
+    // search should fail
+    cy.findByTestId("app-bar")
+      .findByPlaceholderText("Search…")
+      .type("marble shoes");
+
+    cy.findByTestId("search-results-list").findByText(/didn't find anything/i);
   });
 
   it("should be able to search model index values and visit detail records", () => {

frontend/src/metabase/entities/model-indexes/actions.ts

@@ -26,9 +26,9 @@ export const updateModelIndexes =
     }
 
     const existingIndexes: ModelIndex[] =
-      ModelIndexes.selectors.getIndexesForModel(getState(), {
-        modelId: model.id(),
-      });
+      ModelIndexes.selectors.getList(getState(), {
+        entityQuery: { model_id: model.id() },
+      }) ?? [];
 
     const newFieldsToIndex = getFieldsToIndex(
       fieldsWithIndexFlags,
@@ -42,15 +42,17 @@ export const updateModelIndexes =
     if (newFieldsToIndex.length) {
       const pkRef = ModelIndexes.utils.getPkRef(fields);
 
-      await Promise.all(
-        newFieldsToIndex.map(field =>
-          ModelIndexes.api.create({
-            model_id: model.id(),
-            value_ref: field.field_ref,
-            pk_ref: pkRef,
-          }),
-        ),
-      );
+      if (pkRef) {
+        await Promise.all(
+          newFieldsToIndex.map(field =>
+            ModelIndexes.api.create({
+              model_id: model.id(),
+              value_ref: field.field_ref,
+              pk_ref: pkRef,
+            }),
+          ),
+        );
+      }
     }
 
     if (indexIdsToRemove.length) {
@@ -107,8 +109,7 @@ function getIndexIdsToRemove(
   return indexIdsToRemove;
 }
 
-export function cleanIndexFlags(model: Question) {
-  const fields = model.getResultMetadata();
+export function cleanIndexFlags(fields: Field[] = []) {
   const indexesToClean = fields.reduce(
     (
       indexesToClean: number[],
@@ -123,12 +124,11 @@ export function cleanIndexFlags(model: Question) {
     [],
   );
 
-  const newResultMetadata = [...model.getResultMetadata()];
+  const newResultMetadata = [...fields];
   for (const index of indexesToClean) {
     newResultMetadata[index] = dissocIn(newResultMetadata[index], [
       "should_index",
     ]);
   }
-  const newModel = model.setResultsMetadata(newResultMetadata);
-  return newModel;
+  return newResultMetadata;
 }

frontend/src/metabase/entities/model-indexes/actions.unit.spec.ts

@@ -37,9 +37,9 @@ describe("Entities > model-indexes > actions", () => {
         createMockField(),
       ]);
 
-      const cleanedQuestion = cleanIndexFlags(model);
+      const cleanedFields = cleanIndexFlags(model.getResultMetadata());
 
-      cleanedQuestion.getResultMetadata().forEach((field: any) => {
+      cleanedFields.forEach((field: any) => {
         expect(field?.should_index).toBeUndefined();
       });
     });
@@ -50,7 +50,7 @@ describe("Entities > model-indexes > actions", () => {
         createMockField({ should_index: true }),
       ]);
 
-      cleanIndexFlags(model);
+      cleanIndexFlags(model.getResultMetadata());
 
       model.getResultMetadata().forEach((field: any) => {
         expect(field?.should_index).toBe(true);
@@ -85,7 +85,9 @@ describe("Entities > model-indexes > actions", () => {
       setupModelIndexEndpoints(model.id(), []);
 
       const mockDispatch = jest.fn();
-      const mockGetState = jest.fn(() => ({ entities: { modelIndexes: [] } }));
+      const mockGetState = jest.fn(() => ({
+        entities: { modelIndexes: {}, modelIndexes_list: {} },
+      }));
       await updateModelIndexes(model)(mockDispatch, mockGetState);
 
       expect(mockDispatch).toHaveBeenCalled();
@@ -113,6 +115,7 @@ describe("Entities > model-indexes > actions", () => {
 
       const existingModelIndex = createMockModelIndex({
         id: 99,
+        model_id: 1,
         value_ref: indexFieldRef,
       });
 
@@ -123,7 +126,7 @@ describe("Entities > model-indexes > actions", () => {
         entities: {
           modelIndexes: { 99: existingModelIndex },
           modelIndexes_list: {
-            '{"model_id":1': {
+            '{"model_id":1}': {
               list: [99],
               metadata: {},
             },
@@ -163,7 +166,7 @@ describe("Entities > model-indexes > actions", () => {
         entities: {
           modelIndexes: { 99: existingModelIndex },
           modelIndexes_list: {
-            '{"model_id":1': {
+            '{"model_id":1}': {
               list: [99],
               metadata: {},
             },
@@ -194,7 +197,9 @@ describe("Entities > model-indexes > actions", () => {
       setupModelIndexEndpoints(model.id(), []);
 
       const mockDispatch = jest.fn();
-      const mockGetState = jest.fn(() => ({ entities: { modelIndexes: [] } }));
+      const mockGetState = jest.fn(() => ({
+        entities: { modelIndexes: {}, modelIndexes_list: {} },
+      }));
       await updateModelIndexes(model)(mockDispatch, mockGetState);
 
       expect(mockDispatch).toHaveBeenCalled();

frontend/src/metabase/entities/model-indexes/model-indexes.ts

@@ -5,7 +5,6 @@
  */
 
 import type { IndexedEntity } from "metabase-types/api/modelIndexes";
-import type { State } from "metabase-types/store";
 
 import { createEntity } from "metabase/lib/entities";
 import { ModelIndexApi } from "metabase/services";
@@ -29,16 +28,7 @@ export const ModelIndexes = createEntity({
     getUrl: (entity: IndexedEntity) => `/model/${entity.model_id}/${entity.id}`,
     getIcon: () => ({ name: "beaker" }),
   },
-  // objectActions: {},
   reducer: (state = {}, { type, payload }: { type: string; payload: any }) => {
     return state;
   },
-  selectors: {
-    getIndexesForModel: (state: State, { modelId }: { modelId: number }) => {
-      return Object.values(state.entities.modelIndexes).filter(
-        index => index.model_id === modelId,
-      );
-    },
-  },
-  // objectSelectors: {},
 });

frontend/src/metabase/entities/model-indexes/utils.ts

@@ -8,15 +8,18 @@ import {
   isBoolean,
 } from "metabase-lib/types/utils/isa";
 import type FieldEntity from "metabase-lib/metadata/Field";
-import type Table from "metabase-lib/metadata/Table";
+import type Question from "metabase-lib/Question";
 
-const hasPk = (table?: Table) =>
-  !!table?.fields?.some(
-    (field: FieldEntity) => isPK(field) && isInteger(field),
-  );
+const hasSingleIntegerPk = (model?: Question) => {
+  const pkFields = model
+    ?.getResultMetadata()
+    ?.filter((field: Field) => isPK(field));
+
+  return pkFields?.length === 1 && isInteger(pkFields[0]);
+};
 
-export const canIndexField = (field: FieldEntity): boolean => {
-  return !!(isString(field) && !isBoolean(field) && hasPk(field?.table));
+export const canIndexField = (field: FieldEntity, model: Question): boolean => {
+  return !!(isString(field) && !isBoolean(field) && hasSingleIntegerPk(model));
 };
 
 export const getPkRef = (fields?: Field[]) => fields?.find(isPK)?.field_ref;

frontend/src/metabase/entities/model-indexes/utils.unit.spec.ts

+import type { Field as FieldAPI } from "metabase-types/api";
+import { createMockField, createMockCard } from "metabase-types/api/mocks";
+import Question from "metabase-lib/Question";
+import Field from "metabase-lib/metadata/Field";
+
+import { canIndexField } from "./utils";
+
+const createModelWithResultMetadata = (fields: FieldAPI[]) => {
+  return new Question(
+    createMockCard({ result_metadata: fields, dataset: true }),
+  );
+};
+
+describe("Entities > model-indexes > utils", () => {
+  describe("canIndexField", () => {
+    it("should return true for string field in a model with single integer pk", () => {
+      const field = createMockField({ name: "foo", base_type: "type/Text" });
+
+      const model = createModelWithResultMetadata([
+        createMockField({ name: "foo", base_type: "type/Text" }),
+        createMockField({
+          name: "id",
+          base_type: "type/Integer",
+          semantic_type: "type/PK",
+        }),
+      ]);
+
+      expect(canIndexField(new Field(field), model)).toBe(true);
+    });
+
+    it("should return false for boolean field in a model with single integer pk", () => {
+      const field = createMockField({ name: "foo", base_type: "type/Boolean" });
+
+      const model = createModelWithResultMetadata([
+        createMockField({ name: "foo", base_type: "type/Boolean" }),
+        createMockField({
+          name: "id",
+          base_type: "type/Integer",
+          semantic_type: "type/PK",
+        }),
+      ]);
+
+      expect(canIndexField(new Field(field), model)).toBe(false);
+    });
+
+    it("should return false for string field in a model without any pk", () => {
+      const field = createMockField({ name: "foo", base_type: "type/Text" });
+
+      const model = createModelWithResultMetadata([
+        createMockField({ name: "foo", base_type: "type/Text" }),
+        createMockField({ name: "bar", base_type: "type/Integer" }),
+      ]);
+
+      expect(canIndexField(new Field(field), model)).toBe(false);
+    });
+
+    it("should return false for string field in a model with multiple pks", () => {
+      const field = createMockField({ name: "foo", base_type: "type/String" });
+
+      const model = createModelWithResultMetadata([
+        createMockField({ name: "foo", base_type: "type/Boolean" }),
+        createMockField({
+          name: "id",
+          base_type: "type/String",
+          semantic_type: "type/PK",
+        }),
+        createMockField({
+          name: "id2",
+          base_type: "type/Integer",
+          semantic_type: "type/PK",
+        }),
+      ]);
+
+      expect(canIndexField(new Field(field), model)).toBe(false);
+    });
+
+    it("should return false for string field in a model with multiple integer pks", () => {
+      const field = createMockField({ name: "foo", base_type: "type/String" });
+
+      const model = createModelWithResultMetadata([
+        createMockField({ name: "foo", base_type: "type/Boolean" }),
+        createMockField({
+          name: "id",
+          base_type: "type/Integer",
+          semantic_type: "type/PK",
+        }),
+        createMockField({
+          name: "id2",
+          base_type: "type/Integer",
+          semantic_type: "type/PK",
+        }),
+      ]);
+
+      expect(canIndexField(new Field(field), model)).toBe(false);
+    });
+
+    it("should return false for a model with a string pk", () => {
+      const field = createMockField({ name: "foo", base_type: "type/Boolean" });
+
+      const model = createModelWithResultMetadata([
+        createMockField({ name: "foo", base_type: "type/Boolean" }),
+        createMockField({
+          name: "id",
+          base_type: "type/String",
+          semantic_type: "type/PK",
+        }),
+      ]);
+
+      expect(canIndexField(new Field(field), model)).toBe(false);
+    });
+  });
+});

frontend/src/metabase/query_builder/actions/core/core.js

@@ -228,9 +228,12 @@ export const apiUpdateQuestion = (question, { rerunQuery } = {}) => {
     const originalQuestion = getOriginalQuestion(getState());
     question = question || getQuestion(getState());
 
+    const resultsMetadata = getResultsMetadata(getState());
+
     if (question.isDataset()) {
-      await dispatch(ModelIndexes.actions.updateModelIndexes(question));
-      question = ModelIndexes.actions.cleanIndexFlags(question);
+      resultsMetadata.columns = ModelIndexes.actions.cleanIndexFlags(
+        resultsMetadata.columns,
+      );
     }
 
     rerunQuery = rerunQuery ?? getIsResultDirty(getState());
@@ -241,7 +244,6 @@ export const apiUpdateQuestion = (question, { rerunQuery } = {}) => {
       ? getQuestionWithDefaultVisualizationSettings(question, series)
       : question;
 
-    const resultsMetadata = getResultsMetadata(getState());
     const questionToUpdate = questionWithVizSettings
       // Before we clean the query, we make sure question is not treated as a dataset
       // as calling table() method down the line would bring unwanted consequences
@@ -270,7 +272,16 @@ export const apiUpdateQuestion = (question, { rerunQuery } = {}) => {
       updatedQuestion.type(),
     );
 
-    dispatch({ type: API_UPDATE_QUESTION, payload: updatedQuestion.card() });
+    await dispatch({
+      type: API_UPDATE_QUESTION,
+      payload: updatedQuestion.card(),
+    });
+
+    if (question.isDataset()) {
+      // this needs to happen after the question update completes in case we have changed the type
+      // of the primary key field in the same update
+      await dispatch(ModelIndexes.actions.updateModelIndexes(question));
+    }
 
     const metadataOptions = { reload: question.isDataset() };
     await dispatch(loadMetadataForCard(question.card(), metadataOptions));

frontend/src/metabase/query_builder/components/DatasetEditor/DatasetFieldMetadataSidebar/DatasetFieldMetadataSidebar.jsx

@@ -80,7 +80,8 @@ function getFormFields({ dataset, field }) {
       value: type.id,
     }));
 
-  const canIndex = dataset.isSaved() && ModelIndexes.utils.canIndexField(field);
+  const canIndex =
+    dataset.isSaved() && ModelIndexes.utils.canIndexField(field, dataset);
 
   return formFieldValues =>
     [