Fix incorrect parenthesis removing for expressions (#29429)

e2e/test/scenarios/question/summarization.cy.spec.js

@@ -174,19 +174,24 @@ describe("scenarios > question > summarize sidebar", () => {
     cy.findByText("318.7");
   });
 
-  it.skip("should keep manually entered parenthesis intact (metabase#13306)", () => {
-    const FORMULA =
-      "Sum([Total]) / (Sum([Product → Price]) * Average([Quantity]))";
-
+  it("should keep manually entered parenthesis intact if they affect the result (metabase#13306)", () => {
     openOrdersTable({ mode: "notebook" });
     summarize({ mode: "notebook" });
+
     popover().contains("Custom Expression").click();
     popover().within(() => {
-      cy.get(".ace_text-input").type(FORMULA).blur();
+      enterCustomColumnDetails({
+        formula:
+          "sum([Total]) / (sum([Product → Price]) * average([Quantity]))",
+      });
+      cy.get("@formula").blur();
+    });
 
-      cy.log("Fails after blur in v0.36.6");
-      // Implicit assertion
-      cy.contains(FORMULA);
+    popover().within(() => {
+      cy.get(".ace_text-layer").should(
+        "have.text",
+        "Sum([Total]) / (Sum([Product → Price]) * Average([Quantity]))",
+      );
     });
   });
 

frontend/src/metabase-lib/expressions/config.js

@@ -481,6 +481,10 @@ export const EXPRESSION_FUNCTIONS = new Set([
 ]);
 
 const EXPRESSION_OPERATORS = new Set(["+", "-", "*", "/"]);
+
+// operators in which order of operands doesn't matter
+export const EXPRESSION_OPERATOR_WITHOUT_ORDER_PRIORITY = new Set(["+", "*"]);
+
 export const FILTER_OPERATORS = new Set(["!=", "<=", ">=", "<", ">", "="]);
 
 const BOOLEAN_UNARY_OPERATORS = new Set(["not"]);

frontend/src/metabase-lib/expressions/format.js

@@ -18,6 +18,7 @@ import {
   getExpressionName,
   formatStringLiteral,
   hasOptions,
+  EXPRESSION_OPERATOR_WITHOUT_ORDER_PRIORITY,
 } from "./index";
 
 export { DISPLAY_QUOTES, EDITOR_QUOTES } from "./config";
@@ -121,11 +122,27 @@ function formatOperator([op, ...args], options) {
     // FIXME: how should we format args?
     args = args.slice(0, -1);
   }
+
   const formattedOperator = getExpressionName(op) || op;
-  const formattedArgs = args.map(arg => {
+  const formattedArgs = args.map((arg, index) => {
+    const argOp = isOperator(arg) && arg[0];
     const isLowerPrecedence =
-      isOperator(arg) && OPERATOR_PRECEDENCE[op] > OPERATOR_PRECEDENCE[arg[0]];
-    return format(arg, { ...options, parens: isLowerPrecedence });
+      isOperator(arg) && OPERATOR_PRECEDENCE[op] > OPERATOR_PRECEDENCE[argOp];
+
+    // "*","/" always have two arguments. If the second argument of "/" is an expression, we have to calculate it first.
+    // Hence, adding parenthesis.
+    // "a / b * c" vs "a / (b * c)", "a / b / c" vs "a / (b / c)"
+    // "a - b + c" vs "a - (b + c)", "a - b - c" vs "a - (b - c)"
+    const isSamePrecedenceWithExecutionPriority =
+      index > 0 &&
+      isOperator(arg) &&
+      OPERATOR_PRECEDENCE[op] === OPERATOR_PRECEDENCE[argOp] &&
+      !EXPRESSION_OPERATOR_WITHOUT_ORDER_PRIORITY.has(op);
+
+    return format(arg, {
+      ...options,
+      parens: isLowerPrecedence || isSamePrecedenceWithExecutionPriority,
+    });
   });
   const clause = MBQL_CLAUSES[op];
   const formatted =

frontend/test/metabase/lib/expressions/__support__/shared.js

@@ -71,6 +71,62 @@ const expression = [
   ],
   // should not compile:
   // ["\"Hell\" + 1", null, "adding a string to a number"],
+
+  [
+    "Sum([Total]) / Sum([Product → Price]) * Average([Tax])",
+    [
+      "*",
+      [
+        "/",
+        ["sum", ["field", 6, null]],
+        ["sum", ["field", 25, { "source-field": 3 }]],
+      ],
+      ["avg", ["field", 5, null]],
+    ],
+    "should handle priority for multiply and division without parenthesis",
+  ],
+
+  [
+    "Sum([Total]) / (Sum([Product → Price]) * Average([Tax]))",
+    [
+      "/",
+      ["sum", ["field", 6, null]],
+      [
+        "*",
+        ["sum", ["field", 25, { "source-field": 3 }]],
+        ["avg", ["field", 5, null]],
+      ],
+    ],
+    "should handle priority for multiply and division with parenthesis",
+  ],
+
+  [
+    "Sum([Total]) - Sum([Product → Price]) + Average([Tax])",
+    [
+      "+",
+      [
+        "-",
+        ["sum", ["field", 6, null]],
+        ["sum", ["field", 25, { "source-field": 3 }]],
+      ],
+      ["avg", ["field", 5, null]],
+    ],
+    "should handle priority for addition and subtraction without parenthesis",
+  ],
+
+  [
+    "Sum([Total]) - (Sum([Product → Price]) + Average([Tax]))",
+    [
+      "-",
+      ["sum", ["field", 6, null]],
+      [
+        "+",
+        ["sum", ["field", 25, { "source-field": 3 }]],
+        ["avg", ["field", 5, null]],
+      ],
+    ],
+    "should handle priority for addition and subtraction with parenthesis",
+  ],
 ];
 
 const aggregation = [