diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 782efeeca77e702dd8a1cb010e339539570d5d52..128678d789fe5d00d963223644fc5067f387f83a 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -3,11 +3,11 @@
{:unresolved-symbol
{:exclude
[instaparse.core/transform
- (cljs.test/is [=?])
+ (cljs.test/is [=? malli=])
(clojure.core.logic/fresh)
(clojure.core.logic/matcha)
(clojure.core.logic/run)
- (clojure.test/is [query= sql= =?])
+ (clojure.test/is [query= sql= =? malli=])
(clojure.tools.macro/macrolet)
(metabase.async.streaming-response/streaming-response)
(metabase.driver.druid.query-processor-test/druid-query-returning-rows)
diff --git a/frontend/src/metabase-lib/metrics.ts b/frontend/src/metabase-lib/metrics.ts
new file mode 100644
index 0000000000000000000000000000000000000000..b695ffc8c288ec71b9081a87994f36c7b83f50c5
--- /dev/null
+++ b/frontend/src/metabase-lib/metrics.ts
@@ -0,0 +1,7 @@
+import * as ML from "cljs/metabase.lib.js";
+
+import type { MetricMetadata, Query } from "./types";
+
+export function availableMetrics(query: Query): MetricMetadata[] {
+ return ML.available_metrics(query);
+}
diff --git a/frontend/src/metabase-lib/types.ts b/frontend/src/metabase-lib/types.ts
index 55e0da7746038c4aad58a6124cea0a684a634b34..feeb70cfee14585f3bf75317fd970917a86b206c 100644
--- a/frontend/src/metabase-lib/types.ts
+++ b/frontend/src/metabase-lib/types.ts
@@ -14,6 +14,9 @@ export type TableMetadata = unknown & { _opaque: typeof TableMetadata };
declare const CardMetadata: unique symbol;
export type CardMetadata = unknown & { _opaque: typeof CardMetadata };
+declare const MetricMetadata: unique symbol;
+export type MetricMetadata = unknown & { _opaque: typeof MetricMetadata };
+
export type Limit = number | null;
declare const BreakoutClause: unique symbol;
diff --git a/frontend/src/metabase-lib/v2.ts b/frontend/src/metabase-lib/v2.ts
index 2d504f87d95704546d6cd03961384ac0d4fdc098..91a3a8b2a98cc35ec21d596f3487bb94cb05737a 100644
--- a/frontend/src/metabase-lib/v2.ts
+++ b/frontend/src/metabase-lib/v2.ts
@@ -12,6 +12,7 @@ export * from "./limit";
export * from "./order_by";
export * from "./filter";
export * from "./join";
+export * from "./metrics";
export * from "./query";
export * from "./temporal_bucket";
export * from "./types";
diff --git a/src/metabase/api/geojson.clj b/src/metabase/api/geojson.clj
index b92a673c54efe5ab5a9816734dbe59252660db2f..5663edfff6e4e5e97c349cc3e6d5ba4f38d49a1b 100644
--- a/src/metabase/api/geojson.clj
+++ b/src/metabase/api/geojson.clj
@@ -107,6 +107,8 @@
(setting/set-value-of-type! :json :custom-geojson new-value)))
:visibility :public)
+(def ^:private connection-timeout-ms 8000)
+
(defn- read-url-and-respond
"Reads the provided URL and responds with the contents as a stream."
[url respond]
@@ -114,8 +116,8 @@
(io/reader resource)
(:body (http/get url {:as :reader
:redirect-strategy :none
- :socket-timeout 8000
- :connection-timeout 8000})))
+ :socket-timeout connection-timeout-ms
+ :connection-timeout connection-timeout-ms})))
is (ReaderInputStream. reader)]
(respond (-> (response/response is)
(response/content-type "application/json")))))
diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc
index e485ab4d12ab3030705e1c436a12dc3f29b4a2c9..b9e72cd8f29f8031a5d15010de860fae59de9cd6 100644
--- a/src/metabase/lib/aggregation.cljc
+++ b/src/metabase/lib/aggregation.cljc
@@ -3,6 +3,7 @@
(:require
[medley.core :as m]
[metabase.lib.common :as lib.common]
+ [metabase.lib.dispatch :as lib.dispatch]
[metabase.lib.equality :as lib.equality]
[metabase.lib.hierarchy :as lib.hierarchy]
[metabase.lib.metadata :as lib.metadata]
@@ -223,12 +224,25 @@
[aggregation-clause]
aggregation-clause)
+(def ^:private Aggregatable
+ "Schema for something you can pass to [[aggregate]] to add to a query as an aggregation."
+ [:or
+ ::lib.schema.aggregation/aggregation
+ ::lib.schema.common/external-op
+ lib.metadata/MetricMetadata])
+
(mu/defn aggregate :- ::lib.schema/query
"Adds an aggregation to query."
- ([query an-aggregate-clause]
- (aggregate query -1 an-aggregate-clause))
- ([query stage-number an-aggregate-clause]
- (lib.util/add-summary-clause query stage-number :aggregation an-aggregate-clause)))
+ ([query aggregatable]
+ (aggregate query -1 aggregatable))
+
+ ([query :- ::lib.schema/query
+ stage-number :- :int
+ aggregatable :- Aggregatable]
+ ;; if this is a Metric metadata, convert it to `:metric` MBQL clause before adding.
+ (if (= (lib.dispatch/dispatch-value aggregatable) :metadata/metric)
+ (recur query stage-number (lib.ref/ref aggregatable))
+ (lib.util/add-summary-clause query stage-number :aggregation aggregatable))))
(mu/defn aggregations :- [:maybe [:sequential ::lib.schema.aggregation/aggregation]]
"Get the aggregations in a given stage of a query."
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index 8120328a7ee20eb9027a593f1b2008446a957b94..a6280df5578a9d7462939b7dfb1aa2ec8890f0fd 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -188,6 +188,8 @@
display-info
suggested-name
type-of]
+ [lib.metric
+ available-metrics]
[lib.native
#?@(:cljs [->TemplateTags
TemplateTags->])
diff --git a/src/metabase/lib/hierarchy.cljc b/src/metabase/lib/hierarchy.cljc
index 0e19deeff2204e8a4dbb4a886188d70136aa3c1c..a8e5ab977ce2e3f67255dda33da8313e28b734fd 100644
--- a/src/metabase/lib/hierarchy.cljc
+++ b/src/metabase/lib/hierarchy.cljc
@@ -7,7 +7,9 @@
(defn derive
"Like [[clojure.core/derive]], but affects [[hierarchy]] rather than the global hierarchy."
[tag parent]
- (swap! hierarchy clojure.core/derive tag parent))
+ (swap! hierarchy clojure.core/derive tag parent)
+ ;; for REPL convenience so we don't dump a lot of garbage
+ nil)
(defn isa?
"Like [[clojure.core/isa?]], but uses [[hierarchy]]."
diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs
index d24335dcedfc702d85b2aeb8e8fa4ad1c1679b16..3f0e2912c7b15016ea67b41ba22db7bb97cdfef5 100644
--- a/src/metabase/lib/js.cljs
+++ b/src/metabase/lib/js.cljs
@@ -555,3 +555,9 @@
"Returns the native query's template tags"
[a-query]
(lib.core/TemplateTags-> (lib.core/template-tags a-query)))
+
+(defn ^:export available-metrics
+ "Get a list of Metrics that you may consider using as aggregations for a query. Returns JS array of opaque Metric
+ metadata objects."
+ [a-query]
+ (to-array (lib.core/available-metrics a-query)))
diff --git a/src/metabase/lib/js/metadata.cljs b/src/metabase/lib/js/metadata.cljs
index d7daa9943ce326264307ca9c2a926b08b05e7f9b..3207e2e27595ab46d85b86a4dbd6f803cbdb379d 100644
--- a/src/metabase/lib/js/metadata.cljs
+++ b/src/metabase/lib/js/metadata.cljs
@@ -375,6 +375,12 @@
:when (and a-field (= (:table-id a-field) table-id))]
a-field))
+(defn- metrics [metadata table-id]
+ (for [[_id metric-delay] (some-> metadata :metrics deref)
+ :let [a-metric (some-> metric-delay deref)]
+ :when (and a-metric (= (:table-id a-metric) table-id))]
+ a-metric))
+
(defn metadata-provider
"Use a `metabase-lib/metadata/Metadata` as a [[metabase.lib.metadata.protocols/MetadataProvider]]."
[database-id unparsed-metadata]
@@ -389,6 +395,7 @@
(card [_this card-id] (card metadata card-id))
(tables [_this] (tables metadata database-id))
(fields [_this table-id] (fields metadata table-id))
+ (metrics [_this table-id] (metrics metadata table-id))
;; for debugging: call [[clojure.datafy/datafy]] on one of these to parse all of our metadata and see the whole
;; thing at once.
diff --git a/src/metabase/lib/metadata.cljc b/src/metabase/lib/metadata.cljc
index a250f3331794d865043177173b12071fc3956dd9..8c894a96021b89cd562c9636ae27138618e3a3fa 100644
--- a/src/metabase/lib/metadata.cljc
+++ b/src/metabase/lib/metadata.cljc
@@ -117,19 +117,27 @@
;; probably missing `:lib/type` and probably using `:snake_case` keys.
[:result-metadata {:optional true} [:maybe [:sequential :map]]]])
-(def ^:private SegmentMetadata
+(def SegmentMetadata
"More or less the same as a [[metabase.models.segment]], but with kebab-case keys."
[:map
[:lib/type [:= :metadata/segment]]
[:id ::lib.schema.id/segment]
[:name ::lib.schema.common/non-blank-string]])
-(def ^:private MetricMetadata
- "More or less the same as a [[metabase.models.metric]], but with kebab-case keys."
+(def MetricMetadata
+ "Malli schema for a legacy v1 [[metabase.models.metric]], but with kebab-case keys. A Metric defines an MBQL snippet
+ with an aggregation and optionally a filter clause. You can add a `:metric` reference to the `:aggregations` in an
+ MBQL stage, and the QP treats it like a macro and expands it to the underlying clauses --
+ see [[metabase.query-processor.middleware.expand-macros]]."
[:map
- [:lib/type [:= :metadata/metric]]
- [:id ::lib.schema.id/metric]
- [:name ::lib.schema.common/non-blank-string]])
+ [:lib/type [:= :metadata/metric]]
+ [:id ::lib.schema.id/metric]
+ [:name ::lib.schema.common/non-blank-string]
+ [:table-id ::lib.schema.id/table]
+ ;; the MBQL snippet defining this Metric; this may still be in legacy
+ ;; format. [[metabase.lib.metric/metric-definition]] handles conversion to pMBQL if needed.
+ [:definition :map]
+ [:description {:optional true} [:maybe ::lib.schema.common/non-blank-string]]])
(def TableMetadata
"Schema for metadata about a specific [[metabase.models.table]]. More or less the same as a [[metabase.models.table]],
@@ -155,7 +163,9 @@
(def MetadataProvider
"Schema for something that satisfies the [[lib.metadata.protocols/MetadataProvider]] protocol."
- [:fn lib.metadata.protocols/metadata-provider?])
+ [:fn
+ {:error/message "Valid MetadataProvider"}
+ #'lib.metadata.protocols/metadata-provider?])
(def MetadataProviderable
"Something that can be used to get a MetadataProvider. Either a MetadataProvider, or a map with a MetadataProvider in
diff --git a/src/metabase/lib/metadata/cached_provider.cljc b/src/metabase/lib/metadata/cached_provider.cljc
index d44da7ae6680291c5dc352e7a4ef7684a2ff682f..2bf14cb41e17a0c96afcc8d18b1355fb1f9ad9df 100644
--- a/src/metabase/lib/metadata/cached_provider.cljc
+++ b/src/metabase/lib/metadata/cached_provider.cljc
@@ -55,12 +55,13 @@
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/column field-id] #(lib.metadata.protocols/field metadata-provider field-id)))
+ (field [_this field-id] (get-in-cache-or-fetch cache [:metadata/column 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)))
+ (metrics [_this table-id] (get-in-cache-or-fetch cache [::table-metrics table-id] #(lib.metadata.protocols/metrics metadata-provider table-id)))
lib.metadata.protocols/CachedMetadataProvider
(cached-database [_this] (get-in-cache cache [:metadata/database]))
diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc
index 250f7971722942a089433ee36516a5125887dfe0..fe676fa5e778e9f6a4fcede40c41d42e4ab0be6f 100644
--- a/src/metabase/lib/metadata/calculation.cljc
+++ b/src/metabase/lib/metadata/calculation.cljc
@@ -174,7 +174,12 @@
;; otherwise if `:base-type` is specified, we can return that.
(:base-type options)
;; if none of the special cases are true, fall back to [[type-of-method]].
- (type-of-method query stage-number x)))))
+ (let [calculated-type (type-of-method query stage-number x)]
+ ;; if calculated type is not a true type but a placeholder like `:metabase.lib.schema.expression/type.unknown`
+ ;; or a union of types then fall back to `:type/*`, an actual type.
+ (if (isa? calculated-type :type/*)
+ calculated-type
+ :type/*))))))
(defmethod type-of-method :default
[_query _stage-number expr]
@@ -358,7 +363,7 @@
(defmethod display-info-method :metadata/table
[query stage-number table]
(merge (default-display-info query stage-number table)
- {:is-source-table (= (lib.util/source-table query) (:id table))}))
+ {:is-source-table (= (lib.util/source-table-id query) (:id table))}))
(def ColumnsWithUniqueAliases
"Schema for column metadata that should be returned by [[visible-columns]]. This is mostly used
diff --git a/src/metabase/lib/metadata/jvm.clj b/src/metabase/lib/metadata/jvm.clj
index 1b0231b87dbc1e6aec0a3cea134e90c77704d7e4..745aa3fe0dce8e6fe30bd22d142924b21094f1ed 100644
--- a/src/metabase/lib/metadata/jvm.clj
+++ b/src/metabase/lib/metadata/jvm.clj
@@ -50,6 +50,11 @@
(mapv #(instance->metadata % :metadata/column)
(t2/select :model/Field :table_id table-id)))
+(defn- metrics [table-id]
+ (log/debugf "Fetching all Metrics for Table %d" table-id)
+ (mapv #(instance->metadata % :metadata/metric)
+ (t2/select :model/Metric :table_id table-id)))
+
(p/deftype+ UncachedApplicationDatabaseMetadataProvider [database-id]
lib.metadata.protocols/MetadataProvider
(database [_this]
@@ -72,6 +77,9 @@
(fields [_this table-id]
(fields table-id))
+ (metrics [_this table-id]
+ (metrics table-id))
+
lib.metadata.protocols/BulkMetadataProvider
(bulk-metadata [_this metadata-type ids]
(bulk-instances metadata-type ids))
diff --git a/src/metabase/lib/metadata/protocols.cljc b/src/metabase/lib/metadata/protocols.cljc
index 9df210d30a0a8ec6977668b775278086ee33c0c2..876fc0039915bfa2585229e537f86d00098bbb62 100644
--- a/src/metabase/lib/metadata/protocols.cljc
+++ b/src/metabase/lib/metadata/protocols.cljc
@@ -61,7 +61,11 @@
(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."))
+ the [[metabase.lib.metadata/ColumnMetadata]] schema. If no such Table exists, this should error.")
+
+ (metrics [metadata-provider table-id]
+ "Return a sequence of legacy v1 Metrics associated with a Table with the given `table-id`. Metrics should satisfy
+ the [[metabase.lib.metadata/MetricMetadata]] schema. If no such Table exists, this should error."))
(defn metadata-provider?
"Whether `x` is a valid [[MetadataProvider]]."
diff --git a/src/metabase/lib/metric.cljc b/src/metabase/lib/metric.cljc
index 89ce5c3e8f2f16e796d1f1000714e15062079da3..ebd788ae54c75ddb5806c9069b70bd4604668a4f 100644
--- a/src/metabase/lib/metric.cljc
+++ b/src/metabase/lib/metric.cljc
@@ -5,7 +5,11 @@
[metabase.lib.convert :as lib.convert]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+ [metabase.lib.ref :as lib.ref]
[metabase.lib.schema :as lib.schema]
+ [metabase.lib.schema.expression :as lib.schema.expression]
+ [metabase.lib.schema.id :as lib.schema.id]
[metabase.lib.util :as lib.util]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.shared.util.i18n :as i18n]
@@ -16,7 +20,7 @@
(lib.metadata/metric query metric-id)))
(mu/defn ^:private metric-definition :- [:maybe ::lib.schema/stage.mbql]
- [{:keys [definition], :as _metric-metadata}]
+ [{:keys [definition], :as _metric-metadata} :- lib.metadata/MetricMetadata]
(when definition
(if (:mbql/type definition)
definition
@@ -28,6 +32,18 @@
lib.convert/->pMBQL
(lib.util/query-stage -1)))))
+(defmethod lib.ref/ref-method :metadata/metric
+ [{:keys [id], :as metric-metadata}]
+ (let [effective-type (or (:effective-type metric-metadata)
+ (:base-type metric-metadata)
+ (when-let [aggregation (first (:aggregation (metric-definition metric-metadata)))]
+ (let [ag-effective-type (lib.schema.expression/type-of aggregation)]
+ (when (isa? ag-effective-type :type/*)
+ ag-effective-type))))
+ options (cond-> {:lib/uuid (str (random-uuid))}
+ effective-type (assoc :effective-type effective-type))]
+ [:metric options id]))
+
(defmethod lib.metadata.calculation/type-of-method :metadata/metric
[query stage-number metric-metadata]
(or
@@ -41,19 +57,58 @@
(lib.metadata.calculation/type-of query stage-number metric-metadata))
:type/*))
+(defn- fallback-display-name []
+ (i18n/tru "[Unknown Metric]"))
+
(defmethod lib.metadata.calculation/display-name-method :metadata/metric
[_query _stage-number metric-metadata _style]
(or ((some-fn :display-name :name) metric-metadata)
- (i18n/tru "[Unknown Metric]")))
+ (fallback-display-name)))
(defmethod lib.metadata.calculation/display-name-method :metric
[query stage-number [_tag _opts metric-id-or-name] style]
(or (when-let [metric-metadata (resolve-metric query metric-id-or-name)]
(lib.metadata.calculation/display-name query stage-number metric-metadata style))
- (i18n/tru "[Unknown Metric]")))
+ (fallback-display-name)))
+
+(mu/defn ^:private aggregating-by-metric? :- :boolean
+ "Whether a given stage of a query currently includes a `:metric` ref clause in its aggregations."
+ [query :- ::lib.schema/query
+ stage-number :- :int
+ metric-id :- ::lib.schema.id/metric]
+ (let [{aggregations :aggregation} (lib.util/query-stage query stage-number)]
+ (boolean
+ (some (fn [[tag :as clause]]
+ (when (= tag :metric)
+ (let [[_tag _opts id] clause]
+ (= id metric-id))))
+ aggregations))))
+
+(defmethod lib.metadata.calculation/display-info-method :metadata/metric
+ [query stage-number {:keys [id description], :as metric-metadata}]
+ (merge
+ ((get-method lib.metadata.calculation/display-info-method :default) query stage-number metric-metadata)
+ {:description description}
+ (when (aggregating-by-metric? query stage-number id)
+ {:selected true})))
+
+(defmethod lib.metadata.calculation/display-info-method :metric
+ [query stage-number [_tag _opts metric-id-or-name]]
+ (if-let [metric-metadata (resolve-metric query metric-id-or-name)]
+ (lib.metadata.calculation/display-info query stage-number metric-metadata)
+ {:effective-type :type/*
+ :display-name (fallback-display-name)
+ :long-display-name (fallback-display-name)}))
(defmethod lib.metadata.calculation/column-name-method :metric
[query stage-number [_tag _opts metric-id-or-name]]
(or (when-let [metric-metadata (resolve-metric query metric-id-or-name)]
(lib.metadata.calculation/column-name query stage-number metric-metadata))
"metric"))
+
+(mu/defn available-metrics :- [:maybe [:sequential {:min 1} lib.metadata/MetricMetadata]]
+ "Get a list of Metrics that you may consider using as aggregations for a query. Only Metrics that have the same
+ `table-id` as the `source-table` for this query will be suggested."
+ [query :- ::lib.schema/query]
+ (when-let [source-table-id (lib.util/source-table-id query)]
+ (not-empty (lib.metadata.protocols/metrics (lib.metadata/->metadata-provider query) source-table-id))))
diff --git a/src/metabase/lib/schema/expression/conditional.cljc b/src/metabase/lib/schema/expression/conditional.cljc
index 96c28aaff206026635f5db4a61b12cbdbeabb6a9..825e1388c6f6e82552bda38fe8dd9a9bc935d232 100644
--- a/src/metabase/lib/schema/expression/conditional.cljc
+++ b/src/metabase/lib/schema/expression/conditional.cljc
@@ -5,7 +5,8 @@
[metabase.lib.schema.expression :as expression]
[metabase.lib.schema.mbql-clause :as mbql-clause]
[metabase.types :as types]
- [metabase.util.malli.registry :as mr]))
+ [metabase.util.malli.registry :as mr]
+ ))
;;; the logic for calculating the return type of a `:case` or similar statement is not optimal nor perfect. But it
;;; should be ok for now and errors on the side of being permissive. See this Slack thread for more info:
@@ -18,6 +19,11 @@
(nil? x)
y
+ ;; if the type of either x or y is unknown, then the overall type of this has to be unknown as well.
+ (or (= x ::expression/type.unknown)
+ (= y ::expression/type.unknown))
+ ::expression/type.unknown
+
;; if both types are keywords return their most-specific ancestor.
(and (keyword? x)
(keyword? y))
@@ -55,25 +61,15 @@
[:pred-expr-pairs [:sequential {:min 1} [:ref ::case-subclause]]]
[:default [:? [:schema [:ref ::expression/expression]]]])
-(defn- allow-unkown-type
- "A hack to allow case and coalesce expressions using fields only.
- The real fix would pass in metadata so that the types of fields
- can be determined."
- [expr-type]
- (if (= expr-type ::expression/type.unknown)
- :type/*
- expr-type))
-
(defmethod expression/type-of-method :case
[[_tag _opts pred-expr-pairs default]]
- (-> (reduce
- (fn [best-guess [_pred expr]]
- (let [expr-type (expression/type-of expr)]
- (best-return-type best-guess expr-type)))
- (when (some? default)
- (expression/type-of default))
- pred-expr-pairs)
- allow-unkown-type))
+ (reduce
+ (fn [best-guess [_pred expr]]
+ (let [expr-type (expression/type-of expr)]
+ (best-return-type best-guess expr-type)))
+ (when (some? default)
+ (expression/type-of default))
+ pred-expr-pairs))
;;; TODO -- add constraint that these types have to be compatible
(mbql-clause/define-tuple-mbql-clause :coalesce
@@ -82,5 +78,4 @@
(defmethod expression/type-of-method :coalesce
[[_tag _opts expr null-value]]
- (-> (best-return-type (expression/type-of expr) (expression/type-of null-value))
- allow-unkown-type))
+ (best-return-type (expression/type-of expr) (expression/type-of null-value)))
diff --git a/src/metabase/lib/schema/mbql_clause.cljc b/src/metabase/lib/schema/mbql_clause.cljc
index a81ae0e04cfde5d8de9b3beb4916b42eff34110f..15ab19ab39f440244b7691a4d1ead1992b56c6c3 100644
--- a/src/metabase/lib/schema/mbql_clause.cljc
+++ b/src/metabase/lib/schema/mbql_clause.cljc
@@ -85,7 +85,7 @@
([tag :- simple-keyword?
_arrow :- [:= :-]
- return-type :- [:fn {:error/message "valid base type"} #(isa? % :type/*)]
+ return-type :- ::expression/base-type
schema]
(define-mbql-clause tag schema)
(defmethod expression/type-of-method tag
diff --git a/src/metabase/lib/schema/ref.cljc b/src/metabase/lib/schema/ref.cljc
index 499a4078e7ab08a8d6f9b4560771fc1e2ef80cd3..34c4fb4bd9428518ca1b519832a476a60bdefda1 100644
--- a/src/metabase/lib/schema/ref.cljc
+++ b/src/metabase/lib/schema/ref.cljc
@@ -1,7 +1,9 @@
(ns metabase.lib.schema.ref
"Malli schema for a Field, aggregation, or expression reference (etc.)"
(:require
+ [clojure.string :as str]
[metabase.lib.dispatch :as lib.dispatch]
+ [metabase.lib.hierarchy :as lib.hierarchy]
[metabase.lib.schema.common :as common]
[metabase.lib.schema.expression :as expression]
[metabase.lib.schema.id :as id]
@@ -55,6 +57,8 @@
[:dispatch-type/integer ::field.id]
[:dispatch-type/string ::field.literal]]])
+(lib.hierarchy/derive :field ::ref)
+
(defmethod expression/type-of-method :field
[[_tag opts _id-or-name]]
(or ((some-fn :effective-type :base-type) opts)
@@ -68,6 +72,8 @@
(or ((some-fn :effective-type :base-type) opts)
::expression/type.unknown))
+(lib.hierarchy/derive :expression ::ref)
+
(mr/def ::aggregation-options
[:merge
::common/options
@@ -86,10 +92,19 @@
(or ((some-fn :effective-type :base-type) opts)
::expression/type.unknown))
+(lib.hierarchy/derive :aggregation ::ref)
+
+(mbql-clause/define-tuple-mbql-clause :metric :- ::expression/type.unknown
+ #_metric-id [:schema [:ref ::id/metric]])
+
+(lib.hierarchy/derive :metric ::ref)
+
(mr/def ::ref
[:and
::mbql-clause/clause
[:fn
- {:error/message ":field, :expression, :or :aggregation reference"}
+ {:error/fn (fn [_ _]
+ (str "Valid reference, must be one of these clauses: "
+ (str/join ", " (sort (descendants @lib.hierarchy/hierarchy ::ref)))))}
(fn [[tag :as _clause]]
- (#{:field :expression :aggregation} tag))]])
+ (lib.hierarchy/isa? tag ::ref))]])
diff --git a/src/metabase/lib/util.cljc b/src/metabase/lib/util.cljc
index c66d2c156945f4709ef0f10500e61fe7754ce128..ead20fa0eac2b5546aa6c56139ab89b5b8ca95d6 100644
--- a/src/metabase/lib/util.cljc
+++ b/src/metabase/lib/util.cljc
@@ -456,7 +456,7 @@
(when-let [[_match card-id-str] (re-find #"^card__(\d+)$" table-id)]
(parse-long card-id-str))))
-(mu/defn source-table :- [:maybe ::lib.schema.id/table]
+(mu/defn source-table-id :- [:maybe ::lib.schema.id/table]
"If this query has a `:source-table` ID, return it."
[query]
(-> query :stages first :source-table))
diff --git a/src/metabase/query_processor/store.clj b/src/metabase/query_processor/store.clj
index 343d82c7b96ecc4bf88dbddc27b169ba5f911453..f4053c52eaefa65c3bae24de697773b9dcdf1a4d 100644
--- a/src/metabase/query_processor/store.clj
+++ b/src/metabase/query_processor/store.clj
@@ -344,6 +344,7 @@
(segment [_this _segment-id] nil)
(tables [_metadata-provider] nil)
(fields [_metadata-provider _table-id] nil)
+ (metrics [_metadata-provider _table-id] nil)
pretty/PrettyPrintable
(pretty [_this]
diff --git a/src/metabase/query_processor/store/metadata_provider.clj b/src/metabase/query_processor/store/metadata_provider.clj
deleted file mode 100644
index edfff889fea7bd71f90a143b8994ba1f29d3eeb1..0000000000000000000000000000000000000000
--- a/src/metabase/query_processor/store/metadata_provider.clj
+++ /dev/null
@@ -1,39 +0,0 @@
-(ns metabase.query-processor.store.metadata-provider
- (:require
- [metabase.lib.metadata.composed-provider
- :as lib.metadata.composed-provider]
- [metabase.lib.metadata.jvm :as lib.metadata.jvm]
- [metabase.lib.metadata.protocols :as lib.metadata.protocols]
- [metabase.query-processor.store :as qp.store]
- [metabase.util :as u]
- [pretty.core :as pretty]))
-
-(defn- base-metadata-provider []
- (reify
- lib.metadata.protocols/MetadataProvider
- (database [_this]
- (some-> (qp.store/database) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/database)))
-
- (table [_this table-id]
- (some-> (qp.store/table table-id) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/table)))
-
- (field [_this field-id]
- (some-> (qp.store/field field-id) (update-keys u/->kebab-case-en) (assoc :lib/type :metadata/column)))
-
- (card [_this _card-id] nil)
- (metric [_this _metric-id] nil)
- (segment [_this _segment-id] nil)
- (tables [_metadata-provider] nil)
- (fields [_metadata-provider _table-id] nil)
-
- pretty/PrettyPrintable
- (pretty [_this]
- `metadata-provider)))
-
-(defn metadata-provider
- "Create a new MLv2 metadata provider that uses the QP store."
- []
- (qp.store/cached ::metadata-provider
- (lib.metadata.composed-provider/composed-metadata-provider
- (base-metadata-provider)
- (lib.metadata.jvm/application-database-metadata-provider (:id (qp.store/database))))))
diff --git a/test/metabase/api/geojson_test.clj b/test/metabase/api/geojson_test.clj
index b74a0209ec1c6198a7e5cff3eec09760fe6bd121..eb53b2a3a9640283cb4ab0a727dd647461520357 100644
--- a/test/metabase/api/geojson_test.clj
+++ b/test/metabase/api/geojson_test.clj
@@ -35,11 +35,10 @@
:region_key nil
:region_name nil}})
-(deftest geojson-schema-test
- (is (= true
- (boolean (s/validate @#'api.geojson/CustomGeoJSON test-custom-geojson)))))
+(deftest ^:parallel geojson-schema-test
+ (is (s/validate @#'api.geojson/CustomGeoJSON test-custom-geojson)))
-(deftest validate-geojson-test
+(deftest ^:parallel validate-geojson-test
(testing "It validates URLs and files appropriately"
(let [examples {;; Internal metadata for GCP
"metadata.google.internal" false
@@ -135,6 +134,25 @@
{:value resource-geojson})
(mt/user-http-request :crowberto :get 200 "setting/custom-geojson")))))))))
+(deftest ^:parallel url-proxy-endpoint-test
+ (testing "GET /api/geojson"
+ (testing "test the endpoint that fetches JSON files given a URL"
+ (is (= {:type "Point"
+ :coordinates [37.77986 -122.429]}
+ (mt/user-http-request :crowberto :get 200 "geojson" :url test-geojson-url))))
+ (testing "error is returned if URL connection fails"
+ (is (= "GeoJSON URL failed to load"
+ (mt/user-http-request :crowberto :get 400 "geojson"
+ :url test-broken-geojson-url))))
+ (testing "error is returned if URL is invalid"
+ (is (= (str "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to "
+ "a file on the classpath. URLs referring to hosts that supply internal hosting metadata are "
+ "prohibited.")
+ (mt/user-http-request :crowberto :get 400 "geojson" :url "file://tmp"))))
+ (testing "cannot be called by non-admins"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :get 403 "geojson" :url test-geojson-url))))))
+
(defprotocol GeoJsonTestServer
(-port [_]))
@@ -155,17 +173,9 @@
GeoJsonTestServer
(-port [_] (.. server getURI getPort)))))
-(deftest url-proxy-endpoint-test
- (testing "GET /api/geojson"
- (testing "test the endpoint that fetches JSON files given a URL"
- (is (= {:type "Point"
- :coordinates [37.77986 -122.429]}
- (mt/user-http-request :crowberto :get 200 "geojson" :url test-geojson-url))))
- (testing "error is returned if URL connection fails"
- (is (= "GeoJSON URL failed to load"
- (mt/user-http-request :crowberto :get 400 "geojson"
- :url test-broken-geojson-url))))
- (testing "error is returned if URL server never responds (#28752)"
+(deftest url-proxy-endpoint-non-responding-server-test
+ (testing "error is returned if URL server never responds (#28752)"
+ (with-redefs [api.geojson/connection-timeout-ms 200]
;; use a webserver which accepts a connection and never responds. The geojson endpoint opens a reader to the url
;; and responds with it. And if there are never any bytes going across, the whole thing just sits there. Our
;; test flakes after 45 seconds with `mt/user-http-request` times out. And presumably other clients have similar
@@ -175,15 +185,7 @@
(testing "error is returned if URL connection fails"
(is (= "GeoJSON URL failed to load"
(mt/user-http-request :crowberto :get 400 "geojson"
- :url never-responds-url)))))))
- (testing "error is returned if URL is invalid"
- (is (= (str "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to "
- "a file on the classpath. URLs referring to hosts that supply internal hosting metadata are "
- "prohibited.")
- (mt/user-http-request :crowberto :get 400 "geojson" :url "file://tmp"))))
- (testing "cannot be called by non-admins"
- (is (= "You don't have permissions to do that."
- (mt/user-http-request :rasta :get 403 "geojson" :url test-geojson-url))))))
+ :url never-responds-url)))))))))
(deftest key-proxy-endpoint-test
(testing "GET /api/geojson/:key"
diff --git a/test/metabase/lib/convert_test.cljc b/test/metabase/lib/convert_test.cljc
index a47da2d7d341c6402489cfdb97d91299147c5afd..1d0e013df02dae37e0c917d08e19986cc1b7b211 100644
--- a/test/metabase/lib/convert_test.cljc
+++ b/test/metabase/lib/convert_test.cljc
@@ -1,11 +1,12 @@
(ns metabase.lib.convert-test
(:require
- #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
[clojure.test :refer [are deftest is testing]]
[malli.core :as mc]
[metabase.lib.convert :as lib.convert]
[metabase.lib.core :as lib]
- [metabase.lib.test-metadata :as meta]))
+ [metabase.lib.test-metadata :as meta]
+ [metabase.util :as u]
+ #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
@@ -227,9 +228,16 @@
(is (= original
(lib.convert/->legacy-MBQL (lib.convert/->pMBQL original))))))
+(defn- test-round-trip [query]
+ (testing (str "original =\n" (u/pprint-to-str query))
+ (let [converted (lib.convert/->pMBQL query)]
+ (testing (str "\npMBQL =\n" (u/pprint-to-str converted))
+ (is (= query
+ (lib.convert/->legacy-MBQL converted)))))))
+
(deftest ^:parallel round-trip-test
;; Miscellaneous queries that have caused test failures in the past, captured here for quick feedback.
- (are [query] (= query (-> query lib.convert/->pMBQL lib.convert/->legacy-MBQL))
+ (are [query] (test-round-trip query)
;; :aggregation-options on a non-aggregate expression with an inner aggregate.
{:database 194
:query {:aggregation [[:aggregation-options
@@ -375,6 +383,22 @@
:type :query
:query {:source-table "card__1"}}))
+(deftest ^:parallel round-trip-aggregation-with-case-test
+ (test-round-trip
+ {:database 2762
+ :type :query
+ :query {:aggregation [[:sum [:case [[[:< [:field 139657 nil] 2] [:field 139657 nil]]] {:default 0}]]]
+ :breakout [[:field 139658 nil]]
+ :limit 4
+ :source-table 33674}}))
+
+(deftest ^:parallel round-trip-aggregation-with-metric-test
+ (test-round-trip
+ {:database 1
+ :query {:aggregation [[:+ [:metric 82] 1]]
+ :source-table 1}
+ :type :query}))
+
(deftest ^:parallel round-trip-options-test
(testing "Round-tripping (p)MBQL caluses with options (#30280)"
(testing "starting with pMBQL"
diff --git a/test/metabase/lib/metric_test.cljc b/test/metabase/lib/metric_test.cljc
index 086a43c6331a34e4b3d0ad8114cd41b094464523..aaa06b343b44dc1e0d48ca452ac74bfc180bf2f4 100644
--- a/test/metabase/lib/metric_test.cljc
+++ b/test/metabase/lib/metric_test.cljc
@@ -1,7 +1,8 @@
(ns metabase.lib.metric-test
(:require
- [clojure.test :refer [deftest is]]
+ [clojure.test :refer [are deftest is testing]]
[metabase.lib.core :as lib]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
@@ -9,27 +10,130 @@
#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me))
-(defn- mock-metadata-provider []
+(def ^:private metric-id 100)
+
+(def ^:private metric-definition
+ {:source-table (meta/id :venues)
+ :aggregation [[:sum [:field (meta/id :venues :price) nil]]]
+ :filter [:= [:field (meta/id :venues :price) nil] 4]})
+
+(def ^:private metadata-provider
(lib.tu/mock-metadata-provider
{:database meta/metadata
:tables [(meta/table-metadata :venues)]
:fields [(meta/field-metadata :venues :price)]
- :metrics [{:id 100
- :name "My Metric"
- :definition {:source-table (meta/id :venues)
- :aggregation [[:sum [:field (meta/id :venues :price) nil]]]
- :filter [:= [:field (meta/id :venues :price) nil] 4]}}]}))
-
-(deftest ^:parallel metric-display-name-test
- (let [metadata (mock-metadata-provider)
- query (-> (lib/query metadata (meta/table-metadata :venues))
- (lib/aggregate [:metric {:lib/uuid (str (random-uuid))} 100]))]
- (is (= "Venues, My Metric"
- (lib.metadata.calculation/suggested-name query)))))
-
-(deftest ^:parallel metric-type-of-test
- (let [metadata (mock-metadata-provider)
- query (-> (lib/query metadata (meta/table-metadata :venues))
- (lib/aggregate [:metric {:lib/uuid (str (random-uuid))} 100]))]
- (is (= :type/Integer
- (lib.metadata.calculation/type-of query [:metric {:lib/uuid (str (random-uuid))} 100])))))
+ :metrics [{:id metric-id
+ :name "Sum of Cans"
+ :table-id (meta/id :venues)
+ :definition metric-definition
+ :description "Number of toucans plus number of pelicans"}]}))
+
+(def ^:private metric-clause
+ [:metric {:lib/uuid (str (random-uuid))} metric-id])
+
+(def ^:private query-with-metric
+ (-> (lib/query metadata-provider (meta/table-metadata :venues))
+ (lib/aggregate metric-clause)))
+
+(def ^:private metric-metadata
+ (lib.metadata/metric query-with-metric metric-id))
+
+(deftest ^:parallel query-suggested-name-test
+ (is (= "Venues, Sum of Cans"
+ (lib.metadata.calculation/suggested-name query-with-metric))))
+
+(deftest ^:parallel display-name-test
+ (doseq [metric [metric-clause
+ metric-metadata]
+ style [nil
+ :default
+ :long]]
+ (testing (str "metric = " (pr-str metric) "\n"
+ "style = " (pr-str style))
+ (is (= "Sum of Cans"
+ (if style
+ (lib.metadata.calculation/display-name query-with-metric -1 metric style)
+ (lib.metadata.calculation/display-name query-with-metric metric)))))))
+
+(deftest ^:parallel unknown-display-name-test
+ (let [metric [:metric {} 1]]
+ (doseq [style [nil
+ :default
+ :long]]
+ (testing (str "style = " (pr-str style))
+ (is (= "[Unknown Metric]"
+ (if style
+ (lib.metadata.calculation/display-name query-with-metric -1 metric style)
+ (lib.metadata.calculation/display-name query-with-metric metric))))))))
+
+(deftest ^:parallel display-info-test
+ (are [metric] (=? {:name "sum_of_cans"
+ :display-name "Sum of Cans"
+ :long-display-name "Sum of Cans"
+ :effective-type :type/Integer
+ :description "Number of toucans plus number of pelicans"
+ :selected true}
+ (lib.metadata.calculation/display-info query-with-metric metric))
+ metric-clause
+ metric-metadata))
+
+(deftest ^:parallel display-info-unselected-metric-test
+ (testing "Include `:selected false` in display info for Metrics not in aggregations"
+ (are [metric] (not (:selected (lib.metadata.calculation/display-info lib.tu/venues-query metric)))
+ metric-clause
+ metric-metadata)))
+
+(deftest ^:parallel unknown-display-info-test
+ (is (=? {:effective-type :type/*
+ :display-name "[Unknown Metric]"
+ :long-display-name "[Unknown Metric]"}
+ (lib.metadata.calculation/display-info query-with-metric [:metric {} 1]))))
+
+(deftest ^:parallel type-of-test
+ (are [metric] (= :type/Integer
+ (lib.metadata.calculation/type-of query-with-metric metric))
+ metric-clause
+ metric-metadata))
+
+(deftest ^:parallel unknown-type-of-test
+ (is (= :type/*
+ (lib.metadata.calculation/type-of query-with-metric [:metric {} 1]))))
+
+(deftest ^:parallel available-metrics-test
+ (testing "Should return Metrics with the same Table ID as query's `:source-table`"
+ (is (=? [{:lib/type :metadata/metric
+ :id metric-id
+ :name "Sum of Cans"
+ :table-id (meta/id :venues)
+ :definition metric-definition
+ :description "Number of toucans plus number of pelicans"}]
+ (lib/available-metrics (lib/query metadata-provider (meta/table-metadata :venues))))))
+ (testing "query with different Table -- don't return Metrics"
+ (is (nil? (lib/available-metrics (lib/query metadata-provider (meta/table-metadata :orders)))))))
+
+(deftest ^:parallel aggregate-with-metric-test
+ (testing "Should be able to pass a Metric metadata to `aggregate`"
+ (let [query (lib/query metadata-provider (meta/table-metadata :venues))
+ metrics (lib/available-metrics query)]
+ (is (= 1
+ (count metrics)))
+ ;; test with both `:metadata/metric` and with a `:metric` ref clause
+ (doseq [metric [(first metrics)
+ [:metric {:lib/uuid (str (random-uuid))} 100]]]
+ (testing (pr-str (list 'lib/aggregate 'query metric))
+ (let [query' (lib/aggregate query metric)]
+ (is (=? {:lib/type :mbql/query
+ :stages [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :venues)
+ :aggregation [[:metric {:lib/uuid string?} 100]]}]}
+ query'))
+ (is (=? [[:metric {:lib/uuid string?} 100]]
+ (lib/aggregations query')))
+ (is (=? [{:name "sum_of_cans"
+ :display-name "Sum of Cans"
+ :long-display-name "Sum of Cans"
+ :effective-type :type/Integer
+ :description "Number of toucans plus number of pelicans"
+ :selected true}]
+ (map (partial lib/display-info query')
+ (lib/aggregations query'))))))))))
diff --git a/test/metabase/lib/schema/expression/arithmetic_test.cljc b/test/metabase/lib/schema/expression/arithmetic_test.cljc
index 1900214092db84d9631d439afa87a562bbc0fe4f..bc8998e7fa956ba568fed325590b790fa0a8da78 100644
--- a/test/metabase/lib/schema/expression/arithmetic_test.cljc
+++ b/test/metabase/lib/schema/expression/arithmetic_test.cljc
@@ -4,6 +4,7 @@
[malli.core :as mc]
[malli.error :as me]
[metabase.lib.schema]
+ [metabase.lib.schema.aggregation :as aggregation]
[metabase.lib.schema.expression :as expression]
[metabase.lib.test-metadata :as meta]))
@@ -151,3 +152,14 @@
{:lib/uuid "00000000-0000-0000-0000-000000000000"}
[:interval {:lib/uuid "00000000-0000-0000-0000-000000000002"} 3 :minute]
[:field {:base-type :type/Date, :lib/uuid "00000000-0000-0000-0000-000000000001"} 1]]))))))
+
+
+(deftest ^:parallel metric-test
+ (are [schema] (not (me/humanize (mc/explain schema
+ [:+
+ {:lib/uuid "00000000-0000-0000-0000-000000000000"}
+ [:metric {:lib/uuid "00000000-0000-0000-0000-000000000000"} 1]
+ 2])))
+ :mbql.clause/+
+ ::expression/number
+ ::aggregation/aggregation))
diff --git a/test/metabase/lib/schema/expression/conditional_test.cljc b/test/metabase/lib/schema/expression/conditional_test.cljc
index 5f83adf155948ce41d3aa4ddb42d9fb9a07a4182..15cbea5c8e867dd64ec297f91c6666eadace40cc 100644
--- a/test/metabase/lib/schema/expression/conditional_test.cljc
+++ b/test/metabase/lib/schema/expression/conditional_test.cljc
@@ -4,7 +4,16 @@
[malli.core :as mc]
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.expression :as expression]
- [metabase.lib.test-metadata :as meta]))
+ [metabase.lib.schema.expression.conditional :as expression.conditional]
+ [metabase.lib.test-metadata :as meta]
+ [metabase.test-runner.assert-exprs.malli-equals]
+ [metabase.util :as u]))
+
+(comment metabase.test-runner.assert-exprs.malli-equals/keep-me)
+
+(deftest ^:parallel best-return-type-test
+ (is (= ::expression/type.unknown
+ (#'expression.conditional/best-return-type :type/Integer ::expression/type.unknown))))
(defn- case-expr [& args]
(let [clause [:case
@@ -74,24 +83,48 @@
(deftest ^:parallel case-type-of-with-fields-only-test
;; Ideally expression/type-of should have enough information to determine the types of fields.
(testing "The type of a case expression can be determined even if it consists of fields only."
- (is (= :type/*
- (expression/type-of
- [:case
- {:lib/uuid "8c6e099e-b856-4aeb-a8f6-2266b5d3d1e3"}
- [[[:>
- {:lib/uuid "9c4cc3b0-f3c7-4d34-ab53-640ba6e911e5"}
- [:field {:lib/uuid "435b08c8-9404-41a5-8c5a-00b415f14da6"} 25]
- 0]
- [:field {:lib/uuid "1c93ba8b-6a39-4ef2-a9e6-e3bcff042800"} 32]]]
- [:field
- {:source-field 29
- :lib/uuid "a5ab7f91-9826-40a7-9499-4a1a0184a450"}
- 23]])))))
+ (doseq [[message expr] {"no default"
+ [:case
+ {:lib/uuid "8c6e099e-b856-4aeb-a8f6-2266b5d3d1e3"}
+ [[[:>
+ {:lib/uuid "9c4cc3b0-f3c7-4d34-ab53-640ba6e911e5"}
+ [:field {:lib/uuid "435b08c8-9404-41a5-8c5a-00b415f14da6"} 25]
+ 0]
+ [:field {:lib/uuid "1c93ba8b-6a39-4ef2-a9e6-e3bcff042800"} 32]]]]
+
+ "with default :field"
+ [:case
+ {:lib/uuid "8c6e099e-b856-4aeb-a8f6-2266b5d3d1e3"}
+ [[[:>
+ {:lib/uuid "9c4cc3b0-f3c7-4d34-ab53-640ba6e911e5"}
+ [:field {:lib/uuid "435b08c8-9404-41a5-8c5a-00b415f14da6"} 25]
+ 0]
+ [:field {:lib/uuid "1c93ba8b-6a39-4ef2-a9e6-e3bcff042800"} 32]]]
+ [:field
+ {:source-field 29
+ :lib/uuid "a5ab7f91-9826-40a7-9499-4a1a0184a450"}
+ 23]]
+
+ "with default integer literal"
+ [:case
+ {:lib/uuid "66101767-c429-4499-b1b3-512e58267ea4"}
+ [[[:<
+ {:lib/uuid "580d9de8-8ade-4d64-a512-1e43a31fe869"}
+ [:field {:lib/uuid "347d5337-5da8-47ff-bc05-b11154e8d19c"} 139657]
+ 2]
+ [:field {:lib/uuid "a9f83548-d590-4dec-b7dd-ad2bbd0bbe9d"} 139657]]]
+ 0]}]
+ (testing (str message \newline (u/pprint-to-str expr))
+ (is (malli= :mbql.clause/case expr))
+ (is (= ::expression/type.unknown
+ (expression/type-of expr)))
+ (testing "type.unknown expressions should be allowed to be considered numeric expressions"
+ (is (malli= ::expression/number expr)))))))
(deftest ^:parallel coalasce-type-of-with-fields-only-test
;; Ideally expression/type-of should have enough information to determine the types of fields.
(testing "The type of a case expression can be determined even if it consists of fields only."
- (is (= :type/*
+ (is (= ::expression/type.unknown
(expression/type-of
[:coalesce
{:lib/uuid "8c6e099e-b856-4aeb-a8f6-2266b5d3d1e3"}
diff --git a/test/metabase/lib/stage_test.cljc b/test/metabase/lib/stage_test.cljc
index 39f810952527a88324b552a22e539b473432251a..299bb892b0f1a4d074a948ece8f5f2f5cd9e191d 100644
--- a/test/metabase/lib/stage_test.cljc
+++ b/test/metabase/lib/stage_test.cljc
@@ -33,13 +33,17 @@
(testing "expressions in aggregations"
(let [query (lib.tu/venues-query-with-last-stage
{:aggregation [[:*
- {}
+ {:lib/uuid (str (random-uuid))}
0.8
- [:avg {} (lib.tu/field-clause :venues :price)]]
+ [:avg
+ {:lib/uuid (str (random-uuid))}
+ (lib.tu/field-clause :venues :price)]]
[:*
- {}
+ {:lib/uuid (str (random-uuid))}
0.8
- [:avg {} (lib.tu/field-clause :venues :price)]]]})]
+ [:avg
+ {:lib/uuid (str (random-uuid))}
+ (lib.tu/field-clause :venues :price)]]]})]
(is (=? [{:base-type :type/Float
:name "expression"
:display-name "0.8 × Average of Price"
diff --git a/test/metabase/lib/test_metadata/graph_provider.cljc b/test/metabase/lib/test_metadata/graph_provider.cljc
index 21685d3cc637ccbe438944cdc6ce220a13ad3662..efe4288c36b00d02aaeeafb742095d0364a470e1 100644
--- a/test/metabase/lib/test_metadata/graph_provider.cljc
+++ b/test/metabase/lib/test_metadata/graph_provider.cljc
@@ -7,11 +7,12 @@
(defn- graph-database [metadata-graph]
(dissoc metadata-graph :tables))
+(defn- find-table [metadata-graph table-id]
+ (m/find-first #(= (:id %) table-id)
+ (:tables metadata-graph)))
+
(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)))
+ (dissoc (find-table metadata-graph table-id) :fields :metrics :segments))
(defn- graph-field [metadata-graph field-id]
(some (fn [table-metadata]
@@ -40,10 +41,10 @@
(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)))
+ (:fields (find-table metadata-graph table-id)))
+
+(defn- graph-metrics [metadata-graph table-id]
+ (:metrics (find-table metadata-graph table-id)))
(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]
@@ -56,6 +57,7 @@
(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))
+ (metrics [_this table-id] (graph-metrics metadata-graph table-id))
clojure.core.protocols/Datafiable
(datafy [_this]
diff --git a/test/metabase/lib/test_util.cljc b/test/metabase/lib/test_util.cljc
index d35bc62739ed3168c1f13cc8c347d2d59b4a4616..202af1ab3519d9fc73ef158513ae453c4ab932d9 100644
--- a/test/metabase/lib/test_util.cljc
+++ b/test/metabase/lib/test_util.cljc
@@ -6,11 +6,13 @@
[malli.core :as mc]
[medley.core :as m]
[metabase.lib.core :as lib]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.composed-provider
:as lib.metadata.composed-provider]
[metabase.lib.metadata.protocols :as metadata.protocols]
[metabase.lib.schema :as lib.schema]
[metabase.lib.test-metadata :as meta]
+ [metabase.util.malli :as mu]
#?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
#?(:cljs
@@ -35,7 +37,24 @@
options)
(meta/id table field)]))
-(defn mock-metadata-provider
+(defn- with-optional-lib-type
+ "Create a version of `schema` where `:lib/type` is optional rather than required."
+ [schema lib-type]
+ [:merge
+ schema
+ [:map
+ [:lib/type {:optional true} [:= lib-type]]]])
+
+(def ^:private MockMetadata
+ [:map
+ [:database {:optional true} [:maybe (with-optional-lib-type lib.metadata/DatabaseMetadata :metadata/database)]]
+ [:tables {:optional true} [:maybe [:sequential (with-optional-lib-type lib.metadata/TableMetadata :metadata/table)]]]
+ [:fields {:optional true} [:maybe [:sequential (with-optional-lib-type lib.metadata/ColumnMetadata :metadata/column)]]]
+ [:cards {:optional true} [:maybe [:sequential (with-optional-lib-type lib.metadata/CardMetadata :metadata/card)]]]
+ [:metrics {:optional true} [:maybe [:sequential (with-optional-lib-type lib.metadata/MetricMetadata :metadata/metric)]]]
+ [:segments {:optional true} [:maybe [:sequential (with-optional-lib-type lib.metadata/SegmentMetadata :metadata/segment)]]]])
+
+(mu/defn mock-metadata-provider :- lib.metadata/MetadataProvider
"Create a mock metadata provider to facilitate writing tests. All keys except `:database` should be a sequence of maps
e.g.
@@ -44,7 +63,7 @@
Normally you can probably get away with using [[metabase.lib.test-metadata/metadata-provider]] instead of using
this; but this is available for situations when you need to test something not covered by the default test metadata,
e.g. nested Fields."
- [{:keys [database tables fields cards metrics segments] :as m}]
+ [{:keys [database tables fields cards metrics segments] :as m} :- MockMetadata]
(reify
metadata.protocols/MetadataProvider
(database [_this] (some-> database
@@ -65,8 +84,11 @@
(-> (assoc table :lib/type :metadata/table)
(dissoc :fields))))
(fields [_this table-id] (for [field fields
- :when (= (:table_id field) table-id)]
+ :when (= (:table-id field) table-id)]
(assoc field :lib/type :metadata/column)))
+ (metrics [_this table-id] (for [metric metrics
+ :when (= (:table-id metric) table-id)]
+ (assoc metric :lib/type :metadata/metric)))
clojure.core.protocols/Datafiable
(datafy [_this]
diff --git a/test/metabase/lib/util_test.cljc b/test/metabase/lib/util_test.cljc
index d5dbfb21433bf75269286d4c1e605fdc5b412a9b..98030314edf69a04fba0ca6591d42b8633a6c464 100644
--- a/test/metabase/lib/util_test.cljc
+++ b/test/metabase/lib/util_test.cljc
@@ -137,7 +137,7 @@
(is (=? {:database 1
:stages [{:lib/type :mbql.stage/mbql
:source-table 1
- :aggregation [[:count]]}]}
+ :aggregation [[:count {:lib/uuid "00000000-0000-0000-0000-000000000000"}]]}]}
(lib.util/update-query-stage {:database 1
:type :query
:query {:source-table 1}}
@@ -145,7 +145,7 @@
update
:aggregation
conj
- [:count])))
+ [:count {:lib/uuid "00000000-0000-0000-0000-000000000000"}])))
(are [stage expected] (=? expected
(lib.util/update-query-stage {:database 1
:type :query
@@ -154,22 +154,22 @@
update
:aggregation
conj
- [:count]))
+ [:count {:lib/uuid "00000000-0000-0000-0000-000000000000"}]))
0 {:database 1
:stages [{:lib/type :mbql.stage/mbql
:source-table 1
- :aggregation [[:count]]}
+ :aggregation [[:count {:lib/uuid "00000000-0000-0000-0000-000000000000"}]]}
{:lib/type :mbql.stage/mbql}]}
1 {:database 1
:stages [{:lib/type :mbql.stage/mbql
:source-table 1}
{:lib/type :mbql.stage/mbql
- :aggregation [[:count]]}]}
+ :aggregation [[:count {:lib/uuid "00000000-0000-0000-0000-000000000000"}]]}]}
-1 {:database 1
:stages [{:lib/type :mbql.stage/mbql
:source-table 1}
{:lib/type :mbql.stage/mbql
- :aggregation [[:count]]}]})
+ :aggregation [[:count {:lib/uuid "00000000-0000-0000-0000-000000000000"}]]}]})
(testing "out of bounds"
(is (thrown-with-msg?
#?(:clj Throwable :cljs js/Error)
@@ -181,7 +181,7 @@
update
:aggregation
conj
- [:count])))))
+ [:count {:lib/uuid "00000000-0000-0000-0000-000000000000"}])))))
(deftest ^:parallel ensure-mbql-final-stage-test
(is (=? {:database 1
diff --git a/test/metabase/query_processor_test/test_mlv2.clj b/test/metabase/query_processor_test/test_mlv2.clj
index 06c00133d7eb27c33845a9fa8d9c1cb105e7423b..0fcca31fa8516946cc95c1bcfdc3a645220ce75d 100644
--- a/test/metabase/query_processor_test/test_mlv2.clj
+++ b/test/metabase/query_processor_test/test_mlv2.clj
@@ -6,9 +6,8 @@
[metabase.lib.convert :as lib.convert]
[metabase.lib.core :as lib]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
- [metabase.lib.metadata.jvm :as lib.metadata.jvm]
[metabase.lib.schema :as lib.schema]
- [metabase.mbql.util :as mbql.u]
+ [metabase.query-processor.store :as qp.store]
[metabase.util :as u]))
(set! *warn-on-reflection* true)
@@ -29,18 +28,9 @@
place, [[metabase.models.query.permissions-test/invalid-queries-test]]."
false)
-(defn- skip-metadata-calculation-tests? [legacy-query]
- ;; #29907: wrong column name for joined columns in `:breakout`
- (mbql.u/match-one legacy-query
- {:breakout breakouts}
- (mbql.u/match-one breakouts
- [:field _id-or-name {:join-alias _join-alias}]
- "#29907")))
-
(defn- test-mlv2-metadata [original-query _qp-metadata]
{:pre [(map? original-query)]}
- (when-not (or *skip-conversion-tests*
- (skip-metadata-calculation-tests? original-query))
+ (when-not *skip-conversion-tests*
(do-with-legacy-query-testing-context
original-query
(^:once fn* []
@@ -51,9 +41,8 @@
(do-with-pMBQL-query-testing-context
pMBQL
(^:once fn* []
- (let [metadata-provider (lib.metadata.jvm/application-database-metadata-provider (:database original-query))
- mlv2-query (lib/query metadata-provider pMBQL)
- mlv2-metadata (lib.metadata.calculation/metadata mlv2-query)]
+ (let [mlv2-query (lib/query (qp.store/metadata-provider) pMBQL)
+ mlv2-metadata (lib.metadata.calculation/metadata mlv2-query)]
;; Just make sure we can calculate some metadata (any metadata, even nothing) without throwing an
;; Exception at this point; making sure it is CORRECT will be the next step after this.
(is (any? mlv2-metadata)))))))))))
diff --git a/test/metabase/test_runner/assert_exprs.clj b/test/metabase/test_runner/assert_exprs.clj
index ce3f57a3e1f666be1c5d5f254217fe634489a2a2..bc1fc3d901eb40ca07b758fa62bbba9101e30b68 100644
--- a/test/metabase/test_runner/assert_exprs.clj
+++ b/test/metabase/test_runner/assert_exprs.clj
@@ -6,7 +6,10 @@
(:require
[clojure.data :as data]
[clojure.test :as t]
- [clojure.walk :as walk]))
+ [clojure.walk :as walk]
+ [metabase.test-runner.assert-exprs.malli-equals]))
+
+(comment metabase.test-runner.assert-exprs.malli-equals/keep-me)
(defn derecordize
"Convert all record types in `form` to plain maps, so tests won't fail."
diff --git a/test/metabase/test_runner/assert_exprs/malli_equals.cljc b/test/metabase/test_runner/assert_exprs/malli_equals.cljc
new file mode 100644
index 0000000000000000000000000000000000000000..551743eeb0fd7ca323a908d9a73261fc0bf33298
--- /dev/null
+++ b/test/metabase/test_runner/assert_exprs/malli_equals.cljc
@@ -0,0 +1,34 @@
+(ns metabase.test-runner.assert-exprs.malli-equals
+ (:require
+ [clojure.test :as t]
+ [malli.core :as mc]
+ [malli.error :as me]
+ [metabase.util :as u])
+ #?(:cljs
+ (:require-macros [metabase.test-runner.assert-exprs.malli-equals])))
+
+(defn malli=-report [message schema actuals]
+ (doseq [actual actuals]
+ (t/testing (str \newline (u/pprint-to-str actual))
+ (let [error (me/humanize (mc/explain schema actual))]
+ (t/do-report
+ {:type (if error :fail :pass)
+ :message message
+ :expected schema
+ :actual actual
+ :diffs [[actual [error nil]]]})))))
+
+#?(:clj
+ (do
+ ;; Clojure for Clojure usage
+ (defmethod t/assert-expr 'malli=
+ [message [_ schema & actuals]]
+ `(malli=-report ~message ~schema ~(vec actuals)))
+
+ ;; Clojure doing macroexpansion for ClojureScript usage.
+ (when-let [assert-expr (try
+ (requiring-resolve 'cljs.test/assert-expr)
+ (catch Throwable _))]
+ (defmethod (var-get assert-expr) 'malli=
+ [_env message [_ schema & actuals]]
+ `(malli=-report ~message ~schema ~(vec actuals))))))