diff --git a/frontend/src/metabase-lib/expressions/diagnostics.ts b/frontend/src/metabase-lib/expressions/diagnostics.ts index e4f71b77543cc234580f53e60b879b9774a07619..05887f1bf712f8d84b5f0bbd28c330f9ba2e6dd9 100644 --- a/frontend/src/metabase-lib/expressions/diagnostics.ts +++ b/frontend/src/metabase-lib/expressions/diagnostics.ts @@ -137,14 +137,14 @@ function prattCompiler({ throw new ResolverError(t`Unknown Metric: ${name}`, node); } - return Lib.legacyRef(metric); + return Lib.legacyRef(query, stageIndex, metric); } else if (kind === "segment") { const segment = parseSegment(name, options); if (!segment) { throw new ResolverError(t`Unknown Segment: ${name}`, node); } - return Lib.legacyRef(segment); + return Lib.legacyRef(query, stageIndex, segment); } else { const reference = options.name ?? ""; // avoid circular reference @@ -154,7 +154,7 @@ function prattCompiler({ throw new ResolverError(t`Unknown Field: ${name}`, node); } - return Lib.legacyRef(dimension); + return Lib.legacyRef(query, stageIndex, dimension); } } diff --git a/frontend/src/metabase-lib/expressions/format.ts b/frontend/src/metabase-lib/expressions/format.ts index b9f0f5bc818c259cae421101bb5be9cbb86c5d9f..7e6432b3e4a516db131858dc814affd5e788bfbe 100644 --- a/frontend/src/metabase-lib/expressions/format.ts +++ b/frontend/src/metabase-lib/expressions/format.ts @@ -135,7 +135,7 @@ function formatMetric([, metricId]: FieldReference, options: Options) { } const metric = Lib.availableMetrics(query).find(metric => { - const [_, availableMetricId] = Lib.legacyRef(metric); + const [_, availableMetricId] = Lib.legacyRef(query, stageIndex, metric); return availableMetricId === metricId; }); @@ -177,7 +177,7 @@ function formatSegment([, segmentId]: FieldReference, options: Options) { } const segment = Lib.availableSegments(query, stageIndex).find(segment => { - const [_, availableSegmentId] = Lib.legacyRef(segment); + const [_, availableSegmentId] = Lib.legacyRef(query, stageIndex, segment); return availableSegmentId === segmentId; }); diff --git a/frontend/src/metabase-lib/expressions/process.ts b/frontend/src/metabase-lib/expressions/process.ts index 5f9798aa490a06506de812db1ca5e6311a3217e8..67b3f0961baf5d2f06fc9b7366319a044671b8a6 100644 --- a/frontend/src/metabase-lib/expressions/process.ts +++ b/frontend/src/metabase-lib/expressions/process.ts @@ -21,14 +21,14 @@ export function processSource(options: { throw new Error(t`Unknown Metric: ${name}`); } - return Lib.legacyRef(metric); + return Lib.legacyRef(query, stageIndex, metric); } else if (kind === "segment") { const segment = parseSegment(name, options); if (!segment) { throw new Error(t`Unknown Segment: ${name}`); } - return Lib.legacyRef(segment); + return Lib.legacyRef(query, stageIndex, segment); } else { const reference = options.name ?? ""; // avoid circular reference @@ -38,7 +38,7 @@ export function processSource(options: { throw new Error(t`Unknown Field: ${name}`); } - return Lib.legacyRef(dimension); + return Lib.legacyRef(query, stageIndex, dimension); } }; diff --git a/frontend/src/metabase-lib/fields.ts b/frontend/src/metabase-lib/fields.ts index 131195076f9acb015bd8e8cf255c05b4a2137eb8..4df1f4637badc5051df1eece2d9e0eb759f847fe 100644 --- a/frontend/src/metabase-lib/fields.ts +++ b/frontend/src/metabase-lib/fields.ts @@ -52,7 +52,9 @@ export function fieldValuesSearchInfo( } export function legacyRef( + query: Query, + stageIndex: number, column: ColumnMetadata | MetricMetadata | SegmentMetadata, ): FieldReference { - return ML.legacy_ref(column); + return ML.legacy_ref(query, stageIndex, column); } diff --git a/frontend/src/metabase-lib/parameters/utils/click-behavior.ts b/frontend/src/metabase-lib/parameters/utils/click-behavior.ts index 87099d8b072c12e1f523a5d613542b7cbce71e6b..31827b4307d0bd95ed1ccd3fed8608af5e560b01 100644 --- a/frontend/src/metabase-lib/parameters/utils/click-behavior.ts +++ b/frontend/src/metabase-lib/parameters/utils/click-behavior.ts @@ -149,7 +149,7 @@ function getTargetsForStructuredQuestion(question: Question): Target[] { return visibleColumns.map(targetColumn => { const dimension: ClickBehaviorDimensionTarget["dimension"] = [ "dimension", - Lib.legacyRef(targetColumn), + Lib.legacyRef(query, stageIndex, targetColumn), ]; const id = JSON.stringify(dimension); const target: ClickBehaviorTarget = { type: "dimension", id, dimension }; diff --git a/frontend/src/metabase-lib/parameters/utils/mbql.js b/frontend/src/metabase-lib/parameters/utils/mbql.js index 2f5bc9200e84b1518a8c4a7940565412d2b435f6..25b219e4228e75da4a86f02a6b4ff87498f44e38 100644 --- a/frontend/src/metabase-lib/parameters/utils/mbql.js +++ b/frontend/src/metabase-lib/parameters/utils/mbql.js @@ -195,7 +195,7 @@ function fieldFilterParameterToMBQL(query, stageIndex, parameter) { } const column = columns[columnIndex]; - const fieldRef = Lib.legacyRef(column); + const fieldRef = Lib.legacyRef(query, stageIndex, column); if (isDateParameter(parameter)) { return dateParameterValueToMBQL(parameter.value, fieldRef); diff --git a/frontend/src/metabase-lib/parameters/utils/targets.ts b/frontend/src/metabase-lib/parameters/utils/targets.ts index 1026b780e07de823350f40c66c5b28f9fe61b02d..1b8be802e91be369c2bb67edc812ad667be6b763 100644 --- a/frontend/src/metabase-lib/parameters/utils/targets.ts +++ b/frontend/src/metabase-lib/parameters/utils/targets.ts @@ -42,8 +42,12 @@ export function buildDimensionTarget(dimension: Dimension) { return ["dimension", dimension.mbql()]; } -export function buildColumnTarget(column: Lib.ColumnMetadata) { - return ["dimension", Lib.legacyRef(column)]; +export function buildColumnTarget( + query: Lib.Query, + stageIndex: number, + column: Lib.ColumnMetadata, +) { + return ["dimension", Lib.legacyRef(query, stageIndex, column)]; } export function buildTemplateTagVariableTarget(variable: TemplateTagVariable) { diff --git a/frontend/src/metabase/parameters/utils/mapping-options.js b/frontend/src/metabase/parameters/utils/mapping-options.js index 48d6535ab27a4d321ad514b9cc8b46129b63e55c..8a58f72637b6661869a01c53754a6f6f8bce709b 100644 --- a/frontend/src/metabase/parameters/utils/mapping-options.js +++ b/frontend/src/metabase/parameters/utils/mapping-options.js @@ -29,7 +29,7 @@ function buildStructuredQuerySectionOptions(query, stageIndex, group) { sectionName: getColumnGroupName(groupInfo), name: columnInfo.displayName, icon: getColumnIcon(column), - target: buildColumnTarget(column), + target: buildColumnTarget(query, stageIndex, column), isForeign: columnInfo.isFromJoin || columnInfo.isImplicitlyJoinable, }; }); diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts b/frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts index 99a5a392e7e3374c991699d34bb7b7f93866f2b8..53b43b51055528637bafcf166aa480563f85e443 100644 --- a/frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts +++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts @@ -224,7 +224,7 @@ export const addColumnInSettings = ( if (settingIndex >= 0) { newSettings[settingIndex] = { ...newSettings[settingIndex], enabled: true }; } else { - const fieldRef = Lib.legacyRef(column); + const fieldRef = Lib.legacyRef(query, STAGE_INDEX, column); newSettings.push({ name, fieldRef, enabled: true }); } diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc index ba442764be75d810993c2e5319302f3f59a88587..e7b22e2c5fc8236e24ca80af9ba183d1453ab07b 100644 --- a/src/metabase/lib/convert.cljc +++ b/src/metabase/lib/convert.cljc @@ -407,9 +407,11 @@ (defmethod ->legacy-MBQL :aggregation [[_ opts agg-uuid :as ag]] (if (map? opts) (try - (let [opts (options->legacy-MBQL opts)] - (cond-> [:aggregation (get-or-throw! *pMBQL-uuid->legacy-index* agg-uuid)] - opts (conj opts))) + (let [opts (options->legacy-MBQL opts) + base-agg [:aggregation (get-or-throw! *pMBQL-uuid->legacy-index* agg-uuid)]] + (if (seq opts) + [:aggregation-options base-agg opts] + base-agg)) (catch #?(:clj Throwable :cljs :default) e (throw (ex-info (lib.util/format "Error converting aggregation reference to pMBQL: %s" (ex-message e)) {:ref ag} diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index acfba4e1d9d266bb69a76160d3adef8b92122cc3..ca0a2a1bb53930b3a68f799c5f7082d907ad2994 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -663,7 +663,7 @@ (defn- normalize-legacy-ref [a-ref] - (if (#{:metric :segment} (first a-ref)) + (if (#{:aggregation :metric :segment} (first a-ref)) (subvec a-ref 0 2) (update a-ref 2 update-vals #(if (qualified-keyword? %) (u/qualified-name %) @@ -673,12 +673,13 @@ "Given a column, metric or segment metadata from eg. [[fieldable-columns]] or [[available-segments]], return it as a legacy JSON field ref. For compatibility reasons, segment and metric references are always returned without options." - [column] - (-> column - lib.core/ref - lib.convert/->legacy-MBQL - normalize-legacy-ref - clj->js)) + [a-query stage-number column] + (lib.convert/with-aggregation-list (:aggregation (lib.util/query-stage a-query stage-number)) + (-> column + lib.core/ref + lib.convert/->legacy-MBQL + normalize-legacy-ref + clj->js))) (defn- legacy-ref->pMBQL [a-legacy-ref] (-> a-legacy-ref diff --git a/test/metabase/lib/convert_test.cljc b/test/metabase/lib/convert_test.cljc index 65dc32a2dddff7a02fb29f8e1d860dc3f6b96c57..b37f0a7f2f83b829631ff5feee8c4d5b54ced1b4 100644 --- a/test/metabase/lib/convert_test.cljc +++ b/test/metabase/lib/convert_test.cljc @@ -205,19 +205,19 @@ (is (=? {:type :query :query {:source-table 1 :aggregation [[:sum [:field 1 nil]]] - :breakout [[:aggregation 0 {:display-name "Revenue"}]]}} + :breakout [[:aggregation-options [:aggregation 0] {:display-name "Revenue"}]]}} (let [ag-uuid (str (random-uuid))] (lib.convert/->legacy-MBQL - {:lib/type :mbql/query - :stages [{:lib/type :mbql.stage/mbql - :source-table 1 - :aggregation [[:sum {:lib/uuid ag-uuid} - [:field {:lib/uuid string? - :effective-type :type/Integer} 1]]] - :breakout [[:aggregation - {:display-name "Revenue" - :effective-type :type/Integer} - ag-uuid]]}]})))))) + {:lib/type :mbql/query + :stages [{:lib/type :mbql.stage/mbql + :source-table 1 + :aggregation [[:sum {:lib/uuid ag-uuid} + [:field {:lib/uuid string? + :effective-type :type/Integer} 1]]] + :breakout [[:aggregation + {:display-name "Revenue" + :effective-type :type/Integer} + ag-uuid]]}]})))))) (deftest ^:parallel source-card-test (let [original {:database 1 diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs index 3b367eecdb43fbe792daa4ca0c37fba9bbae99a3..e0f2c34abe91404da265686786556b2990112793 100644 --- a/test/metabase/lib/js_test.cljs +++ b/test/metabase/lib/js_test.cljs @@ -292,7 +292,7 @@ query (lib/query metadata-provider (meta/table-metadata :venues)) array-checker #(when (array? %) (js->clj %)) - to-legacy-refs (comp array-checker lib.js/legacy-ref)] + to-legacy-refs (comp array-checker #(lib.js/legacy-ref query -1 %))] (testing "field refs come with options" (is (= [["field" (meta/id :venues :id) {"base-type" "type/BigInteger"}] ["field" (meta/id :venues :name) {"base-type" "type/Text"}] @@ -301,14 +301,34 @@ ["field" (meta/id :venues :longitude) {"base-type" "type/Float"}] ["field" (meta/id :venues :price) {"base-type" "type/Integer"}]] (->> query lib/returned-columns (map to-legacy-refs))))) - (let [legacy-refs (->> query lib/available-segments (map lib.js/legacy-ref))] + (testing "segment refs come without options" + (is (= [["segment" segment-id]] + (->> query lib/available-segments (map to-legacy-refs))))) + (testing "metric refs come without options" + (is (= [["metric" metric-id]] + (->> query lib/available-metrics (map to-legacy-refs))))) + (testing "aggregation references (#37698)" + (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues)) + (lib/aggregate (lib/sum (meta/field-metadata :venues :price))) + ;; The display-name set here gets lost when the corresponding column + ;; is converted to a ref. Currently there is no option that would + ;; survive the aggregation -> column -> ref -> legacy ref conversion + ;; (cf. column-metadata->aggregation-ref and options->legacy-MBQL). + #_(lib/aggregate (lib.options/update-options (lib/avg (meta/field-metadata :venues :price)) + assoc :display-name "avg price")))] + (is (= [["aggregation" 0] + ;; add this back when the second aggregation is added back + #_["aggregation-options" ["aggregation 1"] {"display-name" "avg price"}]] + (map (comp array-checker #(lib.js/legacy-ref query -1 %)) + (lib.js/returned-columns query -1)))))) + (let [legacy-refs (->> query lib/available-segments (map #(lib.js/legacy-ref query -1 %)))] (testing "legacy segment refs come without options" (is (= [["segment" segment-id]] (map array-checker legacy-refs)))) (testing "segment legacy ref can be converted to an expression and back (#37173)" (let [segment-expr (lib.js/expression-clause-for-legacy-expression query -1 (first legacy-refs))] (is (=? [:segment {} segment-id] segment-expr)) (is (= ["segment" segment-id] (js->clj (lib.js/legacy-expression-for-expression-clause query -1 segment-expr))))))) - (let [legacy-refs (->> query lib/available-metrics (map lib.js/legacy-ref))] + (let [legacy-refs (->> query lib/available-metrics (map #(lib.js/legacy-ref query -1 %)))] (testing "metric refs come without options" (is (= [["metric" metric-id]] (map array-checker legacy-refs)))) (testing "metric legacy ref can be converted to an expression and back (#37173)"