Disable saving models without up-to-date result_metadata (#40087)

e2e/test/scenarios/models/create.cy.spec.js

@@ -27,9 +27,10 @@ describe("scenarios > models > create", () => {
 
     cy.get(".ace_editor").should("be.visible").type("select * from ORDERS");
 
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Save").click();
+    cy.findByTestId("native-query-editor-container").icon("play").click();
+    cy.wait("@dataset");
 
+    cy.findByTestId("dataset-edit-bar").button("Save").click();
     cy.findByPlaceholderText("What is the name of your model?").type(modelName);
 
     // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
@@ -47,6 +48,8 @@ describe("scenarios > models > create", () => {
 
     navigateToNewModelPage();
     cy.get(".ace_editor").should("be.visible").type("select * from ORDERS");
+    cy.findByTestId("native-query-editor-container").icon("play").click();
+    cy.wait("@dataset");
 
     cy.findByTestId("dataset-edit-bar").within(() => {
       cy.contains("button", "Save").click();

e2e/test/scenarios/models/reproductions/22518.cy.spec.js

@@ -9,7 +9,7 @@ describe("issue 22518", () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();
-
+    cy.intercept("POST", "/api/dataset").as("dataset");
     cy.createNativeQuestion(
       {
         native: {
@@ -27,9 +27,10 @@ describe("issue 22518", () => {
     cy.findByText("Edit query definition").click();
 
     cy.get(".ace_content").type(", 'b' bar");
+    cy.findByTestId("native-query-editor-container").icon("play").click();
+    cy.wait("@dataset");
 
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Save changes").click();
+    cy.findByTestId("dataset-edit-bar").button("Save changes").click();
 
     cy.findAllByTestId("header-cell")
       .should("have.length", 3)

e2e/test/scenarios/models/reproductions/35039-37009-missing-result-metadata.cy.spec.js

@@ -9,6 +9,7 @@ import {
 describe("issues 35039 and 37009", () => {
   beforeEach(() => {
     restore();
+    cy.intercept("POST", "/api/dataset").as("dataset");
     cy.signInAsNormalUser();
   });
 
@@ -20,8 +21,10 @@ describe("issues 35039 and 37009", () => {
       .click();
 
     focusNativeEditor().type("select * from products -- where true=false");
+    cy.findByTestId("native-query-editor-container").icon("play").click();
+    cy.wait("@dataset");
 
-    cy.findByTestId("dataset-edit-bar").findByText("Save").click();
+    cy.findByTestId("dataset-edit-bar").button("Save").click();
     modal()
       .last()
       .within(() => {
@@ -35,6 +38,9 @@ describe("issues 35039 and 37009", () => {
     focusNativeEditor().type(
       "{backspace}{backspace}{backspace}{backspace}{backspace}",
     );
+    cy.findByTestId("native-query-editor-container").icon("play").click();
+    cy.wait("@dataset");
+
     cy.findByTestId("dataset-edit-bar").within(() => {
       cy.findByText("Save changes").click();
       cy.findByText("Saving…").should("not.exist");

e2e/test/scenarios/models/reproductions/37009-stale-result-metadata.cy.spec.js

+import {
+  focusNativeEditor,
+  modal,
+  openQuestionActions,
+  popover,
+  restore,
+} from "e2e/support/helpers";
+
+describe("issue 37009", () => {
+  beforeEach(() => {
+    restore();
+    cy.intercept("POST", "/api/dataset").as("dataset");
+    cy.intercept("POST", "/api/card").as("saveCard");
+    cy.intercept("PUT", "/api/card/*").as("updateCard");
+    cy.signInAsNormalUser();
+  });
+
+  it("should prevent saving new and updating existing models without result_metadata (metabase#37009)", () => {
+    cy.visit("/model/new");
+    cy.findByTestId("new-model-options")
+      .findByText("Use a native query")
+      .click();
+
+    focusNativeEditor().type("select * from products");
+    cy.findByTestId("dataset-edit-bar")
+      .button("Save")
+      .should("be.disabled")
+      .trigger("mousemove", { force: true });
+    cy.findByRole("tooltip").should(
+      "have.text",
+      "You must run the query before you can save this model",
+    );
+    cy.findByTestId("native-query-editor-container").icon("play").click();
+    cy.wait("@dataset");
+    cy.findByRole("tooltip").should("not.exist");
+    cy.findByTestId("dataset-edit-bar")
+      .button("Save")
+      .should("be.enabled")
+      .click();
+    modal()
+      .last()
+      .within(() => {
+        cy.findByLabelText("Name").type("Model");
+        cy.findByText("Save").click();
+      });
+    cy.wait("@saveCard")
+      .its("request.body")
+      .its("result_metadata")
+      .should("not.be.null");
+
+    openQuestionActions();
+    popover().findByText("Edit query definition").click();
+    focusNativeEditor().type(" WHERE CATEGORY = 'Gadget'");
+    cy.findByTestId("dataset-edit-bar")
+      .button("Save changes")
+      .should("be.disabled")
+      .trigger("mousemove", { force: true });
+    cy.findByRole("tooltip").should(
+      "have.text",
+      "You must run the query before you can save this model",
+    );
+    cy.findByTestId("native-query-editor-container").icon("play").click();
+    cy.wait("@dataset");
+    cy.findByRole("tooltip").should("not.exist");
+    cy.findByTestId("dataset-edit-bar")
+      .button("Save changes")
+      .should("be.enabled")
+      .click();
+    cy.wait("@updateCard")
+      .its("request.body")
+      .its("result_metadata")
+      .should("not.be.null");
+  });
+});

frontend/src/metabase/components/ActionButton/ActionButton.jsx

@@ -103,6 +103,7 @@ export default class ActionButton extends Component {
 
   render() {
     const {
+      innerRef,
       normalText,
       activeText,
       failedText,
@@ -122,6 +123,7 @@ export default class ActionButton extends Component {
 
     return (
       <Button
+        ref={innerRef}
         {...props}
         className={
           forceActiveStyle

frontend/src/metabase/entities/tables.js

@@ -14,7 +14,6 @@ import {
   createThunkAction,
   withAction,
   withCachedDataAndRequestState,
-  withForceReload,
   withNormalize,
 } from "metabase/lib/redux";
 import * as Urls from "metabase/lib/urls";
@@ -84,13 +83,6 @@ const Tables = createEntity({
     // loads `query_metadata` for a single table
     fetchMetadata: compose(
       withAction(FETCH_METADATA),
-      withForceReload((state, { id }) => {
-        // if there is a virtual table without fields,
-        // it might be due to a question with a long-running request that hasn't finished yet.
-        // in this case we should reload the metadata until we get the fields
-        const table = Tables.selectors.getObject(state, { entityId: id });
-        return table?.fields?.length === 0;
-      }),
       withCachedDataAndRequestState(
         ({ id }) => [...Tables.getObjectStatePath(id)],
         ({ id }) => [...Tables.getObjectStatePath(id), "fetchMetadata"],

frontend/src/metabase/lib/redux/utils.js

@@ -253,15 +253,6 @@ export function withRequestState(getRequestStatePath, getQueryKey) {
     };
 }
 
-export function withForceReload(shouldReload) {
-  return thunkCreator => (data, options) => (dispatch, getState) => {
-    return thunkCreator(data, {
-      ...options,
-      reload: options?.reload || shouldReload(getState(), data),
-    })(dispatch, getState);
-  };
-}
-
 /**
  * Decorator that returns cached data if appropriate, otherwise calls the composed thunk.
  * Also tracks request state using withRequestState

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

 import cx from "classnames";
 import { merge } from "icepick";
 import PropTypes from "prop-types";
-import { useEffect, useCallback, useMemo, useState } from "react";
+import { useCallback, useEffect, useMemo, useState } from "react";
 import { connect } from "react-redux";
 import { usePrevious } from "react-use";
 import { t } from "ttag";
@@ -27,10 +27,12 @@ import ViewSidebar from "metabase/query_builder/components/view/ViewSidebar";
 import { MODAL_TYPES } from "metabase/query_builder/constants";
 import {
   getDatasetEditorTab,
+  getIsResultDirty,
   getResultsMetadata,
   isResultsMetadataDirty,
 } from "metabase/query_builder/selectors";
 import { getMetadata } from "metabase/selectors/metadata";
+import { Tooltip } from "metabase/ui";
 import * as Lib from "metabase-lib";
 import {
   checkCanBeModel,
@@ -39,14 +41,14 @@ import {
 import { isSameField } from "metabase-lib/v1/queries/utils/field-ref";
 
 import {
-  Root,
   DatasetEditBar,
   FieldTypeIcon,
   MainContainer,
   QueryEditorContainer,
-  TableHeaderColumnName,
-  TableContainer,
+  Root,
   TabHintToastContainer,
+  TableContainer,
+  TableHeaderColumnName,
 } from "./DatasetEditor.styled";
 import DatasetFieldMetadataSidebar from "./DatasetFieldMetadataSidebar";
 import DatasetQueryEditor from "./DatasetQueryEditor";
@@ -63,6 +65,7 @@ const propTypes = {
   result: PropTypes.object,
   height: PropTypes.number,
   isDirty: PropTypes.bool.isRequired,
+  isResultDirty: PropTypes.bool.isRequired,
   isRunning: PropTypes.bool.isRequired,
   setQueryBuilderMode: PropTypes.func.isRequired,
   setDatasetEditorTab: PropTypes.func.isRequired,
@@ -93,6 +96,7 @@ function mapStateToProps(state) {
     datasetEditorTab: getDatasetEditorTab(state),
     isMetadataDirty: isResultsMetadataDirty(state),
     resultsMetadata: getResultsMetadata(state),
+    isResultDirty: getIsResultDirty(state),
   };
 }
 
@@ -196,6 +200,7 @@ function DatasetEditor(props) {
     isMetadataDirty,
     height,
     isDirty: isModelQueryDirty,
+    isResultDirty,
     setQueryBuilderMode,
     runQuestionQuery,
     setDatasetEditorTab,
@@ -413,18 +418,21 @@ function DatasetEditor(props) {
     [datasetEditorTab, renderSelectableTableColumnHeader],
   );
 
-  const canSaveChanges = useMemo(() => {
-    const { isNative } = Lib.queryDisplayInfo(dataset.query());
-    const isEmpty = !isNative
-      ? Lib.databaseID(dataset.query()) == null
-      : dataset.legacyQuery().isEmpty();
+  const { isNative } = Lib.queryDisplayInfo(dataset.query());
+  const isEmpty = !isNative
+    ? Lib.databaseID(dataset.query()) == null
+    : dataset.legacyQuery().isEmpty();
 
-    if (isEmpty) {
-      return false;
-    }
-    const everyFieldHasDisplayName = fields.every(field => field.display_name);
-    return everyFieldHasDisplayName && isDirty;
-  }, [dataset, fields, isDirty]);
+  const canSaveChanges =
+    !isEmpty &&
+    isDirty &&
+    (!isNative || !isResultDirty) &&
+    fields.every(field => field.display_name);
+
+  const saveButtonTooltipLabel =
+    !isEmpty && isDirty && isNative && isResultDirty
+      ? t`You must run the query before you can save this model`
+      : undefined;
 
   const sidebar = getSidebar(
     { ...props, modelIndexes },
@@ -464,20 +472,27 @@ function DatasetEditor(props) {
             small
             onClick={handleCancelClick}
           >{t`Cancel`}</Button>,
-          <ActionButton
+          <Tooltip
             key="save"
-            disabled={!canSaveChanges}
-            actionFn={handleSave}
-            normalText={dataset.isSaved() ? t`Save changes` : t`Save`}
-            activeText={t`Saving…`}
-            failedText={t`Save failed`}
-            successText={t`Saved`}
-            className={cx(
-              ButtonsS.Button,
-              ButtonsS.ButtonPrimary,
-              ButtonsS.ButtonSmall,
-            )}
-          />,
+            refProp="innerRef"
+            label={saveButtonTooltipLabel}
+            disabled={!saveButtonTooltipLabel}
+          >
+            <ActionButton
+              key="save"
+              disabled={!canSaveChanges}
+              actionFn={handleSave}
+              normalText={dataset.isSaved() ? t`Save changes` : t`Save`}
+              activeText={t`Saving…`}
+              failedText={t`Save failed`}
+              successText={t`Saved`}
+              className={cx(
+                ButtonsS.Button,
+                ButtonsS.ButtonPrimary,
+                ButtonsS.ButtonSmall,
+              )}
+            />
+          </Tooltip>,
         ]}
       />
       <Root>