Convert ExpressionStep to MLv2 (#36455)

e2e/test/scenarios/custom-column/reproductions/19745-cc-nested-query-remove-expressions.cy.spec.js

@@ -57,7 +57,9 @@ describe("issue 19745", () => {
     cy.signInAsAdmin();
   });
 
-  it("should unwrap the nested query when removing the last expression (metabase#19745)", () => {
+  // TODO: unskip when metabase#36574 is resolved
+  // @see https://metaboat.slack.com/archives/C04CYTEL9N2/p1702063378269379
+  it.skip("should unwrap the nested query when removing the last expression (metabase#19745)", () => {
     updateQuestionAndSelectFilter(() => removeExpression("Custom Column"));
   });
 

frontend/src/metabase-lib/test-helpers.ts

@@ -155,6 +155,13 @@ interface BreakoutClauseOpts {
   binningStrategyName?: string;
 }
 
+interface ExpressionClauseOpts {
+  name: string;
+  operator: Lib.ExpressionOperatorName;
+  args: (Lib.ExpressionArg | Lib.ExpressionClause)[];
+  options?: Lib.ExpressionOptions | null;
+}
+
 interface OrderByClauseOpts {
   columnName: string;
   tableName: string;
@@ -163,6 +170,7 @@ interface OrderByClauseOpts {
 
 interface QueryWithClausesOpts {
   query?: Lib.Query;
+  expressions?: ExpressionClauseOpts[];
   aggregations?: AggregationClauseOpts[];
   breakouts?: BreakoutClauseOpts[];
   orderBys?: OrderByClauseOpts[];
@@ -170,10 +178,24 @@ interface QueryWithClausesOpts {
 
 export function createQueryWithClauses({
   query = createQuery(),
+  expressions = [],
   aggregations = [],
   breakouts = [],
   orderBys = [],
 }: QueryWithClausesOpts) {
+  const queryWithExpressions = expressions.reduce((query, expression) => {
+    return Lib.expression(
+      query,
+      -1,
+      expression.name,
+      Lib.expressionClause(
+        expression.operator,
+        expression.args,
+        expression.options,
+      ),
+    );
+  }, query);
+
   const queryWithAggregations = aggregations.reduce((query, aggregation) => {
     return Lib.aggregate(
       query,
@@ -182,7 +204,7 @@ export function createQueryWithClauses({
         findAggregationOperator(query, aggregation.operatorName),
       ),
     );
-  }, query);
+  }, queryWithExpressions);
 
   const queryWithBreakouts = breakouts.reduce((query, breakout) => {
     const breakoutColumn = columnFinder(

frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.unit.spec.tsx

@@ -182,7 +182,7 @@ function setup({
     { storeInitialState: state },
   );
 
-  function getRecentClause() {
+  function getRecentClause(): Lib.Clause {
     expect(onSelect).toHaveBeenCalledWith(expect.anything());
     const [clause] = onSelect.mock.lastCall;
     return clause;

frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx

@@ -202,7 +202,7 @@ function setup(additionalProps?: Partial<ExpressionWidgetProps>) {
   const onChangeClause = jest.fn();
   const onClose = jest.fn();
 
-  function getRecentExpressionClause() {
+  function getRecentExpressionClause(): Lib.Clause {
     expect(onChangeClause).toHaveBeenCalled();
     const [_name, clause] = onChangeClause.mock.lastCall;
     return clause;

frontend/src/metabase/query_builder/components/notebook/NotebookStep/steps.ts

@@ -6,7 +6,7 @@ import { color } from "metabase/lib/colors";
 import type { IconName } from "metabase/core/components/Icon";
 import { DataStep } from "../steps/DataStep";
 import { JoinStep } from "../steps/JoinStep";
-import ExpressionStep from "../steps/ExpressionStep";
+import { ExpressionStep } from "../steps/ExpressionStep";
 import { FilterStep } from "../steps/FilterStep";
 import { AggregateStep } from "../steps/AggregateStep";
 import BreakoutStep from "../steps/BreakoutStep";

frontend/src/metabase/query_builder/components/notebook/steps/AggregateStep/AggregateStep.unit.spec.tsx

@@ -39,7 +39,7 @@ function setup(step = createMockNotebookStep()) {
     />,
   );
 
-  function getNextQuery() {
+  function getNextQuery(): Lib.Query {
     const [lastCall] = updateQuery.mock.calls.slice(-1);
     return lastCall[0];
   }

frontend/src/metabase/query_builder/components/notebook/steps/BreakoutStep/BreakoutStep.unit.spec.tsx

@@ -65,7 +65,7 @@ function setup(step = createMockNotebookStep()) {
     />,
   );
 
-  function getNextQuery() {
+  function getNextQuery(): Lib.Query {
     const [lastCall] = updateQuery.mock.calls.slice(-1);
     return lastCall[0];
   }

frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.tsx

 import { ExpressionWidget } from "metabase/query_builder/components/expressions/ExpressionWidget";
+import * as Lib from "metabase-lib";
+import { getUniqueExpressionName } from "metabase-lib/queries/utils/expression";
 
 import type { NotebookStepUiComponentProps } from "../types";
 import { ClauseStep } from "./ClauseStep";
 
-const ExpressionStep = ({
+export const ExpressionStep = ({
   color,
-  query,
   updateQuery,
   isLastOpened,
   reportTimezone,
   readOnly,
+  step,
 }: NotebookStepUiComponentProps): JSX.Element => {
-  const items = Object.entries(query.expressions()).map(
-    ([name, expression]) => ({ name, expression }),
-  );
+  const { topLevelQuery: query, query: legacyQuery, stageIndex } = step;
+  const expressions = Lib.expressions(query, stageIndex);
+
+  const renderExpressionName = (expression: Lib.ExpressionClause) =>
+    Lib.displayInfo(query, stageIndex, expression).longDisplayName;
 
   return (
     <ClauseStep
       color={color}
-      items={items}
-      renderName={({ name }) => name}
+      items={expressions}
+      renderName={renderExpressionName}
       readOnly={readOnly}
       renderPopover={({ item }) => (
         <ExpressionWidget
-          legacyQuery={query}
-          name={item?.name}
-          expression={item?.expression}
+          legacyQuery={legacyQuery}
+          query={query}
+          stageIndex={stageIndex}
+          name={
+            item
+              ? Lib.displayInfo(query, stageIndex, item).displayName
+              : undefined
+          }
+          clause={item}
           withName
-          onChangeExpression={(newName, newExpression) => {
-            item?.expression
-              ? updateQuery(
-                  query.updateExpression(newName, newExpression, item.name),
-                )
-              : updateQuery(query.addExpression(newName, newExpression));
+          onChangeClause={(name, clause) => {
+            const uniqueName = getUniqueClauseName(
+              query,
+              stageIndex,
+              item,
+              name,
+            );
+            const namedClause = Lib.withExpressionName(clause, uniqueName);
+            const isUpdate = item;
+
+            if (isUpdate) {
+              const nextQuery = Lib.replaceClause(
+                query,
+                stageIndex,
+                item,
+                namedClause,
+              );
+              updateQuery(nextQuery);
+            } else {
+              const nextQuery = Lib.expression(
+                query,
+                stageIndex,
+                uniqueName,
+                namedClause,
+              );
+              updateQuery(nextQuery);
+            }
           }}
           reportTimezone={reportTimezone}
         />
       )}
       isLastOpened={isLastOpened}
-      onRemove={({ name }) => updateQuery(query.removeExpression(name))}
+      onRemove={clause => {
+        const nextQuery = Lib.removeClause(query, stageIndex, clause);
+        updateQuery(nextQuery);
+      }}
       withLegacyPopover
     />
   );
 };
 
-// eslint-disable-next-line import/no-default-export -- deprecated usage
-export default ExpressionStep;
+const getUniqueClauseName = (
+  query: Lib.Query,
+  stageIndex: number,
+  clause: Lib.ExpressionClause | undefined,
+  name: string,
+) => {
+  const isUpdate = clause;
+  // exclude the current clause so that it can be updated without renaming
+  const queryWithoutCurrentClause = isUpdate
+    ? Lib.removeClause(query, stageIndex, clause)
+    : query;
+  const expressions = Lib.expressions(queryWithoutCurrentClause, stageIndex);
+  const expressionsObject = Object.fromEntries(
+    expressions.map(expression => [
+      Lib.displayInfo(query, stageIndex, expression).displayName,
+    ]),
+  );
+  const uniqueName = getUniqueExpressionName(expressionsObject, name);
+  return uniqueName;
+};

frontend/src/metabase/query_builder/components/notebook/steps/ExpressionStep.unit.spec.tsx

 import userEvent from "@testing-library/user-event";
-import { checkNotNull } from "metabase/lib/types";
 import { render, screen, within } from "__support__/ui";
-import { createMockEntitiesState } from "__support__/store";
-import { getMetadata } from "metabase/selectors/metadata";
 
-import type { Expression } from "metabase-types/api";
-import { createMockState } from "metabase-types/store/mocks";
-import {
-  createSampleDatabase,
-  ORDERS_ID,
-} from "metabase-types/api/mocks/presets";
+import * as Lib from "metabase-lib";
+import { createQuery, createQueryWithClauses } from "metabase-lib/test-helpers";
 
-import type { NotebookStepUiComponentProps } from "../types";
 import { createMockNotebookStep } from "../test-utils";
-import ExpressionStep from "./ExpressionStep";
+import { ExpressionStep } from "./ExpressionStep";
+
+interface SetupOpts {
+  query?: Lib.Query;
+}
+
+function setup({ query = createQuery() }: SetupOpts = {}) {
+  const updateQuery = jest.fn();
+
+  const step = createMockNotebookStep({
+    type: "expression",
+    topLevelQuery: query,
+  });
+
+  function getRecentQuery(): Lib.Query {
+    expect(updateQuery).toHaveBeenCalledWith(expect.anything());
+    const [recentQuery] = updateQuery.mock.lastCall;
+    return recentQuery;
+  }
+
+  render(
+    <ExpressionStep
+      step={step}
+      color="#93A1AB"
+      query={step.query}
+      topLevelQuery={step.topLevelQuery}
+      updateQuery={updateQuery}
+      isLastOpened={false}
+      reportTimezone="UTC"
+    />,
+  );
+
+  return { getRecentQuery };
+}
 
 describe("Notebook Editor > Expression Step", () => {
+  it("should handle adding expression", async () => {
+    const { getRecentQuery } = setup();
+
+    userEvent.click(screen.getByRole("img", { name: "add icon" }));
+
+    userEvent.type(screen.getByLabelText("Expression"), "1 + 1");
+    userEvent.type(screen.getByLabelText("Name"), "new expression{enter}");
+
+    const recentQuery = getRecentQuery();
+    const expressions = Lib.expressions(recentQuery, 0);
+    expect(expressions).toHaveLength(1);
+    expect(Lib.displayInfo(recentQuery, 0, expressions[0]).displayName).toBe(
+      "new expression",
+    );
+  });
+
   it("should handle updating existing expression", async () => {
-    const expression: Expression = ["abs", ["field", 17, null]];
-    const {
-      mocks: { addExpression, updateExpression, updateQuery },
-    } = setup(undefined, {
-      // add an existing custom column expression
-      "old name": expression,
+    const query = createQueryWithClauses({
+      expressions: [{ name: "old name", operator: "+", args: [1, 1] }],
     });
+    const { getRecentQuery } = setup({ query });
 
     userEvent.click(screen.getByText("old name"));
 
-    const nameField = await screen.findByPlaceholderText(
-      "Something nice and descriptive",
-    );
-
+    const nameField = screen.getByLabelText("Name");
     userEvent.clear(nameField);
     userEvent.type(nameField, "new name{enter}");
 
-    expect(updateExpression).toHaveBeenCalledTimes(1);
-    expect(updateExpression).toHaveBeenCalledWith(
+    const recentQuery = getRecentQuery();
+    const expressions = Lib.expressions(recentQuery, 0);
+    expect(expressions).toHaveLength(1);
+    expect(Lib.displayInfo(recentQuery, 0, expressions[0]).displayName).toBe(
       "new name",
-      expression,
-      "old name",
     );
-    expect(addExpression).toHaveBeenCalledTimes(0);
-    expect(updateQuery).toHaveBeenCalledTimes(1);
   });
 
   it("should handle removing existing expression", () => {
-    const expression: Expression = ["abs", ["field", 17, null]];
-    const {
-      mocks: { removeExpression },
-    } = setup(undefined, {
-      // add an existing custom column expression
-      "expr name": expression,
+    const query = createQueryWithClauses({
+      expressions: [{ name: "expression name", operator: "+", args: [1, 1] }],
     });
+    const { getRecentQuery } = setup({ query });
 
-    const expressionItem = screen.getByText("expr name");
+    const expressionItem = screen.getByText("expression name");
     const closeIcon = within(expressionItem).getByRole("img", {
-      name: `close icon`,
+      name: "close icon",
     });
 
     userEvent.click(closeIcon);
 
-    expect(removeExpression).toHaveBeenCalledTimes(1);
-    expect(removeExpression).toHaveBeenCalledWith("expr name");
+    expect(Lib.expressions(getRecentQuery(), 0)).toHaveLength(0);
   });
 });
-
-const createMockQueryForExpressions = (
-  expressions?: Record<string, Expression>,
-) => {
-  const state = createMockState({
-    entities: createMockEntitiesState({
-      databases: [createSampleDatabase()],
-    }),
-  });
-
-  const metadata = getMetadata(state);
-  let query = checkNotNull(metadata.table(ORDERS_ID)).query();
-
-  if (expressions) {
-    Object.entries(expressions).forEach(([name, expression]) => {
-      query = query.addExpression(name, expression);
-    });
-  }
-
-  return query;
-};
-
-function setup(
-  additionalProps?: Partial<NotebookStepUiComponentProps>,
-  expressions?: Record<string, Expression>,
-) {
-  const updateQuery = jest.fn();
-  const addExpression = jest.fn();
-  const updateExpression = jest.fn();
-  const removeExpression = jest.fn();
-
-  const query = createMockQueryForExpressions(expressions);
-
-  query.addExpression = addExpression;
-  query.updateExpression = updateExpression;
-  query.removeExpression = removeExpression;
-
-  const step = createMockNotebookStep({
-    type: "expression",
-    query,
-  });
-
-  render(
-    <ExpressionStep
-      step={step}
-      color="#93A1AB"
-      query={query}
-      topLevelQuery={step.topLevelQuery}
-      updateQuery={updateQuery}
-      isLastOpened={false}
-      reportTimezone="UTC"
-      {...additionalProps}
-    />,
-  );
-
-  return {
-    mocks: { addExpression, updateExpression, removeExpression, updateQuery },
-  };
-}

frontend/src/metabase/query_builder/components/notebook/steps/JoinStep/JoinStep.unit.spec.tsx

@@ -150,7 +150,7 @@ function setup(step = createMockNotebookStep(), { readOnly = false } = {}) {
     storeInitialState: STATE,
   });
 
-  function getNextQuery() {
+  function getNextQuery(): Lib.Query {
     const [lastCall] = updateQuery.mock.calls.slice(-1);
     return lastCall[0];
   }