MLv2: fix saved question JS metadata (#30011)

* Add QP test middleware to validate converted pMBQL queries against MLv2 schema

* Revert 'fix' for #29944

* Fix ups

* Hack

* Fix final test failure

* Remove unused namespace

* MLv2 metadata calculation validation

* Handful of test fixes 

* Fix Kondo warning

* Skip #29958

* Don't barf when metadata is missing

* Skip percentile tests

* Update skip rule for #29910 to skip `:convert-timezone` with string literal date

* Fix Saved Question js metadata

* Update e2e test

* Update Cypress test

src/metabase/lib/card.cljc

@@ -16,12 +16,14 @@
   "Metadata associated with a Saved Question with `card-id`."
   [query   :- ::lib.schema/query
    card-id :- ::lib.schema.id/card]
-  ;; it seems like in some cases the FE is renaming `:result_metadata` to `:fields`, not 100% sure why but
-  ;; handle that case anyway. (#29739)
+  ;; it seems like in some cases (unit tests) the FE is renaming `:result_metadata` to `:fields`, not 100% sure why
+  ;; but handle that case anyway. (#29739)
   (when-let [card (lib.metadata/card query card-id)]
     (when-let [result-metadata (or (:result_metadata card)
                                    (:fields card)
                                    (infer-results-metadata (:lib/metadata query) (:dataset_query card)))]
+      ;; Card `result_metadata` SHOULD be a sequence of column infos, but just to be safe handle a map that
+      ;; contains` :columns` as well.
       (when-let [cols (not-empty (cond
                                    (map? result-metadata)        (:columns result-metadata)
                                    (sequential? result-metadata) result-metadata))]
@@ -29,6 +31,7 @@
                 (assoc col
                        :lib/type                :metadata/field
                        :lib/source              :source/card
+                       :lib/card-id             card-id
                        :lib/source-column-alias (:name col)))
               cols)))))
 

src/metabase/lib/field.cljc

@@ -198,6 +198,17 @@
     ;; mostly for the benefit of JS, which does not enforce the Malli schemas.
     "unknown_field"))
 
+(defmethod lib.metadata.calculation/display-info-method :metadata/field
+  [query stage-number field-metadata]
+  (merge
+   ((get-method lib.metadata.calculation/display-info-method :default) query stage-number field-metadata)
+   ;; if this column comes from a source Card (Saved Question/Model/etc.) use the name of the Card as the 'table' name
+   ;; rather than the ACTUAL table name.
+   (when (= (:lib/source field-metadata) :source/card)
+     (when-let [card-id (:lib/card-id field-metadata)]
+       (when-let [card (lib.metadata/card query card-id)]
+         {:table {:name (:name card), :display_name (:name card)}})))))
+
 (defmethod lib.temporal-bucket/current-temporal-bucket-method :field
   [[_tag opts _id-or-name]]
   (:temporal-unit opts))

src/metabase/lib/js/metadata.cljs

@@ -222,15 +222,23 @@
   [_object-type]
   (fn [k v]
     (case k
-      :result_metadata (cond-> v
-                         (:columns v) (update :columns parse-fields))
+      :result_metadata (if ((some-fn sequential? array?) v)
+                         (parse-fields v)
+                         (js->clj v :keywordize-keys true))
       :fields          (parse-fields v)
       :visibility_type (keyword v)
       v)))
 
