MLv2 display-info method (#29818)

* MLv2 display-info method

* Update JS wrappers

* Appease TypeScript

* Remove unused import

* Use hierarchy for new multimethod

* Address PR feedback

frontend/src/metabase-lib/metadata.ts

@@ -2,7 +2,7 @@ import * as ML from "cljs/metabase.lib.js";
 import * as ML_MetadataCalculation from "cljs/metabase.lib.metadata.calculation";
 import type { DatabaseId } from "metabase-types/api";
 import type Metadata from "./metadata/Metadata";
-import type { Clause, MetadataProvider, Query } from "./types";
+import type { Clause, MetadataProvider, Query, ColumnMetadata } from "./types";
 
 export function metadataProvider(
   databaseId: DatabaseId,
@@ -14,3 +14,21 @@ export function metadataProvider(
 export function displayName(query: Query, clause: Clause): string {
   return ML_MetadataCalculation.display_name(query, clause);
 }
+
+export type DisplayInfo = {
+  display_name: string;
+  name?: string;
+  table?: {
+    name: string;
+    display_name: string;
+  };
+};
+
+// x can be any sort of opaque object, e.g. a clause or metadata map. Values returned depend on what you pass in, but it
+// should always have display_name... see :metabase.lib.metadata.calculation/display-info schema
+export function displayInfo(
+  query: Query,
+  x: Clause | ColumnMetadata,
+): DisplayInfo {
+  return ML.display_info(query, x);
+}

frontend/src/metabase-lib/order_by.ts

 import * as ML from "cljs/metabase.lib.js";
-import type { Field } from "metabase-types/api";
-import type { OrderByClause, Query } from "./types";
+import type { OrderByClause, Query, ColumnMetadata } from "./types";
 
-export function orderableColumns(query: Query): Field[] {
+export function orderableColumns(query: Query): ColumnMetadata[] {
   return ML.orderable_columns(query);
 }
 
 export function orderBys(query: Query): OrderByClause[] {
-  const result = ML.order_bys(query);
-  return result === null ? [] : result;
+  return ML.order_bys(query);
 }
 
-declare function OrderByFn(query: Query, field: Field): Query;
+declare function OrderByFn(
+  query: Query,
+  column: ColumnMetadata | OrderByClause,
+): Query;
 declare function OrderByFn(
   query: Query,
   stageIndex: number,
-  field: Field,
+  column: ColumnMetadata | OrderByClause,
 ): Query;
 
 export const orderBy: typeof OrderByFn = ML.order_by;
@@ -25,13 +26,7 @@ type OrderByDirection = "asc" | "desc";
 declare function OrderByClauseFn(
   query: Query,
   stageNumber: number,
-  field: Field,
-  direction?: OrderByDirection,
-): OrderByClause;
-declare function OrderByClauseFn(
-  query: Query,
-  stageNumber: number,
-  field: Field,
+  column: ColumnMetadata,
   direction?: OrderByDirection,
 ): OrderByClause;
 

frontend/src/metabase-lib/order_by.unit.spec.ts

-import type { Field } from "metabase-types/api";
-import { createQuery, SAMPLE_DATABASE } from "./test-helpers";
+import { createQuery } from "./test-helpers";
 import * as ML from "./v2";
 
+// This is a convenience for finding an orderable column (as an opaque object) by name
+const findOrderableColumn = (
+  query: ML.Query,
+  tableName: string,
+  fieldName: string,
+): ML.ColumnMetadata => {
+  const column = ML.orderableColumns(query).find(
+    (column: ML.ColumnMetadata) => {
+      const displayInfo = ML.displayInfo(query, column);
+      return (
+        displayInfo?.table?.name === tableName &&
+        displayInfo?.name === fieldName
+      );
+    },
+  );
+
+  if (!column) {
+    throw new Error(`Could not find ${tableName}.${fieldName}`);
+  }
+
+  return column;
+};
+
 describe("order by", () => {
   describe("orderableColumns", () => {
     const query = createQuery();
-    const columns = ML.orderableColumns(query);
 
     it("returns metadata for columns in the source table", () => {
-      const ordersID = columns.find(
-        ({ id }) => id === SAMPLE_DATABASE.ORDERS.ID.id,
-      );
+      const ordersID = findOrderableColumn(query, "ORDERS", "ID");
 
-      expect(ordersID).toEqual(
+      expect(ML.displayInfo(query, ordersID)).toEqual(
         expect.objectContaining({
-          table_id: SAMPLE_DATABASE.ORDERS.id,
           name: "ID",
-          id: SAMPLE_DATABASE.ORDERS.ID.id,
           display_name: "ID",
-          base_type: "type/BigInteger",
+          effective_type: "type/BigInteger",
+          table: {
+            name: "ORDERS",
+            display_name: "Orders",
+          },
         }),
       );
     });
 
     it("returns metadata for columns in implicitly joinable tables", () => {
-      const productsTitle = columns.find(
-        ({ id }) => id === SAMPLE_DATABASE.PRODUCTS.TITLE.id,
-      );
+      const productsTitle = findOrderableColumn(query, "PRODUCTS", "TITLE");
 
-      expect(productsTitle).toEqual(
+      expect(ML.displayInfo(query, productsTitle)).toEqual(
         expect.objectContaining({
-          table_id: SAMPLE_DATABASE.PRODUCTS.id,
           name: "TITLE",
-          id: SAMPLE_DATABASE.PRODUCTS.TITLE.id,
           display_name: "Title",
-          base_type: "type/Text",
+          effective_type: "type/Text",
+          table: {
+            name: "PRODUCTS",
+            display_name: "Products",
+          },
         }),
       );
     });
@@ -48,11 +69,8 @@ describe("order by", () => {
     });
 
     it("should update the query", () => {
-      const columns = ML.orderableColumns(query);
-      const productTitle = columns.find(
-        column => column.id === SAMPLE_DATABASE.PRODUCTS.TITLE.id,
-      );
-      const nextQuery = ML.orderBy(query, productTitle as Field);
+      const productTitle = findOrderableColumn(query, "PRODUCTS", "TITLE");
+      const nextQuery = ML.orderBy(query, productTitle);
       const orderBys = ML.orderBys(nextQuery);
 
       expect(orderBys).toHaveLength(1);
@@ -64,22 +82,21 @@ describe("order by", () => {
     const query = createQuery();
 
     it("should update the query", () => {
-      const columns = ML.orderableColumns(query);
-      const productTitle = columns.find(
-        column => column.id === SAMPLE_DATABASE.PRODUCTS.TITLE.id,
+      const productTitle = findOrderableColumn(query, "PRODUCTS", "TITLE");
+      const productCategory = findOrderableColumn(
+        query,
+        "PRODUCTS",
+        "CATEGORY",
       );
 
-      const productCategory = columns.find(
-        column => column.id === SAMPLE_DATABASE.PRODUCTS.CATEGORY.id,
-      );
-      const orderedQuery = ML.orderBy(query, productTitle as Field);
+      const orderedQuery = ML.orderBy(query, productTitle);
       const orderBys = ML.orderBys(orderedQuery);
 
       expect(orderBys).toHaveLength(1);
       const nextQuery = ML.replaceClause(
         orderedQuery,
         orderBys[0],
-        ML.orderByClause(orderedQuery, -1, productCategory as Field, "desc"),
+        ML.orderByClause(orderedQuery, -1, productCategory, "desc"),
       );
       const nextOrderBys = ML.orderBys(nextQuery);
       expect(ML.displayName(nextQuery, nextOrderBys[0])).toBe(
@@ -93,12 +110,9 @@ describe("order by", () => {
     const query = createQuery();
 
     it("should update the query", () => {
-      const columns = ML.orderableColumns(query);
-      const productTitle = columns.find(
-        column => column.id === SAMPLE_DATABASE.PRODUCTS.TITLE.id,
-      );
+      const productTitle = findOrderableColumn(query, "PRODUCTS", "TITLE");
 
-      const orderedQuery = ML.orderBy(query, productTitle as Field);
+      const orderedQuery = ML.orderBy(query, productTitle);
       const orderBys = ML.orderBys(orderedQuery);
       expect(orderBys).toHaveLength(1);
 

frontend/src/metabase-lib/types.ts

@@ -14,3 +14,6 @@ declare const OrderByClause: unique symbol;
 export type OrderByClause = unknown & { _opaque: typeof OrderByClause };
 
 export type Clause = OrderByClause;
+
+declare const ColumnMetadata: unique symbol;
+export type ColumnMetadata = unknown & { _opaque: typeof ColumnMetadata };

src/metabase/lib/core.cljc

@@ -146,6 +146,7 @@
    describe-query
    describe-top-level-key
    display-name
+   display-info
    suggested-name
    type-of]
   [lib.native

src/metabase/lib/js.cljs

@@ -101,7 +101,15 @@
   ([a-query]
    (orderable-columns a-query -1))
   ([a-query stage-number]
-   (-> (lib.order-by/orderable-columns a-query stage-number)
+   (to-array (lib.order-by/orderable-columns a-query stage-number))))
+
+(defn ^:export display-info
+  "Given an opaque Cljs object, return a plain JS object with info you'd need to implement UI for it.
+  See `:metabase.lib.metadata.calculation/display-info` for the keys this might contain."
+  ([a-query x]
+   (display-info a-query -1 x))
+  ([a-query stage-number x]
+   (-> (lib.metadata.calculation/display-info a-query stage-number x)
        (clj->js :keyword-fn u/qualified-name))))
 
 (defn ^:export order-by-clause
@@ -120,11 +128,7 @@
    (order-by a-query -1 x direction))
 
   ([a-query stage-number x direction]
-   (lib.order-by/order-by
-    a-query
-    stage-number
-    (lib.normalize/normalize (js->clj x :keywordize-keys true))
-    (js->clj direction))))
+   (lib.order-by/order-by a-query stage-number x (js->clj direction))))
 
 (defn ^:export order-bys
   "Get the order-by clauses (as an array of opaque objects) in `a-query` at a given `stage-number`. Returns `nil` if
@@ -132,9 +136,7 @@
   ([a-query]
    (order-bys a-query -1))
   ([a-query stage-number]
-   (some-> (lib.order-by/order-bys a-query stage-number)
-           not-empty
-           to-array)))
+   (to-array (lib.order-by/order-bys a-query stage-number))))
 
 (defn ^:export remove-clause
   "Removes the `target-clause` in the filter of the `query`."

src/metabase/lib/metadata/calculation.cljc

@@ -12,7 +12,8 @@
    [metabase.shared.util.i18n :as i18n]
    [metabase.util :as u]
    [metabase.util.log :as log]
-   [metabase.util.malli :as mu]))
+   [metabase.util.malli :as mu]
+   [metabase.util.malli.registry :as mr]))
 
 (defmulti display-name-method
   "Calculate a nice human-friendly display name for something."
@@ -168,6 +169,7 @@
 (defmethod metadata-method :default
   [query stage-number x]
   {:lib/type     :metadata/field
+   ;; TODO -- effective-type
    :base_type    (type-of query stage-number x)
    :name         (column-name query stage-number x)
    :display_name (display-name query stage-number x)})
@@ -179,9 +181,7 @@
    [:map
     [:lib/source ::lib.metadata/column-source]]])
 
-(mu/defn metadata :- [:or
-                      lib.metadata/ColumnMetadata
-                      [:sequential ColumnMetadataWithSource]]
+(mu/defn metadata
   "Calculate appropriate metadata for something. What this looks like depends on what we're calculating metadata for.
   If it's a reference or expression of some sort, this should return a single `:metadata/field` map (i.e., something
   satisfying the [[metabase.lib.metadata/ColumnMetadata]] schema. If it's something like a stage of a query or a join
@@ -209,3 +209,69 @@
       (catch #?(:clj Throwable :cljs js/Error) e
         (log/error e (i18n/tru "Error calculating display name for query: {0}" (ex-message e)))
         nil))))
+
+(defmulti display-info-method
+  "Implementation for [[display-info]]."
+  {:arglists '([query stage-number x])}
+  (fn [_query _stage-number x]
+    (lib.dispatch/dispatch-value x))
+  :hierarchy lib.hierarchy/hierarchy)
+
+(mr/register! ::display-info
+  [:map
+   [:display_name ::lib.schema.common/non-blank-string]
+   ;; For Columns. `base_type` not included here, FE doesn't need to know about that.
+   [:effective_type {:optional true} [:maybe [:ref ::lib.schema.common/base-type]]]
+   [:semantic_type  {:optional true} [:maybe [:ref ::lib.schema.common/semantic-type]]]
+   ;; for things that have a Table, e.g. a Field
+   [:table {:optional true} [:maybe [:ref ::display-info]]]
+   ;; these are derived from the `:lib/source`/`:metabase.lib.metadata/column-source`, but instead of using that value
+   ;; directly we're returning a different property so the FE doesn't break if we change those keys in the future,
+   ;; e.g. if we consolidate or split some of those keys. This is all the FE really needs to know.
+   ;;
+   ;; if this is a Column, does it come from a previous stage?
+   [:is_from_previous_stage {:optional true} [:maybe :boolean]]
+   ;; if this is a Column, does it come from a join in this stage?
+   [:is_from_join {:optional true} [:maybe :boolean]]
+   ;; if this is a Column, is it 'calculated', i.e. does it come from an expression in this stage?
+   [:is_calculated {:optional true} [:maybe :boolean]]
+   ;; if this is a Column, is it an implicitly joinable one? I.e. is it from a different table that we have not
+   ;; already joined, but could implicitly join against?
+   [:is_implicitly_joinable {:optional true} [:maybe :boolean]]])
+
+(mu/defn display-info :- ::display-info
+  "Given some sort of Cljs object, return a map with the info you'd need to implement UI for it. This is mostly meant to
+  power the Frontend JavaScript UI; in JS, results will be converted to plain JavaScript objects, so avoid returning
+  things that should remain opaque."
+  ([query x]
+   (display-info query -1 x))
+
+  ([query        :- ::lib.schema/query
+    stage-number :- :int
+    x]
+   (display-info-method query stage-number x)))
+
+(defn default-display-info
+  "Default implementation of [[display-info-method]], available in case you want to use this in a different
+  implementation and add additional information to it."
+  [query stage-number x]
+  (let [x-metadata (metadata query stage-number x)]
+    (merge
+     ;; TODO -- not 100% convinced the FE should actually have access to `:name`, can't it use `:display_name`
+     ;; everywhere? Determine whether or not this is the case.
+     (select-keys x-metadata [:name :display_name :semantic_type])
+     ;; don't return `:base_type`, FE should just use `:effective_type` everywhere and not even need to know
+     ;; `:base_type` exists.
+     (when-let [effective-type ((some-fn :effective_type :base_type) x-metadata)]
+       {:effective_type effective-type})
+     (when-let [table-id (:table_id x-metadata)]
+       {:table (display-info query stage-number (lib.metadata/table query table-id))})
+     (when-let [source (:lib/source x-metadata)]
+       {:is_from_previous_stage (= source :source/previous-stage)
+        :is_from_join           (= source :source/joins)
+        :is_calculated          (= source :source/expressions)
+        :is_implicitly_joinable (= source :source/implicitly-joinable)}))))
+
+(defmethod display-info-method :default
+  [query stage-number x]
+  (default-display-info query stage-number x))

src/metabase/lib/order_by.cljc

@@ -33,6 +33,12 @@
   [query stage-number [_tag _opts expr]]
   (i18n/tru "{0} descending" (lib.metadata.calculation/display-name query stage-number expr)))
 
+(doseq [tag [:asc :desc]]
+  (defmethod lib.metadata.calculation/display-info-method tag
+    [query stage-number [tag _opts expr]]
+    (assoc (lib.metadata.calculation/display-info query stage-number expr)
+           :direction tag)))
+
 (defmulti ^:private ->order-by-clause
   {:arglists '([query stage-number x])}
   (fn [_query _stage-number x]

src/metabase/lib/schema/common.cljc

@@ -27,10 +27,27 @@
   [:map
    [:lib/uuid ::uuid]])
 
+(defn- semantic-type? [x]
+  (or (isa? x :Semantic/*)
+      (isa? x :Relation/*)))
+
+(mr/def ::semantic-type
+  [:fn
+   {:error/message "valid semantic type"
+    :error/fn      (fn [{:keys [value]} _]
+                     (str "Not a valid semantic type: " value))}
+   semantic-type?])
+
+(defn- base-type? [x]
+  (and (isa? x :type/*)
+       (not (semantic-type? x))))
+
 (mr/def ::base-type
   [:fn
-   {:error/message "valid base type"}
-   #(isa? % :type/*)])
+   {:error/message "valid base type"
+    :error/fn      (fn [{:keys [value]} _]
+                (str "Not a valid base type: " value))}
+   base-type?])
 
 (mr/def ::external-op
   [:map

src/metabase/lib/table.cljc

@@ -13,6 +13,10 @@
       (some->> (:name table-metadata)
                (u.humanization/name->human-readable-name :simple))))
 
+(defmethod lib.metadata.calculation/metadata-method :metadata/table
+  [_query _stage-number table-metadata]
+  table-metadata)
+
 (defn- describe-source-table [query stage-number source-table-id]
   (when-let [table-metadata (lib.metadata/table query source-table-id)]
     (lib.metadata.calculation/display-name query stage-number table-metadata)))

test/metabase/lib/order_by_test.cljc

@@ -410,3 +410,39 @@
         (testing "description"
           (is (= "Venues, Sorted by expr ascending"
                  (lib/describe-query updated-query))))))))
+
+(deftest ^:parallel orderable-columns-display-info-test
+  (let [query (lib/query-for-table-name meta/metadata-provider "VENUES")]
+    (is (=? [{:semantic_type          :type/PK
+              :is_calculated          false
+              :table                  {:name "VENUES", :display_name "Venues"}
+              :name                   "ID"
+              :is_from_previous_stage false
+              :is_implicitly_joinable false
+              :effective_type         :type/BigInteger
+              :is_from_join           false
+              :display_name           "ID"}
+             {:display_name "Name"}
+             {:display_name "Category ID"}
+             {:display_name "Latitude"}
+             {:display_name "Longitude"}
+             {:display_name "Price"}
+             {:display_name "ID"}
+             {:display_name "Name"}]
+            (for [col (lib/orderable-columns query)]
+              (lib/display-info query col))))))
+
+(deftest ^:parallel order-bys-display-info-test
+  (let [query             (lib/query-for-table-name meta/metadata-provider "VENUES")
+        orderable-columns (lib/orderable-columns query)
+        col               (m/find-first #(= (:id %) (meta/id :venues :name)) orderable-columns)
+        _                 (is (some? col))
+        query'            (lib/order-by query col)]
+    (is (=? [{:name           "NAME"
+              :display_name   "Name"
+              :semantic_type  :type/Name
+              :effective_type :type/Text
+              :table          {:name "VENUES", :display_name "Venues"}
+              :direction      :asc}]
+            (for [order-by (lib/order-bys query')]
+              (lib/display-info query' order-by))))))