Ensure :effective-type for columns from an aggregation in a card (#47936

) * Ensure :effective-type for columns from an aggregation in a card Fixes #47184 * Synchronize effective-type with the base-type on override When :base-type in the column metadata is overridden with the :base-type in the field ref, set it as :effective-type too. If :effective-type is also set in the field ref, it wins.
metabase · Sep 16, 2024 · 96d46e9 · 96d46e9
1 parent df1ca3d
commit 96d46e9
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 28 deletions.
diff --git a/src/metabase/api/query_metadata.clj b/src/metabase/api/query_metadata.clj
@@ -63,8 +63,7 @@
     ;; since they tend to query only a handful of databases at most.
      :databases (sort-by :id (get-databases database-ids))
      :tables    (sort-by (comp str :id) tables)
-     :fields    (or (sort-by :id (api.field/get-fields template-tag-field-ids))
-                    [])}))
+     :fields    (sort-by :id (api.field/get-fields template-tag-field-ids))}))
 
 (defn batch-fetch-query-metadata
   "Fetch dependent metadata for ad-hoc queries."

diff --git a/src/metabase/lib/card.cljc b/src/metabase/lib/card.cljc
@@ -79,31 +79,34 @@
    card
    field]
   (let [col (-> col
-                (update-keys u/->kebab-case-en))]
-    (cond-> (merge
-             {:base-type :type/*, :lib/type :metadata/column}
-             field
-             col
-             {:lib/type                :metadata/column
-              :lib/source              :source/card
-              :lib/source-column-alias ((some-fn :lib/source-column-alias :name) col)})
-      card-id
-      (assoc :lib/card-id card-id)
-
-      (and *force-broken-card-refs*
-            ;; never force broken refs for Models, because Models can have give columns with completely
-            ;; different names the Field ID of a different column, somehow. See #22715
-           (or
-             ;; we can only do this check if `card-id` is passed in.
-            (not card-id)
-            (not= (:type card) :model)))
-      (assoc ::force-broken-id-refs true)
-
-       ;; If the incoming col doesn't have `:semantic-type :type/FK`, drop `:fk-target-field-id`.
-       ;; This comes up with metadata on SQL cards, which might be linked to their original DB field but should not be
-       ;; treated as FKs unless the metadata is configured accordingly.
-      (not= (:semantic-type col) :type/FK)
-      (assoc :fk-target-field-id nil))))
+                (update-keys u/->kebab-case-en))
+        col-meta (cond-> (merge
+                          {:base-type :type/*, :lib/type :metadata/column}
+                          field
+                          col
+                          {:lib/type                :metadata/column
+                           :lib/source              :source/card
+                           :lib/source-column-alias ((some-fn :lib/source-column-alias :name) col)})
+                   card-id
+                   (assoc :lib/card-id card-id)
+
+                   (and *force-broken-card-refs*
+                        ;; never force broken refs for Models, because Models can have give columns with completely
+                        ;; different names the Field ID of a different column, somehow. See #22715
+                        (or
+                         ;; we can only do this check if `card-id` is passed in.
+                         (not card-id)
+                         (not= (:type card) :model)))
+                   (assoc ::force-broken-id-refs true)
+
+                   ;; If the incoming col doesn't have `:semantic-type :type/FK`, drop `:fk-target-field-id`.
+                   ;; This comes up with metadata on SQL cards, which might be linked to their original DB field but should not be
+                   ;; treated as FKs unless the metadata is configured accordingly.
+                   (not= (:semantic-type col) :type/FK)
+                   (assoc :fk-target-field-id nil))]
+    ;; :effective-type is required, but not always set, see e.g.,
+    ;; metabase.api.table/card-result-metadata->virtual-fields
+    (u/assoc-default col-meta :effective-type (:base-type col-meta))))
 
 (mu/defn ->card-metadata-columns :- [:sequential ::lib.schema.metadata/column]
   "Massage possibly-legacy Card results metadata into MLv2 ColumnMetadata."

diff --git a/src/metabase/lib/field.cljc b/src/metabase/lib/field.cljc
@@ -165,8 +165,8 @@
                   {:display-name (or (:display-name opts)
                                      (lib.metadata.calculation/display-name query stage-number field-ref))})]
     (cond-> metadata
+      base-type      (assoc :base-type base-type, :effective-type base-type)
       effective-type (assoc :effective-type effective-type)
-      base-type      (assoc :base-type base-type)
       temporal-unit  (assoc ::temporal-unit temporal-unit)
       binning        (assoc ::binning binning)
       source-field   (assoc :fk-field-id source-field)

diff --git a/test/metabase/lib/card_test.cljc b/test/metabase/lib/card_test.cljc
@@ -230,3 +230,25 @@
              (map #(lib/display-name query %) (lib/returned-columns query))))
       (is (= ["ID is 1"]
              (map #(lib/display-name query %) (lib/filters query)))))))
+
+(deftest ^:parallel ->card-metadata-column-test
+  (testing ":effective-type is set for columns coming from an aggregation in a card (#47184)"
+    (let [col {:lib/type :metadata/column
+               :base-type :type/Integer
+               :semantic-type :type/Quantity
+               :name "count"
+               :lib/source :source/aggregations}
+          card-id 176
+          card {:type :model}
+          field nil
+          expected-col {:lib/type :metadata/column
+                        :base-type :type/Integer
+                        :effective-type :type/Integer
+                        :semantic-type :type/Quantity
+                        :name "count"
+                        :lib/card-id 176
+                        :lib/source :source/card
+                        :lib/source-column-alias "count"
+                        :fk-target-field-id nil}]
+      (is (=? expected-col
+              (#'lib.card/->card-metadata-column col card-id card field))))))
diff --git a/test/metabase/lib/field_test.cljc b/test/metabase/lib/field_test.cljc
@@ -678,6 +678,7 @@
                                                     :source-card 3}]})]
       (is (= [{:lib/type                 :metadata/column
                :base-type                :type/*
+               :effective-type           :type/*
                :id                       4
                :name                     "Field 4"
                :fk-target-field-id       nil