diff --git a/frontend/src/metabase-lib/lib/queries/StructuredQuery.js b/frontend/src/metabase-lib/lib/queries/StructuredQuery.js
index fc467a71254ec6008f93c25c617adf930b34d1aa..f90ca4cdd2e8466402ae8fc1e5a198ebe1c0a63f 100644
--- a/frontend/src/metabase-lib/lib/queries/StructuredQuery.js
+++ b/frontend/src/metabase-lib/lib/queries/StructuredQuery.js
@@ -3,6 +3,7 @@
*/
import * as Q from "metabase/lib/query/query";
+import { getUniqueExpressionName } from "metabase/lib/query/expression";
import {
format as formatExpression,
DISPLAY_QUOTES,
@@ -999,23 +1000,32 @@ export default class StructuredQuery extends AtomicQuery {
}
addExpression(name, expression) {
- let query = this._updateQuery(Q.addExpression, arguments);
+ const uniqueName = getUniqueExpressionName(this.expressions(), name);
+ let query = this._updateQuery(Q.addExpression, [uniqueName, expression]);
// extra logic for adding expressions in fields clause
// TODO: push into query/expression?
if (query.hasFields() && query.isRaw()) {
- query = query.addField(["expression", name]);
+ query = query.addField(["expression", uniqueName]);
}
return query;
}
updateExpression(name, expression, oldName) {
- let query = this._updateQuery(Q.updateExpression, arguments);
+ const isRename = oldName && oldName !== name;
+ const uniqueName = isRename
+ ? getUniqueExpressionName(this.expressions(), name)
+ : name;
+ let query = this._updateQuery(Q.updateExpression, [
+ uniqueName,
+ expression,
+ oldName,
+ ]);
// extra logic for renaming expressions in fields clause
// TODO: push into query/expression?
- if (name !== oldName) {
+ if (isRename) {
const index = query._indexOfField(["expression", oldName]);
if (index >= 0) {
- query = query.updateField(index, ["expression", name]);
+ query = query.updateField(index, ["expression", uniqueName]);
}
}
return query;
diff --git a/frontend/src/metabase/lib/query/expression.js b/frontend/src/metabase/lib/query/expression.js
index adfdda1917abc2d6f2363d65277090ca3f1649ce..a4f21c0fb3ea3e81c5488f6a419ec76c675536f7 100644
--- a/frontend/src/metabase/lib/query/expression.js
+++ b/frontend/src/metabase/lib/query/expression.js
@@ -50,3 +50,39 @@ export function clearExpressions(
): ?ExpressionClause {
return {};
}
+
+/**
+ * Ensures expression's name uniqueness
+ *
+ * Example: if query has a "Double Total" expression,
+ * and we're adding a new "Double Total" expression,
+ * the second expression will be called "Double Total (1)",
+ * the next one will be "Double Total (2)" and so on
+ *
+ * If the original name is already unique, the fn just returns it
+ *
+ * @param {string} originalName - expression's name
+ * @param {object} expressions - object with existing query expressions
+ * @returns {string}
+ */
+export function getUniqueExpressionName(expressions, originalName) {
+ if (!expressions[originalName]) {
+ return originalName;
+ }
+ const expressionNames = Object.keys(expressions);
+ const handledDuplicateNamePattern = new RegExp(
+ `^${originalName} \\([0-9]+\\)$`,
+ );
+ const duplicateNames = expressionNames.filter(
+ name => name === originalName || handledDuplicateNamePattern.test(name),
+ );
+ return getUniqueName(duplicateNames, originalName, duplicateNames.length);
+}
+
+function getUniqueName(expressionNames, originalName, index) {
+ const nameWithIndexAppended = `${originalName} (${index})`;
+ const isUnique = !expressionNames.includes(nameWithIndexAppended);
+ return isUnique
+ ? nameWithIndexAppended
+ : getUniqueName(expressionNames, originalName, index + 1);
+}
diff --git a/frontend/test/__support__/e2e/cypress.js b/frontend/test/__support__/e2e/cypress.js
index 480bfa1ac91b63e60ddad7d14c9bf6d578ff6728..a99b3b12f74e52bdb65c77497e0437a16febacfc 100644
--- a/frontend/test/__support__/e2e/cypress.js
+++ b/frontend/test/__support__/e2e/cypress.js
@@ -17,6 +17,7 @@ export * from "./helpers/e2e-qa-databases-helpers";
export * from "./helpers/e2e-ad-hoc-question-helpers";
export * from "./helpers/e2e-enterprise-helpers";
export * from "./helpers/e2e-mock-app-settings-helpers";
+export * from "./helpers/e2e-notebook-helpers";
export * from "./helpers/e2e-assertion-helpers";
export * from "./helpers/e2e-cloud-helpers";
export * from "./helpers/e2e-data-model-helpers";
diff --git a/frontend/test/__support__/e2e/helpers/e2e-notebook-helpers.js b/frontend/test/__support__/e2e/helpers/e2e-notebook-helpers.js
new file mode 100644
index 0000000000000000000000000000000000000000..2509ab2342dfdb9e81be36e8c84c80cc585f044a
--- /dev/null
+++ b/frontend/test/__support__/e2e/helpers/e2e-notebook-helpers.js
@@ -0,0 +1,13 @@
+/**
+ * Helps to select specific notebook steps like filters, joins, break outs, etc.
+ *
+ * Details:
+ * https://github.com/metabase/metabase/pull/17708#discussion_r700082403
+ *
+ * @param {string} type — notebook step type (filter, join, expression, summarize, etc.)
+ * @param {{stage: number; index: number}} positionConfig - indexes specifying step's position
+ * @returns
+ */
+export function getNotebookStep(type, { stage = 0, index = 0 } = {}) {
+ return cy.findByTestId(`step-${type}-${stage}-${index}`);
+}
diff --git a/frontend/test/metabase-lib/lib/queries/StructuredQuery-expressions.unit.spec.js b/frontend/test/metabase-lib/lib/queries/StructuredQuery-expressions.unit.spec.js
index 62308661125f54115e40ce83dbe817770b75b182..a8331517a8306af6d861c7354ddff7be882ddd1f 100644
--- a/frontend/test/metabase-lib/lib/queries/StructuredQuery-expressions.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/queries/StructuredQuery-expressions.unit.spec.js
@@ -1,18 +1,251 @@
-import { ORDERS } from "__support__/sample_dataset_fixture";
+import { ORDERS, SAMPLE_DATASET } from "__support__/sample_dataset_fixture";
+import Question from "metabase-lib/lib/Question";
+
+const TEST_EXPRESSION = ["+", 1, 1];
+const TEST_EXPRESSION_2 = ["+", 2, 2];
+const TEST_EXPRESSION_3 = ["+", 3, 3];
+
+function getQuery({ expressions } = {}) {
+ const question = new Question({
+ dataset_query: {
+ type: "query",
+ database: SAMPLE_DATASET.id,
+ query: {
+ "source-table": ORDERS.id,
+ expressions: expressions,
+ },
+ },
+ });
+ return question.query();
+}
describe("StructuredQuery", () => {
+ describe("expressions", () => {
+ it("should return empty object when there are no expressions", () => {
+ expect(getQuery().expressions()).toEqual({});
+ });
+
+ it("should return expressions object", () => {
+ const query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ diff: TEST_EXPRESSION_2,
+ },
+ });
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ diff: TEST_EXPRESSION_2,
+ });
+ });
+ });
+
describe("hasExpressions", () => {
it("should return false for queries without expressions", () => {
- const q = ORDERS.query();
- expect(q.hasExpressions()).toBe(false);
+ const query = getQuery();
+ expect(query.hasExpressions()).toBe(false);
});
+
it("should return true for queries with expressions", () => {
- const q = ORDERS.query().addExpression("double_total", [
- "*",
- ["field", ORDERS.TOTAL.id, null],
- 2,
- ]);
- expect(q.hasExpressions()).toBe(true);
+ const query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ },
+ });
+ expect(query.hasExpressions()).toBe(true);
+ });
+ });
+
+ describe("addExpression", () => {
+ it("should add new expressions correctly", () => {
+ let query = getQuery();
+
+ query = query.addExpression("double_total", TEST_EXPRESSION);
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ });
+ });
+
+ it("should append indexes to duplicate names", () => {
+ let query = getQuery({
+ expressions: { double_total: TEST_EXPRESSION },
+ });
+
+ query = query
+ .addExpression("double_total", TEST_EXPRESSION)
+ .addExpression("double_total", TEST_EXPRESSION);
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ "double_total (1)": TEST_EXPRESSION,
+ "double_total (2)": TEST_EXPRESSION,
+ });
+ });
+
+ it("should handle gaps in names with indexes appended", () => {
+ let query = getQuery({
+ expressions: { double_total: TEST_EXPRESSION },
+ });
+
+ query = query
+ .addExpression("double_total", TEST_EXPRESSION)
+ .addExpression("double_total", TEST_EXPRESSION_2)
+ .removeExpression("double_total (1)", TEST_EXPRESSION)
+ .addExpression("double_total", TEST_EXPRESSION);
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ "double_total (2)": TEST_EXPRESSION_2,
+ "double_total (3)": TEST_EXPRESSION,
+ });
+ });
+
+ it("should detect duplicate names in case-sensitive way", () => {
+ let query = getQuery({
+ expressions: { double_total: TEST_EXPRESSION },
+ });
+
+ query = query.addExpression("DOUBLE_TOTAL", TEST_EXPRESSION);
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ DOUBLE_TOTAL: TEST_EXPRESSION,
+ });
+ });
+
+ it("should not append indexes when expression names are similar", () => {
+ let query = getQuery({
+ expressions: { double_total: TEST_EXPRESSION },
+ });
+
+ query = query
+ .addExpression("double_total", TEST_EXPRESSION)
+ .addExpression("double", TEST_EXPRESSION)
+ .addExpression("total", TEST_EXPRESSION)
+ .addExpression("double_total", TEST_EXPRESSION)
+ .addExpression("foo_double_total", TEST_EXPRESSION)
+ .addExpression("double_total_bar", TEST_EXPRESSION)
+ .addExpression("double_total", TEST_EXPRESSION);
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ "double_total (1)": TEST_EXPRESSION,
+ "double_total (2)": TEST_EXPRESSION,
+ "double_total (3)": TEST_EXPRESSION,
+ double: TEST_EXPRESSION,
+ total: TEST_EXPRESSION,
+ foo_double_total: TEST_EXPRESSION,
+ double_total_bar: TEST_EXPRESSION,
+ });
+ });
+ });
+
+ describe("updateExpression", () => {
+ it("should update expressions correctly", () => {
+ let query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ },
+ });
+
+ query = query.updateExpression("double_total", TEST_EXPRESSION_2);
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION_2,
+ });
+ });
+
+ it("should update expression names correctly", () => {
+ let query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ },
+ });
+
+ query = query.updateExpression(
+ "Double Total",
+ TEST_EXPRESSION,
+ "double_total",
+ );
+
+ expect(query.expressions()).toEqual({
+ "Double Total": TEST_EXPRESSION,
+ });
+ });
+
+ it("should handle duplicate expression names", () => {
+ let query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ "double_total (1)": TEST_EXPRESSION_2,
+ "double_total (2)": TEST_EXPRESSION_3,
+ },
+ });
+
+ query = query.updateExpression(
+ "double_total",
+ TEST_EXPRESSION_3,
+ "double_total (2)",
+ );
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ "double_total (1)": TEST_EXPRESSION_2,
+ "double_total (3)": TEST_EXPRESSION_3,
+ });
+ });
+ });
+
+ describe("removeExpression", () => {
+ it("should remove expression correctly", () => {
+ let query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ diff: TEST_EXPRESSION_2,
+ },
+ });
+
+ query = query.removeExpression("double_total");
+
+ expect(query.expressions()).toEqual({
+ diff: TEST_EXPRESSION_2,
+ });
+ });
+
+ it("should do nothing when removing not existing expression", () => {
+ let query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ },
+ });
+
+ query = query.removeExpression("this_query_has_no_expression_like_that");
+
+ expect(query.expressions()).toEqual({
+ double_total: TEST_EXPRESSION,
+ });
+ });
+ });
+
+ describe("clearExpressions", () => {
+ it("should remove all existing expressions", () => {
+ let query = getQuery({
+ expressions: {
+ double_total: TEST_EXPRESSION,
+ diff: TEST_EXPRESSION_2,
+ },
+ });
+
+ query = query.clearExpressions();
+
+ expect(query.expressions()).toEqual({});
+ });
+
+ it("should do nothing if there are no expressions", () => {
+ let query = getQuery();
+ query = query.clearExpressions();
+ expect(query.expressions()).toEqual({});
});
});
});
diff --git a/frontend/test/metabase/scenarios/question/notebook.cy.spec.js b/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
index fc4034256cb550fce196287efee74618be4f3448..b4a073b526b72f0116063ddb7a7dac20da53db55 100644
--- a/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
@@ -7,6 +7,7 @@ import {
modal,
visitQuestionAdhoc,
interceptPromise,
+ getNotebookStep,
} from "__support__/e2e/cypress";
import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
@@ -133,6 +134,37 @@ describe("scenarios > question > notebook", () => {
});
});
+ it("should append indexes to duplicate custom expression names (metabase#12104)", () => {
+ cy.intercept("POST", "/api/dataset").as("dataset");
+ openProductsTable({ mode: "notebook" });
+
+ cy.findByText("Custom column").click();
+ addSimpleCustomColumn("EXPR");
+
+ getNotebookStep("expression").within(() => {
+ cy.icon("add").click();
+ });
+ addSimpleCustomColumn("EXPR");
+
+ getNotebookStep("expression").within(() => {
+ cy.icon("add").click();
+ });
+ addSimpleCustomColumn("EXPR");
+
+ getNotebookStep("expression").within(() => {
+ cy.findByText("EXPR");
+ cy.findByText("EXPR (1)");
+ cy.findByText("EXPR (2)");
+ });
+
+ cy.button("Visualize").click();
+ cy.wait("@dataset");
+
+ cy.findByText("EXPR");
+ cy.findByText("EXPR (1)");
+ cy.findByText("EXPR (2)");
+ });
+
it("should process the updated expression when pressing Enter", () => {
openProductsTable({ mode: "notebook" });
cy.findByText("Filter").click();
@@ -1022,3 +1054,13 @@ function joinTwoSavedQuestions() {
});
});
}
+
+function addSimpleCustomColumn(name) {
+ popover()
+ .findByText("Category")
+ .click();
+ cy.findByPlaceholderText("Something nice and descriptive")
+ .click()
+ .type(name);
+ cy.button("Done").click();
+}