+(defn- unwrap-card
+  "Sometimes a card is stored in the metadata as some sort of weird object where the thing we actually want is under the
+  key `_card` (not sure why), but if it is just unwrap it and then parse it normally."
+  [obj]
+  (or (object-get obj "_card")
+      obj))
+
 (defmethod parse-objects :card
   [object-type metadata]
-  (let [parse-card (parse-object-fn object-type)]
+  (let [parse-card (comp (parse-object-fn object-type) unwrap-card)]
     (merge
      (obj->clj (comp (filter (fn [[k _v]]
                                (str/starts-with? k "card__")))

test/metabase/lib/card_test.cljc

@@ -39,5 +39,26 @@
                 :lib/source               :source/card
                 :lib/source-column-alias  "count"
                 :lib/desired-column-alias "count"}]
-              (map #(select-keys % [:id :name :lib/source :lib/source-column-alias :lib/desired-column-alias])
-                   (lib.metadata.calculation/metadata query)))))))
+              (lib.metadata.calculation/metadata query)))
+      (testing `lib/display-info
+        (is (=? [{:name                   "USER_ID"
+                  :display_name           "User ID"
+                  :table                  {:name         "My Card"
+                                           :display_name "My Card"}
+                  :effective_type         :type/Integer
+                  :semantic_type          :type/FK
+                  :is_calculated          false
+                  :is_from_previous_stage false
+                  :is_implicitly_joinable false
+                  :is_from_join           false}
+                 {:name                   "count"
+                  :display_name           "Count"
+                  :table                  {:name         "My Card"
+                                           :display_name "My Card"}
+                  :effective_type         :type/Integer
+                  :is_from_previous_stage false
+                  :is_from_join           false
+                  :is_calculated          false
+                  :is_implicitly_joinable false}]
+                (for [col (lib.metadata.calculation/metadata query)]
+                  (lib/display-info query col))))))))

test/metabase/lib/convert_test.cljc

@@ -179,4 +179,8 @@
                                          [:field 23402 nil]
                                          [:field 23100 {:join-alias "CATEGORIES__via__CATEGORY_ID"}]]
                           :strategy     :left-join
-                          :fk-field-id  23402}]}}))
+                          :fk-field-id  23402}]}}
+
+    {:database 1
+     :type     :query
+     :query    {:order-by [[:asc [:field 1 nil]]]}}))

test/metabase/lib/order_by_test.cljc

@@ -311,19 +311,28 @@
   (testing "orderable-columns should use metadata for source query."
     (let [query (lib.tu/query-with-card-source-table)]
       (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
-        (is (=? [{:name "ID"
+        (is (=? [{:name      "ID"
                   :base_type :type/BigInteger}
-                 {:name "NAME"
+                 {:name      "NAME"
                   :base_type :type/Text}
-                 {:name "CATEGORY_ID"
+                 {:name      "CATEGORY_ID"
                   :base_type :type/Integer}
-                 {:name "LATITUDE"
+                 {:name      "LATITUDE"
                   :base_type :type/Float}
-                 {:name "LONGITUDE"
+                 {:name      "LONGITUDE"
                   :base_type :type/Float}
-                 {:name "PRICE"
+                 {:name      "PRICE"
                   :base_type :type/Integer}]
-                (lib/orderable-columns query)))))))
+                (lib/orderable-columns query)))
+        (testing `lib/display-info
+          (is (=? [{:name "ID",          :table {:name "My Card", :display_name "My Card"}}
+                   {:name "NAME",        :table {:name "My Card", :display_name "My Card"}}
+                   {:name "CATEGORY_ID", :table {:name "My Card", :display_name "My Card"}}
+                   {:name "LATITUDE",    :table {:name "My Card", :display_name "My Card"}}
+                   {:name "LONGITUDE",   :table {:name "My Card", :display_name "My Card"}}
+                   {:name "PRICE",       :table {:name "My Card", :display_name "My Card"}}]
+                  (for [col (lib/orderable-columns query)]
+                    (lib/display-info query col)))))))))
 
 (deftest ^:parallel orderable-columns-e2e-test
   (testing "Use the metadata returned by `orderable-columns` to add a new order-by to a query."
@@ -352,6 +361,28 @@
                     [:field {:lib/uuid string? :base-type :type/Text} (meta/id :venues :name)]]]
                   (lib/order-bys query'))))))))
 
+(deftest ^:parallel orderable-columns-with-source-card-e2e-test
+  (testing "Make sure you can order by a column that comes from a source Card (Saved Question/Model/etc)"
+    (let [query (lib.tu/query-with-card-source-table)]
+      (testing (lib.util/format "Query =\n%s" (u/pprint-to-str query))
+        (let [name-col (m/find-first #(= (:name %) "NAME")
+                                     (lib/orderable-columns query))]
+          (is (=? {:name      "NAME"
+                   :base_type :type/Text}
+                  name-col))
+          (let [query' (lib/order-by query name-col)]
+            (is (=? {:stages
+                     [{:source-table "card__1"
+                       :order-by [[:asc
+                                   {}
+                                   [:field {:base-type :type/Text} "NAME"]]]}]}
+                    query'))
+            (is (= "My Card, Sorted by Name ascending"
+                   (lib/describe-query query')))
+            (is (= ["Name ascending"]
+                   (for [order-by (lib/order-bys query')]
+                     (lib/display-name query' order-by))))))))))
+
 (deftest ^:parallel orderable-columns-with-join-test
   (is (=? [{:name                     "ID"
             :lib/source-column-alias  "ID"