Fix broken line/area/bar charts after adding a second stage filter (#44053)

* Rework column sync to work with series objects

* Reuse previous metrics and dimensions if available

* Add repro test

e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js

@@ -26,6 +26,7 @@ import {
   resyncDatabase,
   createQuestion,
   saveQuestion,
+  echartsContainer,
 } from "e2e/support/helpers";
 
 const { ORDERS, ORDERS_ID, PRODUCTS } = SAMPLE_DATABASE;
@@ -631,3 +632,55 @@ describe("issue 42957", () => {
     });
   });
 });
+
+describe("issue 10493", () => {
+  beforeEach(() => {
+    restore();
+    cy.intercept("POST", "/api/dataset").as("dataset");
+    cy.signInAsAdmin();
+  });
+
+  it("should not reset chart axes after adding a new query stage (metabase#10493)", () => {
+    visitQuestionAdhoc({
+      display: "bar",
+      dataset_query: {
+        type: "query",
+        database: SAMPLE_DB_ID,
+        query: {
+          aggregation: [["count"]],
+          breakout: [
+            [
+              "field",
+              ORDERS.QUANTITY,
+              { "base-type": "type/Integer", binning: { strategy: "default" } },
+            ],
+          ],
+          "source-table": ORDERS_ID,
+        },
+      },
+    });
+
+    filter();
+    modal().within(() => {
+      cy.findByText("Summaries").click();
+      cy.findByTestId("filter-column-Count").within(() => {
+        cy.findByPlaceholderText("Min").type("0");
+        cy.findByPlaceholderText("Max").type("30000");
+      });
+      cy.button("Apply filters").click();
+    });
+    cy.wait("@dataset");
+
+    echartsContainer().within(() => {
+      // y axis
+      cy.findByText("Count").should("exist");
+      cy.findByText("21,000").should("exist");
+      cy.findByText("3,000").should("exist");
+
+      // x axis
+      cy.findByText("Quantity").should("exist");
+      cy.findByText("25").should("exist");
+      cy.findByText("75").should("exist");
+    });
+  });
+});

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

@@ -34,7 +34,6 @@ import {
   isBasedOnExistingQuestion,
   getParameters,
   getSubmittableQuestion,
-  getQueryResults,
 } from "../../selectors";
 import { updateUrl } from "../navigation";
 import { zoomInRow } from "../object-detail";
