diff --git a/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js b/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js index 5f9ad9ac26707ac5302584274b58833675253390..e8fa1c6821de242d2c3bb312cae4efe26b3046b3 100644 --- a/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js +++ b/e2e/test/scenarios/question-reproductions/reproductions-3.cy.spec.js @@ -798,7 +798,7 @@ describe("issue 40064", () => { getNotebookStep("expression").findByText("Tax3").click(); enterCustomColumnDetails({ formula: "[Tax3] * 3", name: "Tax3" }); popover().within(() => { - cy.findByText("Unknown Field: Tax3").should("be.visible"); + cy.findByText("Cycle detected: Tax3 → Tax3").should("be.visible"); cy.button("Update").should("be.disabled"); }); }); diff --git a/package.json b/package.json index 871d1d927caf74f59dfdd9c68f0aaa13ad761fcf..1feb6a92e219a2d7f3f0fee27f63c9fd3a1d4563 100644 --- a/package.json +++ b/package.json @@ -372,7 +372,7 @@ "remove-webpack-cache": "rm -rf ./node_modules/.cache", "storybook": "yarn build:cljs && start-storybook -p 6006", "test": "yarn test-unit && yarn test-timezones && yarn test-cypress", - "test-cljs": "yarn && shadow-cljs compile test && node target/node-tests.js", + "test-cljs": "yarn && shadow-cljs compile test", "test-cypress": "yarn build && ./bin/build-for-test && yarn test-cypress-run", "test-cypress-open": "./bin/build-for-test && CYPRESS_FE_HEALTHCHECK=false yarn test-cypress-run --e2e --open", "test-cypress-open-no-backend": "E2E_HOST='http://localhost:3000' yarn test-cypress-run --e2e --open", diff --git a/src/metabase/lib/expression.cljc b/src/metabase/lib/expression.cljc index 2df9413671873d943f47d4e9c302397de2f24ac3..ac29608a90d796e1a085441c6fc5941ea9dfc846 100644 --- a/src/metabase/lib/expression.cljc +++ b/src/metabase/lib/expression.cljc @@ -331,7 +331,7 @@ Pass nil to `expression-position` for new expressions. The rules for determining which columns can be broken out by are as follows: - 1. custom `:expressions` in this stage of the query, that come before the `expression-position` + 1. Custom `:expressions` in this stage of the query` 2. Fields 'exported' by the previous stage of the query, if there is one; otherwise Fields from the current `:source-table` @@ -346,19 +346,21 @@ ([query :- ::lib.schema/query stage-number :- :int - expression-position :- [:maybe ::lib.schema.common/int-greater-than-or-equal-to-zero]] - (let [indexed-expressions (into {} (map-indexed (fn [idx expr] - [(lib.util/expression-name expr) idx]) - (expressions query stage-number))) - unavailable-expressions (fn [column] - (or (not expression-position) - (not= (:lib/source column) :source/expressions) - (< (get indexed-expressions (:name column)) expression-position))) - stage (lib.util/query-stage query stage-number) + ;; The legacy format, which uses a map to represent the expressions loses the ordering, + ;; if ten or more expressions are used. Preserving the order would require to use a + ;; map type preserving the order both when converting to the legacy format and when + ;; converting from JS to CLJ. This could be done by changing the legacy format or + ;; using flatland.ordered.map/ordered-map or array-map or something similar. + ;; Unfortunately, ordered-map doesn't implement IEditableCollection in CLJS, which means + ;; that some functions (e.g., update-keys, update-vals) unexpectedly convert them to a + ;; potentially unordered map. (One might even forget that select-keys returns a "normal" + ;; Clojure map, so there are plenty of possibilities to mess this up.) + ;; Changing the legacy/wire format is probably the right way to go, but that's a bigger + ;; endeavor. + _expression-position :- [:maybe ::lib.schema.common/int-greater-than-or-equal-to-zero]] + (let [stage (lib.util/query-stage query stage-number) columns (lib.metadata.calculation/visible-columns query stage-number stage)] - (->> columns - (filterv unavailable-expressions) - not-empty)))) + (not-empty columns)))) (mu/defn expression-ref :- :mbql.clause/expression "Find the expression with `expression-name` using [[resolve-expression]], then create a ref for it. Intended for use @@ -441,7 +443,7 @@ (cyclic-definition node->children start [])) ([node->children node path] (if (some #{node} path) - (conj path node) + (drop-while (complement #{node}) (conj path node)) (some #(cyclic-definition node->children % (conj path node)) (node->children node))))) diff --git a/test/metabase/lib/expression_test.cljc b/test/metabase/lib/expression_test.cljc index b42beed759692bde6e617d3bcf002e41a4b19822..5db01523c1f8cb938b34ead5a501b5e5193b674c 100644 --- a/test/metabase/lib/expression_test.cljc +++ b/test/metabase/lib/expression_test.cljc @@ -296,8 +296,10 @@ expressionable-expressions-for-position (fn [pos] (some->> (lib/expressionable-columns query pos) (map :lib/desired-column-alias)))] - (is (= ["ID" "NAME"] (expressionable-expressions-for-position 0))) - (is (= ["ID" "NAME" "a"] (expressionable-expressions-for-position 1))) + ;; Because of (the second problem in) #44584, the expression-position argument is ignored, + ;; so the first two calls behave the same as the last two. + (is (= ["ID" "NAME" "a" "b"] (expressionable-expressions-for-position 0))) + (is (= ["ID" "NAME" "a" "b"] (expressionable-expressions-for-position 1))) (is (= ["ID" "NAME" "a" "b"] (expressionable-expressions-for-position nil))) (is (= ["ID" "NAME" "a" "b"] (expressionable-expressions-for-position 2))) (is (= (lib/visible-columns query)