From dd58aa16ef53003e3271c62b855371b9143fcc74 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Tue, 28 Mar 2023 11:55:29 -0700
Subject: [PATCH] MLv2: Finish porting query description generation (#29507)
* Improved implementation
* Amazing code cleanup
* Cleanup
* More tests
---
.clj-kondo/config.edn | 1 -
bin/kondo.sh | 3 +
.../metabase-lib/queries/utils/description.js | 156 ------------------
.../queries/utils/description.styled.tsx | 12 --
.../admin/datamodel/components/MetricItem.jsx | 8 +-
.../datamodel/components/SegmentItem.jsx | 8 +-
shared/src/metabase/mbql/util.cljc | 12 ++
src/metabase/api/metric.clj | 24 +--
src/metabase/api/query_description.clj | 128 --------------
src/metabase/api/segment.clj | 26 +--
src/metabase/lib/aggregation.cljc | 2 +-
src/metabase/lib/breakout.cljc | 2 +-
src/metabase/lib/convert.cljc | 9 +
src/metabase/lib/core.cljc | 3 +
src/metabase/lib/field.cljc | 10 +-
src/metabase/lib/filter.cljc | 2 +-
src/metabase/lib/js/metadata.cljs | 4 +-
src/metabase/lib/limit.cljc | 2 +-
src/metabase/lib/metadata.cljc | 6 +-
.../lib/metadata/cached_provider.cljc | 71 ++++++++
src/metabase/lib/metadata/calculation.cljc | 57 ++++---
src/metabase/lib/metadata/graph_provider.cljc | 56 +++++++
src/metabase/lib/metadata/jvm.clj | 86 ++++++++++
src/metabase/lib/metadata/protocols.cljc | 108 ++++--------
src/metabase/lib/metric.cljc | 4 +-
src/metabase/lib/order_by.cljc | 2 +-
src/metabase/lib/query.cljc | 36 +++-
src/metabase/lib/segment.cljc | 17 ++
src/metabase/lib/stage.cljc | 3 +-
src/metabase/lib/table.cljc | 2 +-
src/metabase/models/metric.clj | 52 +++++-
src/metabase/models/segment.clj | 46 +++++-
test/metabase/api/metric_test.clj | 47 ++++--
test/metabase/api/query_description_test.clj | 119 -------------
test/metabase/api/segment_test.clj | 74 +++++----
test/metabase/lib/breakout_test.cljc | 2 +
.../lib/metadata/cached_provider_test.cljc | 54 ++++++
test/metabase/lib/test_metadata.cljc | 4 +-
test/metabase/models/metric_test.clj | 26 ++-
test/metabase/models/segment_test.clj | 27 ++-
40 files changed, 662 insertions(+), 649 deletions(-)
delete mode 100644 frontend/src/metabase-lib/queries/utils/description.js
delete mode 100644 frontend/src/metabase-lib/queries/utils/description.styled.tsx
delete mode 100644 src/metabase/api/query_description.clj
create mode 100644 src/metabase/lib/metadata/cached_provider.cljc
create mode 100644 src/metabase/lib/metadata/graph_provider.cljc
create mode 100644 src/metabase/lib/metadata/jvm.clj
create mode 100644 src/metabase/lib/segment.cljc
delete mode 100644 test/metabase/api/query_description_test.clj
create mode 100644 test/metabase/lib/metadata/cached_provider_test.cljc
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 12266d75f68..c7c11a7a08f 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -243,7 +243,6 @@
metabase.api.preview-embed api.preview-embed
metabase.api.public api.public
metabase.api.pulse api.pulse
- metabase.api.query-description api.qd
metabase.api.revision api.revision
metabase.api.search api.search
metabase.api.segment api.segment
diff --git a/bin/kondo.sh b/bin/kondo.sh
index f2269f565f9..0fb1261ae8f 100755
--- a/bin/kondo.sh
+++ b/bin/kondo.sh
@@ -8,6 +8,9 @@ set -euxo pipefail
clj-kondo --copy-configs --dependencies --lint "$(clojure -A:dev -Spath)" --skip-lint --parallel
+rm -rf .clj-kondo/metosin/malli-types-clj/
+rm -rf .clj-kondo/.cache
+
# Run Kondo against all of our Clojure files in the various directories they might live.
find modules/drivers shared enterprise/backend \
-maxdepth 2 \
diff --git a/frontend/src/metabase-lib/queries/utils/description.js b/frontend/src/metabase-lib/queries/utils/description.js
deleted file mode 100644
index 061f3ba8f8c..00000000000
--- a/frontend/src/metabase-lib/queries/utils/description.js
+++ /dev/null
@@ -1,156 +0,0 @@
-import React from "react";
-
-import { t } from "ttag";
-import _ from "underscore";
-import inflection from "inflection";
-
-import { FilterClause, MetricClause } from "./description.styled.tsx";
-
-// -----------------------------------------------------------------------------
-// These functions use the `query_description` field returned by the Segment and
-// Metric APIs. They're meant for cases where you do not have the full database
-// metadata available, and the server side will generate a data structure
-// containing all the applicable data for formatting a user-friendly description
-// of a query.
-//
-// TODO: This is almost 100% duplicated with code in Question.ts, find a way to
-// consolidate them
-// -----------------------------------------------------------------------------
-
-function formatTableDescription({ table }, options = {}) {
- return [inflection.pluralize(table)];
-}
-
-function formatAggregationDescription({ aggregation }, options = {}) {
- if (!aggregation || !aggregation.length) {
- return [];
- }
-
- return conjunctList(
- aggregation.map(agg => {
- switch (agg["type"]) {
- case "aggregation":
- return [agg["arg"]];
- case "metric":
- return [
- options.jsx ? (
- <MetricClause>{agg["arg"]}</MetricClause>
- ) : (
- agg["arg"]
- ),
- ];
- case "rows":
- return [t`Raw data`];
- case "count":
- return [t`Count`];
- case "cum-count":
- return [t`Cumulative count`];
- case "avg":
- return [t`Average of `, agg["arg"]];
- case "median":
- return [t`Median of `, agg["arg"]];
- case "distinct":
- return [t`Distinct values of `, agg["arg"]];
- case "stddev":
- return [t`Standard deviation of `, agg["arg"]];
- case "sum":
- return [t`Sum of `, agg["arg"]];
- case "cum-sum":
- return [t`Cumulative sum of `, agg["arg"]];
- case "max":
- return [t`Maximum of `, agg["arg"]];
- case "min":
- return [t`Minimum of `, agg["arg"]];
- default: {
- console.warn(
- "Unexpected aggregation type in formatAggregationDescription: ",
- agg["type"],
- );
- return null;
- }
- }
- }),
- );
-}
-
-function formatFilterDescription({ filter }, options = {}) {
- if (!filter || !filter.length) {
- return [];
- }
-
- return [
- t`Filtered by `,
- joinList(
- filter.map(f => {
- if (f["segment"] != null) {
- return options.jsx ? (
- <FilterClause>{f["segment"]}</FilterClause>
- ) : (
- f["segment"]
- );
- } else if (f["field"] != null) {
- return f["field"];
- }
- }),
- ", ",
- ),
- ];
-}
-
-export function formatQueryDescription(parts, options = {}) {
- if (!parts) {
- return "";
- }
-
- options = {
- jsx: false,
- sections: ["table", "aggregation", "filter"],
- ...options,
- };
-
- const sectionFns = {
- table: formatTableDescription,
- aggregation: formatAggregationDescription,
- filter: formatFilterDescription,
- };
-
- // these array gymnastics are needed to support JSX formatting
- const sections = (options.sections || [])
- .map(section =>
- _.flatten(sectionFns[section](parts, options)).filter(s => !!s),
- )
- .filter(s => s && s.length > 0);
-
- const description = _.flatten(joinList(sections, ", "));
- if (options.jsx) {
- return <span>{description}</span>;
- } else {
- return description.join("");
- }
-}
-
-function joinList(list, joiner) {
- return _.flatten(
- list.map((l, i) => (i === list.length - 1 ? [l] : [l, joiner])),
- true,
- );
-}
-
-function conjunctList(list, conjunction) {
- switch (list.length) {
- case 0:
- return null;
- case 1:
- return list[0];
- case 2:
- return [list[0], " ", conjunction, " ", list[1]];
- default:
- return [
- list.slice(0, -1).join(", "),
- ", ",
- conjunction,
- " ",
- list[list.length - 1],
- ];
- }
-}
diff --git a/frontend/src/metabase-lib/queries/utils/description.styled.tsx b/frontend/src/metabase-lib/queries/utils/description.styled.tsx
deleted file mode 100644
index e187ba0f083..00000000000
--- a/frontend/src/metabase-lib/queries/utils/description.styled.tsx
+++ /dev/null
@@ -1,12 +0,0 @@
-import styled from "@emotion/styled";
-import { color } from "metabase/lib/colors";
-
-export const MetricClause = styled.span`
- color: ${color("summarize")};
- font-weight: 700;
-`;
-
-export const FilterClause = styled.span`
- color: ${color("filter")};
- font-weight: 700;
-`;
diff --git a/frontend/src/metabase/admin/datamodel/components/MetricItem.jsx b/frontend/src/metabase/admin/datamodel/components/MetricItem.jsx
index 0f420eac79c..c228f1300af 100644
--- a/frontend/src/metabase/admin/datamodel/components/MetricItem.jsx
+++ b/frontend/src/metabase/admin/datamodel/components/MetricItem.jsx
@@ -2,7 +2,6 @@ import React, { Component } from "react";
import PropTypes from "prop-types";
import Icon from "metabase/components/Icon";
-import { formatQueryDescription } from "metabase-lib/queries/utils/description";
import ObjectActionSelect from "./ObjectActionSelect";
export default class MetricItem extends Component {
@@ -14,11 +13,6 @@ export default class MetricItem extends Component {
render() {
const { metric, onRetire } = this.props;
- const description = formatQueryDescription(metric.query_description, {
- sections: ["table", "aggregation", "filter"],
- jsx: true,
- });
-
return (
<tr>
<td className="px1 py1 text-wrap">
@@ -27,7 +21,7 @@ export default class MetricItem extends Component {
<span className="text-dark text-bold">{metric.name}</span>
</span>
</td>
- <td className="px1 py1 text-wrap">{description}</td>
+ <td className="px1 py1 text-wrap">{metric.definition_description}</td>
<td className="px1 py1 text-centered">
<ObjectActionSelect
object={metric}
diff --git a/frontend/src/metabase/admin/datamodel/components/SegmentItem.jsx b/frontend/src/metabase/admin/datamodel/components/SegmentItem.jsx
index 7252c06a0c6..a92ddb35d18 100644
--- a/frontend/src/metabase/admin/datamodel/components/SegmentItem.jsx
+++ b/frontend/src/metabase/admin/datamodel/components/SegmentItem.jsx
@@ -2,7 +2,6 @@ import React, { Component } from "react";
import PropTypes from "prop-types";
import Icon from "metabase/components/Icon";
-import { formatQueryDescription } from "metabase-lib/queries/utils/description";
import ObjectActionSelect from "./ObjectActionSelect";
export default class SegmentItem extends Component {
@@ -14,11 +13,6 @@ export default class SegmentItem extends Component {
render() {
const { segment, onRetire } = this.props;
- const description = formatQueryDescription(segment.query_description, {
- sections: ["filter"],
- jsx: true,
- });
-
return (
<tr className="mt1 mb3">
<td className="px1 py1 text-wrap">
@@ -31,7 +25,7 @@ export default class SegmentItem extends Component {
<span className="text-dark text-bold">{segment.name}</span>
</span>
</td>
- <td className="px1 py1 text-wrap">{description}</td>
+ <td className="px1 py1 text-wrap">{segment.definition_description}</td>
<td className="px1 py1 text-centered">
<ObjectActionSelect
object={segment}
diff --git a/shared/src/metabase/mbql/util.cljc b/shared/src/metabase/mbql/util.cljc
index 329428867c1..9d6c58c5c50 100644
--- a/shared/src/metabase/mbql/util.cljc
+++ b/shared/src/metabase/mbql/util.cljc
@@ -733,6 +733,18 @@
(when (keyword? k)
(namespace k)))))))
+(defn referenced-field-ids
+ "Find all the `:field` references with integer IDs in `coll`, which can be a full MBQL query, a snippet of MBQL, or a
+ sequence of those things; return a set of Field IDs. Includes Fields referenced indirectly via `:source-field`.
+ Returns `nil` if no IDs are found."
+ [coll]
+ (not-empty
+ (into #{}
+ (comp cat (filter some?))
+ (mbql.match/match coll
+ [:field (id :guard integer?) opts]
+ [id (:source-field opts)]))))
+
#?(:clj
(p/import-vars
[mbql.match
diff --git a/src/metabase/api/metric.clj b/src/metabase/api/metric.clj
index 5d91c15a188..25ac7d25fd2 100644
--- a/src/metabase/api/metric.clj
+++ b/src/metabase/api/metric.clj
@@ -4,7 +4,6 @@
[clojure.data :as data]
[compojure.core :refer [DELETE GET POST PUT]]
[metabase.api.common :as api]
- [metabase.api.query-description :as api.qd]
[metabase.events :as events]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.models :refer [Metric MetricImportantField Table]]
@@ -14,6 +13,7 @@
[metabase.util :as u]
[metabase.util.i18n :refer [trs]]
[metabase.util.log :as log]
+ [metabase.util.malli.schema :as ms]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.db :as db]
@@ -44,20 +44,11 @@
(-> (api/read-check (t2/select-one Metric :id id))
(hydrate :creator)))
-(defn- add-query-descriptions
- [metrics] {:pre [(coll? metrics)]}
- (when (some? metrics)
- (for [metric metrics]
- (let [table (t2/select-one Table :id (:table_id metric))]
- (assoc metric
- :query_description
- (api.qd/generate-query-description table (:definition metric)))))))
-
-#_{:clj-kondo/ignore [:deprecated-var]}
-(api/defendpoint-schema GET "/:id"
+(api/defendpoint GET "/:id"
"Fetch `Metric` with ID."
[id]
- (first (add-query-descriptions [(hydrated-metric id)])))
+ {id ms/PositiveInt}
+ (hydrated-metric id))
(defn- add-db-ids
"Add `:database_id` fields to `metrics` by looking them up from their `:table_id`."
@@ -67,15 +58,14 @@
(for [metric metrics]
(assoc metric :database_id (table-id->db-id (:table_id metric)))))))
-#_{:clj-kondo/ignore [:deprecated-var]}
-(api/defendpoint-schema GET "/"
+(api/defendpoint GET "/"
"Fetch *all* `Metrics`."
[]
(as-> (t2/select Metric, :archived false, {:order-by [:%lower.name]}) metrics
- (hydrate metrics :creator)
+ (hydrate metrics :creator :definition_description)
(add-db-ids metrics)
(filter mi/can-read? metrics)
- (add-query-descriptions metrics)))
+ metrics))
(defn- write-check-and-update-metric!
"Check whether current user has write permissions, then update Metric with values in `body`. Publishes appropriate
diff --git a/src/metabase/api/query_description.clj b/src/metabase/api/query_description.clj
deleted file mode 100644
index 42c399c8bdd..00000000000
--- a/src/metabase/api/query_description.clj
+++ /dev/null
@@ -1,128 +0,0 @@
-(ns metabase.api.query-description
- "Functions for generating human friendly query descriptions"
- (:require
- [clojure.string :as str]
- [metabase.mbql.predicates :as mbql.preds]
- [metabase.mbql.util :as mbql.u]
- [metabase.models.field :refer [Field]]
- [metabase.models.metric :refer [Metric]]
- [metabase.models.segment :refer [Segment]]
- [metabase.util.i18n :refer [deferred-tru]]
- [metabase.util.log :as log]
- [toucan2.core :as t2]))
-
-(defn- get-table-description
- [metadata _query]
- {:table (:display_name metadata)})
-
-(defn- field-clause->display-name [clause]
- (mbql.u/match-one clause
- [:field (id :guard integer?) _]
- (t2/select-one-fn :display_name Field :id id)
-
- [:field (field-name :guard string?) _]
- field-name))
-
-(defn- get-aggregation-details
- [metadata query]
- (letfn [(field-name [match] (or (when (mbql.preds/Field? match)
- (field-clause->display-name match))
- (flatten (get-aggregation-details metadata match))))]
- (when-let [agg-matches (mbql.u/match query
- [:aggregation-options _ (options :guard :display-name)]
- {:type :aggregation :arg (:display-name options)}
-
- [:aggregation-options ag _]
- #_:clj-kondo/ignore
- (recur ag)
-
- [(operator :guard #{:+ :- :/ :*}) & args]
- (interpose (name operator) (map field-name args))
-
- [:metric (arg :guard integer?)]
- {:type :metric
- :arg (let [metric-name (t2/select-one-fn :name Metric :id arg)]
- (if-not (str/blank? metric-name)
- metric-name
- (deferred-tru "[Unknown Metric]")))}
-
- [:rows] {:type :rows}
- [:count] {:type :count}
- [:cum-count] {:type :cum-count}
- [:avg arg] {:type :avg :arg (field-name arg)}
- [:distinct arg] {:type :distinct :arg (field-name arg)}
- [:stddev arg] {:type :stddev :arg (field-name arg)}
- [:sum arg] {:type :sum :arg (field-name arg)}
- [:cum-sum arg] {:type :cum-sum :arg (field-name arg)}
- [:max arg] {:type :max :arg (field-name arg)}
- [:min arg] {:type :min :arg (field-name arg)})]
- agg-matches)))
-
-(defn- get-aggregation-description
- [metadata query]
- (when-let [details (get-aggregation-details metadata query)]
- {:aggregation details}))
-
-(defn- get-breakout-description
- [_metadata query]
- (when-let [breakouts (seq (:breakout query))]
- {:breakout (map #(t2/select-one-fn :display_name Field :id %) breakouts)}))
-
-(defn- get-filter-clause-description
- [_metadata filt]
- (let [typ (first filt)]
- (condp = typ
- :field {:field (field-clause->display-name filt)}
- :segment {:segment (let [segment (t2/select-one Segment :id (second filt))]
- (if segment
- (:name segment)
- (deferred-tru "[Unknown Segment]")))}
- nil)))
-
-(defn- get-filter-description
- [metadata query]
- (when-let [filters (:filter query)]
- {:filter (map #(get-filter-clause-description metadata %)
- (mbql.u/match filters #{:field :segment} &match))}))
-
-(defn- get-order-by-description
- [_metadata query]
- (when (:order-by query)
- {:order-by (map (fn [[direction field]]
- {:field (field-clause->display-name field)
- :direction direction})
- (mbql.u/match query #{:asc :desc} &match))}))
-
-(defn- get-limit-description
- [_metadata query]
- (when-let [limit (:limit query)]
- {:limit limit}))
-
-(def ^:private query-descriptor-functions
- [get-table-description
- get-aggregation-description
- get-breakout-description
- get-filter-description
- get-order-by-description
- get-limit-description])
-
-(defn generate-query-description
- "Analyze a query and return a data structure with the parts broken down for display
- in the UI.
-
- Ex:
- {
- :table \"Orders\"
- :filters [\"Created At\", \"Product ID\"]
- :order-by [{\"Created At\" :asc}]
- }
-
- This data structure allows the UI to format the strings appropriately (including JSX)"
- [metadata query]
- (try
- (apply merge
- (map (fn [f] (f metadata query))
- query-descriptor-functions))
- (catch Exception e
- (log/warn e "Error generating query description")
- {})))
diff --git a/src/metabase/api/segment.clj b/src/metabase/api/segment.clj
index c5dd3e41921..3cb62092897 100644
--- a/src/metabase/api/segment.clj
+++ b/src/metabase/api/segment.clj
@@ -3,17 +3,16 @@
(:require
[compojure.core :refer [DELETE GET POST PUT]]
[metabase.api.common :as api]
- [metabase.api.query-description :as api.qd]
[metabase.events :as events]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.models.interface :as mi]
[metabase.models.revision :as revision]
[metabase.models.segment :as segment :refer [Segment]]
- [metabase.models.table :as table :refer [Table]]
[metabase.related :as related]
[metabase.util :as u]
[metabase.util.i18n :refer [trs]]
[metabase.util.log :as log]
+ [metabase.util.malli.schema :as ms]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.hydrate :refer [hydrate]]
@@ -43,29 +42,18 @@
(-> (api/read-check (t2/select-one Segment :id id))
(hydrate :creator)))
-(defn- add-query-descriptions
- [segments] {:pre [(coll? segments)]}
- (when (some? segments)
- (for [segment segments]
- (let [table (t2/select-one Table :id (:table_id segment))]
- (assoc segment
- :query_description
- (api.qd/generate-query-description table (:definition segment)))))))
-
-#_{:clj-kondo/ignore [:deprecated-var]}
-(api/defendpoint-schema GET "/:id"
+(api/defendpoint GET "/:id"
"Fetch `Segment` with ID."
[id]
- (first (add-query-descriptions [(hydrated-segment id)])))
+ {id ms/PositiveInt}
+ (hydrated-segment id))
-#_{:clj-kondo/ignore [:deprecated-var]}
-(api/defendpoint-schema GET "/"
+(api/defendpoint GET "/"
"Fetch *all* `Segments`."
[]
(as-> (t2/select Segment, :archived false, {:order-by [[:%lower.name :asc]]}) segments
(filter mi/can-read? segments)
- (hydrate segments :creator)
- (add-query-descriptions segments)))
+ (hydrate segments :creator :definition_description)))
(defn- write-check-and-update-segment!
"Check whether current user has write permissions, then update Segment with values in `body`. Publishes appropriate
@@ -114,7 +102,6 @@
(write-check-and-update-segment! id {:archived true, :revision_message revision_message})
api/generic-204-no-content)
-
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema GET "/:id/revisions"
"Fetch `Revisions` for `Segment` with ID."
@@ -122,7 +109,6 @@
(api/read-check Segment id)
(revision/revisions+details Segment id))
-
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema POST "/:id/revert"
"Revert a `Segement` to a prior `Revision`."
diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc
index 3aa20c3eead..aa1426984c2 100644
--- a/src/metabase/lib/aggregation.cljc
+++ b/src/metabase/lib/aggregation.cljc
@@ -24,7 +24,7 @@
:stage-number stage-number})))
(nth aggregations index)))
-(defmethod lib.metadata.calculation/describe-top-level-key :aggregation
+(defmethod lib.metadata.calculation/describe-top-level-key-method :aggregation
[query stage-number _k]
(when-let [aggregations (not-empty (:aggregation (lib.util/query-stage query stage-number)))]
(lib.util/join-strings-with-conjunction
diff --git a/src/metabase/lib/breakout.cljc b/src/metabase/lib/breakout.cljc
index edcdc280154..2c1ac99b4ef 100644
--- a/src/metabase/lib/breakout.cljc
+++ b/src/metabase/lib/breakout.cljc
@@ -9,7 +9,7 @@
[metabase.shared.util.i18n :as i18n]
[metabase.util.malli :as mu]))
-(defmethod lib.metadata.calculation/describe-top-level-key :breakout
+(defmethod lib.metadata.calculation/describe-top-level-key-method :breakout
[query stage-number _k]
(when-let [breakouts (not-empty (:breakout (lib.util/query-stage query stage-number)))]
(i18n/tru "Grouped by {0}"
diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc
index f77735e4822..38c83573fb9 100644
--- a/src/metabase/lib/convert.cljc
+++ b/src/metabase/lib/convert.cljc
@@ -68,6 +68,15 @@
(let [[tag opts & args] (->pMBQL aggregation)]
(into [tag (merge opts options)] args)))
+(defn legacy-query-from-inner-query
+ "Convert a legacy 'inner query' to a full legacy 'outer query' so you can pass it to stuff
+ like [[metabase.mbql.normalize/normalize]], and then probably to [[->pMBQL]]."
+ [database-id inner-query]
+ (merge {:database database-id, :type :query}
+ (if (:native inner-query)
+ {:native (set/rename-keys inner-query {:native :query})}
+ {:query inner-query})))
+
(defmulti ->legacy-MBQL
"Coerce something to legacy MBQL (the version of MBQL understood by the query processor and Metabase Lib v1) if it's
not already legacy MBQL."
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index a148d11410e..849e36c9a50 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -16,6 +16,7 @@
[metabase.lib.metric :as lib.metric]
[metabase.lib.order-by :as lib.order-by]
[metabase.lib.query :as lib.query]
+ [metabase.lib.segment :as lib.segment]
[metabase.lib.stage :as lib.stage]
[metabase.lib.table :as lib.table]
[metabase.lib.temporal-bucket :as lib.temporal-bucket]
@@ -33,6 +34,7 @@
lib.metric/keep-me
lib.order-by/keep-me
lib.query/keep-me
+ lib.segment/keep-me
lib.stage/keep-me
lib.table/keep-me
lib.temporal-bucket/keep-me)
@@ -131,6 +133,7 @@
[lib.metadata.calculation
column-name
describe-query
+ describe-top-level-key
display-name
suggested-name]
[lib.order-by
diff --git a/src/metabase/lib/field.cljc b/src/metabase/lib/field.cljc
index 24e3c91ef2f..f7849ce5554 100644
--- a/src/metabase/lib/field.cljc
+++ b/src/metabase/lib/field.cljc
@@ -105,10 +105,12 @@
(defmethod lib.metadata.calculation/display-name-method :field
[query stage-number [_field {:keys [join-alias temporal-unit], :as _opts} _id-or-name, :as field-clause]]
- (let [field-metadata (cond-> (resolve-field-metadata query stage-number field-clause)
- join-alias (assoc :source_alias join-alias)
- temporal-unit (assoc :unit temporal-unit))]
- (lib.metadata.calculation/display-name query stage-number field-metadata)))
+ (if-let [field-metadata (cond-> (resolve-field-metadata query stage-number field-clause)
+ join-alias (assoc :source_alias join-alias)
+ temporal-unit (assoc :unit temporal-unit))]
+ (lib.metadata.calculation/display-name query stage-number field-metadata)
+ ;; mostly for the benefit of JS, which does not enforce the Malli schemas.
+ (i18n/tru "[Unknown Field]")))
(defmulti ^:private ->field
{:arglists '([query stage-number field])}
diff --git a/src/metabase/lib/filter.cljc b/src/metabase/lib/filter.cljc
index b2ea06bdc3a..430b80b1c46 100644
--- a/src/metabase/lib/filter.cljc
+++ b/src/metabase/lib/filter.cljc
@@ -16,7 +16,7 @@
(comment metabase.lib.schema/keep-me)
-(defmethod lib.metadata.calculation/describe-top-level-key :filter
+(defmethod lib.metadata.calculation/describe-top-level-key-method :filter
[query stage-number _key]
(when-let [filter-clause (:filter (lib.util/query-stage query stage-number))]
(i18n/tru "Filtered by {0}" (lib.metadata.calculation/display-name query stage-number filter-clause))))
diff --git a/src/metabase/lib/js/metadata.cljs b/src/metabase/lib/js/metadata.cljs
index 734b0c0a309..3c9fcad9814 100644
--- a/src/metabase/lib/js/metadata.cljs
+++ b/src/metabase/lib/js/metadata.cljs
@@ -87,7 +87,7 @@
(fn [object]
(try
(let [parsed (assoc (obj->clj xform object) :lib/type lib-type-name)]
- (log/infof "Parsed metadata %s %s\n%s" object-type (:id parsed) (u/pprint-to-str parsed))
+ (log/debugf "Parsed metadata %s %s\n%s" object-type (:id parsed) (u/pprint-to-str parsed))
parsed)
(catch js/Error e
(log/errorf e "Error parsing %s %s: %s" object-type (pr-str object) (ex-message e))
@@ -304,7 +304,7 @@
"Use a `metabase-lib/metadata/Metadata` as a [[metabase.lib.metadata.protocols/MetadataProvider]]."
[database-id metadata]
(let [metadata (parse-metadata metadata)]
- (log/info "Created metadata provider for metadata")
+ (log/debug "Created metadata provider for metadata")
(reify lib.metadata.protocols/MetadataProvider
(database [_this] (database metadata database-id))
(table [_this table-id] (table metadata table-id))
diff --git a/src/metabase/lib/limit.cljc b/src/metabase/lib/limit.cljc
index dd6780fa3cd..29851c33728 100644
--- a/src/metabase/lib/limit.cljc
+++ b/src/metabase/lib/limit.cljc
@@ -7,7 +7,7 @@
[metabase.shared.util.i18n :as i18n]
[metabase.util.malli :as mu]))
-(defmethod lib.metadata.calculation/describe-top-level-key :limit
+(defmethod lib.metadata.calculation/describe-top-level-key-method :limit
[query stage-number _k]
(when-let [limit (:limit (lib.util/query-stage query stage-number))]
(str limit \space (i18n/trun "row" "rows" limit))))
diff --git a/src/metabase/lib/metadata.cljc b/src/metabase/lib/metadata.cljc
index 958689e8963..72c3429ef1e 100644
--- a/src/metabase/lib/metadata.cljc
+++ b/src/metabase/lib/metadata.cljc
@@ -44,7 +44,8 @@
[:map
[:lib/type [:= :metadata/field]] ; TODO -- should this be changed to `:metadata/column`?
[:id {:optional true} ::lib.schema.id/field]
- [:name ::lib.schema.common/non-blank-string]])
+ [:name ::lib.schema.common/non-blank-string]
+ [:display_name {:optional true} [:maybe ::lib.schema.common/non-blank-string]]])
(def ^:private CardMetadata
[:map
@@ -69,7 +70,8 @@
[:lib/type [:= :metadata/table]]
[:id ::lib.schema.id/table]
[:name ::lib.schema.common/non-blank-string]
- [:schema {:optional true} [:maybe ::lib.schema.common/non-blank-string]]
+ [:display_name {:optional true} [:maybe ::lib.schema.common/non-blank-string]]
+ [:schema {:optional true} [:maybe ::lib.schema.common/non-blank-string]]
;; This is now optional! If the [[MetadataProvider]] provides it, great, but if not we can always make the
;; subsequent request to fetch fields separately.
[:fields {:optional true} [:maybe [:sequential ColumnMetadata]]]
diff --git a/src/metabase/lib/metadata/cached_provider.cljc b/src/metabase/lib/metadata/cached_provider.cljc
new file mode 100644
index 00000000000..2995280baad
--- /dev/null
+++ b/src/metabase/lib/metadata/cached_provider.cljc
@@ -0,0 +1,71 @@
+(ns metabase.lib.metadata.cached-provider
+ (:require
+ [clojure.set :as set]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+ [metabase.util.log :as log]
+ #?@(:clj ([pretty.core :as pretty]))))
+
+(defn- get-in-cache [cache ks]
+ (when-some [cached-value (get-in @cache ks)]
+ (when-not (= cached-value ::nil)
+ cached-value)))
+
+(defn- store-in-cache! [cache ks value]
+ (let [value (if (some? value) value ::nil)]
+ (swap! cache assoc-in ks value)
+ value))
+
+(defn- get-in-cache-or-fetch [cache ks fetch-thunk]
+ (if-some [cached-value (get-in @cache ks)]
+ (when-not (= cached-value ::nil)
+ cached-value)
+ (store-in-cache! cache ks (fetch-thunk))))
+
+(defn- bulk-metadata [cache uncached-provider metadata-type ids]
+ (when (seq ids)
+ (log/debugf "Getting %s metadata with IDs %s" metadata-type (pr-str (sort ids)))
+ (let [existing-ids (set (keys (get @cache metadata-type)))
+ missing-ids (set/difference (set ids) existing-ids)]
+ (log/debugf "Already fetched %s: %s" metadata-type (pr-str (sort (set/intersection (set ids) existing-ids))))
+ (when (seq missing-ids)
+ (log/debugf "Need to fetch %s: %s" metadata-type (pr-str (sort missing-ids)))
+ ;; TODO -- we should probably store `::nil` markers for things we tried to fetch that didn't exist
+ (doseq [instance (lib.metadata.protocols/bulk-metadata uncached-provider metadata-type missing-ids)]
+ (store-in-cache! cache [metadata-type (:id instance)] instance))))
+ (for [id ids]
+ (get-in-cache cache [metadata-type id]))))
+
+(deftype CachedProxyMetadataProvider [cache metadata-provider]
+ lib.metadata.protocols/MetadataProvider
+ (database [_this] (get-in-cache-or-fetch cache [:metadata/database] #(lib.metadata.protocols/database metadata-provider)))
+ (table [_this table-id] (get-in-cache-or-fetch cache [:metadata/table table-id] #(lib.metadata.protocols/table metadata-provider table-id)))
+ (field [_this field-id] (get-in-cache-or-fetch cache [:metadata/field field-id] #(lib.metadata.protocols/field metadata-provider field-id)))
+ (card [_this card-id] (get-in-cache-or-fetch cache [:metadata/card card-id] #(lib.metadata.protocols/card metadata-provider card-id)))
+ (metric [_this metric-id] (get-in-cache-or-fetch cache [:metadata/metric metric-id] #(lib.metadata.protocols/metric metadata-provider metric-id)))
+ (segment [_this segment-id] (get-in-cache-or-fetch cache [:metadata/segment segment-id] #(lib.metadata.protocols/segment metadata-provider segment-id)))
+ (tables [_this] (get-in-cache-or-fetch cache [::database-tables] #(lib.metadata.protocols/tables metadata-provider)))
+ (fields [_this table-id] (get-in-cache-or-fetch cache [::table-fields table-id] #(lib.metadata.protocols/fields metadata-provider table-id)))
+
+ lib.metadata.protocols/CachedMetadataProvider
+ (cached-database [_this] (get-in-cache cache [:metadata/database]))
+ (cached-metadata [_this metadata-type id] (get-in-cache cache [metadata-type id]))
+ (store-database! [_this database-metadata] (store-in-cache! cache [:metadata/database] (assoc database-metadata :lib/type :metadata/database)))
+ (store-metadata! [_this metadata-type id metadata] (store-in-cache! cache [metadata-type id] (assoc metadata :lib/type metadata-type)))
+
+ ;; these only work if the underlying metadata provider is also a [[BulkMetadataProvider]].
+ lib.metadata.protocols/BulkMetadataProvider
+ (bulk-metadata [_this metadata-type ids]
+ (bulk-metadata cache metadata-provider metadata-type ids))
+
+ #?@(:clj
+ [pretty.core/PrettyPrintable
+ (pretty [_this]
+ (list `->CachedProxyMetadataProvider cache metadata-provider))]))
+
+(defn cached-metadata-provider
+ "Wrap `metadata-provider` with an implementation that automatically caches results.
+
+ If the metadata provider implements [[lib.metadata.protocols/BulkMetadataProvider]],
+ then [[lib.metadata.protocols/bulk-metadata]] will work as expected; it can be done for side-effects as well."
+ ^CachedProxyMetadataProvider [metadata-provider]
+ (->CachedProxyMetadataProvider (atom {}) metadata-provider))
diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc
index fdc6f8205e8..70e1d8daaf9 100644
--- a/src/metabase/lib/metadata/calculation.cljc
+++ b/src/metabase/lib/metadata/calculation.cljc
@@ -24,22 +24,25 @@
(fn [_query _stage-number x]
(lib.dispatch/dispatch-value x)))
-(mu/defn display-name :- ::lib.schema.common/non-blank-string
+(mu/defn ^:export display-name :- ::lib.schema.common/non-blank-string
"Calculate a nice human-friendly display name for something."
- [query :- ::lib.schema/query
- stage-number :- :int
- x]
- (or
- ;; if this is an MBQL clause with `:display-name` in the options map, then use that rather than calculating a name.
- (:display-name (lib.options/options x))
- (try
- (display-name-method query stage-number x)
- ;; if this errors, just catch the error and return something like `Unknown :field`. We shouldn't blow up the
- ;; whole Query Builder if there's a bug
- (catch #?(:clj Throwable :cljs js/Error) e
- (throw (ex-info (i18n/tru "Error calculating display name for {0}: {1}" (pr-str x) (ex-message e))
- {:query query, :x x}
- e))))))
+ ([query x]
+ (display-name query -1 x))
+
+ ([query :- ::lib.schema/query
+ stage-number :- :int
+ x]
+ (or
+ ;; if this is an MBQL clause with `:display-name` in the options map, then use that rather than calculating a name.
+ (:display-name (lib.options/options x))
+ (try
+ (display-name-method query stage-number x)
+ ;; if this errors, just catch the error and return something like `Unknown :field`. We shouldn't blow up the
+ ;; whole Query Builder if there's a bug
+ (catch #?(:clj Throwable :cljs js/Error) e
+ (throw (ex-info (i18n/tru "Error calculating display name for {0}: {1}" (pr-str x) (ex-message e))
+ {:query query, :x x}
+ e)))))))
(mu/defn column-name :- ::lib.schema.common/non-blank-string
"Calculate a database-friendly name to use for an expression."
@@ -63,7 +66,7 @@
;; hopefully this is dev-facing only, so not i18n'ed.
(log/warnf "Don't know how to calculate display name for %s. Add an impl for %s for %s"
(pr-str x)
- `display-name*
+ `display-name-method
(lib.dispatch/dispatch-value x))
(if (and (vector? x)
(keyword? (first x)))
@@ -84,13 +87,27 @@
[query stage-number x]
(slugify (display-name query stage-number x)))
-(defmulti describe-top-level-key
- "'top-level' here means the top level of an individual stage. Describe part of a stage of a query, e.g. the `:filter`
- part or the `:aggregation` part."
+(defmulti describe-top-level-key-method
+ "Implementation for [[describe-top-level-key]]. Describe part of a stage of a query, e.g. the `:filter` part or the
+ `:aggregation` part. Return `nil` if there is nothing to describe."
{:arglists '([query stage-number top-level-key])}
(fn [_query _stage-number top-level-key]
top-level-key))
+(def ^:private TopLevelKey
+ "In the interest of making this easy to use in JS-land we'll accept either strings or keywords."
+ [:enum :aggregation :breakout :filter :limit :order-by :source-table])
+
+(mu/defn describe-top-level-key :- [:maybe ::lib.schema.common/non-blank-string]
+ "'top-level' here means the top level of an individual stage. Generate a human-friendly string describing a specific
+ part of an MBQL stage, or `nil` if that part doesn't exist."
+ ([query top-level-key]
+ (describe-top-level-key query -1 top-level-key))
+ ([query :- ::lib.schema/query
+ stage-number :- :int
+ top-level-key :- TopLevelKey]
+ (describe-top-level-key-method query stage-number (keyword top-level-key))))
+
(defmulti 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
@@ -110,7 +127,7 @@
(mu/defn describe-query :- ::lib.schema.common/non-blank-string
"Convenience for calling [[display-name]] on a query to describe the results of its final stage."
[query]
- (display-name query -1 query))
+ (display-name query query))
(mu/defn suggested-name :- [:maybe ::lib.schema.common/non-blank-string]
"Name you might want to use for a query when saving an previously-unsaved query. This is the same
diff --git a/src/metabase/lib/metadata/graph_provider.cljc b/src/metabase/lib/metadata/graph_provider.cljc
new file mode 100644
index 00000000000..bc62d0b4021
--- /dev/null
+++ b/src/metabase/lib/metadata/graph_provider.cljc
@@ -0,0 +1,56 @@
+(ns metabase.lib.metadata.graph-provider
+ (:require [medley.core :as m]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]))
+
+(defn- graph-database [metadata-graph]
+ (dissoc metadata-graph :tables))
+
+(defn- graph-table [metadata-graph table-id]
+ (some (fn [table-metadata]
+ (when (= (:id table-metadata) table-id)
+ (dissoc table-metadata :fields :metrics :segments)))
+ (:tables metadata-graph)))
+
+(defn- graph-field [metadata-graph field-id]
+ (some (fn [table-metadata]
+ (m/find-first #(= (:id %) field-id)
+ (:fields table-metadata)))
+ (:tables metadata-graph)))
+
+(defn- graph-metric [metadata-graph metric-id]
+ (some (fn [table-metadata]
+ (m/find-first #(= (:id %) metric-id)
+ (:metrics table-metadata)))
+ (:tables metadata-graph)))
+
+(defn- graph-segment [metadata-graph segment-id]
+ (some (fn [table-metadata]
+ (m/find-first #(= (:id %) segment-id)
+ (:segments table-metadata)))
+ (:tables metadata-graph)))
+
+(defn- graph-card [_metadata-graph _card-id]
+ ;; not implemented for the simple graph metadata provider.
+ nil)
+
+(defn- graph-tables [metadata-graph]
+ (for [table-metadata (:tables metadata-graph)]
+ (dissoc table-metadata :fields :metrics :segments)))
+
+(defn- graph-fields [metadata-graph table-id]
+ (some (fn [table-metadata]
+ (when (= (:id table-metadata) table-id)
+ (:fields table-metadata)))
+ (:tables metadata-graph)))
+
+(deftype ^{:doc "A simple implementation of [[MetadataProvider]] that returns data from a complete graph
+ e.g. the response provided by `GET /api/database/:id/metadata`."} SimpleGraphMetadataProvider [metadata-graph]
+ lib.metadata.protocols/MetadataProvider
+ (database [_this] (graph-database metadata-graph))
+ (table [_this table-id] (graph-table metadata-graph table-id))
+ (field [_this field-id] (graph-field metadata-graph field-id))
+ (metric [_this metric-id] (graph-metric metadata-graph metric-id))
+ (segment [_this segment-id] (graph-segment metadata-graph segment-id))
+ (card [_this card-id] (graph-card metadata-graph card-id))
+ (tables [_this] (graph-tables metadata-graph))
+ (fields [_this table-id] (graph-fields metadata-graph table-id)))
diff --git a/src/metabase/lib/metadata/jvm.clj b/src/metabase/lib/metadata/jvm.clj
new file mode 100644
index 00000000000..0f43e9bd0a4
--- /dev/null
+++ b/src/metabase/lib/metadata/jvm.clj
@@ -0,0 +1,86 @@
+(ns metabase.lib.metadata.jvm
+ "Implementation(s) of [[metabase.lib.metadata.protocols/MetadataProvider]] only for the JVM."
+ (:require
+ [metabase.lib.metadata.cached-provider :as lib.metadata.cached-provider]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+ [metabase.util.log :as log]
+ [potemkin :as p]
+ [pretty.core :as pretty]
+ [toucan2.core :as t2]))
+
+(defn- metadata-type->model [metadata-type]
+ (case metadata-type
+ :metadata/database :metabase.models.database/Database
+ :metadata/table :metabase.models.table/Table
+ :metadata/field :metabase.models.field/Field
+ :metadata/card :metabase.models.card/Card
+ :metadata/metric :metabase.models.metric/Metric
+ :metadata/segment :metabase.models.segment/Segment))
+
+(defn- fetch-instance [metadata-type id]
+ {:pre [(integer? id)]}
+ (let [model (metadata-type->model metadata-type)]
+ (log/debugf "Fetching %s %d" model id)
+ (when-some [instance (t2/select-one model :id id)]
+ (assoc instance :lib/type metadata-type))))
+
+(defn- bulk-instances [metadata-type ids]
+ (let [model (metadata-type->model metadata-type)]
+ (log/debugf "Fetching instances of %s with ID in %s" model (pr-str (sort ids)))
+ (for [instance (t2/select model :id [:in ids])]
+ (assoc instance :lib/type metadata-type))))
+
+(p/deftype+ UncachedApplicationDatabaseMetadataProvider [database-id]
+ lib.metadata.protocols/MetadataProvider
+ (database [_this]
+ (when-not database-id
+ (throw (ex-info (format "Cannot use %s with %s with a nil Database ID"
+ `lib.metadata.protocols/database
+ `UncachedApplicationDatabaseMetadataProvider)
+ {})))
+ (fetch-instance :metadata.models.database/Database database-id))
+
+ (table [_this table-id] (fetch-instance :metadata/table table-id))
+ (field [_this field-id] (fetch-instance :metadata/field field-id))
+ (card [_this card-id] (fetch-instance :metadata/card card-id))
+ (metric [_this metric-id] (fetch-instance :metadata/metric metric-id))
+ (segment [_this segment-id] (fetch-instance :metadata/segment segment-id))
+
+ (tables [_this]
+ (when-not database-id
+ (throw (ex-info (format "Cannot use %s with %s with a nil Database ID"
+ `lib.metadata.protocols/tables
+ `UncachedApplicationDatabaseMetadataProvider)
+ {})))
+ (log/debugf "Fetching all Tables for Database %d" database-id)
+ (into []
+ (map #(assoc % :lib/type :metadata/table))
+ (t2/reducible-select :metabase.models.table/Table :db_id database-id)))
+
+ (fields [_this table-id]
+ (log/debugf "Fetching all Fields for Table %d" table-id)
+ (into []
+ (map #(assoc % :lib/type :metadata/field))
+ (t2/reducible-select :table_id table-id)))
+
+ lib.metadata.protocols/BulkMetadataProvider
+ (bulk-metadata [_this metadata-type ids]
+ (bulk-instances metadata-type ids))
+
+ pretty/PrettyPrintable
+ (pretty [_this]
+ (list `->UncachedApplicationDatabaseMetadataProvider database-id)))
+
+(defn application-database-metadata-provider
+ "An implementation of [[metabase.lib.metadata.protocols/MetadataProvider]] for the application database.
+
+ The application database metadata provider implements both of the optional
+ protocols, [[metabase.lib.metadata.protocols/CachedMetadataProvider]]
+ and [[metabase.lib.metadata.protocols/BulkMetadataProvider]]. All operations are cached; so you can use the bulk
+ operations to pre-warm the cache if you need to."
+ ([]
+ (application-database-metadata-provider nil))
+
+ ([database-id]
+ (lib.metadata.cached-provider/cached-metadata-provider
+ (->UncachedApplicationDatabaseMetadataProvider database-id))))
diff --git a/src/metabase/lib/metadata/protocols.cljc b/src/metabase/lib/metadata/protocols.cljc
index 40981760ca5..6b8045930e9 100644
--- a/src/metabase/lib/metadata/protocols.cljc
+++ b/src/metabase/lib/metadata/protocols.cljc
@@ -1,8 +1,6 @@
(ns metabase.lib.metadata.protocols
(:require
- [medley.core :as m]
- #?@(:clj ([potemkin :as p]
- [pretty.core :as pretty]))))
+ #?@(:clj ([potemkin :as p]))))
(#?(:clj p/defprotocol+ :cljs defprotocol) MetadataProvider
"Protocol for something that we can get information about Tables and Fields from. This can be provided in various ways
@@ -26,26 +24,26 @@
For all of these methods: if no matching object can be found, you should generally return `nil` rather than throwing
an Exception. Let [[metabase.lib.metadata]] worry about throwing exceptions."
- (database [this]
+ (database [metadata-provider]
"Metadata about the Database we're querying. Should match the [[metabase.lib.metadata/DatabaseMetadata]] schema.
This includes important info such as the supported `:features` and the like.")
- (table [this table-id]
+ (table [metadata-provider table-id]
"Return metadata for a specific Table. Metadata should satisfy [[metabase.lib.metadata/TableMetadata]].")
- (field [this field-id]
+ (field [metadata-provider field-id]
"Return metadata for a specific Field. Metadata should satisfy [[metabase.lib.metadata/ColumnMetadata]].")
- (card [this card-id]
+ (card [metadata-provider card-id]
"Return information about a specific Saved Question, aka a Card. This should
match [[metabase.lib.metadata/CardMetadata]. Currently just used for display name purposes if you have a Card as a
source query.")
- (metric [this metric-id]
+ (metric [metadata-provider metric-id]
"Return metadata for a particular capital-M Metric, i.e. something from the `metric` table in the application
database. Metadata should match [[metabase.lib.metadata/MetricMetadata]].")
- (segment [this segment-id]
+ (segment [metadata-provider segment-id]
"Return metadata for a particular captial-S Segment, i.e. something from the `segment` table in the application
database. Metadata should match [[metabase.lib.metadata/SegmentMetadata]]." )
@@ -54,14 +52,14 @@
;; I'm on the fence about maybe putting these in a different protocol. They're part of this protocol for now tho so
;; implement them anyway.
- (tables [this]
+ (tables [metadata-provider]
"Return a sequence of Tables in this Database. Tables should satisfy the [[metabase.lib.metadata/TableMetadata]]
schema. This should also include things that serve as 'virtual' tables, e.g. Saved Questions or Models. But users of
MLv2 should not need to know that! If we add support for Super Models or Quantum Questions in the future, they can
just come back from this method in the same shape as everything else, the Query Builder can display them, and the
internals can be tucked away here in MLv2.")
- (fields [this table-id]
+ (fields [metadata-provider table-id]
"Return a sequence of Fields associated with a Table with the given `table-id`. Fields should satisfy
the [[metabase.lib.metadata/ColumnMetadata]] schema. If no such Table exists, this should error."))
@@ -70,64 +68,30 @@
[x]
(satisfies? MetadataProvider x))
-;;;; Graph provider and impl
-
-(defn- graph-database [metadata-graph]
- (dissoc metadata-graph :tables))
-
-(defn- graph-table [metadata-graph table-id]
- (some (fn [table-metadata]
- (when (= (:id table-metadata) table-id)
- (dissoc table-metadata :fields :metrics :segments)))
- (:tables metadata-graph)))
-
-(defn- graph-field [metadata-graph field-id]
- (some (fn [table-metadata]
- (m/find-first #(= (:id %) field-id)
- (:fields table-metadata)))
- (:tables metadata-graph)))
-
-(defn- graph-metric [metadata-graph metric-id]
- (some (fn [table-metadata]
- (m/find-first #(= (:id %) metric-id)
- (:metrics table-metadata)))
- (:tables metadata-graph)))
-
-(defn- graph-segment [metadata-graph segment-id]
- (some (fn [table-metadata]
- (m/find-first #(= (:id %) segment-id)
- (:segments table-metadata)))
- (:tables metadata-graph)))
-
-(defn- graph-card [_metadata-graph _card-id]
- ;; not implemented for the simple graph metadata provider.
- nil)
-
-(defn- graph-tables [metadata-graph]
- (for [table-metadata (:tables metadata-graph)]
- (dissoc table-metadata :fields :metrics :segments)))
-
-(defn- graph-fields [metadata-graph table-id]
- (some (fn [table-metadata]
- (when (= (:id table-metadata) table-id)
- (:fields table-metadata)))
- (:tables metadata-graph)))
-
-(deftype ^{:doc "A simple implementation of [[MetadataProvider]] that returns data from a complete graph
- e.g. the response provided by `GET /api/database/:id/metadata`."} SimpleGraphMetadataProvider [metadata-graph]
- MetadataProvider
- (database [_this] (graph-database metadata-graph))
- (table [_this table-id] (graph-table metadata-graph table-id))
- (field [_this field-id] (graph-field metadata-graph field-id))
- (metric [_this metric-id] (graph-metric metadata-graph metric-id))
- (segment [_this segment-id] (graph-segment metadata-graph segment-id))
- (card [_this card-id] (graph-card metadata-graph card-id))
- (tables [_this] (graph-tables metadata-graph))
- (fields [_this table-id] (graph-fields metadata-graph table-id))
-
- #?@(:clj
- [pretty/PrettyPrintable
- (pretty [_this]
- ;; don't actually print the whole thing because it's going to make my eyes bleed to see all
- ;; of [[metabase.lib.test-metadata]] every single time a test fails
- `SimpleGraphMetadataProvider)]))
+(#?(:clj p/defprotocol+ :cljs defprotocol) CachedMetadataProvider
+ "Optional. A protocol for a MetadataProvider that some sort of internal cache. This is mostly useful for MetadataProviders that
+ can hit some sort of relatively expensive external service,
+ e.g. [[metabase.lib.metadata.jvm/application-database-metadata-provider]].
+
+ See [[cached-metadata-provider]] below to wrap for a way to wrap an existing MetadataProvider to add caching on top
+ of it."
+ (cached-database [cached-metadata-provider]
+ "Get cached metadata for the query's Database.")
+ (cached-metadata [cached-metadata-provider metadata-type id]
+ "Get cached metadata of a specific type, e.g. `:metadata/table`.")
+ (store-database! [cached-metadata-provider database-metadata]
+ "Store metadata for the query's Database.")
+ (store-metadata! [cached-metadata-provider metadata-type id metadata]
+ "Store metadata of a specific type, e.g. `:metadata/table`."))
+
+(#?(:clj p/defprotocol+ :cljs defprotocol) BulkMetadataProvider
+ "A protocol for a MetadataProvider that can fetch several objects in a single batched operation. This is mostly
+ useful for MetadataProviders e.g. [[metabase.lib.metadata.jvm/application-database-metadata-provider]]."
+ (bulk-metadata [bulk-metadata-provider metadata-type ids]
+ "Fetch lots of metadata of a specific type, e.g. `:metadata/table`, in a single bulk operation."))
+
+(defn store-metadatas!
+ "Convenience. Store several metadata maps at once."
+ [cached-metadata-provider metadata-type metadatas]
+ (doseq [metadata metadatas]
+ (store-metadata! cached-metadata-provider metadata-type (:id metadata) metadata)))
diff --git a/src/metabase/lib/metric.cljc b/src/metabase/lib/metric.cljc
index 13dd7fd59a6..2a7fee07c65 100644
--- a/src/metabase/lib/metric.cljc
+++ b/src/metabase/lib/metric.cljc
@@ -7,11 +7,11 @@
(defmethod lib.metadata.calculation/display-name-method :metadata/metric
[_query _stage-number metric-metadata]
(or ((some-fn :display_name :name) metric-metadata)
- (i18n/tru "Metric")))
+ (i18n/tru "[Unknown Metric]")))
(defmethod lib.metadata.calculation/display-name-method :metric
[query stage-number [_tag _opts metric-id-or-name]]
(or (when (integer? metric-id-or-name)
(when-let [metric-metadata (lib.metadata/metric query metric-id-or-name)]
(lib.metadata.calculation/display-name query stage-number metric-metadata)))
- (i18n/tru "Metric")))
+ (i18n/tru "[Unknown Metric]")))
diff --git a/src/metabase/lib/order_by.cljc b/src/metabase/lib/order_by.cljc
index c4f7814054b..6d11f6f8425 100644
--- a/src/metabase/lib/order_by.cljc
+++ b/src/metabase/lib/order_by.cljc
@@ -15,7 +15,7 @@
[metabase.shared.util.i18n :as i18n]
[metabase.util.malli :as mu]))
-(defmethod lib.metadata.calculation/describe-top-level-key :order-by
+(defmethod lib.metadata.calculation/describe-top-level-key-method :order-by
[query stage-number _k]
(when-let [order-bys (not-empty (:order-by (lib.util/query-stage query stage-number)))]
(i18n/tru "Sorted by {0}"
diff --git a/src/metabase/lib/query.cljc b/src/metabase/lib/query.cljc
index 81b7852e635..f3e0d628b35 100644
--- a/src/metabase/lib/query.cljc
+++ b/src/metabase/lib/query.cljc
@@ -1,10 +1,12 @@
(ns metabase.lib.query
(:require
+ [metabase.lib.convert :as lib.convert]
[metabase.lib.dispatch :as lib.dispatch]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.options :as lib.options]
[metabase.lib.schema :as lib.schema]
+ [metabase.lib.schema.id :as lib.schema.id]
[metabase.lib.util :as lib.util]
[metabase.util.malli :as mu]))
@@ -16,12 +18,25 @@
[query stage-number x]
(lib.metadata.calculation/display-name query stage-number (lib.util/query-stage x stage-number)))
-(defn- query-with-stages [metadata-provider stages]
- {:lib/type :mbql/query
- :lib/metadata metadata-provider
- :database (:id (lib.metadata/database metadata-provider))
- :type :pipeline
- :stages (mapv lib.options/ensure-uuid stages)})
+(defn query-with-stages
+ "Create a query from a sequence of stages."
+ ([metadata-provider stages]
+ (query-with-stages (:id (lib.metadata/database metadata-provider)) metadata-provider stages))
+
+ ([database-id metadata-provider stages]
+ {:lib/type :mbql/query
+ :lib/metadata metadata-provider
+ :database database-id
+ :type :pipeline
+ :stages (mapv lib.options/ensure-uuid stages)}))
+
+(defn query-with-stage
+ "Create a query from a specific stage."
+ ([metadata-provider stage]
+ (query-with-stages metadata-provider [stage]))
+
+ ([database-id metadata-provider stage]
+ (query-with-stages database-id metadata-provider [stage])))
(defn- query-from-existing [metadata-provider query]
(let [query (lib.util/pipeline query)]
@@ -84,3 +99,12 @@
metadata
(lib.util/update-query-stage -1 assoc :lib/stage-metadata metadata))]
(query metadata-provider mbql-query)))
+
+(mu/defn query-from-legacy-inner-query :- ::lib.schema/query
+ "Create a pMBQL query from a legacy inner query."
+ [metadata-provider :- lib.metadata/MetadataProvider
+ database-id :- ::lib.schema.id/database
+ inner-query :- :map]
+ (->> (lib.convert/legacy-query-from-inner-query database-id inner-query)
+ lib.convert/->pMBQL
+ (query metadata-provider)))
diff --git a/src/metabase/lib/segment.cljc b/src/metabase/lib/segment.cljc
new file mode 100644
index 00000000000..e77cf376ec5
--- /dev/null
+++ b/src/metabase/lib/segment.cljc
@@ -0,0 +1,17 @@
+(ns metabase.lib.segment
+ (:require
+ [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.metadata.calculation :as lib.metadata.calculation]
+ [metabase.shared.util.i18n :as i18n]))
+
+(defmethod lib.metadata.calculation/display-name-method :metadata/segment
+ [_query _stage-number segment-metadata]
+ (or ((some-fn :display_name :name) segment-metadata)
+ (i18n/tru "[Unknown Segment]")))
+
+(defmethod lib.metadata.calculation/display-name-method :segment
+ [query stage-number [_tag _opts segment-id-or-name]]
+ (or (when (integer? segment-id-or-name)
+ (when-let [segment-metadata (lib.metadata/segment query segment-id-or-name)]
+ (lib.metadata.calculation/display-name query stage-number segment-metadata)))
+ (i18n/tru "[Unknown Segment]")))
diff --git a/src/metabase/lib/stage.cljc b/src/metabase/lib/stage.cljc
index 24395ec728b..74eafaebfbd 100644
--- a/src/metabase/lib/stage.cljc
+++ b/src/metabase/lib/stage.cljc
@@ -203,8 +203,7 @@
(defmethod lib.metadata.calculation/display-name-method :mbql.stage/mbql
[query stage-number _stage]
- (let [parts display-name-parts
- descriptions (for [k parts]
+ (let [descriptions (for [k display-name-parts]
(lib.metadata.calculation/describe-top-level-key query stage-number k))]
(str/join ", " (remove str/blank? descriptions))))
diff --git a/src/metabase/lib/table.cljc b/src/metabase/lib/table.cljc
index ed0864134a3..5ef144bdf67 100644
--- a/src/metabase/lib/table.cljc
+++ b/src/metabase/lib/table.cljc
@@ -28,7 +28,7 @@
;; If for some reason the metadata is unavailable. This is better than returning nothing I guess
(i18n/tru "Saved Question {0}" card-id)))))
-(defmethod lib.metadata.calculation/describe-top-level-key :source-table
+(defmethod lib.metadata.calculation/describe-top-level-key-method :source-table
[query stage-number _k]
(let [stage (lib.util/query-stage query stage-number)]
(when-let [source-table-id (:source-table stage)]
diff --git a/src/metabase/models/metric.clj b/src/metabase/models/metric.clj
index d38e13d1f5d..f473694a5ea 100644
--- a/src/metabase/models/metric.clj
+++ b/src/metabase/models/metric.clj
@@ -5,13 +5,23 @@
(:require
[clojure.set :as set]
[medley.core :as m]
+ [metabase.lib.core :as lib]
+ [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.metadata.jvm :as lib.metadata.jvm]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+ [metabase.lib.query :as lib.query]
+ [metabase.lib.schema.common :as lib.schema.common]
+ [metabase.mbql.util :as mbql.u]
[metabase.models.interface :as mi]
[metabase.models.revision :as revision]
[metabase.models.serialization :as serdes]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
+ [metabase.util.malli :as mu]
+ [methodical.core :as methodical]
[toucan.models :as models]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2]
+ [toucan2.tools.hydrate :as t2.hydrate]))
(models/defmodel Metric :metric)
@@ -40,9 +50,38 @@
::mi/entity-id true})
:pre-update pre-update})
-(defmethod serdes/hash-fields Metric
- [_metric]
- [:name (serdes/hydrated-hash :table) :created_at])
+(mu/defn ^:private definition-description :- [:maybe ::lib.schema.common/non-blank-string]
+ "Calculate a nice description of a Metric's definition."
+ [metadata-provider :- lib.metadata/MetadataProvider
+ {:keys [definition], table-id :table_id, :as _metric}]
+ (when (seq definition)
+ (when-let [{database-id :db_id} (lib.metadata.protocols/table metadata-provider table-id)]
+ (let [query (lib.query/query-from-legacy-inner-query metadata-provider database-id definition)]
+ (lib/describe-query query)))))
+
+(defn- warmed-metadata-provider [metrics]
+ (let [metadata-provider (doto (lib.metadata.jvm/application-database-metadata-provider)
+ (lib.metadata.protocols/store-metadatas! :metadata/metric metrics))
+ segment-ids (into #{} (mbql.u/match (map :definition metrics)
+ [:segment (id :guard integer?) & _]
+ id))
+ segments (lib.metadata.protocols/bulk-metadata metadata-provider :metadata/segment segment-ids)
+ field-ids (mbql.u/referenced-field-ids (into []
+ (comp cat (map :definition))
+ [metrics segments]))
+ fields (lib.metadata.protocols/bulk-metadata metadata-provider :metadata/field field-ids)
+ table-ids (into #{}
+ (comp cat (map :table_id))
+ [fields segments metrics])]
+ ;; this is done for side-effects
+ (lib.metadata.protocols/bulk-metadata metadata-provider :metadata/table table-ids)
+ metadata-provider))
+
+(methodical/defmethod t2.hydrate/batched-hydrate [Metric :definition_description]
+ [_model _key metrics]
+ (let [metadata-provider (warmed-metadata-provider metrics)]
+ (for [metric metrics]
+ (assoc metric :definition_description (definition-description metadata-provider metric)))))
;;; --------------------------------------------------- REVISIONS ----------------------------------------------------
@@ -70,6 +109,11 @@
;;; ------------------------------------------------- SERIALIZATION --------------------------------------------------
+
+(defmethod serdes/hash-fields Metric
+ [_metric]
+ [:name (serdes/hydrated-hash :table) :created_at])
+
(defmethod serdes/extract-one "Metric"
[_model-name _opts metric]
(-> (serdes/extract-one-basics "Metric" metric)
diff --git a/src/metabase/models/segment.clj b/src/metabase/models/segment.clj
index 20f5a88f435..4165cd104c1 100644
--- a/src/metabase/models/segment.clj
+++ b/src/metabase/models/segment.clj
@@ -4,13 +4,23 @@
(:require
[clojure.set :as set]
[medley.core :as m]
+ [metabase.lib.core :as lib]
+ [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.metadata.jvm :as lib.metadata.jvm]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+ [metabase.lib.query :as lib.query]
+ [metabase.lib.schema.common :as lib.schema.common]
+ [metabase.mbql.util :as mbql.u]
[metabase.models.interface :as mi]
[metabase.models.revision :as revision]
[metabase.models.serialization :as serdes]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
+ [metabase.util.malli :as mu]
+ [methodical.core :as methodical]
[toucan.models :as models]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2]
+ [toucan2.tools.hydrate :as t2.hydrate]))
(models/defmodel Segment :segment)
@@ -40,9 +50,32 @@
:hydration-keys (constantly [:segment])
:pre-update pre-update})
-(defmethod serdes/hash-fields Segment
- [_segment]
- [:name (serdes/hydrated-hash :table) :created_at])
+(mu/defn ^:private definition-description :- [:maybe ::lib.schema.common/non-blank-string]
+ "Calculate a nice description of a Segment's definition."
+ [metadata-provider :- lib.metadata/MetadataProvider
+ {table-id :table_id, :keys [definition], :as _segment}]
+ (when (seq definition)
+ (when-let [{database-id :db_id} (lib.metadata.protocols/table metadata-provider table-id)]
+ (let [query (lib.query/query-from-legacy-inner-query metadata-provider database-id definition)]
+ (lib/describe-top-level-key query :filter)))))
+
+(defn- warmed-metadata-provider [segments]
+ (let [metadata-provider (doto (lib.metadata.jvm/application-database-metadata-provider)
+ (lib.metadata.protocols/store-metadatas! :metadata/segment segments))
+ field-ids (mbql.u/referenced-field-ids (map :definition segments))
+ fields (lib.metadata.protocols/bulk-metadata metadata-provider :metadata/field field-ids)
+ table-ids (into #{}
+ (comp cat (map :table_id))
+ [fields segments])]
+ ;; this is done for side effects
+ (lib.metadata.protocols/bulk-metadata metadata-provider :metadata/table table-ids)
+ metadata-provider))
+
+(methodical/defmethod t2.hydrate/batched-hydrate [Segment :definition_description]
+ [_model _key segments]
+ (let [metadata-provider (warmed-metadata-provider segments)]
+ (for [segment segments]
+ (assoc segment :definition_description (definition-description metadata-provider segment)))))
;;; --------------------------------------------------- Revisions ----------------------------------------------------
@@ -70,6 +103,11 @@
;;; ------------------------------------------------ Serialization ---------------------------------------------------
+
+(defmethod serdes/hash-fields Segment
+ [_segment]
+ [:name (serdes/hydrated-hash :table) :created_at])
+
(defmethod serdes/extract-one "Segment"
[_model-name _opts segment]
(-> (serdes/extract-one-basics "Segment" segment)
diff --git a/test/metabase/api/metric_test.clj b/test/metabase/api/metric_test.clj
index d2aacb45c00..c6159a8cec0 100644
--- a/test/metabase/api/metric_test.clj
+++ b/test/metabase/api/metric_test.clj
@@ -3,18 +3,15 @@
(:require
[clojure.test :refer :all]
[metabase.http-client :as client]
- [metabase.models.database :refer [Database]]
+ [metabase.models :refer [Database Revision Segment Table]]
[metabase.models.metric :as metric :refer [Metric]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
- [metabase.models.revision :refer [Revision]]
- [metabase.models.table :refer [Table]]
[metabase.server.middleware.util :as mw.util]
[metabase.test :as mt]
- [metabase.test.data :as data]
[metabase.util :as u]
- [toucan.hydrate :refer [hydrate]]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2]
+ [toucan2.tools.with-temp :as t2.with-temp]))
;; ## Helper Fns
@@ -393,18 +390,32 @@
(deftest list-metrics-test
(testing "GET /api/metric/"
- (mt/with-temp* [Metric [metric-1 {:name "Metric A"}]
- Metric [metric-2 {:name "Metric B"}]
- ;; inactive metrics shouldn't show up
- Metric [_ {:archived true}]]
- (is (= (mt/derecordize (hydrate [(assoc metric-1 :database_id (data/id))
- (assoc metric-2 :database_id (data/id))]
- :creator))
- (map #(dissoc % :query_description) (mt/user-http-request
- :rasta :get 200 "metric/"))))))
-
- (is (= []
- (mt/user-http-request :rasta :get 200 "metric/"))))
+ (t2.with-temp/with-temp [Segment {segment-id :id} {:name "Segment"
+ :table_id (mt/id :checkins)
+ :definition (:query (mt/mbql-query checkins
+ {:filter [:= $id 1]}))}
+ Metric {id-1 :id} {:name "Metric A"
+ :table_id (mt/id :users)}
+ Metric {id-2 :id} {:name "Metric B"
+ :definition (:query (mt/mbql-query venues
+ {:aggregation [[:sum $category_id->categories.name]]
+ :filter [:and
+ [:= $price 4]
+ [:segment segment-id]]}))
+ :table_id (mt/id :venues)}
+ ;; inactive metrics shouldn't show up
+ Metric {id-3 :id} {:archived true}]
+ (is (=? [{:name "Metric A"
+ :id id-1
+ :creator {}
+ :definition_description nil}
+ {:name "Metric B"
+ :id id-2
+ :creator {}
+ :definition_description "Venues, Sum of Name, Filtered by Price equals 4 and Segment"}]
+ (filter (fn [{metric-id :id}]
+ (contains? #{id-1 id-2 id-3} metric-id))
+ (mt/user-http-request :rasta :get 200 "metric/")))))))
(deftest metric-related-entities-test
(testing "Test related/recommended entities"
diff --git a/test/metabase/api/query_description_test.clj b/test/metabase/api/query_description_test.clj
deleted file mode 100644
index af4cbcb09b2..00000000000
--- a/test/metabase/api/query_description_test.clj
+++ /dev/null
@@ -1,119 +0,0 @@
-(ns metabase.api.query-description-test
- (:require
- [clojure.test :refer :all]
- [metabase.api.query-description :as api.qd]
- [metabase.models.metric :refer [Metric]]
- [metabase.models.segment :refer [Segment]]
- [metabase.models.table :refer [Table]]
- [metabase.test :as mt]
- [metabase.test.fixtures :as fixtures]
- [metabase.util.i18n :refer [deferred-tru]]
- [toucan.util.test :as tt]
- [toucan2.core :as t2]))
-
-(use-fixtures :once (fixtures/initialize :db))
-
-(deftest metrics-query-test
- (testing "queries"
-
- (testing "without any arguments, just the table"
- (is (= {:table "Venues"}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues))))))
-
- (testing "with limit"
- (is (= {:table "Venues"
- :limit 10}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:limit 10}))))))
-
- (testing "with cumulative sum of price"
- (is (= {:table "Venues"
- :aggregation [{:type :cum-sum
- :arg "Price"}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:aggregation [[:cum-sum $price]]}))))))
- (testing "with equality filter"
- (is (= {:table "Venues"
- :filter [{:field "Price"}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:filter [:= [$price 1234]]}))))))
-
- (testing "with order-by clause"
- (is (= {:table "Venues"
- :order-by [{:field "Price"
- :direction :asc}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:order-by [[:asc $price]]}))))))
-
- (testing "with an aggregation metric"
- (tt/with-temp Metric [metric {:table_id (mt/id :venues)
- :name "Test Metric 1"
- :definition {:aggregation [[:count]]}}]
- (is (= {:table "Venues"
- :aggregation [{:type :metric
- :arg "Test Metric 1"}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:aggregation [[:metric (:id metric)]]}))))))
-
- (is (= {:table "Venues"
- :aggregation [{:type :metric
- :arg (deferred-tru "[Unknown Metric]")}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:aggregation [[:metric -1]]})))))
-
- ;; confirm that it doesn't crash for non-integer metrics
- (is (= {:table "Venues"}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:aggregation [[:metric "not-a-integer"]]}))))))
-
- (testing "with segment filters"
- (tt/with-temp Segment [segment {:name "Test Segment 1"}]
- (is (= {:table "Venues"
- :filter [{:segment "Test Segment 1"}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:filter [[:segment (:id segment)]]}))))))
-
- (is (= {:table "Venues"
- :filter [{:segment (deferred-tru "[Unknown Segment]")}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- (:query (mt/mbql-query venues
- {:filter [[:segment -1]]}))))))
-
- (testing "with named aggregation"
- (is (= {:table "Venues"
- :aggregation [{:type :aggregation
- :arg "Nonsensical named metric"}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- {:aggregation [[:aggregation-options
- [:sum [:*
- [:field (mt/id :venues :latitude) nil]
- [:field (mt/id :venues :longitude) nil]]]
- {:display-name "Nonsensical named metric"}]]}))))
-
- (testing "with unnamed complex aggregation"
- (is (= {:table "Venues"
- :aggregation [{:type :sum
- :arg ["Latitude" "*" "Longitude"]}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- {:aggregation [[:sum [:*
- [:field (mt/id :venues :latitude) nil]
- [:field (mt/id :venues :longitude) nil]]]]}))))
-
- (testing "with unnamed complex aggregation with multiple arguments"
- (is (= {:table "Venues"
- :aggregation [{:type :sum
- :arg ["Latitude" "+" "Longitude" "+" "ID"]}]}
- (api.qd/generate-query-description (t2/select-one Table :id (mt/id :venues))
- {:aggregation [[:sum [:+
- [:field (mt/id :venues :latitude) nil]
- [:field (mt/id :venues :longitude) nil]
- [:field (mt/id :venues :id) nil]]]]}))))))
diff --git a/test/metabase/api/segment_test.clj b/test/metabase/api/segment_test.clj
index 5da2ceee06b..c00f22b7bb1 100644
--- a/test/metabase/api/segment_test.clj
+++ b/test/metabase/api/segment_test.clj
@@ -3,17 +3,15 @@
(:require
[clojure.test :refer :all]
[metabase.http-client :as client]
- [metabase.models.database :refer [Database]]
+ [metabase.models :refer [Database Revision Table]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
- [metabase.models.revision :refer [Revision]]
[metabase.models.segment :as segment :refer [Segment]]
- [metabase.models.table :refer [Table]]
[metabase.server.middleware.util :as mw.util]
[metabase.test :as mt]
[metabase.util :as u]
- [toucan.hydrate :refer [hydrate]]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2]
+ [toucan2.tools.with-temp :as t2.with-temp]))
;; ## Helper Fns
@@ -211,21 +209,20 @@
(is (= nil
(mt/user-http-request :crowberto :delete 204 (format "segment/%d" id) :revision_message "carryon")))
(testing "should still be able to fetch the archived segment"
- (is (= {:name "Toucans in the rainforest"
- :description "Lookin' for a blueberry"
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :creator_id (mt/user->id :rasta)
- :creator (user-details (mt/fetch-user :rasta))
- :created_at true
- :updated_at true
- :entity_id true
- :archived true
- :definition nil}
- (-> (mt/user-http-request :crowberto :get 200 (format "segment/%d" id))
- segment-response
- (dissoc :query_description))))))))
+ (is (=? {:name "Toucans in the rainforest"
+ :description "Lookin' for a blueberry"
+ :show_in_getting_started false
+ :caveats nil
+ :points_of_interest nil
+ :creator_id (mt/user->id :rasta)
+ :creator (user-details (mt/fetch-user :rasta))
+ :created_at true
+ :updated_at true
+ :entity_id true
+ :archived true
+ :definition nil}
+ (-> (mt/user-http-request :crowberto :get 200 (format "segment/%d" id))
+ segment-response)))))))
;; ## GET /api/segment/:id
@@ -408,21 +405,30 @@
(for [revision (mt/user-http-request :crowberto :get 200 (format "segment/%d/revisions" id))]
(dissoc revision :timestamp :id))))))))
-
-;;; GET /api/segement/
-
(deftest list-test
- (testing "GET /api/segement/"
- (mt/with-temp* [Segment [segment-1 {:name "Segment 1"}]
- Segment [segment-2 {:name "Segment 2"}]
- ;; inactive segments shouldn't show up
- Segment [_ {:archived true}]]
- (is (= (mt/derecordize (hydrate [segment-1
- segment-2] :creator))
- (map #(dissoc % :query_description) (mt/user-http-request :rasta :get 200 "segment/")))))
-
- (is (= []
- (mt/user-http-request :rasta :get 200 "segment/")))))
+ (testing "GET /api/segment/"
+ (t2.with-temp/with-temp [Segment {id-1 :id} {:name "Segment 1"
+ :table_id (mt/id :users)}
+ Segment {id-2 :id} {:name "Segment 2"
+ :definition (:query (mt/mbql-query venues
+ {:filter
+ [:and
+ [:= $price 4]
+ [:= $category_id->categories.name "BBQ"]]}))}
+ ;; inactive segments shouldn't show up
+ Segment {id-3 :id} {:archived true}]
+ (is (=? [{:id id-1
+ :name "Segment 1"
+ :creator {}
+ :definition_description nil}
+ {:id id-2
+ :name "Segment 2"
+ :definition {}
+ :creator {}
+ :definition_description "Filtered by Price equals 4 and Name equals \"BBQ\""}]
+ (filter (fn [{segment-id :id}]
+ (contains? #{id-1 id-2 id-3} segment-id))
+ (mt/user-http-request :rasta :get 200 "segment/")))))))
(deftest related-entities-test
(testing "GET /api/segment/:id/related"
diff --git a/test/metabase/lib/breakout_test.cljc b/test/metabase/lib/breakout_test.cljc
index 396cd74120c..1dafabc4238 100644
--- a/test/metabase/lib/breakout_test.cljc
+++ b/test/metabase/lib/breakout_test.cljc
@@ -22,4 +22,6 @@
(meta/id :checkins :date)]]}]}
query))
(is (= "Checkins, Count, Grouped by Date (year)"
+ (lib/display-name query query)
+ (lib/describe-query query)
(lib/suggested-name query)))))
diff --git a/test/metabase/lib/metadata/cached_provider_test.cljc b/test/metabase/lib/metadata/cached_provider_test.cljc
new file mode 100644
index 00000000000..c0b0df2d906
--- /dev/null
+++ b/test/metabase/lib/metadata/cached_provider_test.cljc
@@ -0,0 +1,54 @@
+(ns metabase.lib.metadata.cached-provider-test
+ (:require
+ [clojure.test :refer [deftest is testing]]
+ [metabase.lib.metadata.cached-provider :as lib.metadata.cached-provider]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]))
+
+(deftest ^:parallel caching-test
+ (let [fetch-count (atom 0)
+ provider (lib.metadata.cached-provider/cached-metadata-provider
+ (reify
+ lib.metadata.protocols/MetadataProvider
+ (table [_this id]
+ (swap! fetch-count inc)
+ {:id id})
+
+ lib.metadata.protocols/BulkMetadataProvider
+ (bulk-metadata [_this metadata-type ids]
+ (case metadata-type
+ :metadata/table (for [id ids]
+ (do
+ (swap! fetch-count inc)
+ {:id id}))))))]
+ (testing "Initial fetch"
+ (is (= {:id 1}
+ (lib.metadata.protocols/table provider 1)))
+ (is (= 1
+ @fetch-count)))
+ (testing "Second fetch"
+ (is (= {:id 1}
+ (lib.metadata.protocols/table provider 1)))
+ (is (= 1
+ @fetch-count)))
+ (testing "Second fetch"
+ (is (= {:id 1}
+ (lib.metadata.protocols/table provider 1)))
+ (is (= 1
+ @fetch-count)))
+ (testing "Bulk fetch"
+ (is (= [{:id 1}]
+ (lib.metadata.protocols/bulk-metadata provider :metadata/table #{1})))
+ (is (= 1
+ @fetch-count))
+ (testing "Fetch a new Table, 1 Table already fetched"
+ (is (= [{:id 1}
+ {:id 2}]
+ (lib.metadata.protocols/bulk-metadata provider :metadata/table #{1 2})))
+ (is (= 2
+ @fetch-count)))
+ (testing "Bulk fetch again, should use cached results"
+ (is (= [{:id 1}
+ {:id 2}]
+ (lib.metadata.protocols/bulk-metadata provider :metadata/table #{1 2})))
+ (is (= 2
+ @fetch-count))))))
diff --git a/test/metabase/lib/test_metadata.cljc b/test/metabase/lib/test_metadata.cljc
index 9885e057d32..8b54a78a42e 100644
--- a/test/metabase/lib/test_metadata.cljc
+++ b/test/metabase/lib/test_metadata.cljc
@@ -7,7 +7,7 @@
will not be reflected here, for example if we add new information to the metadata. We'll have to manually update
these things if that happens and Metabase lib is meant to consume it."
(:require
- [metabase.lib.metadata.protocols :as lib.metadata.protocols]))
+ [metabase.lib.metadata.graph-provider :as lib.metadata.graph-provider]))
(defonce ^:private ^{:doc "Generate a random prefix to add to all of the [[id]]s below, so that they change between
test runs to catch places where people are hardcoding IDs rather than using [[id]]."}
@@ -786,7 +786,7 @@
(def metadata-provider
"[[metabase.lib.metadata.protocols/MetadataProvider]] using the test [[metadata]]."
- (lib.metadata.protocols/->SimpleGraphMetadataProvider metadata))
+ (lib.metadata.graph-provider/->SimpleGraphMetadataProvider metadata))
(def results-metadata
"Capture of the `data.results_metadata` that would come back when running `SELECT * FROM VENUES;` with the Query
diff --git a/test/metabase/models/metric_test.clj b/test/metabase/models/metric_test.clj
index cbca9a4823f..e866190c62c 100644
--- a/test/metabase/models/metric_test.clj
+++ b/test/metabase/models/metric_test.clj
@@ -1,13 +1,13 @@
(ns metabase.models.metric-test
(:require
[clojure.test :refer :all]
- [metabase.models.database :refer [Database]]
+ [metabase.models :refer [Database Segment Table]]
[metabase.models.metric :as metric :refer [Metric]]
[metabase.models.revision :as revision]
[metabase.models.serialization :as serdes]
- [metabase.models.table :refer [Table]]
[metabase.test :as mt]
- [toucan2.core :as t2])
+ [toucan2.core :as t2]
+ [toucan2.tools.with-temp :as t2.with-temp])
(:import
(java.time LocalDateTime)))
@@ -124,3 +124,23 @@
(is (= "a2318866"
(serdes/raw-hash ["measurement" (serdes/identity-hash table) now])
(serdes/identity-hash metric)))))))
+
+(deftest definition-descriptions-test
+ (testing ":definition_description should hydrate to nil if :definition is missing"
+ (t2.with-temp/with-temp [Metric metric {:name "Metric A"
+ :table_id (mt/id :users)}]
+ (is (= nil
+ (:definition_description (t2/hydrate metric :definition_description))))))
+ (t2.with-temp/with-temp [Segment {segment-id :id} {:name "Checkins with ID = 1"
+ :table_id (mt/id :checkins)
+ :definition (:query (mt/mbql-query checkins
+ {:filter [:= $id 1]}))}
+ Metric metric {:name "Metric B"
+ :table_id (mt/id :venues)
+ :definition (:query (mt/mbql-query venues
+ {:aggregation [[:sum $category_id->categories.name]]
+ :filter [:and
+ [:= $price 4]
+ [:segment segment-id]]}))}]
+ (is (= "Venues, Sum of Name, Filtered by Price equals 4 and Checkins with ID = 1"
+ (:definition_description (t2/hydrate metric :definition_description))))))
diff --git a/test/metabase/models/segment_test.clj b/test/metabase/models/segment_test.clj
index b7e09dcbf0c..02f3a4e3d68 100644
--- a/test/metabase/models/segment_test.clj
+++ b/test/metabase/models/segment_test.clj
@@ -7,7 +7,8 @@
[metabase.models.serialization :as serdes]
[metabase.models.table :refer [Table]]
[metabase.test :as mt]
- [toucan2.core :as t2])
+ [toucan2.core :as t2]
+ [toucan2.tools.with-temp :as t2.with-temp])
(:import
(java.time LocalDateTime)))
@@ -115,3 +116,27 @@
(is (= "be199b7c"
(serdes/raw-hash ["big customers" (serdes/identity-hash table) now])
(serdes/identity-hash segment)))))))
+
+(deftest definition-description-test
+ (testing "Do not hydrate definition description if definition is nil"
+ (t2.with-temp/with-temp [Segment segment {:name "Segment"
+ :table_id (mt/id :users)}]
+ (is (=? {:definition_description nil}
+ (t2/hydrate segment :definition_description)))))
+ (t2.with-temp/with-temp [Segment segment {:name "Expensive BBQ Spots"
+ :definition (:query (mt/mbql-query venues
+ {:filter
+ [:and
+ [:= $price 4]
+ [:= $category_id->categories.name "BBQ"]]}))}]
+ (is (= "Filtered by Price equals 4 and Name equals \"BBQ\""
+ (:definition_description (t2/hydrate segment :definition_description))))
+ (testing "Segments that reference other Segments (inception)"
+ (t2.with-temp/with-temp [Segment segment-2 {:name "Segment 2"
+ :definition (:query (mt/mbql-query categories
+ {:filter
+ [:and
+ [:segment (:id segment)]
+ [:not-null $id]]}))}]
+ (is (= "Filtered by Expensive BBQ Spots and ID is not empty"
+ (:definition_description (t2/hydrate segment-2 :definition_description))))))))
--
GitLab