@@ -112,10 +111,7 @@ export const reloadCard = createThunkAction(RELOAD_CARD, () => {
  *     - `navigateToNewCardInsideQB` is being called (see below)
  */
 export const SET_CARD_AND_RUN = "metabase/qb/SET_CARD_AND_RUN";
-export const setCardAndRun = (
-  nextCard,
-  { shouldUpdateUrl = true, prevQueryResults } = {},
-) => {
+export const setCardAndRun = (nextCard, { shouldUpdateUrl = true } = {}) => {
   return async (dispatch, getState) => {
     // clone
     const card = copy(nextCard);
@@ -131,12 +127,7 @@ export const setCardAndRun = (
 
     // Update the card and originalCard before running the actual query
     dispatch({ type: SET_CARD_AND_RUN, payload: { card, originalCard } });
-    dispatch(
-      runQuestionQuery({
-        shouldUpdateUrl,
-        prevQueryResults,
-      }),
-    );
+    dispatch(runQuestionQuery({ shouldUpdateUrl }));
 
     // Load table & database metadata for the current question
     dispatch(loadMetadataForCard(card));
@@ -175,16 +166,13 @@ export const navigateToNewCardInsideQB = createThunkAction(
           dispatch(openUrl(url));
         } else {
           dispatch(onCloseSidebars());
-          const prevQueryResults = getQueryResults(getState());
           if (!cardQueryIsEquivalent(previousCard, nextCard)) {
             // clear the query result so we don't try to display the new visualization before running the new query
             dispatch(clearQueryResult());
           }
           // When the dataset query changes, we should change the type,
           // to start building a new ad-hoc question based on a dataset
-          dispatch(
-            setCardAndRun({ ...card, type: "question" }, { prevQueryResults }),
-          );
+          dispatch(setCardAndRun({ ...card, type: "question" }));
         }
         if (objectId !== undefined) {
           dispatch(zoomInRow({ objectId }));

frontend/src/metabase/query_builder/actions/core/updateQuestion.ts

@@ -6,7 +6,7 @@ import Questions from "metabase/entities/questions";
 import { createThunkAction } from "metabase/lib/redux";
 import { loadMetadataForCard } from "metabase/questions/actions";
 import { addUndo } from "metabase/redux/undo";
-import { syncVizSettingsWithQueryResults } from "metabase/visualizations/lib/sync-settings";
+import { syncVizSettingsWithSeries } from "metabase/visualizations/lib/sync-settings";
 import * as Lib from "metabase-lib";
 import type Question from "metabase-lib/v1/Question";
 import { getTemplateTagParametersFromCard } from "metabase-lib/v1/parameters/utils/template-tags";
@@ -139,7 +139,13 @@ export const updateQuestion = (
 
     const queryResult = getFirstQueryResult(getState());
     newQuestion = newQuestion.setSettings(
-      syncVizSettingsWithQueryResults(newQuestion.settings(), queryResult),
+      syncVizSettingsWithSeries(newQuestion.settings(), [
+        {
+          card: newQuestion.card(),
+          data: queryResult?.data,
+          error: queryResult?.error,
+        },
+      ]),
     );
 
     if (!newQuestion.canAutoRun()) {

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

@@ -8,12 +8,14 @@ import { createThunkAction } from "metabase/lib/redux";
 import { getWhiteLabeledLoadingMessageFactory } from "metabase/selectors/whitelabel";
 import { runQuestionQuery as apiRunQuestionQuery } from "metabase/services";
 import { getSensibleDisplays } from "metabase/visualizations";
-import { syncVizSettingsWithQueryResults } from "metabase/visualizations/lib/sync-settings";
+import { syncVizSettingsWithSeries } from "metabase/visualizations/lib/sync-settings";
 import * as Lib from "metabase-lib";
 import { isAdHocModelQuestion } from "metabase-lib/v1/metadata/utils/models";
 import { isSameField } from "metabase-lib/v1/queries/utils/field-ref";
 
 import {
+  getCard,
+  getFirstQueryResult,
   getIsResultDirty,
   getIsRunning,
   getOriginalQuestion,
@@ -95,7 +97,6 @@ export const runQuestionQuery = ({
   shouldUpdateUrl = true,
   ignoreCache = false,
   overrideWithQuestion = null,
-  prevQueryResults = undefined,
 } = {}) => {
   return async (dispatch, getState) => {
     dispatch(loadStartUIControls());
@@ -139,11 +140,7 @@ export const runQuestionQuery = ({
             duration,
           ),
         );
-        return dispatch(
-          queryCompleted(question, queryResults, {
-            prevQueryResults: prevQueryResults ?? getQueryResults(getState()),
-          }),
-        );
+        return dispatch(queryCompleted(question, queryResults));
       })
       .catch(error => dispatch(queryErrored(startTime, error)));
 
@@ -178,25 +175,25 @@ export const CLEAR_QUERY_RESULT = "metabase/query_builder/CLEAR_QUERY_RESULT";
 export const clearQueryResult = createAction(CLEAR_QUERY_RESULT);
 
 export const QUERY_COMPLETED = "metabase/qb/QUERY_COMPLETED";
-export const queryCompleted = (
-  question,
-  queryResults,
-  { prevQueryResults } = {},
-) => {
+export const queryCompleted = (question, queryResults) => {
   return async (dispatch, getState) => {
-    const [{ data }] = queryResults;
-    const [{ data: prevData }] = prevQueryResults ?? [{}];
+    const [{ data, error }] = queryResults;
+    const prevCard = getCard(getState());
+    const { data: prevData, error: prevError } =
+      getFirstQueryResult(getState()) ?? {};
+
     const originalQuestion = getOriginalQuestionWithParameterValues(getState());
     const { isEditable } = Lib.queryDisplayInfo(question.query());
     const isDirty = isEditable && question.isDirtyComparedTo(originalQuestion);
 
     if (isDirty) {
+      const series = [{ card: question.card(), data, error }];
+      const previousSeries =
+        prevCard && (prevData || prevError)
+          ? [{ card: prevCard, data: prevData, error: prevError }]
+          : null;
       question = question.setSettings(
-        syncVizSettingsWithQueryResults(
-          question.settings(),
-          queryResults[0],
-          prevQueryResults?.[0],
-        ),
+        syncVizSettingsWithSeries(question.settings(), series, previousSeries),
       );
 
       question = question.maybeResetDisplay(

frontend/src/metabase/visualizations/index.ts

@@ -128,6 +128,15 @@ export function getDefaultSize(display: string) {
   return visualization?.defaultSize;
 }
 
+export function hasGraphDataSettings(display: string) {
+  const visualization = visualizations.get(display);
+  const settingDefinitions = visualization?.settings ?? {};
+  return (
+    "graph.dimensions" in settingDefinitions &&
+    "graph.metrics" in settingDefinitions
+  );
+}
+
 // removes columns with `remapped_from` property and adds a `remapping` to the appropriate column
 export const extractRemappedColumns = (data: DatasetData) => {
   const cols: RemappingHydratedDatasetColumn[] = data.cols.map(col => ({

frontend/src/metabase/visualizations/lib/sync-settings.ts

+import { isNotNull } from "metabase/lib/types";
+import {
+  getMaxDimensionsSupported,
+  getMaxMetricsSupported,
+  hasGraphDataSettings,
+} from "metabase/visualizations";
 import {
   findColumnIndexesForColumnSettings,
   findColumnSettingIndexesForColumns,
 } from "metabase-lib/v1/queries/utils/dataset";
 import { getColumnKey } from "metabase-lib/v1/queries/utils/get-column-key";
-import type { Dataset, VisualizationSettings } from "metabase-types/api";
+import type {
+  Series,
+  SingleSeries,
+  VisualizationSettings,
+} from "metabase-types/api";
+
+import { getSingleSeriesDimensionsAndMetrics } from "./utils";
 
-export function syncVizSettingsWithQueryResults(
+export function syncVizSettingsWithSeries(
   settings: VisualizationSettings,
-  queryResults?: Dataset,
-  prevQueryResults?: Dataset,
+  _series?: Series | null,
+  _previousSeries?: Series | null,
 ): VisualizationSettings {
   let newSettings = settings;
 
-  if (queryResults && !queryResults.error) {
-    newSettings = syncTableColumnSettings(newSettings, queryResults);
+  const series = _series?.[0];
+  const previousSeries = _previousSeries?.[0];
+
+  if (series && !series.error) {
+    newSettings = syncTableColumnSettings(newSettings, series);
 
-    if (prevQueryResults && !prevQueryResults.error) {
+    if (previousSeries && !previousSeries.error) {
       newSettings = syncGraphMetricSettings(
         newSettings,
-        queryResults,
-        prevQueryResults,
+        series,
+        previousSeries,
       );
+
+      if (hasGraphDataSettings(series.card.display)) {
+        newSettings = ensureMetricsAndDimensions(
+          newSettings,
+          series,
+          previousSeries,
+        );
+      }
     }
   }
 
   return newSettings;
 }
 
+function ensureMetricsAndDimensions(
+  settings: VisualizationSettings,
+  series: SingleSeries,
+  previousSeries: SingleSeries,
+) {
+  const hasExplicitGraphDataSettings =
+    "graph.dimensions" in settings || "graph.metrics" in settings;
+
+  if (hasExplicitGraphDataSettings) {
+    return settings;
+  }
+
+  const nextSettings = { ...settings };
+
+  const availableColumnNames = series.data.cols.map(col => col.name);
+  const maxDimensions = getMaxDimensionsSupported(series.card.display);
+  const maxMetrics = getMaxMetricsSupported(series.card.display);
+
+  const { dimensions: currentDimensions, metrics: currentMetrics } =
+    getSingleSeriesDimensionsAndMetrics(series, maxDimensions, maxMetrics);
+  const { dimensions: previousDimensions, metrics: previousMetrics } =
+    getSingleSeriesDimensionsAndMetrics(
+      previousSeries,
+      maxDimensions,
+      maxMetrics,
+    );
+
+  const dimensions =
+    currentDimensions.filter(isNotNull).length > 0
+      ? currentDimensions
+      : previousDimensions.filter((columnName: string) =>
+          availableColumnNames.includes(columnName),
+        );
+
+  const metrics =
+    currentMetrics.filter(isNotNull).length > 0
+      ? currentMetrics
+      : previousMetrics.filter((columnName: string) =>
+          availableColumnNames.includes(columnName),
+        );
+
+  if (dimensions.length > 0) {
+    nextSettings["graph.dimensions"] = dimensions;
+  }
+  if (metrics.length > 0) {
+    nextSettings["graph.metrics"] = metrics;
+  }
+
+  return nextSettings;
+}
+
 function syncTableColumnSettings(
   settings: VisualizationSettings,
-  { data }: Dataset,
+  { data }: SingleSeries,
 ): VisualizationSettings {
   // "table.columns" receive a value only if there are custom settings
   // e.g. some columns are hidden. If it's empty, it means everything is visible
@@ -77,8 +151,8 @@ function syncTableColumnSettings(
 
 function syncGraphMetricSettings(
   settings: VisualizationSettings,
-  { data: { cols } }: Dataset,
-  { data: { cols: prevCols } }: Dataset,
+  { data: { cols } }: SingleSeries,
+  { data: { cols: prevCols } }: SingleSeries,
 ): VisualizationSettings {
   const graphMetrics = settings["graph.metrics"];
   if (!graphMetrics) {

frontend/src/metabase/visualizations/lib/sync-settings.unit.spec.ts

@@ -2,12 +2,13 @@ import { getColumnKey } from "metabase-lib/v1/queries/utils/get-column-key";
 import type {
   DatasetColumn,
   TableColumnOrderSetting,
+  VisualizationSettings,
 } from "metabase-types/api";
-import { createMockDataset } from "metabase-types/api/mocks";
+import { createMockSingleSeries } from "metabase-types/api/mocks";
 
-import { syncVizSettingsWithQueryResults } from "./sync-settings";
+import { syncVizSettingsWithSeries } from "./sync-settings";
 
-const cols: DatasetColumn[] = [
+const columns: DatasetColumn[] = [
   {
     display_name: "num",
     source: "native",
@@ -36,47 +37,52 @@ const cols: DatasetColumn[] = [
   },
 ];
 
-const vizSettingColumns: TableColumnOrderSetting[] = cols.map(column => ({
+const vizSettingColumns: TableColumnOrderSetting[] = columns.map(column => ({
   key: getColumnKey(column),
   name: column.name,
   fieldRef: column.field_ref,
   enabled: true,
 }));
 
-const noVizSettings = {};
-const vizSettings = {
+const noVizSettings: VisualizationSettings = {};
+const vizSettings: VisualizationSettings = {
   "table.columns": vizSettingColumns,
 };
 
-describe("syncVizSettingsWithQueryResults", () => {
+function createSeries({
+  cols = columns,
+  settings = vizSettings,
+}: { cols?: DatasetColumn[]; settings?: VisualizationSettings } = {}) {
+  return [
+    createMockSingleSeries(
+      { visualization_settings: settings },
+      { data: { cols, rows: [] } },
+    ),
+  ];
+}
+
+describe("syncVizSettingsWithSeries", () => {
   describe("when 'table.columns' setting is not defined", () => {
     it("should do nothing when given empty query results", () => {
-      const queryResults = createMockDataset({
-        error: { status: 500 },
-        data: { cols: [], rows: [] },
-      });
-      const syncedSettings = syncVizSettingsWithQueryResults(
-        noVizSettings,
-        queryResults,
-      );
+      const series = [
+        createMockSingleSeries(
+          { visualization_settings: noVizSettings },
+          { error: { status: 500 }, data: { cols: [], rows: [] } },
+        ),
+      ];
+      const syncedSettings = syncVizSettingsWithSeries(noVizSettings, series);
       expect(syncedSettings).toEqual({});
     });
 
     it("should do nothing when given query results with no columns", () => {
-      const queryResults = createMockDataset({ data: { cols: [], rows: [] } });
-      const syncedSettings = syncVizSettingsWithQueryResults(
-        noVizSettings,
-        queryResults,
-      );
+      const series = createSeries({ settings: noVizSettings, cols: [] });
+      const syncedSettings = syncVizSettingsWithSeries(noVizSettings, series);
       expect(syncedSettings).toEqual({});
     });
 
     it("should do nothing when given query results with columns", () => {
-      const queryResults = createMockDataset({ data: { cols } });
-      const syncedSettings = syncVizSettingsWithQueryResults(
-        noVizSettings,
-        queryResults,
-      );
+      const series = createSeries({ settings: noVizSettings, cols: columns });
+      const syncedSettings = syncVizSettingsWithSeries(noVizSettings, series);
       expect(syncedSettings).toEqual({});
     });
   });
@@ -98,18 +104,13 @@ describe("syncVizSettingsWithQueryResults", () => {
         ],
         base_type: "type/Float",
       };
-      const queryResults = createMockDataset({
-        data: { cols: [...cols.slice(1), addedColumn] },
-      });
+      const series = createSeries({ cols: [...columns.slice(1), addedColumn] });
 
-      const syncedSettings = syncVizSettingsWithQueryResults(
-        vizSettings,
-        queryResults,
-      );
+      const syncedSettings = syncVizSettingsWithSeries(vizSettings, series);
 
       expect(syncedSettings).toEqual({
         "table.columns": [
-          ...vizSettings["table.columns"].slice(1),
+          ...vizSettingColumns.slice(1),
           {
             key: getColumnKey(addedColumn),
             name: addedColumn.name,
@@ -134,18 +135,15 @@ describe("syncVizSettingsWithQueryResults", () => {
         name: "foo",
         base_type: "type/Float",
       };
-      const queryResults = createMockDataset({
-        data: { cols: [updatedColumn, ...cols.slice(1)] },
+      const series = createSeries({
+        cols: [updatedColumn, ...columns.slice(1)],
       });
 
-      const syncedSettings = syncVizSettingsWithQueryResults(
-        vizSettings,
-        queryResults,
-      );
+      const syncedSettings = syncVizSettingsWithSeries(vizSettings, series);
 
       expect(syncedSettings).toEqual({
         "table.columns": [
-          ...vizSettings["table.columns"].slice(1),
+          ...vizSettingColumns.slice(1),
           {
             key: getColumnKey(updatedColumn),
             name: updatedColumn.name,
@@ -170,18 +168,15 @@ describe("syncVizSettingsWithQueryResults", () => {
         ],
         base_type: "type/Integer",
       };
-      const queryResults = createMockDataset({
-        data: { cols: [updatedColumn, ...cols.slice(1)] },
+      const series = createSeries({
+        cols: [updatedColumn, ...columns.slice(1)],
       });
 
-      const syncedSettings = syncVizSettingsWithQueryResults(
-        vizSettings,
-        queryResults,
-      );
+      const syncedSettings = syncVizSettingsWithSeries(vizSettings, series);
 
       expect(syncedSettings).toEqual({
         "table.columns": [
-          ...vizSettings["table.columns"].slice(1),
+          ...vizSettingColumns.slice(1),
           {
             key: getColumnKey(updatedColumn),
             name: updatedColumn.name,
@@ -193,15 +188,8 @@ describe("syncVizSettingsWithQueryResults", () => {
     });
 
     it("shouldn't update settings if order of columns has changed", () => {
-      const queryResults = createMockDataset({
-        data: { cols: [cols[1], cols[0]] },
-      });
-
-      const syncedSettings = syncVizSettingsWithQueryResults(
-        vizSettings,
-        queryResults,
-      );
-
+      const series = createSeries({ cols: [columns[1], columns[0]] });
+      const syncedSettings = syncVizSettingsWithSeries(vizSettings, series);
       expect(syncedSettings).toEqual(vizSettings);
     });
   });