From 5bbc0aa7ea7edb843645ab802e0be059dfeab619 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Mon, 11 Mar 2024 13:12:12 -0700
Subject: [PATCH] Convert QP to MLv2 Part 2 (#39824)
* Keep fetch_source_query.clj
* Copy fetch_source_query.clj into fetch_source_query_legacy.clj
* Set back fetch_source_query.clj file
* Convert QP to MLv2, part 1
* Keep legacy implementation around for time being
* Port thru convert-to-legacy
* expand-macros is converted to legacy
* Test fixes :wrench:
* Test fixes and better keys
* Test fixes
* Test fixes :wrench:
* More test fixes :wrench:
* Test fix? :wrench:
* Wow, maybe I finally fixed everything :wrench:
* Last few test fixes :wrench:
* PR feedback; lib.walk should support splicing in multiple replacement stages
* Update fetch-source-query to use improved lib.walk
* Have I fixed everything now? :wrench:
* Remove stray println
* Another MongoDB fix :wrench:
* Convert QP to MLv2 Part 2
* Remove most of the fetch-source-query-legacy stuff
* Remove the changes to expand-macros for now, we can do that in a follow-on PR.
* Test fix
* Test fix
* TEST FIXES?! :wrench:
* Kondo & test fixes
* Oops, make sure we have the updated tests
* Fixes :wrench:
* Test fixes :wrench:
* Convert resolve-source-table
* Appease Kondo
* Unparallelize the tests
* Another test that shouldn't be parallel
* Make tests more robust
* Test fixes :wrench:
---
.gitignore | 2 +-
src/metabase/api/common.clj | 10 +-
src/metabase/lib/card.cljc | 3 +-
src/metabase/lib/core.cljc | 18 +-
src/metabase/lib/limit.cljc | 5 +-
src/metabase/lib/metadata.cljc | 106 +++--
.../lib/metadata/cached_provider.cljc | 10 +-
.../lib/metadata/composed_provider.cljc | 29 +-
src/metabase/lib/native.cljc | 29 +-
src/metabase/lib/schema/binning.cljc | 2 +-
src/metabase/lib/schema/common.cljc | 3 -
src/metabase/lib/schema/filter.cljc | 2 +-
src/metabase/lib/schema/id.cljc | 17 +-
src/metabase/lib/schema/metadata.cljc | 33 ++
src/metabase/mbql/schema.cljc | 2 +-
src/metabase/mbql/util.cljc | 4 +-
src/metabase/models/data_permissions.clj | 8 +-
src/metabase/models/params/chain_filter.clj | 9 +-
src/metabase/models/user.clj | 6 +-
.../middleware/convert_to_legacy.clj | 12 -
.../middleware/expand_macros.clj | 374 +++++++++---------
.../middleware/fetch_source_query.clj | 19 +-
.../middleware/fetch_source_query_legacy.clj | 177 +--------
.../middleware/normalize_query.clj | 2 +-
.../middleware/permissions.clj | 5 +-
.../middleware/resolve_fields.clj | 47 ++-
.../middleware/resolve_referenced.clj | 108 ++---
.../middleware/resolve_source_table.clj | 37 +-
src/metabase/query_processor/preprocess.clj | 100 +++--
src/metabase/query_processor/store.clj | 115 +-----
.../util/tag_referenced_cards.clj | 27 --
src/metabase/shared/util/i18n.clj | 4 +
test/metabase/lib/metadata_test.cljc | 10 +
test/metabase/lib/native_test.cljc | 32 ++
.../middleware/expand_macros_test.clj | 120 ++++--
.../middleware/permissions_test.clj | 82 ++--
.../middleware/resolve_fields_test.clj | 15 +
.../middleware/resolve_referenced_test.clj | 170 +++-----
.../middleware/resolve_source_table_test.clj | 72 ++--
.../upgrade_field_literals_test.clj | 4 +-
.../util/tag_referenced_cards_test.clj | 25 --
41 files changed, 843 insertions(+), 1012 deletions(-)
delete mode 100644 src/metabase/query_processor/middleware/convert_to_legacy.clj
delete mode 100644 src/metabase/query_processor/util/tag_referenced_cards.clj
create mode 100644 test/metabase/query_processor/middleware/resolve_fields_test.clj
delete mode 100644 test/metabase/query_processor/util/tag_referenced_cards_test.clj
diff --git a/.gitignore b/.gitignore
index 865a72f456c..fb81b891690 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,5 @@
*.class
-*.cljc.rej
+*.*.rej
*.iml
*.jar
*.log
diff --git a/src/metabase/api/common.clj b/src/metabase/api/common.clj
index b76dcec3459..2f05d03e592 100644
--- a/src/metabase/api/common.clj
+++ b/src/metabase/api/common.clj
@@ -389,7 +389,7 @@
"Create a `(defroutes routes ...)` form that automatically includes all functions created with `defendpoint` in the
current namespace. Optionally specify middleware that will apply to all of the endpoints in the current namespace.
- (api/define-routes api/+check-superuser) ; all API endpoints in this namespace will require superuser access"
+ (api/define-routes api/+check-superuser) ; all API endpoints in this namespace will require superuser access"
{:style/indent 0}
[& middleware]
(let [api-route-fns (namespace->api-route-fns *ns*)
@@ -404,7 +404,7 @@
(defn +check-superuser
"Wrap a Ring handler to make sure the current user is a superuser before handling any requests.
- (api/+check-superuser routes)"
+ (api/+check-superuser routes)"
[handler]
(fn
([request]
@@ -444,9 +444,9 @@
(read-check (apply t2/select-one entity :id id other-conditions))))
(defn write-check
- "Check whether we can write an existing OBJ, or ENTITY with ID.
- If the object doesn't exist, throw a 404; if we don't have proper permissions, throw a 403.
- This will fetch the object if it was not already fetched, and returns OBJ if the check is successful."
+ "Check whether we can write an existing `obj`, or `entity` with `id`. If the object doesn't exist, throw a 404; if we
+ don't have proper permissions, throw a 403. This will fetch the object if it was not already fetched, and returns
+ `obj` if the check is successful."
{:style/indent 2}
([obj]
(check-404 obj)
diff --git a/src/metabase/lib/card.cljc b/src/metabase/lib/card.cljc
index 07d22074339..4f9df7f7b92 100644
--- a/src/metabase/lib/card.cljc
+++ b/src/metabase/lib/card.cljc
@@ -1,6 +1,5 @@
(ns metabase.lib.card
(:require
- [metabase.lib.convert :as lib.convert]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.query :as lib.query]
@@ -50,7 +49,7 @@
[metadata-providerable :- lib.metadata/MetadataProviderable
card-query :- :map]
(when (some? card-query)
- (lib.metadata.calculation/returned-columns (lib.query/query metadata-providerable (lib.convert/->pMBQL card-query)))))
+ (lib.metadata.calculation/returned-columns (lib.query/query metadata-providerable card-query))))
(def ^:private Card
[:map
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index 4c988f823b2..949f68592e8 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -248,18 +248,20 @@
[lib.metric
available-metrics]
[lib.native
+ engine
+ extract-template-tags
+ has-write-permission
+ native-extras
native-query
raw-native-query
- with-native-query
- template-tags
- engine
- with-template-tags
required-native-extras
- native-extras
- with-native-extras
+ template-tag-card-ids
+ template-tags-referenced-cards
+ template-tags
with-different-database
- has-write-permission
- extract-template-tags]
+ with-native-extras
+ with-native-query
+ with-template-tags]
[lib.order-by
change-direction
order-by
diff --git a/src/metabase/lib/limit.cljc b/src/metabase/lib/limit.cljc
index ae0c31ba55c..36eb165a5f9 100644
--- a/src/metabase/lib/limit.cljc
+++ b/src/metabase/lib/limit.cljc
@@ -2,7 +2,6 @@
(:require
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.schema :as lib.schema]
- [metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
[metabase.util.malli :as mu]))
@@ -19,13 +18,13 @@
([query :- ::lib.schema/query
stage-number :- :int
- n :- [:maybe ::lib.schema.common/positive-int]]
+ n :- [:maybe pos-int?]]
(lib.util/update-query-stage query stage-number (fn [stage]
(if n
(assoc stage :limit n)
(dissoc stage :limit))))))
-(mu/defn ^:export current-limit :- [:maybe ::lib.schema.common/positive-int]
+(mu/defn ^:export current-limit :- [:maybe pos-int?]
"Get the maximum number of rows to be returned by a stage of a query. `nil` indicates there is no limit"
([query :- ::lib.schema/query]
(current-limit query -1))
diff --git a/src/metabase/lib/metadata.cljc b/src/metabase/lib/metadata.cljc
index dbfd70d89f0..79fdeaf0a19 100644
--- a/src/metabase/lib/metadata.cljc
+++ b/src/metabase/lib/metadata.cljc
@@ -6,6 +6,7 @@
[metabase.lib.schema.id :as lib.schema.id]
[metabase.lib.schema.metadata :as lib.schema.metadata]
[metabase.lib.util :as lib.util]
+ [metabase.shared.util.i18n :as i18n]
[metabase.util.malli :as mu]))
;;; TODO -- deprecate all the schemas below, and just use the versions in [[lib.schema.metadata]] instead.
@@ -110,39 +111,7 @@
;;;; Stage metadata
-(def StageMetadata
- "Metadata about the columns returned by a particular stage of a pMBQL query. For example a single-stage native query
- like
-
- {:database 1
- :lib/type :mbql/query
- :stages [{:lib/type :mbql.stage/mbql
- :native \"SELECT id, name FROM VENUES;\"}]}
-
- might have stage metadata like
-
- {:columns [{:name \"id\", :base-type :type/Integer}
- {:name \"name\", :base-type :type/Text}]}
-
- associated with the query's lone stage.
-
- At some point in the near future we will hopefully attach this metadata directly to each stage in a query, so a
- multi-stage query will have `:lib/stage-metadata` for each stage. The main goal is to facilitate things like
- returning lists of visible or filterable columns for a given stage of a query. This is TBD, see #28717 for a WIP
- implementation of this idea.
-
- This is the same format as the results metadata returned with QP results in `data.results_metadata`. The `:columns`
- portion of this (`data.results_metadata.columns`) is also saved as `Card.result_metadata` for Saved Questions.
-
- Note that queries currently actually come back with both `data.results_metadata` AND `data.cols`; it looks like the
- Frontend actually *merges* these together -- see `applyMetadataDiff` in
- `frontend/src/metabase/query_builder/selectors.js` -- but this is ridiculous. Let's try to merge anything missing in
- `results_metadata` into `cols` going forward so things don't need to be manually merged in the future."
- [:map
- [:lib/type [:= :metadata/results]]
- [:columns [:sequential ColumnMetadata]]])
-
-(mu/defn stage :- [:maybe StageMetadata]
+(mu/defn stage :- [:maybe ::lib.schema.metadata/stage]
"Get metadata associated with a particular `stage-number` of the query, if any. `stage-number` can be a negative
index.
@@ -227,3 +196,74 @@
;; Couldn't import and use `lib.native/has-write-permissions` here due to a circular dependency
;; TODO Find a way to unify has-write-permissions and this function?
(= :write (:native-permissions (database query)))))))))
+
+(mu/defn fetch-bulk-metadata-with-non-bulk-provider :- [:maybe [:sequential :map]]
+ "Adapter to use a non-BulkMetadataProvider like one by calling the single-instance methods repeatedly. This is
+ mostly useful for mock metadata providers and the like; the only metadata provider where the performance boost
+ from [[bulk-metadata]] is important, the application database MetadataProvider, implements `BulkMetadata` natively."
+ [provider :- ::lib.schema.metadata/metadata-provider
+ metadata-type :- [:enum :metadata/card :metadata/column :metadata/metric :metadata/segment :metadata/table]
+ ids :- [:maybe
+ [:or
+ [:set pos-int?]
+ [:sequential pos-int?]]]]
+ (let [f (case metadata-type
+ :metadata/card lib.metadata.protocols/card
+ :metadata/column lib.metadata.protocols/field
+ :metadata/metric lib.metadata.protocols/metric
+ :metadata/segment lib.metadata.protocols/segment
+ :metadata/table lib.metadata.protocols/table)]
+ (into []
+ (keep (fn [id]
+ (f provider id)))
+ ids)))
+
+(mu/defn bulk-metadata :- [:maybe [:sequential [:map
+ [:lib/type :keyword]
+ [:id pos-int?]]]]
+ "Fetch multiple objects in bulk. If our metadata provider is a bulk provider (e.g., the application database
+ metadata provider), does a single fetch with [[lib.metadata.protocols/bulk-metadata]] if not (i.e., if this is a
+ mock provider), fetches them with repeated calls to the appropriate single-object method,
+ e.g. [[lib.metadata.protocols/field]].
+
+ The order of the returned objects will match the order of `ids`, but does check that all objects are returned. If
+ you want that behavior, use [[bulk-metadata-or-throw]] instead.
+
+ This can also be called for side-effects to warm the cache."
+ [metadata-providerable :- ::lib.schema.metadata/metadata-providerable
+ metadata-type :- [:enum :metadata/card :metadata/column :metadata/metric :metadata/segment :metadata/table]
+ ids :- [:maybe [:or [:sequential pos-int?] [:set pos-int?]]]]
+ (when-let [ids (not-empty (cond-> ids
+ (not (set? ids)) distinct))] ; remove duplicates but preserve order.
+ (let [provider (->metadata-provider metadata-providerable)
+ f (if (satisfies? lib.metadata.protocols/BulkMetadataProvider provider)
+ lib.metadata.protocols/bulk-metadata
+ fetch-bulk-metadata-with-non-bulk-provider)
+ results (f provider metadata-type ids)
+ id->result (into {} (map (juxt :id identity)) results)]
+ (into []
+ (comp (map id->result)
+ (filter some?))
+ ids))))
+
+(defn- missing-bulk-metadata-error [metadata-type id]
+ (ex-info (i18n/tru "Failed to fetch {0} {1}: either it does not exist, or it belongs to a different Database"
+ (pr-str metadata-type)
+ (pr-str id))
+ {:status-code 400
+ :metadata-type metadata-type
+ :id id}))
+
+(mu/defn bulk-metadata-or-throw :- [:maybe [:sequential [:map
+ [:lib/type :keyword]
+ [:id pos-int?]]]]
+ "Like [[bulk-metadata]], but verifies that all the requested objects were returned; throws an Exception otherwise."
+ [metadata-providerable :- ::lib.schema.metadata/metadata-providerable
+ metadata-type :- [:enum :metadata/card :metadata/column :metadata/metric :metadata/segment :metadata/table]
+ ids :- [:maybe [:or [:sequential pos-int?] [:set pos-int?]]]]
+ (let [results (bulk-metadata metadata-providerable metadata-type ids)
+ fetched-ids (into #{} (keep :id) results)]
+ (doseq [id ids]
+ (when-not (contains? fetched-ids id)
+ (throw (missing-bulk-metadata-error metadata-type id))))
+ results))
diff --git a/src/metabase/lib/metadata/cached_provider.cljc b/src/metabase/lib/metadata/cached_provider.cljc
index 1ae579871a1..d0c14933ee2 100644
--- a/src/metabase/lib/metadata/cached_provider.cljc
+++ b/src/metabase/lib/metadata/cached_provider.cljc
@@ -3,7 +3,6 @@
[clojure.set :as set]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.protocols :as lib.metadata.protocols]
- [metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.metadata :as lib.schema.metadata]
[metabase.util :as u]
[metabase.util.log :as log]
@@ -34,7 +33,7 @@
(mu/defn ^:private store-metadata!
[cache
metadata-type :- [:enum :metadata/database :metadata/table :metadata/column :metadata/card :metadata/metric :metadata/segment]
- id :- ::lib.schema.common/positive-int
+ id :- pos-int?
metadata :- [:multi
{:dispatch :lib/type}
[:metadata/database lib.metadata/DatabaseMetadata]
@@ -65,8 +64,11 @@
;; 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]))))
+ (into []
+ (comp (map (fn [id]
+ (get-in-cache cache [metadata-type id])))
+ (filter some?))
+ ids)))
(defn- tables [metadata-provider cache]
(let [fetched-tables #(lib.metadata.protocols/tables metadata-provider)]
diff --git a/src/metabase/lib/metadata/composed_provider.cljc b/src/metabase/lib/metadata/composed_provider.cljc
index aebae90a635..f3059547983 100644
--- a/src/metabase/lib/metadata/composed_provider.cljc
+++ b/src/metabase/lib/metadata/composed_provider.cljc
@@ -5,10 +5,8 @@
[clojure.datafy :as datafy]
[clojure.set :as set]
[medley.core :as m]
- [metabase.lib.metadata.protocols :as metadata.protocols]
- [metabase.lib.schema.common :as lib.schema.common]
- [metabase.lib.schema.metadata :as lib.schema.metadata]
- [metabase.util.malli :as mu]))
+ [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.metadata.protocols :as metadata.protocols]))
(defn- cached-providers [providers]
(filter #(satisfies? metadata.protocols/CachedMetadataProvider %)
@@ -27,24 +25,7 @@
(m/distinct-by :id))
metadata-providers))
-(mu/defn fetch-bulk-metadata-with-non-bulk-provider :- [:maybe [:sequential :map]]
- "Call a non-bulk metadata provider repeatedly to fetch multiple metadatas of the same type."
- [provider :- ::lib.schema.metadata/metadata-provider
- metadata-type :- [:enum :metadata/card :metadata/column :metadata/metric :metadata/segment :metadata/table]
- ids :- [:maybe
- [:or
- [:set ::lib.schema.common/positive-int]
- [:sequential ::lib.schema.common/positive-int]]]]
- (let [f (case metadata-type
- :metadata/card metadata.protocols/card
- :metadata/column metadata.protocols/field
- :metadata/metric metadata.protocols/metric
- :metadata/segment metadata.protocols/segment
- :metadata/table metadata.protocols/table)]
- (into []
- (keep (fn [id]
- (f provider id)))
- ids)))
+
(defn- bulk-metadata [providers metadata-type ids]
(loop [[provider & more-providers] providers, unfetched-ids (set ids), fetched []]
@@ -56,9 +37,7 @@
fetched
:else
- (let [newly-fetched (if (satisfies? metadata.protocols/BulkMetadataProvider provider)
- (metadata.protocols/bulk-metadata provider metadata-type unfetched-ids)
- (fetch-bulk-metadata-with-non-bulk-provider provider metadata-type unfetched-ids))
+ (let [newly-fetched (lib.metadata/bulk-metadata provider metadata-type unfetched-ids)
newly-fetched-ids (into #{} (map :id) newly-fetched)
unfetched-ids (set/difference unfetched-ids newly-fetched-ids)]
(recur more-providers
diff --git a/src/metabase/lib/native.cljc b/src/metabase/lib/native.cljc
index 8375b4447ec..b0f77112c2d 100644
--- a/src/metabase/lib/native.cljc
+++ b/src/metabase/lib/native.cljc
@@ -8,6 +8,8 @@
[metabase.lib.query :as lib.query]
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.common :as common]
+ [metabase.lib.schema.id :as lib.schema.id]
+ [metabase.lib.schema.metadata :as lib.schema.metadata]
[metabase.lib.schema.template-tag :as lib.schema.template-tag]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
@@ -125,7 +127,7 @@
(mu/defn required-native-extras :- set?
"Returns the extra keys that are required for this database's native queries, for example `:collection` name is
needed for MongoDB queries."
- [metadata-provider :- lib.metadata/MetadataProviderable]
+ [metadata-provider :- ::lib.schema.metadata/metadata-providerable]
(let [db (lib.metadata/database metadata-provider)]
(cond-> #{}
(get-in db [:features :native-requires-specified-collection])
@@ -154,13 +156,13 @@
"Create a new native query.
Native in this sense means a pMBQL query with a first stage that is a native query."
- ([metadata-providerable :- lib.metadata/MetadataProviderable
+ ([metadata-providerable :- ::lib.schema.metadata/metadata-providerable
inner-query :- ::common/non-blank-string]
(native-query metadata-providerable inner-query nil nil))
- ([metadata-providerable :- lib.metadata/MetadataProviderable
+ ([metadata-providerable :- ::lib.schema.metadata/metadata-providerable
inner-query :- ::common/non-blank-string
- results-metadata :- [:maybe lib.metadata/StageMetadata]
+ results-metadata :- [:maybe ::lib.schema.metadata/stage]
native-extras :- [:maybe ::native-extras]]
(let [tags (extract-template-tags inner-query)]
(-> (lib.query/query-with-stages metadata-providerable
@@ -174,10 +176,10 @@
"Changes the database for this query. The first stage must be a native type.
Native extras must be provided if the new database requires it."
([query :- ::lib.schema/query
- metadata-provider :- lib.metadata/MetadataProviderable]
+ metadata-provider :- ::lib.schema.metadata/metadata-providerable]
(with-different-database query metadata-provider nil))
([query :- ::lib.schema/query
- metadata-provider :- lib.metadata/MetadataProviderable
+ metadata-provider :- ::lib.schema.metadata/metadata-providerable
native-extras :- [:maybe ::native-extras]]
(assert-native-query! (lib.util/query-stage query 0))
;; Changing the database should also clean up template tags, see #31926
@@ -219,11 +221,24 @@
[query :- ::lib.schema/query]
(:native (lib.util/query-stage query 0)))
-(mu/defn template-tags :- ::lib.schema.template-tag/template-tag-map
+(mu/defn template-tags :- [:maybe ::lib.schema.template-tag/template-tag-map]
"Returns the native query's template tags"
[query :- ::lib.schema/query]
(:template-tags (lib.util/query-stage query 0)))
+(mu/defn template-tag-card-ids :- [:maybe [:set {:min 1} ::lib.schema.id/card]]
+ "Returns the card IDs from the template tags of the native query of `query`."
+ [query :- ::lib.schema/query]
+ (not-empty (into #{} (keep (fn [[_k m]] (:card-id m))) (template-tags query))))
+
+(mu/defn template-tags-referenced-cards :- [:maybe [:sequential ::lib.schema.metadata/card]]
+ "Returns Card instances referenced by the given native `query`."
+ [query :- ::lib.schema/query]
+ (mapv
+ (fn [card-id]
+ (lib.metadata/card query card-id))
+ (template-tag-card-ids query)))
+
(mu/defn has-write-permission :- :boolean
"Returns whether the database has native write permissions.
This is only filled in by [[metabase.api.database/add-native-perms-info]]
diff --git a/src/metabase/lib/schema/binning.cljc b/src/metabase/lib/schema/binning.cljc
index 41eea935d96..f427acd3ac6 100644
--- a/src/metabase/lib/schema/binning.cljc
+++ b/src/metabase/lib/schema/binning.cljc
@@ -12,7 +12,7 @@
[:enum :bin-width :default :num-bins])
(mr/def ::num-bins
- ::lib.schema.common/positive-int)
+ pos-int?)
(mr/def ::bin-width
::lib.schema.common/positive-number)
diff --git a/src/metabase/lib/schema/common.cljc b/src/metabase/lib/schema/common.cljc
index c6ca30a77b5..986ed1c50ec 100644
--- a/src/metabase/lib/schema/common.cljc
+++ b/src/metabase/lib/schema/common.cljc
@@ -18,9 +18,6 @@
(mr/def ::int-greater-than-or-equal-to-zero
[:int {:min 0}])
-(mr/def ::positive-int
- pos-int?)
-
(mr/def ::positive-number
[:fn
{:error/message "positive number"}
diff --git a/src/metabase/lib/schema/filter.cljc b/src/metabase/lib/schema/filter.cljc
index 735cdbb3d4a..67077e9c282 100644
--- a/src/metabase/lib/schema/filter.cljc
+++ b/src/metabase/lib/schema/filter.cljc
@@ -117,7 +117,7 @@
[:tuple
[:= :segment]
::common/options
- [:or ::common/positive-int ::common/non-blank-string]])
+ [:or pos-int? ::common/non-blank-string]])
(mr/def ::operator
[:map
diff --git a/src/metabase/lib/schema/id.cljc b/src/metabase/lib/schema/id.cljc
index a6ea94377dc..78f72841dd3 100644
--- a/src/metabase/lib/schema/id.cljc
+++ b/src/metabase/lib/schema/id.cljc
@@ -1,13 +1,12 @@
(ns metabase.lib.schema.id
(:require
- [metabase.lib.schema.common :as common]
[metabase.util.malli.registry :as mr]))
;;; these aren't anything special right now, but maybe in the future we can do something special/intelligent with
;;; them, e.g. when we start working on the generative stuff
(mr/def ::database
- ::common/positive-int)
+ pos-int?)
(def saved-questions-virtual-database-id
"The ID used to signify that a database is 'virtual' rather than physical.
@@ -30,22 +29,22 @@
[:= saved-questions-virtual-database-id])
(mr/def ::table
- ::common/positive-int)
+ pos-int?)
(mr/def ::field
- ::common/positive-int)
+ pos-int?)
(mr/def ::card
- ::common/positive-int)
+ pos-int?)
(mr/def ::segment
- ::common/positive-int)
+ pos-int?)
(mr/def ::metric
- ::common/positive-int)
+ pos-int?)
(mr/def ::snippet
- ::common/positive-int)
+ pos-int?)
(mr/def ::dimension
- ::common/positive-int)
+ pos-int?)
diff --git a/src/metabase/lib/schema/metadata.cljc b/src/metabase/lib/schema/metadata.cljc
index 181088a00c1..797bdceb5d8 100644
--- a/src/metabase/lib/schema/metadata.cljc
+++ b/src/metabase/lib/schema/metadata.cljc
@@ -300,3 +300,36 @@
[:map
{:error/message "map with a MetadataProvider in the key :lib/metadata (i.e. a query)"}
[:lib/metadata [:ref ::metadata-provider]]]])
+
+;;; Metadata about the columns returned by a particular stage of a pMBQL query. For example a single-stage native
+;;; query like
+;;;
+;;; {:database 1
+;;; :lib/type :mbql/query
+;;; :stages [{:lib/type :mbql.stage/mbql
+;;; :native "SELECT id, name FROM VENUES;"}]}
+;;;
+;;; might have stage metadata like
+;;;
+;;; {:columns [{:name "id", :base-type :type/Integer}
+;;; {:name "name", :base-type :type/Text}]}
+;;;
+;;; associated with the query's lone stage.
+;;;
+;;; At some point in the near future we will hopefully attach this metadata directly to each stage in a query, so a
+;;; multi-stage query will have `:lib/stage-metadata` for each stage. The main goal is to facilitate things like
+;;; returning lists of visible or filterable columns for a given stage of a query. This is TBD, see #28717 for a WIP
+;;; implementation of this idea.
+;;;
+;;; This is the same format as the results metadata returned with QP results in `data.results_metadata`. The
+;;; `:columns` portion of this (`data.results_metadata.columns`) is also saved as `Card.result_metadata` for Saved
+;;; Questions.
+;;;
+;;; Note that queries currently actually come back with both `data.results_metadata` AND `data.cols`; it looks like
+;;; the Frontend actually *merges* these together -- see `applyMetadataDiff` in
+;;; `frontend/src/metabase/query_builder/selectors.js` -- but this is ridiculous. Let's try to merge anything missing
+;;; in `results_metadata` into `cols` going forward so things don't need to be manually merged in the future.
+(mr/def ::stage
+ [:map
+ [:lib/type [:= :metadata/results]]
+ [:columns [:sequential ::column]]])
diff --git a/src/metabase/mbql/schema.cljc b/src/metabase/mbql/schema.cljc
index a987f6c46d1..ba877419b89 100644
--- a/src/metabase/mbql/schema.cljc
+++ b/src/metabase/mbql/schema.cljc
@@ -42,7 +42,7 @@
[:ref ::lib.schema.common/semantic-or-relation-type])
(def ^:private PositiveInt
- [:ref ::lib.schema.common/positive-int])
+ pos-int?)
(def ^:private IntGreaterThanOrEqualToZero
[:ref ::lib.schema.common/int-greater-than-or-equal-to-zero])
diff --git a/src/metabase/mbql/util.cljc b/src/metabase/mbql/util.cljc
index 496b55d18ca..30ce299d355 100644
--- a/src/metabase/mbql/util.cljc
+++ b/src/metabase/mbql/util.cljc
@@ -349,7 +349,7 @@
[filter-clause :- mbql.s/Filter]
(-> filter-clause desugar-filter-clause negate* simplify-compound-filter))
-(mu/defn query->source-table-id :- [:maybe ::lib.schema.common/positive-int]
+(mu/defn query->source-table-id :- [:maybe pos-int?]
"Return the source Table ID associated with `query`, if applicable; handles nested queries as well. If `query` is
`nil`, returns `nil`.
@@ -382,7 +382,7 @@
:else
source-table-id))
-(mu/defn join->source-table-id :- [:maybe ::lib.schema.common/positive-int]
+(mu/defn join->source-table-id :- [:maybe pos-int?]
"Like `query->source-table-id`, but for a join."
[join]
(query->source-table-id {:type :query, :query join}))
diff --git a/src/metabase/models/data_permissions.clj b/src/metabase/models/data_permissions.clj
index bd41439f2ee..c514b91c10f 100644
--- a/src/metabase/models/data_permissions.clj
+++ b/src/metabase/models/data_permissions.clj
@@ -579,7 +579,7 @@
being a table-level permission."
[group-or-id :- TheIdable
db-or-id :- TheIdable
- perm-type :- :keyword
+ perm-type :- PermissionType
value :- :keyword]
(t2/with-transaction [_conn]
(let [group-id (u/the-id group-or-id)
@@ -604,7 +604,7 @@
is removed and table-level rows are are added for all of its tables. Similarly, if setting a table-level permission to a value
that results in all of the database's tables having the same permission, it is replaced with a single database-level row."
[group-or-id :- TheIdable
- perm-type :- :keyword
+ perm-type :- PermissionType
table-perms :- [:map-of TheIdable :keyword]]
(when (not= :model/Table (model-by-perm-type perm-type))
(throw (ex-info (tru "Permission type {0} cannot be set on tables." perm-type)
@@ -684,7 +684,7 @@
"Sets permissions for a single table to the specified value for a given group."
[group-or-id :- TheIdable
table-or-id :- TheIdable
- perm-type :- :keyword
+ perm-type :- PermissionType
value :- :keyword]
(set-table-permissions! group-or-id perm-type {table-or-id value}))
@@ -711,7 +711,7 @@
Otherwise, the new table permission is added with the provided value."
[groups-or-ids :- [:sequential TheIdable]
table-or-id :- TheIdable
- perm-type :- :keyword
+ perm-type :- PermissionType
value :- :keyword]
(when (not= :model/Table (model-by-perm-type perm-type))
(throw (ex-info (tru "Permission type {0} cannot be set on tables." perm-type)
diff --git a/src/metabase/models/params/chain_filter.clj b/src/metabase/models/params/chain_filter.clj
index 5f2838ab9aa..9e1cfa5b6fb 100644
--- a/src/metabase/models/params/chain_filter.clj
+++ b/src/metabase/models/params/chain_filter.clj
@@ -80,6 +80,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.query-processor.preprocess :as qp.preprocess]
+ [metabase.query-processor.setup :as qp.setup]
[metabase.types :as types]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
@@ -526,9 +527,11 @@
(defn- check-field-value-query-permissions
"Check query permissions against the chain-filter-mbql-query (private #196)"
[field-id constraints options]
- (->> (chain-filter-mbql-query field-id constraints options)
- qp.preprocess/preprocess
- qp.perms/check-query-permissions*))
+ (let [query (chain-filter-mbql-query field-id constraints options)]
+ (qp.setup/with-qp-setup [query query]
+ (->> query
+ qp.preprocess/preprocess
+ qp.perms/check-query-permissions*))))
(defn- cached-field-values [field-id constraints {:keys [limit]}]
;; TODO: why don't we remap the human readable values here?
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index ecf1ef8d17b..e516f2b5126 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -267,7 +267,11 @@
[:id (when (premium-features/enable-advanced-permissions?)
:is_group_manager)]))]
(for [user users]
- (assoc user :user_group_memberships (map membership->group (user-id->memberships (u/the-id user))))))))
+ (assoc user :user_group_memberships (->> (user-id->memberships (u/the-id user))
+ (map membership->group)
+ ;; sort these so the id returned is consistent so our tests don't
+ ;; randomly fail
+ (sort-by :id)))))))
(mi/define-batched-hydration-method add-group-ids
:group_ids
diff --git a/src/metabase/query_processor/middleware/convert_to_legacy.clj b/src/metabase/query_processor/middleware/convert_to_legacy.clj
deleted file mode 100644
index b3ef95a8a41..00000000000
--- a/src/metabase/query_processor/middleware/convert_to_legacy.clj
+++ /dev/null
@@ -1,12 +0,0 @@
-(ns metabase.query-processor.middleware.convert-to-legacy
- (:require
- [metabase.lib.convert :as lib.convert]
- [metabase.mbql.schema :as mbql.s]
- [metabase.util.malli :as mu]))
-
-(mu/defn convert-to-legacy :- mbql.s/Query
- "Middleware that converts and MLv2 query back to a legacy query. This is temporary until we concert the entire QP to
- use MLv2 everywhere."
- [query :- :map]
- (cond-> query
- (= (:lib/type query) :mbql/query) lib.convert/->legacy-MBQL))
diff --git a/src/metabase/query_processor/middleware/expand_macros.clj b/src/metabase/query_processor/middleware/expand_macros.clj
index 47e282be0e8..83408dd66e1 100644
--- a/src/metabase/query_processor/middleware/expand_macros.clj
+++ b/src/metabase/query_processor/middleware/expand_macros.clj
@@ -1,206 +1,194 @@
(ns metabase.query-processor.middleware.expand-macros
- "Middleware for expanding `:metric` and `:segment` 'macros' in *unexpanded* MBQL queries.
+ "Middleware for expanding LEGACY `:metric` and `:segment` 'macros' in *unexpanded* MBQL queries.
(`:metric` forms are expanded into aggregations and sometimes filter clauses, while `:segment` forms are expanded
- into filter clauses.)
-
- TODO - this namespace is ancient and written with MBQL '95 in mind, e.g. it is case-sensitive.
- At some point this ought to be reworked to be case-insensitive and cleaned up."
+ into filter clauses.)"
(:require
- [malli.core :as mc]
- [malli.error :as me]
- [metabase.mbql.schema :as mbql.s]
- [metabase.mbql.schema.helpers :as helpers]
+ [metabase.lib.convert :as lib.convert]
+ [metabase.lib.filter :as lib.filter]
+ [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.options :as lib.options]
+ [metabase.lib.schema :as lib.schema]
+ [metabase.lib.schema.aggregation :as lib.schema.aggregation]
+ [metabase.lib.schema.expression :as lib.schema.expression]
+ [metabase.lib.schema.metadata :as lib.schema.metadata]
+ [metabase.lib.util :as lib.util]
+ [metabase.lib.walk :as lib.walk]
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.error-type :as qp.error-type]
- [metabase.query-processor.store :as qp.store]
- [metabase.util.i18n :refer [trs tru]]
+ [metabase.util :as u]
+ [metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
- [metabase.util.malli.schema :as ms]))
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | SEGMENTS |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defn- segment-clauses->id->definition [segment-clauses]
- (when-let [segment-ids (not-empty (into #{}
- (comp (map second)
- (filter integer?))
- segment-clauses))]
- (into {}
- (map (juxt :id :definition))
- (qp.store/bulk-metadata :metadata/segment segment-ids))))
-
-(defn- replace-segment-clauses [outer-query segment-id->definition]
- (mbql.u/replace-in outer-query [:query]
- [:segment (segment-id :guard (complement mbql.u/ga-id?))]
- (or (:filter (segment-id->definition segment-id))
- (throw (IllegalArgumentException. (tru "Segment {0} does not exist, or is invalid." segment-id))))))
-
-(mu/defn ^:private expand-segments :- mbql.s/Query
- "Recursively expand segments in the `query`."
- [query :- mbql.s/Query]
- (loop [{inner-query :query :as outer-query} query
- depth 0]
- (if-let [segments (mbql.u/match inner-query [:segment (_ :guard (complement mbql.u/ga-id?))])]
- (let [segment-id->definition (segment-clauses->id->definition segments)
- expanded-query (replace-segment-clauses outer-query segment-id->definition)]
- ;; Following line is in place to avoid infinite recursion caused by mutually recursive
- ;; segment definitions or other unforseen circumstances. Number 41 is arbitrary.
- (if (or (= expanded-query outer-query) (= depth 41))
- (throw (ex-info (tru "Segment expansion failed. Check mutually recursive segment definitions.")
- {:type qp.error-type/invalid-query
- :original-query query
- :expanded-query expanded-query
- :segment-id->definition segment-id->definition
- :depth depth}))
- (recur expanded-query (inc depth))))
- outer-query)))
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | METRICS |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defn- metrics
- "Return a sequence of any (non-GA) `:metric` MBQL clauses in `query`."
- [query]
- ;; metrics won't be in a native query but they could be in source-query or aggregation clause
- (mbql.u/match query [:metric (_ :guard (complement mbql.u/ga-id?))]))
-
-(def ^:private MetricInfo
- [:map
- [:id ms/PositiveInt]
- [:name ms/NonBlankString]
- [:definition [:map
- [:aggregation [:tuple mbql.s/Aggregation]]
- [:filter {:optional true} [:maybe mbql.s/Filter]]]]])
-
-(defn- metric-info-validation-errors [metric-info]
- (me/humanize (mc/explain MetricInfo metric-info)))
-
-(mu/defn ^:private metric-clauses->id->info :- [:map-of ms/PositiveInt MetricInfo]
- [metric-clauses :- [:sequential mbql.s/metric]]
- (when-let [metric-ids (not-empty (into #{} (map second) metric-clauses))]
- (into {}
- (comp (remove (fn [metric]
- (when-let [errors (metric-info-validation-errors metric)]
- (log/warn (trs "Invalid metric: {0} reason: {1}" metric errors))
- errors)))
- (map (juxt :id #(select-keys % [:id :name :definition]))))
- (qp.store/bulk-metadata :metadata/metric metric-ids))))
-
-(mu/defn ^:private add-metrics-filters-this-level :- mbql.s/MBQLQuery
- [inner-query :- mbql.s/MBQLQuery
- this-level-metric-id->info :- [:map-of ms/PositiveInt MetricInfo]]
- (let [filters (for [{{filter-clause :filter} :definition} (vals this-level-metric-id->info)
- :when filter-clause]
- filter-clause)]
- (reduce mbql.u/add-filter-clause-to-inner-query inner-query filters)))
-
-(mu/defn ^:private metric-info->ag-clause :- mbql.s/Aggregation
- "Return an appropriate aggregation clause from `metric-info`."
- [{{[aggregation] :aggregation} :definition, metric-name :name} :- MetricInfo
- {:keys [use-metric-name-as-display-name?]} :- [:map [:use-metric-name-as-display-name? :boolean]]]
- (if-not use-metric-name-as-display-name?
- aggregation
- ;; try to give the resulting aggregation the name of the Metric it came from, unless it already has a display
- ;; name in which case keep that name
- (mbql.u/match-one aggregation
- [:aggregation-options _ (_ :guard :display-name)]
- &match
-
- [:aggregation-options ag options]
- [:aggregation-options ag (assoc options :display-name metric-name)]
-
- _
- [:aggregation-options &match {:display-name metric-name}])))
-
-(mu/defn ^:private replace-metrics-aggregations-this-level :- mbql.s/MBQLQuery
- [inner-query :- mbql.s/MBQLQuery
- this-level-metric-id->info :- [:map-of ms/PositiveInt MetricInfo]]
- (letfn [(metric [metric-id]
- (or (get this-level-metric-id->info metric-id)
- (throw (ex-info (tru "Metric {0} does not exist, or is invalid." metric-id)
- {:type :invalid-query
- :metric metric-id
- :query inner-query}))))]
- (mbql.u/replace-in inner-query [:aggregation]
- ;; if metric is wrapped in aggregation options that give it a display name, expand the metric but do not name it
- [:aggregation-options [:metric (metric-id :guard (complement mbql.u/ga-id?))] (options :guard :display-name)]
- [:aggregation-options
- (metric-info->ag-clause (metric metric-id) {:use-metric-name-as-display-name? false})
- options]
-
- ;; if metric is wrapped in aggregation options that *do not* give it a display name, expand the metric and then
- ;; merge the options
- [:aggregation-options [:metric (metric-id :guard (complement mbql.u/ga-id?))] options]
- (let [[_ ag ag-options] (metric-info->ag-clause (metric metric-id) {:use-metric-name-as-display-name? true})]
- [:aggregation-options ag (merge ag-options options)])
-
- ;; otherwise for unwrapped metrics expand them in-place
- [:metric (metric-id :guard (complement mbql.u/ga-id?))]
- (metric-info->ag-clause (metric metric-id) {:use-metric-name-as-display-name? true}))))
-
-(mu/defn ^:private metric-ids-this-level :- [:maybe [:set ms/PositiveInt]]
- [inner-query]
- (when (map? inner-query)
- (when-let [aggregations (:aggregation inner-query)]
- (not-empty
- (set
- (mbql.u/match aggregations
- [:metric (metric-id :guard (complement mbql.u/ga-id?))]
- metric-id))))))
-
-(mu/defn ^:private expand-metrics-clauses-this-level :- [:and
- mbql.s/MBQLQuery
- [:fn
- {:error/message "Inner MBQL query with no :metric clauses at this level"}
- (complement metric-ids-this-level)]]
- [inner-query :- mbql.s/MBQLQuery
- metric-id->info :- [:map-of ms/PositiveInt MetricInfo]]
- (let [this-level-metric-ids (metric-ids-this-level inner-query)
- this-level-metric-id->info (select-keys metric-id->info this-level-metric-ids)]
- (-> inner-query
- (add-metrics-filters-this-level this-level-metric-id->info)
- (replace-metrics-aggregations-this-level this-level-metric-id->info))))
-
-(mu/defn ^:private expand-metrics-clauses :- ms/Map
- "Add appropriate `filter` and `aggregation` clauses for a sequence of Metrics.
-
- (expand-metrics-clauses {:query {}} [[:metric 10]])
- ;; -> {:query {:aggregation [[:count]], :filter [:= [:field-id 10] 20]}}"
- [query :- ms/Map metric-id->info :- (helpers/non-empty [:map-of ms/PositiveInt MetricInfo])]
- (mbql.u/replace query
- (m :guard metric-ids-this-level)
- (-> m
- ;; expand this this level...
- (expand-metrics-clauses-this-level metric-id->info)
- ;; then recursively expand things at any other levels.
- (expand-metrics-clauses metric-id->info))))
-
-(mu/defn ^:private expand-metrics :- mbql.s/Query
- [query :- mbql.s/Query]
- (if-let [metrics (metrics query)]
- (expand-metrics-clauses query (metric-clauses->id->info metrics))
+ [metabase.util.malli.registry :as mr]))
+
+;;; "legacy macro" as used below means EITHER a legacy Metric or a legacy Segment.
+(mr/def ::legacy-macro
+ [:or
+ ::lib.schema.metadata/metric
+ ::lib.schema.metadata/segment])
+
+(mr/def ::macro-type
+ [:enum :metric :segment])
+
+(mu/defn unresolved-legacy-macro-ids :- [:maybe [:set {:min 1} pos-int?]]
+ "Find all the unresolved legacy :metric and :segment references in `query`.
+
+ :metric references only appear in aggregations; :segment references can appear anywhere a boolean expression is
+ allowed, including `:filters`, join conditions, expression aggregations like `:sum-where`, etc."
+ [macro-type :- ::macro-type
+ query :- ::lib.schema/query]
+ (let [ids (transient #{})]
+ (lib.walk/walk-stages
+ query
+ (fn [_query _path stage]
+ (mbql.u/match stage
+ [macro-type _opts (id :guard pos-int?)]
+ (conj! ids id))))
+ (not-empty (persistent! ids))))
+
+;;; a legacy Metric has exactly one aggregation clause, and possibly one or more filter clauses as well.
+;;;
+;;; a legacy Segment has one or more filter clauses.
+
+(defn- legacy-macro-definition->pMBQL
+ "Get the definition of a legacy Metric as a pMBQL stage."
+ [definition]
+ (log/tracef "Converting legacy MBQL for macro definition from\n%s" (u/pprint-to-str definition))
+ (u/prog1 (-> (lib.convert/->pMBQL {:type :query
+ :query definition})
+ (lib.util/query-stage -1))
+ (log/tracef "to pMBQL\n%s" (u/pprint-to-str <>))))
+
+(mu/defn ^:private legacy-metric-aggregation :- ::lib.schema.aggregation/aggregation
+ "Get the aggregation associated with a legacy Metric."
+ [legacy-metric :- ::lib.schema.metadata/metric]
+ (-> (or (first (get-in legacy-metric [:definition :aggregation]))
+ (throw (ex-info (tru "Invalid legacy Metric: missing aggregation")
+ {:type qp.error-type/invalid-query, :legacy-metric legacy-metric})))
+ ;; make sure aggregation has a display-name: keep the one attached directly to the aggregation if there is one;
+ ;; otherwise use the Metric's name
+ (lib.options/update-options update :display-name #(or % (:name legacy-metric)))
+ ;; make sure it has fresh UUIDs in case we need to add it to the query more than once (multiple Metric references
+ ;; are possible if the query joins the same source query twice for example)
+ lib.util/fresh-uuids))
+
+(mu/defn ^:private legacy-macro-filters :- [:maybe [:sequential ::lib.schema.expression/boolean]]
+ "Get the filter(s) associated with a legacy Metric or Segment."
+ [legacy-macro :- ::legacy-macro]
+ (mapv lib.util/fresh-uuids
+ (get-in legacy-macro [:definition :filters])))
+
+(mr/def ::id->legacy-macro
+ [:map-of pos-int? ::legacy-macro])
+
+(mu/defn ^:private fetch-legacy-macros :- ::id->legacy-macro
+ [macro-type :- ::macro-type
+ metadata-providerable :- ::lib.schema.metadata/metadata-providerable
+ legacy-macro-ids :- [:maybe [:set {:min 1} pos-int?]]]
+ (let [metadata-type (case macro-type
+ :metric :metadata/metric
+ :segment :metadata/segment)]
+ (u/prog1 (into {}
+ (map (juxt :id (fn [legacy-macro]
+ (update legacy-macro :definition legacy-macro-definition->pMBQL))))
+ (lib.metadata/bulk-metadata metadata-providerable metadata-type legacy-macro-ids))
+ ;; make sure all the IDs exist.
+ (doseq [id legacy-macro-ids]
+ (or (get <> id)
+ (throw (ex-info (tru "Legacy Metric/Segment {0} does not exist, belongs to a different Database, or is invalid."
+ id)
+ {:type qp.error-type/invalid-query, :macro-type macro-type, :id id})))))))
+
+(defmulti ^:private resolve-legacy-macros-in-stage
+ {:arglists '([macro-type stage id->legacy-macro])}
+ (fn [macro-type _stage _id->legacy-macro]
+ macro-type))
+
+(mu/defmethod resolve-legacy-macros-in-stage :metric :- ::lib.schema/stage
+ [_macro-type :- [:= :metric]
+ stage :- ::lib.schema/stage
+ id->legacy-metric :- ::id->legacy-macro]
+ (let [new-filters (atom [])
+ stage' (mbql.u/replace-in stage [:aggregation]
+ [:metric opts-from-ref (id :guard pos-int?)]
+ (let [legacy-metric (get id->legacy-metric id)
+ aggregation (-> (legacy-metric-aggregation legacy-metric)
+ ;; preserve the `:name` and `:display-name` from the `:metric` ref itself
+ ;; if there are any. Very important! Preserve `:lib/uuid` so anything
+ ;; `:aggregation` references referring to the Metric will still be valid
+ ;; after macroexpansion.
+ (lib.options/update-options merge (select-keys opts-from-ref
+ [:name :display-name :lib/uuid])))
+ filters (legacy-macro-filters legacy-metric)]
+ (log/debugf "Expanding legacy Metric macro\n%s" (u/pprint-to-str &match))
+ (log/tracef "Adding aggregation clause for legacy Metric %d:\n%s" id (u/pprint-to-str aggregation))
+ (doseq [filter-clause filters]
+ (log/tracef "Adding filter clause for legacy Metric %d:\n%s" id (u/pprint-to-str filter-clause)))
+ (swap! new-filters concat filters)
+ aggregation))
+ new-filters @new-filters]
+ (cond-> stage'
+ (seq new-filters) (lib.filter/add-filters-to-stage new-filters))))
+
+(mu/defmethod resolve-legacy-macros-in-stage :segment :- ::lib.schema/stage
+ [_macro-type :- [:= :segment]
+ stage :- ::lib.schema/stage
+ id->legacy-segment :- ::id->legacy-macro]
+ (-> (mbql.u/replace stage
+ [:segment _opts (id :guard pos-int?)]
+ (let [legacy-segment (get id->legacy-segment id)
+ filter-clauses (legacy-macro-filters legacy-segment)]
+ (log/debugf "Expanding legacy Segment macro\n%s" (u/pprint-to-str &match))
+ (doseq [filter-clause filter-clauses]
+ (log/tracef "Adding filter clause for legacy Segment %d:\n%s" id (u/pprint-to-str filter-clause)))
+ ;; replace a single segment with a single filter, wrapping them in `:and` if needed... we will unwrap once
+ ;; we've expanded all of the :segment refs.
+ (if (> (count filter-clauses) 1)
+ (apply lib.filter/and filter-clauses)
+ (first filter-clauses))))
+ lib.filter/flatten-compound-filters-in-stage
+ lib.filter/remove-duplicate-filters-in-stage))
+
+(mu/defn ^:private resolve-legacy-macros :- ::lib.schema/query
+ [macro-type :- ::macro-type
+ query :- ::lib.schema/query
+ legacy-macro-ids :- [:maybe [:set {:min 1} pos-int?]]]
+ (log/debugf "Resolving legacy %s macros with IDs %s" macro-type legacy-macro-ids)
+ (let [id->legacy-macro (fetch-legacy-macros macro-type query legacy-macro-ids)]
+ (lib.walk/walk-stages
+ query
+ (fn [_query _path stage]
+ (resolve-legacy-macros-in-stage macro-type stage id->legacy-macro)))))
+
+(mu/defn ^:private expand-legacy-macros :- ::lib.schema/query
+ [macro-type :- ::macro-type
+ query :- ::lib.schema/query]
+ (if-let [legacy-macro-ids (not-empty (unresolved-legacy-macro-ids macro-type query))]
+ (resolve-legacy-macros macro-type query legacy-macro-ids)
query))
+(def ^:private max-recursion-depth
+ "Detect infinite recursion for macro expansion."
+ 50)
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | MIDDLEWARE |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(mu/defn ^:private expand-metrics-and-segments :- mbql.s/Query
- "Expand the macros (`segment`, `metric`) in a `query`."
- [query :- mbql.s/Query]
- (-> query
- expand-metrics
- expand-segments))
-
-(defn expand-macros
+(mu/defn expand-macros
"Middleware that looks for `:metric` and `:segment` macros in an unexpanded MBQL query and substitute the macros for
their contents."
- [{query-type :type, :as query}]
- (if-not (= query-type :query)
- query
- (expand-metrics-and-segments query)))
+ ([query :- ::lib.schema/query]
+ (expand-macros query 0))
+
+ ([query recursion-depth]
+ (when (> recursion-depth max-recursion-depth)
+ (throw (ex-info (tru "Metric/Segment expansion failed. Check mutually recursive segment definitions.")
+ {:type qp.error-type/invalid-query, :query query})))
+ (let [query' (->> query
+ (expand-legacy-macros :metric)
+ (expand-legacy-macros :segment))]
+ ;; if we expanded anything, we need to recursively try expanding again until nothing is left to expand, in case a
+ ;; legacy Metric or Segment references another legacy Metric or Segment.
+ (if-not (= query' query)
+ (recur query' (inc recursion-depth))
+ (do
+ (log/tracef "No more legacy Metrics/Segments to expand.")
+ query')))))
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index 49fc047c982..555548bbc82 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -21,6 +21,8 @@
[metabase.util.malli.schema :as ms]
[weavejester.dependency :as dep]))
+;;; TODO -- consider whether [[normalize-card-query]] should be moved into [[metabase.lib.card]], seems like it would
+;;; make sense but it would involve teasing out some QP-specific stuff to make it work.
(defn- fix-mongodb-first-stage
"MongoDB native queries consist of a collection and a pipelne (query).
@@ -38,9 +40,10 @@
:query x}))))]
(cons first-stage more)))
-(defn- normalize-card-query
+(mu/defn normalize-card-query :- ::lib.schema.metadata/card
"Convert Card's query (`:datasaet-query`) to pMBQL as needed; splice in stage metadata and some extra keys."
- [metadata-providerable {card-id :id, :as card}]
+ [metadata-providerable :- ::lib.schema.metadata/metadata-providerable
+ {card-id :id, :as card} :- ::lib.schema.metadata/card]
(let [persisted-info (:lib/persisted-info card)
persisted? (qp.persisted/can-substitute? card persisted-info)]
(when persisted?
@@ -92,7 +95,7 @@
(mu/defn ^:private resolve-source-cards-in-stage :- [:maybe ::lib.schema/stages]
[query :- ::lib.schema/query
stage :- ::lib.schema/stage
- dep-graph :- (ms/InstanceOfClass clojure.lang.Atom)]
+ dep-graph :- (ms/InstanceOfClass clojure.lang.Volatile)]
(when (and (= (:lib/type stage) :mbql.stage/mbql)
(:source-card stage))
;; make sure nested queries are enabled before resolving them.
@@ -103,10 +106,10 @@
;; dependency of the previously-resolved source card on the one we're about to resolve. We can check for circular
;; dependencies this way.
(when (:qp/stage-is-from-source-card stage)
- (u/prog1 (swap! dep-graph
- dep/depend
- (tru "Card {0}" (:qp/stage-is-from-source-card stage))
- (tru "Card {0}" (:source-card stage)))
+ (u/prog1 (vswap! dep-graph
+ dep/depend
+ (tru "Card {0}" (:qp/stage-is-from-source-card stage))
+ (tru "Card {0}" (:source-card stage)))
;; This will throw if there's a cycle
(dep/topo-sort <>)))
(let [card (card query (:source-card stage))
@@ -149,7 +152,7 @@
"If a stage has a `:source-card`, fetch the Card and prepend its underlying stages to the pipeline."
[query :- ::lib.schema/query]
(let [query (dissoc query :source-card-id :qp/source-card-id)] ; `:source-card-id` was the old key
- (resolve-source-cards* query 0 (atom (dep/graph)))))
+ (resolve-source-cards* query 0 (volatile! (dep/graph)))))
(defn add-dataset-info
"Post-processing middleware that adds `:model` and `:dataset` (for historic reasons) `true` or `false` to queries with
diff --git a/src/metabase/query_processor/middleware/fetch_source_query_legacy.clj b/src/metabase/query_processor/middleware/fetch_source_query_legacy.clj
index 396f01ac6d0..37f80c3c33f 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query_legacy.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query_legacy.clj
@@ -2,25 +2,20 @@
"LEGACY IMPLEMENTATION of [[metabase.query-processor.middleware.fetch-source-query]], will be removed soon."
(:require
[clojure.set :as set]
- [medley.core :as m]
[metabase.driver.ddl.interface :as ddl.i]
[metabase.driver.util :as driver.u]
[metabase.lib.convert :as lib.convert]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.schema.id :as lib.schema.id]
- [metabase.lib.util :as lib.util]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.mbql.schema :as mbql.s]
- [metabase.mbql.util :as mbql.u]
[metabase.public-settings :as public-settings]
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.util.persisted-cache :as qp.persisted]
[metabase.util :as u]
[metabase.util.i18n :refer [trs tru]]
[metabase.util.log :as log]
- [metabase.util.malli :as mu]
- [metabase.util.malli.schema :as ms]
- [weavejester.dependency :as dep]))
+ [metabase.util.malli :as mu]))
(set! *warn-on-reflection* true)
@@ -34,44 +29,6 @@
[:source-query/model? {:optional true} :boolean]
[:persisted-info/native {:optional true} :string]])
-(def ^:private MapWithResolvedSourceQuery
- [:and
- [:map
- [:database mbql.s/DatabaseID]
- [:source-metadata [:maybe [:sequential mbql.s/SourceQueryMetadata]]]
- [:source-query mbql.s/SourceQuery]
- [:source-card-id ms/PositiveInt]]
- [:fn
- {:error/message "`:source-table` should be removed"}
- (complement :source-table)]])
-
-(defn- query-has-unresolved-card-id-source-tables? [{inner-mbql-query :query}]
- (when inner-mbql-query
- (mbql.u/match-one inner-mbql-query
- (&match :guard (every-pred map? (comp string? :source-table))))))
-
-(defn- query-has-resolved-database-id? [{:keys [database]}]
- ((every-pred integer? pos?) database))
-
-(def ^:private FullyResolvedQuery
- "Schema for a MBQL query where all `card__id` `:source-tables` have been removes and appropriate `:source-query`s have
- been added instead, and where the top-level `:database` ID, if it was the 'source query placeholder`, is replaced by
- the actual database ID of the source query.
-
- This schema represents the way the query should look after this middleware finishes preprocessing it."
- [:and
- mbql.s/Query
- [:fn
- {:error/message "Query where all card__id :source-tables are fully resolved"}
- (complement query-has-unresolved-card-id-source-tables?)]
- [:fn
- {:error/message "Query where source-query virtual `:database` has been replaced with actual Database ID"}
- query-has-resolved-database-id?]])
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Resolving card__id -> source query |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
(defn- source-query
"Get the query to be run from the card"
[{dataset-query :dataset-query, card-id :id, :as card}]
@@ -129,135 +86,3 @@
:database database-id
:source-metadata (seq (map mbql.normalize/normalize-source-metadata result-metadata))}
(= card-type :model) (assoc :source-query/model? true)))))
-
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Logic for traversing the query |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(def ^:private ^{:arglists '([x])} map-with-card-id-source-table?
- "Is `x` a map with a \"card__id\" `:source-table`, i.e., something this middleware needs to resolve?"
- (every-pred
- map?
- (comp string? :source-table)
- (comp (partial re-matches mbql.s/source-table-card-id-regex) :source-table)))
-
-(mu/defn ^:private resolve-one :- MapWithResolvedSourceQuery
- [{:keys [source-table], :as m} :- [:map [:source-table mbql.s/source-table-card-id-regex]]]
- (let [card-id (-> source-table lib.util/legacy-string-table-id->card-id)
- source-query-and-metadata (-> card-id (card-id->source-query-and-metadata true))]
- (merge
- (dissoc m :source-table)
- ;; record the `card-id` we've resolved here. We'll include it in `:info` for permissions purposes later
- {:source-card-id card-id}
- source-query-and-metadata)))
-
-(defn- resolve-all*
- [m]
- (mbql.u/replace m
- map-with-card-id-source-table?
- ;; if this is a map that has a Card ID `:source-table`, resolve that (replacing it with the appropriate
- ;; `:source-query`, then recurse and resolve any nested-nested queries that need to be resolved too
- (let [resolved (if (public-settings/enable-nested-queries)
- (resolve-one &match)
- (throw (ex-info (trs "Nested queries are disabled")
- {:clause &match})))]
- ;; wrap the recursive call in a try-catch; if the recursive resolution fails, add context about the
- ;; resolution that were we in the process of
- (try
- (resolve-all* resolved)
- (catch Throwable e
- (throw (ex-info (tru "Error resolving source query")
- {:resolving &match, :resolved resolved}
- e)))))))
-
-(defn- check-for-circular-references
- "Check that there are no circular dependencies among source cards. This is equivalent to
- finding a topological sort of the dependency graph.
- https://en.wikipedia.org/wiki/Topological_sorting"
- ([m]
- (check-for-circular-references (dep/graph) m)
- m)
- ([g m]
- (transduce (comp (filter map-with-card-id-source-table?)
- (map (comp card-id->source-query-and-metadata
- lib.util/legacy-string-table-id->card-id
- :source-table)))
- (fn
- ([] g)
- ([g source-query]
- (-> g
- (dep/depend m source-query)
- ;; Recursive call will circuit break the moment there's a cycle, so no
- ;; danger of unbounded recursion.
- (check-for-circular-references source-query)))
- ([g]
- ;; This will throw if there's a cycle
- (dep/topo-sort g)
- g))
- (tree-seq coll? identity m))))
-
-(defn- copy-source-query-database-ids
- "If `m` has the saved questions virtual `:database` ID, (recursively) look for actual resolved Database IDs in the
- next level down and copy it to our level."
- [{:keys [database], :as m}]
- (if (and database (not= database lib.schema.id/saved-questions-virtual-database-id))
- m
- (let [{:keys [query source-query], :as m}
- (cond-> m
- (:query m) (update :query copy-source-query-database-ids)
- (:source-query m) (update :source-query copy-source-query-database-ids))
-
- db-id
- (some (fn [{:keys [database]}]
- (when (some-> database (not= lib.schema.id/saved-questions-virtual-database-id))
- database))
- [source-query query])]
- (cond-> m
- db-id (assoc :database db-id)))))
-
-(defn- remove-unneeded-database-ids
- "Remove `:database` from all levels besides the top level."
- [m]
- (mbql.u/replace-in m [:query]
- (&match :guard (every-pred map? :database (comp integer? :database)))
- (recur (dissoc &match :database))))
-
-(mu/defn ^:private extract-resolved-card-id :- [:map
- [:card-id [:maybe ms/PositiveInt]]
- [:query :map]]
- "If the ID of the Card we've resolved (`:source-card-id`) was added by a previous step, add it
- to `:query` `:info` (so it can be included in the QueryExecution log), then return a map with the resolved
- `:card-id` and updated `:query`."
- [query :- :map]
- (let [card-id (get-in query [:query :source-card-id])]
- {:query (cond-> query
- card-id (update-in [:info :card-id] #(or % card-id)))
- :card-id card-id}))
-
-(mu/defn ^:private resolve-all :- [:map
- [:card-id [:maybe ms/PositiveInt]]
- [:query :map]]
- "Recursively replace all Card ID source tables in `query` with resolved `:source-query` and `:source-metadata`. Since
- the `:database` is only useful for top-level source queries, we'll remove it from all other levels."
- [query :- :map]
- ;; if a `:source-card-id` is already in the query, remove it, so we don't pull user-supplied input up into `:info`
- ;; allowing someone to bypass permissions
- (-> (m/dissoc-in query [:query :source-card-id])
- check-for-circular-references
- resolve-all*
- copy-source-query-database-ids
- remove-unneeded-database-ids
- extract-resolved-card-id))
-
-(mu/defn resolve-card-id-source-tables* :- [:map
- [:card-id [:maybe ms/PositiveInt]]
- [:query FullyResolvedQuery]]
- "Resolve `card__n`-style `:source-tables` in `query`."
- [{inner-query :query, :as outer-query} :- mbql.s/Query]
- (if-not inner-query
- ;; for non-MBQL queries there's nothing to do since they have nested queries
- {:query outer-query, :card-id nil}
- ;; Otherwise attempt to expand any source queries as needed. Pull the `:database` key up into the top-level if it
- ;; exists
- (resolve-all outer-query)))
diff --git a/src/metabase/query_processor/middleware/normalize_query.clj b/src/metabase/query_processor/middleware/normalize_query.clj
index 334be17d44b..80fa63dfe20 100644
--- a/src/metabase/query_processor/middleware/normalize_query.clj
+++ b/src/metabase/query_processor/middleware/normalize_query.clj
@@ -55,7 +55,7 @@
(some-fn :lib/type :type)]]
"Preprocessing middleware. Normalize a query, meaning do things like convert keys and MBQL clause tags to kebab-case
keywords. Convert query to pMBQL if needed."
- [query :- :map]
+ [query :- [:map [:database ::lib.schema.id/database]]]
(try
(u/prog1 (normalize* query)
(log/tracef "Normalized query:\n%s\n=>\n%s" (u/pprint-to-str query) (u/pprint-to-str <>)))
diff --git a/src/metabase/query_processor/middleware/permissions.clj b/src/metabase/query_processor/middleware/permissions.clj
index 69b7bcf5dfe..bf50644f8b2 100644
--- a/src/metabase/query_processor/middleware/permissions.clj
+++ b/src/metabase/query_processor/middleware/permissions.clj
@@ -3,6 +3,7 @@
(:require
[metabase.api.common
:refer [*current-user-id* *current-user-permissions-set*]]
+ [metabase.lib.core :as lib]
[metabase.lib.metadata.protocols :as lib.metadata.protocols]
[metabase.lib.schema.id :as lib.schema.id]
[metabase.models.data-permissions :as data-perms]
@@ -12,7 +13,6 @@
[metabase.public-settings.premium-features :refer [defenterprise]]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.store :as qp.store]
- [metabase.query-processor.util.tag-referenced-cards :as qp.u.tag-referenced-cards]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
@@ -103,7 +103,8 @@
(do
(query-perms/check-data-perms outer-query required-perms :throw-exceptions? true)
;; check perms for any Cards referenced by this query (if it is a native query)
- (doseq [{query :dataset-query} (qp.u.tag-referenced-cards/tags-referenced-cards outer-query)]
+ (doseq [{query :dataset-query} (lib/template-tags-referenced-cards
+ (lib/query (qp.store/metadata-provider) outer-query))]
(check-query-permissions* query)))))))
(defn check-query-permissions
diff --git a/src/metabase/query_processor/middleware/resolve_fields.clj b/src/metabase/query_processor/middleware/resolve_fields.clj
index a39bab76447..0331f528f26 100644
--- a/src/metabase/query_processor/middleware/resolve_fields.clj
+++ b/src/metabase/query_processor/middleware/resolve_fields.clj
@@ -2,32 +2,59 @@
"Middleware that resolves the Fields referenced by a query."
(:require
[metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.schema :as lib.schema]
+ [metabase.lib.schema.id :as lib.schema.id]
+ [metabase.mbql.schema :as mbql.s]
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.store :as qp.store]
[metabase.util :as u]
- [metabase.util.i18n :refer [tru]]))
+ [metabase.util.i18n :refer [tru]]
+ [metabase.util.malli :as mu]))
(defn- resolve-fields-with-ids!
- [field-ids]
- (qp.store/bulk-metadata :metadata/column field-ids)
+ [metadata-providerable field-ids]
+ (lib.metadata/bulk-metadata metadata-providerable :metadata/column field-ids)
(when-let [parent-ids (not-empty
- (into []
+ (into #{}
(comp (map (fn [field-id]
- (:parent-id (lib.metadata/field (qp.store/metadata-provider) field-id))))
+ (:parent-id (lib.metadata/field metadata-providerable field-id))))
(filter some?))
field-ids))]
- (recur parent-ids)))
+ (recur metadata-providerable parent-ids)))
+
+(defmulti ^:private field-ids
+ {:arglists '([query])}
+ (fn [query]
+ (if (:lib/type query) ::pmbql ::legacy)))
+
+(mu/defmethod field-ids ::pmbql :- [:set ::lib.schema.id/field]
+ [query :- ::lib.schema/query]
+ (into #{}
+ (mbql.u/match (:stages query)
+ [:field _opts (id :guard pos-int?)]
+ id
+
+ ;; stage metadata
+ {:lib/type :metadata/column, :id (id :guard pos-int?)}
+ id)))
+
+(mu/defmethod field-ids ::legacy :- [:set ::lib.schema.id/field]
+ [query :- ::mbql.s/Query]
+ (into (set (mbql.u/match (:query query) [:field (id :guard integer?) _] id))
+ (comp cat (keep :id))
+ (mbql.u/match (:query query) {:source-metadata source-metadata} source-metadata)))
(defn resolve-fields
"Resolve all field referenced in the `query`, and store them in the QP Store."
[query]
- (let [ids (into (set (mbql.u/match (:query query) [:field (id :guard integer?) _] id))
- (comp cat (keep :id))
- (mbql.u/match (:query query) {:source-metadata source-metadata} source-metadata))]
+ (let [ids (field-ids query)]
(try
(u/prog1 query
- (resolve-fields-with-ids! ids))
+ (let [metadata-providerable (if (:lib/type query)
+ query
+ (qp.store/metadata-provider))]
+ (resolve-fields-with-ids! metadata-providerable ids)))
(catch Throwable e
(throw (ex-info (tru "Error resolving Fields in query: {0}" (ex-message e))
{:field-ids ids
diff --git a/src/metabase/query_processor/middleware/resolve_referenced.clj b/src/metabase/query_processor/middleware/resolve_referenced.clj
index a35ec3c734d..9fc4334c176 100644
--- a/src/metabase/query_processor/middleware/resolve_referenced.clj
+++ b/src/metabase/query_processor/middleware/resolve_referenced.clj
@@ -1,76 +1,82 @@
(ns metabase.query-processor.middleware.resolve-referenced
(:require
- [metabase.lib.metadata.protocols :as lib.metadata.protocols]
+ [metabase.lib.core :as lib]
+ [metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.schema :as lib.schema]
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.id :as lib.schema.id]
- #_{:clj-kondo/ignore [:deprecated-namespace]}
- [metabase.query-processor.middleware.fetch-source-query-legacy
- :as fetch-source-query-legacy]
- [metabase.query-processor.middleware.resolve-fields
- :as qp.resolve-fields]
- [metabase.query-processor.middleware.resolve-source-table
- :as qp.resolve-source-table]
- [metabase.query-processor.store :as qp.store]
- [metabase.query-processor.util.tag-referenced-cards
- :as qp.u.tag-referenced-cards]
+ [metabase.lib.schema.metadata :as lib.schema.metadata]
+ [metabase.query-processor.error-type :as qp.error-type]
+ [metabase.query-processor.middleware.fetch-source-query :as qp.fetch-source-query]
+ [metabase.query-processor.middleware.resolve-fields :as qp.resolve-fields]
+ [metabase.query-processor.middleware.resolve-source-table :as qp.resolve-source-table]
[metabase.util.i18n :refer [tru]]
[metabase.util.malli :as mu]
[weavejester.dependency :as dep])
(:import
(clojure.lang ExceptionInfo)))
-(defn- check-query-database-id=
- [query database-id]
- (when-not (= (:database query) database-id)
- (throw (ex-info (tru "Referenced query is from a different database")
- {:referenced-query query
- :expected-database-id database-id}))))
-
-(mu/defn ^:private resolve-referenced-card-resources* :- :map
- [query]
- (doseq [referenced-card (qp.u.tag-referenced-cards/tags-referenced-cards query)
- :let [referenced-query (:dataset-query referenced-card)
- resolved-query (fetch-source-query-legacy/resolve-card-id-source-tables* referenced-query)]]
- (check-query-database-id= referenced-query (:database query))
+(mu/defn ^:private resolve-referenced-card-resources*
+ "Done for side effects; warm the MetadataProvider."
+ [query :- ::lib.schema/query]
+ (doseq [referenced-card (lib/template-tags-referenced-cards query)
+ :let [referenced-query (->> referenced-card
+ (qp.fetch-source-query/normalize-card-query query)
+ :dataset-query)
+ resolved-query (qp.fetch-source-query/resolve-source-cards referenced-query)]]
(qp.resolve-source-table/resolve-source-tables resolved-query)
- (qp.resolve-fields/resolve-fields resolved-query))
- query)
+ (qp.resolve-fields/resolve-fields resolved-query)))
-(defn- card-subquery-graph
- [graph card-id]
- (let [card-query (:dataset-query (lib.metadata.protocols/card (qp.store/metadata-provider) card-id))]
+(mu/defn ^:private card-subquery-graph
+ [metadata-providerable :- ::lib.schema.metadata/metadata-providerable
+ graph :- :map
+ card-id :- ::lib.schema.id/card]
+ (let [card-query (->> (or (lib.metadata/card metadata-providerable card-id)
+ (throw (ex-info (tru "Card {0} does not exist, or is from a different Database." (pr-str card-id))
+ {:type qp.error-type/invalid-query, :card-id card-id})))
+ (qp.fetch-source-query/normalize-card-query metadata-providerable)
+ :dataset-query)]
(reduce
(fn [g sub-card-id]
- (card-subquery-graph (dep/depend g card-id sub-card-id)
+ (card-subquery-graph metadata-providerable
+ (dep/depend g card-id sub-card-id)
sub-card-id))
graph
- (qp.u.tag-referenced-cards/query->tag-card-ids card-query))))
+ (lib/template-tag-card-ids card-query))))
(mu/defn ^:private circular-ref-error :- ::lib.schema.common/non-blank-string
- [from-card :- ::lib.schema.id/card
- to-card :- ::lib.schema.id/card]
- (let [cards (into {}
- (map (juxt :id :name))
- (qp.store/bulk-metadata :metadata/card #{from-card to-card}))
- from-name (get cards from-card)
- to-name (get cards to-card)]
+ [metadata-providerable :- ::lib.schema.metadata/metadata-providerable
+ from-card :- ::lib.schema.id/card
+ to-card :- ::lib.schema.id/card]
+ (let [cards (into {}
+ (map (juxt :id :name))
+ (lib.metadata/bulk-metadata metadata-providerable :metadata/card #{from-card to-card}))
+ from-name (or (get cards from-card)
+ (throw (ex-info (tru "Referenced query is from a different database")
+ {:type qp.error-type/invalid-query, :card-id from-card})))
+ to-name (or (get cards to-card)
+ (throw (ex-info (tru "Referenced query is from a different database")
+ {:type qp.error-type/invalid-query, :card-id to-card})))]
(str
(tru "This query has circular referencing sub-queries. ")
(tru "These questions seem to be part of the problem: \"{0}\" and \"{1}\"." from-name to-name))))
-(defn- check-for-circular-references
- [query]
+(mu/defn ^:private check-for-circular-references
+ "Done for side effects; [[card-subquery-graph]] will throw if there are circular references."
+ [query :- ::lib.schema/query]
(try
- ;; `card-subquery-graph` will throw if there are circular references
- (reduce card-subquery-graph (dep/graph) (qp.u.tag-referenced-cards/query->tag-card-ids query))
- (catch ExceptionInfo e
- (let [{:keys [reason node dependency]} (ex-data e)]
- (if (= reason :weavejester.dependency/circular-dependency)
- (throw (ex-info (circular-ref-error node dependency) {:original-exception e}))
- (throw e)))))
- query)
+ (reduce (partial card-subquery-graph query)
+ (dep/graph)
+ (lib/template-tag-card-ids query))
+ (catch ExceptionInfo e
+ (let [{:keys [reason node dependency]} (ex-data e)]
+ (if (= reason :weavejester.dependency/circular-dependency)
+ (throw (ex-info (circular-ref-error query node dependency) {:original-exception e}))
+ (throw e))))))
-(defn resolve-referenced-card-resources
+(mu/defn resolve-referenced-card-resources :- ::lib.schema/query
"Resolves tables and fields referenced in card query template tags."
- [query]
- (-> query check-for-circular-references resolve-referenced-card-resources*))
+ [query :- ::lib.schema/query]
+ (check-for-circular-references query)
+ (resolve-referenced-card-resources* query)
+ query)
diff --git a/src/metabase/query_processor/middleware/resolve_source_table.clj b/src/metabase/query_processor/middleware/resolve_source_table.clj
index 8e68dc0a092..8af67f5bfe3 100644
--- a/src/metabase/query_processor/middleware/resolve_source_table.clj
+++ b/src/metabase/query_processor/middleware/resolve_source_table.clj
@@ -1,41 +1,26 @@
(ns metabase.query-processor.middleware.resolve-source-table
"Fetches Tables corresponding to any `:source-table` IDs anywhere in the query."
(:require
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.schema.id :as lib.schema.id]
- [metabase.mbql.util :as mbql.u]
- [metabase.query-processor.store :as qp.store]
- [metabase.util.i18n :refer [tru]]
+ [metabase.lib.walk :as lib.walk]
[metabase.util.malli :as mu]))
-(defn- check-all-source-table-ids-are-valid
- "Sanity check: Any non-positive-integer value of `:source-table` should have been resolved by now. The
- `resolve-card-id-source-tables` middleware should have already taken care of it."
- [query]
- (mbql.u/match-one query
- (m :guard (every-pred map? :source-table #(string? (:source-table %))))
- (throw
- (ex-info
- (tru "Invalid :source-table ''{0}'': should be resolved to a Table ID by now." (:source-table m))
- {:form m}))))
-
-(mu/defn ^:private query->source-table-ids :- [:maybe [:set {:min 1} ::lib.schema.id/table]]
+(mu/defn ^:private query->source-table-ids :- [:maybe [:set {:min 1} ::lib.schema.id/table]]
"Fetch a set of all `:source-table` IDs anywhere in `query`."
[query]
- (some->
- (mbql.u/match query
- (m :guard (every-pred map? :source-table))
- ;; Recursively look in the rest of `m` for any other source tables
- (cons
- (:source-table m)
- (filter some? (recur (dissoc m :source-table)))))
- flatten
- set))
+ (let [source-table-ids (volatile! #{})]
+ (lib.walk/walk-stages
+ query
+ (fn [_query _path {:keys [source-table], :as _stage}]
+ (when source-table
+ (vswap! source-table-ids conj source-table))))
+ (not-empty @source-table-ids)))
(defn resolve-source-tables
"Middleware that will take any `:source-table`s (integer IDs) anywhere in the query and fetch and save the
corresponding Table in the Query Processor Store."
[query]
- (check-all-source-table-ids-are-valid query)
;; this is done for side effects
- (qp.store/bulk-metadata :metadata/table (query->source-table-ids query))
+ (lib.metadata/bulk-metadata-or-throw query :metadata/table (query->source-table-ids query))
query)
diff --git a/src/metabase/query_processor/preprocess.clj b/src/metabase/query_processor/preprocess.clj
index 6c2d2b585d7..8353e193b76 100644
--- a/src/metabase/query_processor/preprocess.clj
+++ b/src/metabase/query_processor/preprocess.clj
@@ -1,6 +1,9 @@
(ns metabase.query-processor.preprocess
(:require
+ [metabase.lib.convert :as lib.convert]
+ [metabase.lib.query :as lib.query]
[metabase.lib.schema.id :as lib.schema.id]
+ [metabase.mbql.schema :as mbql.s]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.middleware.add-default-temporal-unit :as qp.add-default-temporal-unit]
[metabase.query-processor.middleware.add-dimension-projections :as qp.add-dimension-projections]
@@ -13,7 +16,6 @@
[metabase.query-processor.middleware.binning :as binning]
[metabase.query-processor.middleware.check-features :as check-features]
[metabase.query-processor.middleware.constraints :as qp.constraints]
- [metabase.query-processor.middleware.convert-to-legacy :as qp.convert-to-legacy]
[metabase.query-processor.middleware.cumulative-aggregations :as qp.cumulative-aggregations]
[metabase.query-processor.middleware.desugar :as desugar]
[metabase.query-processor.middleware.enterprise :as qp.middleware.enterprise]
@@ -39,56 +41,75 @@
[metabase.query-processor.middleware.validate-temporal-bucketing :as validate-temporal-bucketing]
[metabase.query-processor.middleware.wrap-value-literals :as qp.wrap-value-literals]
[metabase.query-processor.setup :as qp.setup]
+ [metabase.query-processor.store :as qp.store]
[metabase.util :as u]
[metabase.util.i18n :as i18n]
[metabase.util.log :as log]
[metabase.util.malli :as mu]))
+;;; the following helper functions are temporary, to aid in the transition from a legacy MBQL QP to a pMBQL QP. Each
+;;; individual middleware function is wrapped in either [[ensure-legacy]] or [[ensure-pmbql]], and will then see the
+;;; flavor of MBQL it is written for.
+
+(mu/defn ^:private ->legacy :- mbql.s/Query
+ [query]
+ (lib.convert/->legacy-MBQL query))
+
+(defn- ^:deprecated ensure-legacy [middleware-fn]
+ (fn [query]
+ (let [query (cond-> query
+ (:lib/type query) ->legacy)]
+ (middleware-fn query))))
+
+(defn- ensure-pmbql [middleware-fn]
+ (fn [query]
+ (let [query (cond->> query
+ (not (:lib/type query)) (lib.query/query (qp.store/metadata-provider)))]
+ (middleware-fn query))))
+
(def ^:private middleware
"Pre-processing middleware. Has the form
(f query) -> query"
;; ↓↓↓ PRE-PROCESSING ↓↓↓ happens from TOP TO BOTTOM
+ #_{:clj-kondo/ignore [:deprecated-var]}
[#'normalize/normalize-preprocessing-middleware
- #'qp.perms/remove-permissions-key
- #'qp.constraints/maybe-add-default-userland-constraints
- #'validate/validate-query
- #'fetch-source-query/resolve-source-cards
- ;; ↑↑↑ ALL MIDDLEWARE ABOVE THIS POINT WILL SEE MLV2 PMBQL QUERIES ↑↑↑
- #'qp.convert-to-legacy/convert-to-legacy
- ;; ↓↓↓ ALL MIDDLEWARE BELOW THIS POINT WILL SEE LEGACY MBQL QUERIES ↓↓↓
- #'expand-macros/expand-macros
- #'qp.resolve-referenced/resolve-referenced-card-resources
- #'parameters/substitute-parameters
- #'qp.resolve-source-table/resolve-source-tables
- #'qp.auto-bucket-datetimes/auto-bucket-datetimes
- #'reconcile-bucketing/reconcile-breakout-and-order-by-bucketing
- #'qp.add-source-metadata/add-source-metadata-for-source-queries
- #'upgrade-field-literals/upgrade-field-literals
- #'qp.middleware.enterprise/apply-sandboxing
- #'qp.persistence/substitute-persisted-query
- #'qp.add-implicit-clauses/add-implicit-clauses
- #'qp.add-dimension-projections/add-remapped-columns
- #'qp.resolve-fields/resolve-fields
- #'binning/update-binning-strategy
- #'desugar/desugar
- #'qp.add-default-temporal-unit/add-default-temporal-unit
- #'qp.add-implicit-joins/add-implicit-joins
- #'resolve-joins/resolve-joins
- #'resolve-joined-fields/resolve-joined-fields
- #'fix-bad-refs/fix-bad-references
- #'escape-join-aliases/escape-join-aliases
+ (ensure-pmbql #'qp.perms/remove-permissions-key)
+ (ensure-pmbql #'qp.constraints/maybe-add-default-userland-constraints)
+ (ensure-pmbql #'validate/validate-query)
+ (ensure-pmbql #'fetch-source-query/resolve-source-cards)
+ (ensure-pmbql #'expand-macros/expand-macros)
+ (ensure-pmbql #'qp.resolve-referenced/resolve-referenced-card-resources)
+ (ensure-legacy #'parameters/substitute-parameters)
+ (ensure-pmbql #'qp.resolve-source-table/resolve-source-tables)
+ (ensure-legacy #'qp.auto-bucket-datetimes/auto-bucket-datetimes)
+ (ensure-legacy #'reconcile-bucketing/reconcile-breakout-and-order-by-bucketing)
+ (ensure-legacy #'qp.add-source-metadata/add-source-metadata-for-source-queries)
+ (ensure-legacy #'upgrade-field-literals/upgrade-field-literals)
+ (ensure-legacy #'qp.middleware.enterprise/apply-sandboxing)
+ (ensure-legacy #'qp.persistence/substitute-persisted-query)
+ (ensure-legacy #'qp.add-implicit-clauses/add-implicit-clauses)
+ (ensure-legacy #'qp.add-dimension-projections/add-remapped-columns)
+ (ensure-legacy #'qp.resolve-fields/resolve-fields)
+ (ensure-legacy #'binning/update-binning-strategy)
+ (ensure-legacy #'desugar/desugar)
+ (ensure-legacy #'qp.add-default-temporal-unit/add-default-temporal-unit)
+ (ensure-legacy #'qp.add-implicit-joins/add-implicit-joins)
+ (ensure-legacy #'resolve-joins/resolve-joins)
+ (ensure-legacy #'resolve-joined-fields/resolve-joined-fields)
+ (ensure-legacy #'fix-bad-refs/fix-bad-references)
+ (ensure-legacy #'escape-join-aliases/escape-join-aliases)
;; yes, this is called a second time, because we need to handle any joins that got added
- #'qp.middleware.enterprise/apply-sandboxing
- #'qp.cumulative-aggregations/rewrite-cumulative-aggregations
- #'qp.pre-alias-aggregations/pre-alias-aggregations
- #'qp.wrap-value-literals/wrap-value-literals
- #'auto-parse-filter-values/auto-parse-filter-values
- #'validate-temporal-bucketing/validate-temporal-bucketing
- #'optimize-temporal-filters/optimize-temporal-filters
- #'limit/add-default-limit
- #'qp.middleware.enterprise/apply-download-limit
- #'check-features/check-features])
+ (ensure-legacy #'qp.middleware.enterprise/apply-sandboxing)
+ (ensure-legacy #'qp.cumulative-aggregations/rewrite-cumulative-aggregations)
+ (ensure-legacy #'qp.pre-alias-aggregations/pre-alias-aggregations)
+ (ensure-legacy #'qp.wrap-value-literals/wrap-value-literals)
+ (ensure-legacy #'auto-parse-filter-values/auto-parse-filter-values)
+ (ensure-legacy #'validate-temporal-bucketing/validate-temporal-bucketing)
+ (ensure-legacy #'optimize-temporal-filters/optimize-temporal-filters)
+ (ensure-legacy #'limit/add-default-limit)
+ (ensure-legacy #'qp.middleware.enterprise/apply-download-limit)
+ (ensure-legacy #'check-features/check-features)])
(mu/defn preprocess :- [:map
[:database ::lib.schema.id/database]]
@@ -103,7 +124,6 @@
preprocessed)
([query middleware-fn]
(try
- (assert (ifn? middleware-fn))
;; make sure the middleware returns a valid query... this should be dev-facing only so no need to i18n
(u/prog1 (middleware-fn query)
(when-not (map? <>)
diff --git a/src/metabase/query_processor/store.clj b/src/metabase/query_processor/store.clj
index a540a1f15de..9e91f41d766 100644
--- a/src/metabase/query_processor/store.clj
+++ b/src/metabase/query_processor/store.clj
@@ -16,17 +16,13 @@
[medley.core :as m]
[metabase.lib.convert :as lib.convert]
[metabase.lib.metadata :as lib.metadata]
- [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.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.id :as lib.schema.id]
[metabase.lib.schema.metadata :as lib.schema.metadata]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
- [metabase.util.malli :as mu]
- [metabase.util.malli.schema :as ms]))
+ [metabase.util.malli :as mu]))
(set! *warn-on-reflection* true)
@@ -180,79 +176,22 @@
[database-id-or-metadata-provider & body]
`(do-with-metadata-provider ~database-id-or-metadata-provider (^:once fn* [] ~@body)))
-(defn- missing-bulk-metadata-error [metadata-type id]
- (ex-info (tru "Failed to fetch {0} {1}" (pr-str metadata-type) (pr-str id))
- {:status-code 400
- :type qp.error-type/invalid-query
- :metadata-provider (metadata-provider)
- :metadata-type metadata-type
- :id id}))
-
+;;; TODO -- mark this deprecated in favor of the version in `lib.metadata`
(mu/defn bulk-metadata :- [:maybe [:sequential [:map
[:lib/type :keyword]
- [:id ::lib.schema.common/positive-int]]]]
- "Fetch multiple objects in bulk. If our metadata provider is a bulk provider (e.g., the application database metadata
- provider), does a single fetch with [[lib.metadata.protocols/bulk-metadata]] if not (i.e., if this is a mock
- provider), fetches them with repeated calls to the appropriate single-object method,
- e.g. [[lib.metadata.protocols/field]].
-
- The order of the returned objects will match the order of `ids`, and the response is guaranteed to contain every
- object referred to by `ids`. Throws an exception if any objects could not be fetched.
-
- This can also be called for side-effects to warm the cache."
+ [:id pos-int?]]]]
+ "DEPRECATED -- prefer [[metabase.lib.metadata/bulk-metadata-or-throw]] instead."
[metadata-type :- [:enum :metadata/card :metadata/column :metadata/metric :metadata/segment :metadata/table]
ids :- [:maybe
[:or
- [:set ::lib.schema.common/positive-int]
- [:sequential ::lib.schema.common/positive-int]]]]
- (when-let [ids-set (not-empty (set ids))]
- (let [provider (metadata-provider)
- objects (vec (if (satisfies? lib.metadata.protocols/BulkMetadataProvider provider)
- (filter some? (lib.metadata.protocols/bulk-metadata provider metadata-type ids-set))
- (lib.metadata.composed-provider/fetch-bulk-metadata-with-non-bulk-provider
- provider
- metadata-type
- ids-set)))
- id->object (m/index-by :id objects)]
- (mapv (fn [id]
- (or (get id->object id)
- (throw (missing-bulk-metadata-error metadata-type id))))
- ids))))
+ [:set pos-int?]
+ [:sequential pos-int?]]]]
+ (lib.metadata/bulk-metadata-or-throw (metadata-provider) metadata-type ids))
;;;;
;;;; DEPRECATED STUFF
;;;;
-(def ^:private ^{:deprecated "0.48.0"} LegacyDatabaseMetadata
- [:map
- [:id ::lib.schema.id/database]
- [:engine :keyword]
- [:name ms/NonBlankString]
- [:details :map]
- [:settings [:maybe :map]]])
-
-(def ^:private ^{:deprecated "0.48.0"} LegacyTableMetadata
- [:map
- [:schema [:maybe :string]]
- [:name ms/NonBlankString]])
-
-(def ^:private ^{:deprecated "0.48.0"} LegacyFieldMetadata
- [:map
- [:name ms/NonBlankString]
- [:table_id ::lib.schema.id/table]
- [:display_name ms/NonBlankString]
- [:description [:maybe :string]]
- [:database_type ms/NonBlankString]
- [:base_type ms/FieldType]
- [:semantic_type [:maybe ms/FieldSemanticOrRelationType]]
- [:fingerprint [:maybe :map]]
- [:parent_id [:maybe ::lib.schema.id/field]]
- [:nfc_path [:maybe [:sequential ms/NonBlankString]]]
- ;; there's a tension as we sometimes store fields from the db, and sometimes store computed fields. ideally we
- ;; would make everything just use base_type.
- [:effective_type {:optional true} [:maybe ms/FieldType]]
- [:coercion_strategy {:optional true} [:maybe ms/CoercionStrategy]]])
-
(defn ->legacy-metadata
"For compatibility: convert MLv2-style metadata as returned by [[metabase.lib.metadata.protocols]]
or [[metabase.lib.metadata]] functions
@@ -271,43 +210,3 @@
(update-keys u/->snake_case_en)
(vary-meta assoc :type model)
(m/update-existing :field_ref lib.convert/->legacy-MBQL))))
-
-#_{:clj-kondo/ignore [:deprecated-var]}
-(mu/defn database :- LegacyDatabaseMetadata
- "Fetch the Database referenced by the current query from the QP Store. Throws an Exception if valid item is not
- returned.
-
- Deprecated in favor of [[metabase.lib.metadata/database]] + [[metadata-provider]]."
- {:deprecated "0.48.0"}
- []
- (->legacy-metadata (lib.metadata/database (metadata-provider))))
-
-#_{:clj-kondo/ignore [:deprecated-var]}
-(mu/defn ^:deprecated table :- LegacyTableMetadata
- "Fetch Table with `table-id` from the QP Store. Throws an Exception if valid item is not returned.
-
- Deprecated in favor of [[metabase.lib.metadata/table]] + [[metadata-provider]]."
- {:deprecated "0.48.0"}
- [table-id :- ::lib.schema.id/table]
- (-> (or (lib.metadata.protocols/table (metadata-provider) table-id)
- (throw (ex-info (tru "Failed to fetch Table {0}: Table does not exist, or belongs to a different Database."
- (pr-str table-id))
- {:status-code 404
- :type qp.error-type/invalid-query
- :table-id table-id})))
- ->legacy-metadata))
-
-#_{:clj-kondo/ignore [:deprecated-var]}
-(mu/defn ^:deprecated field :- LegacyFieldMetadata
- "Fetch Field with `field-id` from the QP Store. Throws an Exception if valid item is not returned.
-
- Deprecated in favor of [[metabase.lib.metadata/field]] + [[metadata-provider]]."
- {:deprecated "0.48.0"}
- [field-id :- ::lib.schema.id/field]
- (-> (or (lib.metadata.protocols/field (metadata-provider) field-id)
- (throw (ex-info (tru "Failed to fetch Field {0}: Field does not exist, or belongs to a different Database."
- (pr-str field-id))
- {:status-code 404
- :type qp.error-type/invalid-query
- :field-id field-id})))
- ->legacy-metadata))
diff --git a/src/metabase/query_processor/util/tag_referenced_cards.clj b/src/metabase/query_processor/util/tag_referenced_cards.clj
deleted file mode 100644
index 5ef131498bc..00000000000
--- a/src/metabase/query_processor/util/tag_referenced_cards.clj
+++ /dev/null
@@ -1,27 +0,0 @@
-(ns metabase.query-processor.util.tag-referenced-cards
- (:require
- [metabase.lib.metadata.protocols :as lib.metadata.protocols]
- [metabase.lib.schema.metadata :as lib.schema.metadata]
- [metabase.query-processor.store :as qp.store]
- [metabase.util.i18n :refer [tru]]
- [metabase.util.malli :as mu]))
-
-(defn- query->template-tags
- [query]
- (vals (get-in query [:native :template-tags])))
-
-(defn query->tag-card-ids
- "Returns the card IDs from the template tags of the native query of `query`."
- [query]
- (keep :card-id (query->template-tags query)))
-
-(mu/defn tags-referenced-cards :- [:maybe [:sequential ::lib.schema.metadata/card]]
- "Returns Card instances referenced by the given native `query`."
- [query]
- (mapv
- (fn [card-id]
- (if-let [card (lib.metadata.protocols/card (qp.store/metadata-provider) card-id)]
- card
- (throw (ex-info (tru "Referenced question #{0} could not be found" (str card-id))
- {:card-id card-id}))))
- (query->tag-card-ids query)))
diff --git a/src/metabase/shared/util/i18n.clj b/src/metabase/shared/util/i18n.clj
index 1f07d0f93c9..16b77d5d317 100644
--- a/src/metabase/shared/util/i18n.clj
+++ b/src/metabase/shared/util/i18n.clj
@@ -8,6 +8,7 @@
Placeholders should use `gettext` format e.g. `{0}`, `{1}`, and so forth.
(tru \"Number of cans: {0}\" 2)"
+ {:style/indent [:form]}
[format-string & args]
(macros/case
:clj
@@ -24,6 +25,7 @@
NOTE: When called from ClojureScript, this function behaves identically to `tru`. The originating JS callsite must
temporarily override the locale used by ttag using the `withInstanceLocalization` wrapper function."
+ {:style/indent [:form]}
[format-string & args]
(macros/case
:clj
@@ -38,6 +40,7 @@
"i18n a string with both singular and plural forms, using the current user's locale. The appropriate plural form will
be returned based on the value of `n`. `n` can be interpolated into the format strings using the `{0}`
syntax. (Other placeholders are not supported)."
+ {:style/indent [:form]}
[format-string format-string-pl n]
(macros/case
:clj
@@ -50,6 +53,7 @@
"i18n a string with both singular and plural forms, using the site's locale. The appropriate plural form will be
returned based on the value of `n`. `n` can be interpolated into the format strings using the `{0}` syntax. (Other
placeholders are not supported)."
+ {:style/indent [:form]}
[format-string format-string-pl n]
(macros/case
:clj
diff --git a/test/metabase/lib/metadata_test.cljc b/test/metabase/lib/metadata_test.cljc
index a897fa950e6..8d2cad964f2 100644
--- a/test/metabase/lib/metadata_test.cljc
+++ b/test/metabase/lib/metadata_test.cljc
@@ -63,3 +63,13 @@
:cljs
;; `Integer/MAX_VALUE`, but I don't know what the Cljs way to do this
(is (nil? (lib.metadata/table-or-card lib.tu/metadata-provider-with-card 2147483647)))))
+
+(deftest ^:parallel bulk-metadata-preserve-order-test
+ (testing "bulk-metadata should return things in the same order as the IDs passed in"
+ (are [ids expected] (= expected
+ (map :name (lib.metadata/bulk-metadata meta/metadata-provider :metadata/table (map meta/id ids))))
+ [:venues :orders :people]
+ ["VENUES" "ORDERS" "PEOPLE"]
+
+ [:people :orders :venues]
+ ["PEOPLE" "ORDERS" "VENUES"])))
diff --git a/test/metabase/lib/native_test.cljc b/test/metabase/lib/native_test.cljc
index c35a854ea4c..938d8a82e14 100644
--- a/test/metabase/lib/native_test.cljc
+++ b/test/metabase/lib/native_test.cljc
@@ -313,3 +313,35 @@
(deftest ^:parallel engine-test
(is (= :h2 (lib/engine lib.tu/native-query))))
+
+(deftest ^:parallel template-tag-card-ids-test
+ (let [query (lib/query lib.tu/metadata-provider-with-mock-cards
+ {:database (meta/id)
+ :type :native
+ :native {:query {}
+ :template-tags {"tag-name-not-important1" {:type :card
+ :display-name "X"
+ :card-id 1}
+ "tag-name-not-important2" {:type :card
+ :display-name "Y"
+ :card-id 2}}}})]
+ (is (= #{1 2}
+ (lib/template-tag-card-ids query)))))
+
+(deftest ^:parallel template-tags-referenced-cards-test
+ (testing "returns Card instances from raw query"
+ (let [query (lib/query lib.tu/metadata-provider-with-mock-cards
+ {:database (meta/id)
+ :type :native
+ :native {:query {}
+ :template-tags {"tag-name-not-important1" {:type :card
+ :display-name "X"
+ :card-id 1}
+ "tag-name-not-important2" {:type :card
+ :display-name "Y"
+ :card-id 2}}}})]
+ (is (=? [{:id 1
+ :dataset-query {}}
+ {:id 2
+ :dataset-query {}}]
+ (lib/template-tags-referenced-cards query))))))
diff --git a/test/metabase/query_processor/middleware/expand_macros_test.clj b/test/metabase/query_processor/middleware/expand_macros_test.clj
index e804de1e38a..9a15ff44f01 100644
--- a/test/metabase/query_processor/middleware/expand_macros_test.clj
+++ b/test/metabase/query_processor/middleware/expand_macros_test.clj
@@ -1,8 +1,10 @@
(ns metabase.query-processor.middleware.expand-macros-test
(:require
[clojure.test :refer :all]
+ [metabase.lib.convert :as lib.convert]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.jvm :as lib.metadata.jvm]
+ [metabase.lib.query :as lib.query]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
[metabase.lib.test-util.macros :as lib.tu.macros]
@@ -11,15 +13,30 @@
[metabase.test :as mt]))
(defn- mbql-query [inner-query]
- {:database 1, :type :query, :query (merge {:source-table 1}
- inner-query)})
+ {:database (meta/id)
+ :type :query
+ :query (merge {:source-table 1}
+ inner-query)})
+
+(defn- expand-macros
+ "If input is a legacy query, convert to pMBQL, call [[expand-macros/expand-macros]], then convert back to legacy. This
+ way we don't need to update all the tests below right away."
+ [query]
+ (if (:type query) ; legacy query
+ (let [metadata-provider (if (qp.store/initialized?)
+ (qp.store/metadata-provider)
+ meta/metadata-provider)]
+ (-> (lib.query/query metadata-provider query)
+ (#'expand-macros/expand-macros)
+ lib.convert/->legacy-MBQL))
+ (#'expand-macros/expand-macros query)))
(deftest ^:parallel basic-expansion-test
(testing "no Segment or Metric should yield exact same query"
(is (= (mbql-query
{:filter [:> [:field 4 nil] 1]
:breakout [[:field 17 nil]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query
{:filter [:> [:field 4 nil] 1]
:breakout [[:field 17 nil]]}))))))
@@ -52,7 +69,7 @@
[:is-null [:field 7 nil]]
[:> [:field 4 nil] 1]]]
:breakout [[:field 17 nil]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query
{:filter [:and
[:segment 1]
@@ -76,7 +93,7 @@
{:filter [:and
[:= [:field 5 nil] "abc"]
[:> [:field 6 nil] 1]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(lib.tu.macros/mbql-query venues
{:filter [:segment 2]}))))))
;; Next line makes temporary segment definitions mutually recursive.
@@ -90,7 +107,7 @@
(is (thrown-with-msg?
Exception
#"\QSegment expansion failed. Check mutually recursive segment definitions.\E"
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(lib.tu.macros/mbql-query venues {:filter [:segment 2]})))))))))
(deftest ^:parallel metric-test
@@ -105,7 +122,7 @@
[:= [:field 5 nil] "abc"]]
:breakout [[:field 17 nil]]
:order-by [[:asc [:field 1 nil]]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query
{:aggregation [[:metric 1]]
:filter [:> [:field 4 nil] 1]
@@ -127,7 +144,7 @@
:filter [:= [:field 5 nil] "abc"]
:breakout [[:field 17 nil]]
:order-by [[:asc [:field 1 nil]]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query
{:source-table 1000
:aggregation [[:metric 1]]
@@ -147,7 +164,7 @@
:filter [:= [:field 5 nil] "abc"]
:breakout [[:field 17 nil]]
:order-by [[:asc [:field 1 nil]]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query
{:aggregation [[:metric 1]]
:filter [:= [:field 5 nil] "abc"]
@@ -165,17 +182,20 @@
:filter [:and
[:between [:field 9 nil] 0 25]
[:segment 1]]}}]})
+ (testing "Sanity check: make sure we're overriding the old Metric 1 from mock-metadata-provider"
+ (is (=? {:name "My Metric"}
+ (lib.metadata/metric (qp.store/metadata-provider) 1))))
(is (= (mbql-query
{:source-table 1000
:aggregation [[:aggregation-options [:sum [:field 18 nil]] {:display-name "My Metric"}]]
:filter [:and
[:> [:field 4 nil] 1]
- [:is-null [:field 7 nil]]
- [:between [:field 9 nil] 0 25]
- [:= [:field 5 nil] "abc"]]
+ [:is-null [:field 7 nil]] ; from Segment 2
+ [:between [:field 9 nil] 0 25] ; from Metric 1
+ [:= [:field 5 nil] "abc"]] ; from Metric 1 => Segment 1
:breakout [[:field 17 nil]]
:order-by [[:asc [:field 1 nil]]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query
{:source-table 1000
:aggregation [[:metric 1]]
@@ -209,19 +229,19 @@
(testing "make sure that we don't try to expand GA \"metrics\" (#6104)"
(doseq [metric ["ga:users" "gaid:users"]]
(is (= (mbql-query {:aggregation [[:metric metric]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query {:aggregation [[:metric metric]]}))))))
(testing "make sure expansion works with multiple GA \"metrics\" (#7399)"
(is (= (mbql-query {:aggregation [[:metric "ga:users"]
[:metric "ga:1dayUsers"]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query {:aggregation [[:metric "ga:users"]
[:metric "ga:1dayUsers"]]}))))))
(deftest ^:parallel dont-expand-ga-segments-test
(testing "make sure we don't try to expand GA 'segments'"
(is (= (mbql-query {:filter [:segment "gaid:-11"]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query {:filter [:segment "gaid:-11"]}))))))
(deftest ^:parallel named-metrics-test
@@ -235,7 +255,7 @@
(is (= (mbql-query
{:aggregation [[:aggregation-options [:sum [:field 20 nil]] {:display-name "Named Metric"}]]
:breakout [[:field 10 nil]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query {:aggregation [[:aggregation-options
[:metric 1] {:display-name "Named Metric"}]]
:breakout [[:field 10 nil]]})))))))
@@ -254,7 +274,7 @@
[:sum [:field 20 nil]]
{:name "auto_generated_name", :display-name "Metric 1"}]]
:breakout [[:field 10 nil]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query {:aggregation [[:aggregation-options
[:metric 1] {:name "auto_generated_name"}]]
:breakout [[:field 10 nil]]})))))))
@@ -265,7 +285,7 @@
(is (=? (mbql-query
{:aggregation [[:aggregation-options [:sum [:field 20 nil]] {:display-name "My Cool Aggregation"}]]
:breakout [[:field 10 nil]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query {:aggregation [[:metric 1]]
:breakout [[:field 10 nil]]})))))))
@@ -284,7 +304,7 @@
[:sum [:field 20 nil]]
{:name "auto_generated_name", :display-name "Metric 1"}]]
:breakout [[:field 10 nil]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query {:aggregation [[:metric 1]]
:breakout [[:field 10 nil]]})))))))
@@ -297,7 +317,7 @@
[:or
[:is-null [:field 7 nil]]
[:> [:field 4 nil] 1]]]]]})
- (#'expand-macros/expand-metrics-and-segments
+ (expand-macros
(mbql-query
{:aggregation [[:share [:and
[:segment 1]
@@ -326,19 +346,19 @@
(testing "nested 1 level"
(is (= (lib.tu.macros/mbql-query nil
{:source-query after})
- (expand-macros/expand-macros
+ (expand-macros
(lib.tu.macros/mbql-query nil
{:source-query before})))))
(testing "nested 2 levels"
(is (= (lib.tu.macros/mbql-query nil
{:source-query {:source-query after}})
- (expand-macros/expand-macros
+ (expand-macros
(lib.tu.macros/mbql-query nil
{:source-query {:source-query before}})))))
(testing "nested 3 levels"
(is (= (lib.tu.macros/mbql-query nil
{:source-query {:source-query {:source-query after}}})
- (expand-macros/expand-macros
+ (expand-macros
(lib.tu.macros/mbql-query nil
{:source-query {:source-query {:source-query before}}})))))
(testing "nested at different levels"
@@ -346,34 +366,70 @@
{:source-query (-> after
(dissoc :source-table)
(assoc :source-query after))})
- (expand-macros/expand-macros
+ (expand-macros
(lib.tu.macros/mbql-query nil
{:source-query (-> before
(dissoc :source-table)
(assoc :source-query before))})))))
(testing "inside :source-query inside :joins"
(is (= (lib.tu.macros/mbql-query checkins
- {:joins [{:condition [:= 1 2]
+ {:joins [{:condition [:= [:field 1 nil] 2]
:source-query after}]})
- (expand-macros/expand-macros
+ (expand-macros
(lib.tu.macros/mbql-query checkins
- {:joins [{:condition [:= 1 2]
+ {:joins [{:condition [:= [:field 1 nil] 2]
:source-query before}]})))))
(when (= macro-type "Segments")
(testing "inside join condition"
(is (= (lib.tu.macros/mbql-query checkins
{:joins [{:source-table $$checkins
:condition (:filter after)}]})
- (expand-macros/expand-macros
+ (expand-macros
(lib.tu.macros/mbql-query checkins
{:joins [{:source-table $$checkins
:condition (:filter before)}]}))))))
(testing "inside :joins inside :source-query"
(is (= (lib.tu.macros/mbql-query nil
{:source-query {:source-table $$checkins
- :joins [{:condition [:= 1 2]
+ :joins [{:condition [:= [:field 1 nil] 2]
:source-query after}]}})
- (expand-macros/expand-macros (lib.tu.macros/mbql-query nil
+ (expand-macros (lib.tu.macros/mbql-query nil
{:source-query {:source-table $$checkins
- :joins [{:condition [:= 1 2]
+ :joins [{:condition [:= [:field 1 nil] 2]
:source-query before}]}}))))))))))
+
+(deftest ^:parallel preserve-uuids-test
+ (testing "the aggregation that replaces a :metric ref should keep the :metric's :lib/uuid, so :aggregation refs pointing to it are still valid"
+ (let [metric {:lib/type :metadata/metric
+ :id 1
+ :table-id 2
+ :name "Revenue"
+ :description "Sum of orders subtotal"
+ :archived false
+ :definition {:source-table 2, :aggregation [[:sum [:field 17 nil]]]}}
+ metadata-provider (lib.tu/mock-metadata-provider
+ meta/metadata-provider
+ {:metrics [metric]})]
+ (is (=? {:stages [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :orders)
+ :aggregation [[:sum
+ {:lib/uuid "0c586819-5288-4da9-adba-1ae904a34d5e"}
+ [:field {} 17]]]
+ :order-by [[:desc
+ {}
+ [:aggregation
+ {}
+ "0c586819-5288-4da9-adba-1ae904a34d5e"]]]}]
+ :database (meta/id)}
+ (#'expand-macros/expand-macros
+ {:lib/type :mbql/query
+ :lib/metadata metadata-provider
+ :stages [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :orders)
+ :aggregation [[:metric {:lib/uuid "0c586819-5288-4da9-adba-1ae904a34d5e"} 1]]
+ :order-by [[:desc
+ {:lib/uuid "4bfcc7da-1e47-41aa-af2c-bbdf11b8d6be"}
+ [:aggregation
+ {:lib/uuid "7fb46618-622c-40f3-b0d4-4a779179f055"}
+ "0c586819-5288-4da9-adba-1ae904a34d5e"]]]}]
+ :database (meta/id)}))))))
diff --git a/test/metabase/query_processor/middleware/permissions_test.clj b/test/metabase/query_processor/middleware/permissions_test.clj
index a6387857138..7ac7e321ed0 100644
--- a/test/metabase/query_processor/middleware/permissions_test.clj
+++ b/test/metabase/query_processor/middleware/permissions_test.clj
@@ -11,6 +11,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.query-processor.pipeline :as qp.pipeline]
+ [metabase.query-processor.setup :as qp.setup]
[metabase.query-processor.store :as qp.store]
[metabase.test :as mt]
[metabase.util :as u]
@@ -22,7 +23,8 @@
(let [qp (fn [query _rff]
(qp.pipeline/*result* query))
qp (qp.perms/check-query-permissions qp)]
- (qp query (constantly conj))))
+ (qp.setup/with-qp-setup [query query]
+ (qp query (constantly conj)))))
(defn- check-perms-for-rasta
"Check permissions for `query` with rasta as the current user."
@@ -31,15 +33,17 @@
(def ^:private perms-error-msg #"You do not have permissions to run this query\.")
-(deftest ^:parallel native-query-perms-test
+(deftest native-query-perms-test
(testing "Make sure the NATIVE query fails to run if current user doesn't have perms"
- (is (thrown-with-msg?
- ExceptionInfo
- perms-error-msg
- (check-perms-for-rasta
- {:database 1000
- :type :native
- :native {:query "SELECT * FROM VENUES"}})))))
+ (t2.with-temp/with-temp [:model/Database db {}]
+ (data-perms/set-database-permission! (perms-group/all-users) (u/the-id db) :perms/native-query-editing :no)
+ (is (thrown-with-msg?
+ ExceptionInfo
+ perms-error-msg
+ (check-perms-for-rasta
+ {:database (u/the-id db)
+ :type :native
+ :native {:query "SELECT * FROM VENUES"}}))))))
(deftest native-query-perms-test-2
(testing "...but it should work if user has perms"
@@ -80,15 +84,17 @@
:type :query
:query {:source-table (u/the-id table)}}))))))
-(deftest ^:parallel nested-native-query-test
+(deftest nested-native-query-test
(testing "Make sure nested native query fails to run if current user doesn't have perms"
- (is (thrown-with-msg?
- ExceptionInfo
- perms-error-msg
- (check-perms-for-rasta
- {:database 1000
- :type :query
- :query {:source-query {:native "SELECT * FROM VENUES"}}})))))
+ (t2.with-temp/with-temp [:model/Database db {}]
+ (data-perms/set-database-permission! (perms-group/all-users) (u/the-id db) :perms/native-query-editing :no)
+ (is (thrown-with-msg?
+ ExceptionInfo
+ perms-error-msg
+ (check-perms-for-rasta
+ {:database (u/the-id db)
+ :type :query
+ :query {:source-query {:native "SELECT * FROM VENUES"}}}))))))
(deftest nested-native-query-test-2
(testing "...but it should work if user has perms [nested native queries]"
@@ -156,23 +162,29 @@
Table _ {:db_id (u/the-id db)}
Table table-2 {:db_id (u/the-id db)}
Card card {:dataset_query {:database (u/the-id db), :type :query,
- :query {:source-table (u/the-id table-2)}}}]
+ :query {:source-table (u/the-id table-2)}}}]
(let [card-id (:id card)
tag-name (str "#" card-id)
query-sql (format "SELECT * FROM {{%s}} AS x" tag-name)]
;; query should be returned by middleware unchanged
(is (= {:database (u/the-id db)
- :type :native
- :native {:query query-sql,
- :template-tags {tag-name {:id tag-name, :name tag-name, :display-name tag-name, :type "card",
- :card card-id}}}}
+ :type :native
+ :native {:query query-sql
+ :template-tags {tag-name {:id tag-name
+ :name tag-name
+ :display-name tag-name
+ :type "card"
+ :card-id card-id}}}}
(check-perms-for-rasta
{:database (u/the-id db)
:type :native
:native {:query query-sql
:template-tags {tag-name
- {:id tag-name, :name tag-name, :display-name tag-name,
- :type "card", :card card-id}}}})))))))
+ {:id tag-name
+ :name tag-name
+ :display-name tag-name
+ :type "card"
+ :card-id card-id}}}})))))))
(deftest template-tags-referenced-queries-test-3
(testing "Fails for native query referenced in template tag, when user has no perms to referenced query"
@@ -200,23 +212,29 @@
(mt/with-temp [Database db {}
Card card {:dataset_query
{:database (u/the-id db), :type :native,
- :native {:query "SELECT 1 AS \"foo\", 2 AS \"bar\", 3 AS \"baz\""}}}]
- (let [card-id (:id card)
- tag-name (str "#" card-id)
+ :native {:query "SELECT 1 AS \"foo\", 2 AS \"bar\", 3 AS \"baz\""}}}]
+ (let [card-id (:id card)
+ tag-name (str "#" card-id)
query-sql (format "SELECT * FROM {{%s}} AS x" tag-name)]
;; query should be returned by middleware unchanged
(is (= {:database (u/the-id db)
:type :native
- :native {:query query-sql
- :template-tags {tag-name {:id tag-name, :name tag-name, :display-name tag-name, :type "card",
- :card card-id}}}}
+ :native {:query query-sql
+ :template-tags {tag-name {:id tag-name
+ :name tag-name
+ :display-name tag-name
+ :type "card"
+ :card-id card-id}}}}
(check-perms-for-rasta
{:database (u/the-id db)
:type :native
:native {:query query-sql
:template-tags {tag-name
- {:id tag-name, :name tag-name, :display-name tag-name,
- :type "card", :card card-id}}}})))))))
+ {:id tag-name
+ :name tag-name
+ :display-name tag-name
+ :type "card"
+ :card-id card-id}}}})))))))
(deftest query-action-permissions-test
(testing "Query action permissions"
diff --git a/test/metabase/query_processor/middleware/resolve_fields_test.clj b/test/metabase/query_processor/middleware/resolve_fields_test.clj
new file mode 100644
index 00000000000..87d8a920915
--- /dev/null
+++ b/test/metabase/query_processor/middleware/resolve_fields_test.clj
@@ -0,0 +1,15 @@
+(ns metabase.query-processor.middleware.resolve-fields-test
+ (:require
+ [clojure.test :refer :all]
+ [metabase.query-processor.middleware.resolve-fields :as qp.resolve-fields]))
+
+(deftest ^:parallel field-ids-test
+ (testing "Field IDs in metadata should get picked up correctly"
+ (is (= #{1}
+ (#'qp.resolve-fields/field-ids
+ {:lib/type :mbql/query
+ :database 1
+ :stages [{:lib/type :mbql.stage/mbql
+ :lib/stage-metadata {:columns [{:lib/type :metadata/column, :id 1}
+ {:lib/type :metadata/column, :id 0}]}
+ :source-table 1}]})))))
diff --git a/test/metabase/query_processor/middleware/resolve_referenced_test.clj b/test/metabase/query_processor/middleware/resolve_referenced_test.clj
index daf6dbaf2a8..b24e3532c2f 100644
--- a/test/metabase/query_processor/middleware/resolve_referenced_test.clj
+++ b/test/metabase/query_processor/middleware/resolve_referenced_test.clj
@@ -1,17 +1,14 @@
(ns metabase.query-processor.middleware.resolve-referenced-test
(:require
[clojure.test :refer :all]
+ [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.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
- [metabase.lib.test-util.macros :as lib.tu.macros]
- [metabase.query-processor.middleware.parameters-test
- :refer [card-template-tags]]
- [metabase.query-processor.middleware.resolve-referenced
- :as qp.resolve-referenced]
- [metabase.query-processor.store :as qp.store]
+ [metabase.query-processor.middleware.parameters-test :refer [card-template-tags]]
+ [metabase.query-processor.middleware.resolve-referenced :as qp.resolve-referenced]
[metabase.test :as mt])
(:import
(clojure.lang ExceptionInfo)))
@@ -20,112 +17,73 @@
(deftest ^:parallel resolve-card-resources-test
(testing "resolve stores source table from referenced card"
- (qp.store/with-metadata-provider (lib.tu/metadata-provider-with-cards-for-queries
- (lib.metadata.jvm/application-database-metadata-provider (mt/id))
- [(mt/mbql-query venues
- {:filter [:< $price 3]})])
- (let [query {:database (mt/id)
- :native {:template-tags
- {"tag-name-not-important1" {:type :card
- :card-id 1}}}}]
- (is (= query
- (#'qp.resolve-referenced/resolve-referenced-card-resources* query)))
- (is (some? (lib.metadata.protocols/cached-metadata
- (qp.store/metadata-provider)
- :metadata/table
- (mt/id :venues))))
- (is (some? (lib.metadata.protocols/cached-metadata
- (qp.store/metadata-provider)
- :metadata/column
- (mt/id :venues :price))))))))
+ (let [metadata-provider (lib.tu/metadata-provider-with-cards-for-queries
+ (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+ [(mt/mbql-query venues
+ {:filter [:< $price 3]})])
+ query (lib/query
+ metadata-provider
+ {:database (mt/id)
+ :type :native
+ :native {:query {}
+ :template-tags
+ {"tag-name-not-important1" {:type :card
+ :display-name "X"
+ :card-id 1}}}})]
+ (is (= query
+ (#'qp.resolve-referenced/resolve-referenced-card-resources query)))
+ (is (some? (lib.metadata.protocols/cached-metadata
+ metadata-provider
+ :metadata/table
+ (mt/id :venues))))
+ (is (some? (lib.metadata.protocols/cached-metadata
+ metadata-provider
+ :metadata/column
+ (mt/id :venues :price)))))))
(deftest ^:parallel referenced-query-from-different-db-test
(testing "fails on query that references a native query from a different database"
- (qp.store/with-metadata-provider (lib.tu/metadata-provider-with-cards-for-queries
- meta/metadata-provider
- [{:database (meta/id)
- :type :native
- :native {:query "SELECT 1 AS \"foo\", 2 AS \"bar\", 3 AS \"baz\""}}])
- (let [card-query (:dataset-query (lib.metadata/card (qp.store/metadata-provider) 1))
- tag-name "#1"
- query {:database 1234
- :type :native
- :native {:query (format "SELECT * FROM {{%s}} AS x" tag-name)
- :template-tags {tag-name ; This tag's query is from the test db
- {:id tag-name, :name tag-name, :display-name tag-name,
- :type "card", :card-id 1}}}}]
- (is (= {:referenced-query card-query
- :expected-database-id 1234}
- (try
- (#'qp.resolve-referenced/check-query-database-id= card-query 1234)
- (catch ExceptionInfo exc
- (ex-data exc)))))
- (is (nil? (#'qp.resolve-referenced/check-query-database-id= card-query (meta/id))))
- (is (thrown-with-msg?
- clojure.lang.ExceptionInfo
- #"\QReferenced query is from a different database\E"
- (#'qp.resolve-referenced/resolve-referenced-card-resources* query)))
- (is (= {:referenced-query card-query
- :expected-database-id 1234}
- (try
- (#'qp.resolve-referenced/resolve-referenced-card-resources* query)
- (catch ExceptionInfo exc
- (ex-data exc)))))))))
-
-(deftest ^:parallel referenced-query-from-different-db-test-2
- (testing "fails on query that references an MBQL query from a different database"
- (qp.store/with-metadata-provider (lib.tu/metadata-provider-with-cards-for-queries
- meta/metadata-provider
- [(lib.tu.macros/mbql-query venues
- {:filter [:< $price 3]})])
- (let [card-query (:dataset-query (lib.metadata/card (qp.store/metadata-provider) 1))
- tag-name "#1"
- query-db-id 1234
- query {:database query-db-id ; Note outer-query-db is used here
- :type :native
- :native {:query (format "SELECT * FROM {{%s}} AS x" tag-name)
- :template-tags {tag-name ; This tag's query is from the test db
- {:id tag-name, :name tag-name, :display-name tag-name,
- :type "card", :card-id 1}}}}]
- (is (= {:referenced-query card-query
- :expected-database-id query-db-id}
- (try
- (#'qp.resolve-referenced/check-query-database-id= card-query query-db-id)
- (catch ExceptionInfo exc
- (ex-data exc)))))
- (is (nil? (#'qp.resolve-referenced/check-query-database-id= card-query (meta/id))))
- (is (thrown-with-msg?
- clojure.lang.ExceptionInfo
- #"\QReferenced query is from a different database\E"
- (#'qp.resolve-referenced/resolve-referenced-card-resources* query)))
- (is (= {:referenced-query card-query
- :expected-database-id query-db-id}
- (try
- (#'qp.resolve-referenced/resolve-referenced-card-resources* query)
- (catch ExceptionInfo exc
- (ex-data exc)))))))))
+ (let [metadata-provider meta/metadata-provider
+ tag-name "#1"
+ query (lib/query
+ (lib.tu/mock-metadata-provider
+ metadata-provider
+ {:database (assoc (lib.metadata/database metadata-provider) :id 1234)})
+ {:database 1234
+ :type :native
+ :native {:query (format "SELECT * FROM {{%s}} AS x" tag-name)
+ :template-tags {tag-name ; This tag's query is from the test db
+ {:id tag-name, :name tag-name, :display-name tag-name,
+ :type "card", :card-id 1}}}})]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"\QCard 1 does not exist, or is from a different Database.\E"
+ (qp.resolve-referenced/resolve-referenced-card-resources query))))))
(deftest ^:parallel circular-referencing-tags-test
(testing "fails on query with circular referencing sub-queries"
- (qp.store/with-metadata-provider (lib.tu/metadata-provider-with-cards-for-queries
- meta/metadata-provider
- [{:database (meta/id)
- :type :native
- :native {:query "SELECT * FROM {{#2}} AS c2"
- :template-tags (card-template-tags [2])}}
- {:database (meta/id)
- :type :native
- :native {:query "SELECT * FROM {{#1}} AS c1"
- :template-tags (card-template-tags [1])}}])
- (let [entrypoint-query {:database (meta/id)
+ (let [metadata-provider (lib.tu/metadata-provider-with-cards-for-queries
+ meta/metadata-provider
+ [{:database (meta/id)
+ :type :native
+ :native {:query "SELECT * FROM {{#2}} AS c2"
+ :template-tags (card-template-tags [2])}}
+ {:database (meta/id)
+ :type :native
+ :native {:query "SELECT * FROM {{#1}} AS c1"
+ :template-tags (card-template-tags [1])}}])
+ entrypoint-query (lib/query
+ metadata-provider
+ {:database (meta/id)
:type :native
:native {:query "SELECT * FROM {{#1}}"
- :template-tags (card-template-tags [1])}}]
- (is (thrown?
- ExceptionInfo
- (#'qp.resolve-referenced/check-for-circular-references entrypoint-query)))
- (try
- (#'qp.resolve-referenced/check-for-circular-references entrypoint-query)
- (catch ExceptionInfo e
- (is (= (#'qp.resolve-referenced/circular-ref-error 2 1)
+ :template-tags (card-template-tags [1])}})]
+ (is (thrown?
+ ExceptionInfo
+ (#'qp.resolve-referenced/check-for-circular-references entrypoint-query)))
+ (try
+ (#'qp.resolve-referenced/check-for-circular-references entrypoint-query)
+ (catch ExceptionInfo e
+ (testing e
+ (is (= (#'qp.resolve-referenced/circular-ref-error entrypoint-query 2 1)
(ex-message e)))))))))
diff --git a/test/metabase/query_processor/middleware/resolve_source_table_test.clj b/test/metabase/query_processor/middleware/resolve_source_table_test.clj
index 3b8e7377346..b78d1df0ddf 100644
--- a/test/metabase/query_processor/middleware/resolve_source_table_test.clj
+++ b/test/metabase/query_processor/middleware/resolve_source_table_test.clj
@@ -1,75 +1,51 @@
(ns metabase.query-processor.middleware.resolve-source-table-test
(:require
[clojure.test :refer :all]
+ [metabase.lib.core :as lib]
+ [metabase.lib.metadata.jvm :as lib.metadata.jvm]
[metabase.lib.metadata.protocols :as lib.metadata.protocols]
- [metabase.models.database :refer [Database]]
- [metabase.models.table :refer [Table]]
- [metabase.query-processor.middleware.resolve-source-table
- :as qp.resolve-source-table]
- [metabase.query-processor.store :as qp.store]
+ [metabase.query-processor.middleware.resolve-source-table :as qp.resolve-source-table]
[metabase.test :as mt]
[toucan2.tools.with-temp :as t2.with-temp]))
-(defn- store-contents
+(defn- cached-metadata
"Fetch the names of all the objects currently in the QP Store."
- []
- (let [provider (qp.store/metadata-provider)
- tables [:venues :categories :users :checkins]]
+ [metadata-provider]
+ (let [tables [:venues :categories :users :checkins]]
{:tables (into #{}
(keep (fn [table]
- (:name (lib.metadata.protocols/cached-metadata provider :metadata/table (mt/id table)))))
+ (:name (lib.metadata.protocols/cached-metadata metadata-provider :metadata/table (mt/id table)))))
tables)}))
-(defn- resolve-source-tables [query]
- (qp.resolve-source-table/resolve-source-tables query))
-
-(defn- do-with-store-contents [thunk]
- (qp.store/with-metadata-provider (mt/id)
- (thunk)
- (store-contents)))
-
-(defmacro ^:private with-store-contents {:style/indent 0} [& body]
- `(do-with-store-contents (fn [] ~@body)))
-
-(defn- resolve-and-return-store-contents [query]
- (with-store-contents
- (resolve-source-tables query)))
+(defn- resolve-and-return-cached-metadata [query]
+ (let [metadata-provider (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+ query (lib/query metadata-provider query)]
+ (qp.resolve-source-table/resolve-source-tables query)
+ (cached-metadata metadata-provider)))
(deftest ^:parallel basic-test
(testing "does `resolve-source-tables` resolve source tables?"
(is (= {:tables #{"VENUES"}}
- (resolve-and-return-store-contents (mt/mbql-query venues))))))
+ (resolve-and-return-cached-metadata (mt/mbql-query venues))))))
(deftest ^:parallel validate-database-test
(testing "If the Table does not belong to the current Database, does it throw an Exception?"
- (t2.with-temp/with-temp [Database {database-id :id} {}
- Table {table-id :id} {:db_id database-id}]
+ (t2.with-temp/with-temp [:model/Database {database-id :id} {}
+ :model/Table {table-id :id} {:db_id database-id}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"\QFailed to fetch :metadata/table\E"
- (resolve-and-return-store-contents
+ #"Failed to fetch :metadata/table \d+: either it does not exist, or it belongs to a different Database"
+ (resolve-and-return-cached-metadata
{:database (mt/id)
:type :query
:query {:source-table table-id}}))))))
(deftest ^:parallel validate-source-table-test
(testing "Should throw an Exception if there's a `:source-table` in the query that IS NOT a positive int"
- (is (thrown-with-msg?
- clojure.lang.ExceptionInfo
- #"\QInvalid :source-table 'ABC': should be resolved to a Table ID by now\E"
- (resolve-and-return-store-contents
- {:database (mt/id)
- :type :query
- :query {:source-table "ABC"}})))))
-
-(deftest ^:parallel validate-source-table-test-2
- (testing "Should throw an Exception if there's a `:source-table` in the query that IS NOT a positive int"
- ;; TODO -- a little weird that this triggers a schema validation error while the string Table ID gets a more
- ;; useful error message
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"Invalid output:.*should be a positive int, got: 0"
- (resolve-and-return-store-contents
+ (resolve-and-return-cached-metadata
{:database (mt/id)
:type :query
:query {:source-table 0}})))))
@@ -77,19 +53,19 @@
(deftest ^:parallel nested-queries-test
(testing "Does `resolve-source-tables` resolve source tables in nested source queries?"
(is (= {:tables #{"VENUES"}}
- (resolve-and-return-store-contents
+ (resolve-and-return-cached-metadata
(mt/mbql-query nil
{:source-query {:source-table $$venues}}))))
(is (= {:tables #{"VENUES"}}
- (resolve-and-return-store-contents
+ (resolve-and-return-cached-metadata
(mt/mbql-query nil
{:source-query {:source-query {:source-table $$venues}}}))))))
(deftest ^:parallel joins-test
(testing "Does `resolve-source-tables` resolve source tables in joins?"
(is (= {:tables #{"CATEGORIES" "VENUES"}}
- (resolve-and-return-store-contents
+ (resolve-and-return-cached-metadata
(mt/mbql-query venues
{:joins [{:source-table $$categories
:alias "c"
@@ -98,7 +74,7 @@
(deftest ^:parallel joins-in-nested-queries-test
(testing "Does `resolve-source-tables` resolve source tables in joins inside nested source queries?"
(is (= {:tables #{"CATEGORIES" "VENUES"}}
- (resolve-and-return-store-contents
+ (resolve-and-return-cached-metadata
(mt/mbql-query venues
{:source-query {:source-table $$venues
:joins [{:source-table $$categories
@@ -108,7 +84,7 @@
(deftest ^:parallel nested-queries-in-joins-test
(testing "Does `resolve-source-tables` resolve source tables inside nested source queries inside joins?"
(is (= {:tables #{"CATEGORIES" "VENUES"}}
- (resolve-and-return-store-contents
+ (resolve-and-return-cached-metadata
(mt/mbql-query venues
{:joins [{:source-query {:source-table $$categories}
:alias "c"
@@ -118,7 +94,7 @@
(testing (str "Does `resolve-source-tables` resolve source tables inside nested source queries inside joins inside "
"nested source queries?")
(is (= {:tables #{"CATEGORIES" "VENUES"}}
- (resolve-and-return-store-contents
+ (resolve-and-return-cached-metadata
(mt/mbql-query venues
{:source-query {:source-table $$venues
:joins [{:source-query {:source-table $$categories}
diff --git a/test/metabase/query_processor/middleware/upgrade_field_literals_test.clj b/test/metabase/query_processor/middleware/upgrade_field_literals_test.clj
index ea2238e0b3b..b1d67c417b6 100644
--- a/test/metabase/query_processor/middleware/upgrade_field_literals_test.clj
+++ b/test/metabase/query_processor/middleware/upgrade_field_literals_test.clj
@@ -44,14 +44,14 @@
{:aggregation [[:count]]
:breakout [!week.date]
:filter [:between !week.date "2014-02-01T00:00:00-08:00" "2014-05-01T00:00:00-07:00"]
- :source-query source-query
+ :source-query (:query source-query)
:source-metadata source-metadata})
(upgrade-field-literals
(mt/mbql-query nil
{:aggregation [[:count]]
:breakout [!week.*DATE/Date]
:filter [:between !week.*DATE/Date "2014-02-01T00:00:00-08:00" "2014-05-01T00:00:00-07:00"]
- :source-query source-query
+ :source-query (:query source-query)
:source-metadata source-metadata}))))))
(deftest support-legacy-filter-clauses-test
diff --git a/test/metabase/query_processor/util/tag_referenced_cards_test.clj b/test/metabase/query_processor/util/tag_referenced_cards_test.clj
deleted file mode 100644
index 0dd1dadb24e..00000000000
--- a/test/metabase/query_processor/util/tag_referenced_cards_test.clj
+++ /dev/null
@@ -1,25 +0,0 @@
-(ns metabase.query-processor.util.tag-referenced-cards-test
- (:require
- [clojure.test :refer :all]
- [metabase.models.card :refer [Card]]
- [metabase.query-processor.store :as qp.store]
- [metabase.query-processor.util.tag-referenced-cards
- :as qp.u.tag-referenced-cards]
- [metabase.test :as mt]))
-
-(deftest tags-referenced-cards-lookup-test
- (testing "returns Card instances from raw query"
- (mt/with-temp [Card c1 {}
- Card c2 {}]
- (qp.store/with-metadata-provider (mt/id)
- (is (=? [{:id (:id c1)
- :dataset-query {}}
- {:id (:id c2)
- :dataset-query {}}]
- (qp.u.tag-referenced-cards/tags-referenced-cards
- {:native
- {:template-tags
- {"tag-name-not-important1" {:type :card
- :card-id (:id c1)}
- "tag-name-not-important2" {:type :card
- :card-id (:id c2)}}}})))))))
--
GitLab