From ea9dfd1d2b7e91b604c3bc05b055a301bffde98e Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Mon, 17 Jun 2024 12:29:57 -0600
Subject: [PATCH] Change run to make-run in process-query-for-card (#44244)
* Change run to make-run in process-query-for-card
Fixes #44160
There were two levers where a card could change how it was run.
`qp` this is basically `qp/process-query` with some optional userland
flags and constraints marked on the passed in query.
In the case of pivot tables in the
`/pivot/:card-id/query` endpoints it was `qp.pivot/process-query`
'run' this defaults to a streaming response that takes the `qp` param
and `export-format` and passes a query to qp. Most often this is
overriden to not stream the response.
Unfortunately, if `run` is overwritten, then `qp` is thrown away, and
the caller who overrides `run` must determine the correct `qp` to use.
However, when running `process-query-for-dashcard` it is difficult to properly set
`qp` for all of the dashcards without doing card lookups. So [this commit](https://github.com/metabase/metabase/blob/release-x.50.x/src/metabase/query_processor/card.clj#L230-L232) did a check to swap the runner in the presence of a pivot query that calls the **default** runner with a `qp.pivot/run-pivot-query`. The problem is that pulse needs to use a non-default runner, but there's no way for `process-query-for-card` to do both.
This commit changes `run` to `make-run` which raises the abstraction to
a function that produces a runer given a `qp`. This allows
`process-query-for-card` to change the `qp` as needed while keeping the
passed in runner.
There's really three things going on that could be teased apart
further/better.
1. `qp` - this conflates the processor (`qp/process-query` `qp.pivot/run-pivot-query`)
and modifying query (`qp/userland-query`,
`qp/userland-query-with-default-constraints` `(update query :info merge info)`)
2. `make-run` - How the above are processed: (streaming/realized)
* Fix tests
---
src/metabase/api/public.clj | 7 ++--
src/metabase/pulse/util.clj | 24 ++++++------
src/metabase/query_processor/card.clj | 37 +++++++++++--------
src/metabase/query_processor/dashboard.clj | 2 +-
test/metabase/api/card_test.clj | 4 +-
test/metabase/query_processor/card_test.clj | 25 ++++++++++---
.../query_processor/dashboard_test.clj | 5 ++-
7 files changed, 63 insertions(+), 41 deletions(-)
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index 9ca248b0387..cf662629c31 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -127,7 +127,7 @@
{:error (tru "An error occurred while running the query.")})))
(defn- process-query-for-card-with-id-run-fn
- "Create the `:run` function used for [[process-query-for-card-with-id]] and [[process-query-for-dashcard]]."
+ "Create the `:make-run` function used for [[process-query-for-card-with-id]] and [[process-query-for-dashcard]]."
[qp export-format]
(fn run [query info]
(qp.streaming/streaming-response [rff export-format (u/slugify (:card-name info))]
@@ -153,7 +153,8 @@
(m/mapply qp.card/process-query-for-card card-id export-format
:parameters parameters
:context :public-question
- :run (process-query-for-card-with-id-run-fn qp export-format)
+ :qp qp
+ :make-run process-query-for-card-with-id-run-fn
options)))
(defn ^:private process-query-for-card-with-public-uuid
@@ -267,7 +268,7 @@
(string? parameters) (json/parse-string keyword))
:export-format export-format
:qp qp
- :run (process-query-for-card-with-id-run-fn qp export-format)})]
+ :make-run process-query-for-card-with-id-run-fn})]
;; Run this query with full superuser perms. We don't want the various perms checks failing because there are no
;; current user perms; if this Dashcard is public you're by definition allowed to run it without a perms check
;; anyway
diff --git a/src/metabase/pulse/util.clj b/src/metabase/pulse/util.clj
index 6d830cfa588..e409f8b6326 100644
--- a/src/metabase/pulse/util.clj
+++ b/src/metabase/pulse/util.clj
@@ -74,17 +74,19 @@
:dashcard dashcard
:type :card
:result (qp.dashboard/process-query-for-dashcard
- :dashboard-id dashboard-id
- :card-id card-id
- :dashcard-id (u/the-id dashcard)
- :context :dashboard-subscription
- :export-format :api
- :parameters parameters
- :middleware {:process-viz-settings? true
- :js-int-to-string? false}
- :run (^:once fn* [query info]
- (qp/process-query
- (qp/userland-query-with-default-constraints query info))))})
+ :dashboard-id dashboard-id
+ :card-id card-id
+ :dashcard-id (u/the-id dashcard)
+ :context :dashboard-subscription
+ :export-format :api
+ :parameters parameters
+ :middleware {:process-viz-settings? true
+ :js-int-to-string? false}
+ :make-run (fn make-run [qp _export-format]
+ (^:once fn* [query info]
+ (qp
+ (qp/userland-query-with-default-constraints query info)
+ nil))))})
result (result-fn card-id)
series-results (map (comp result-fn :id) multi-cards)]
(when-not (and (get-in dashcard [:visualization_settings :card.hide_empty])
diff --git a/src/metabase/query_processor/card.clj b/src/metabase/query_processor/card.clj
index 61b3a2c3deb..87cda35bb8f 100644
--- a/src/metabase/query_processor/card.clj
+++ b/src/metabase/query_processor/card.clj
@@ -166,11 +166,11 @@
(mu/defn process-query-for-card-default-qp :- :some
"Default value of the `:qp` option for [[process-query-for-card]]."
[query :- ::qp.schema/query
- rff :- ::qp.schema/rff]
+ rff :- [:maybe ::qp.schema/rff]]
(qp/process-query (qp/userland-query query) rff))
-(defn- process-query-for-card-default-run-fn
- "Create the default `:run` function for [[process-query-for-card]]."
+(defn process-query-for-card-default-run-fn
+ "Create the default `:make-run` function for [[process-query-for-card]]."
[qp export-format]
(^:once fn* [query info]
(qp.streaming/streaming-response [rff export-format (u/slugify (:card-name info))]
@@ -179,10 +179,14 @@
(mu/defn process-query-for-card
"Run the query for Card with `parameters` and `constraints`. By default, returns results in a
`metabase.async.streaming_response.StreamingResponse` (see [[metabase.async.streaming-response]]) that should be
- returned as the result of an API endpoint fn, but you can return something different by passing a different `:run`
- option. `:run` has a signature
+ returned as the result of an API endpoint fn, but you can return something different by passing a different `:make-run`
+ option. `:make-run` has a signature.
- (run query info) => result
+ (make-run qp export-format) => (fn run [query info])
+
+ The produced `run` fn has a signature, it should use the qp in to produce the results.
+
+ (run query info) => results
Will throw an Exception if preconditions (such as read perms) are not met *before* returning the
`StreamingResponse`.
@@ -192,13 +196,12 @@
options."
[card-id :- ::lib.schema.id/card
export-format
- & {:keys [parameters constraints context dashboard-id dashcard-id middleware qp run ignore-cache]
+ & {:keys [parameters constraints context dashboard-id dashcard-id middleware qp make-run ignore-cache]
:or {constraints (qp.constraints/default-query-constraints)
context :question
- qp process-query-for-card-default-qp
- ;; param `run` can be used to control how the query is ran, e.g. if you need to customize the `context`
+ ;; param `make-run` can be used to control how the query is ran, e.g. if you need to customize the `context`
;; passed to the QP
- run (process-query-for-card-default-run-fn qp export-format)}}]
+ make-run process-query-for-card-default-run-fn}}]
{:pre [(int? card-id) (u/maybe? sequential? parameters)]}
(let [dash-viz (when (and (not= context :question)
dashcard-id)
@@ -207,12 +210,17 @@
:type :result_metadata :visualization_settings :display
:cache_invalidated_at]
:id card-id))
+ ;; We need to check this here because dashcards don't get selected until this point
+ qp (if (= :pivot (:display card))
+ qp.pivot/run-pivot-query
+ (or qp process-query-for-card-default-qp))
+ runner (make-run qp export-format)
query (-> (query-for-card card parameters constraints middleware {:dashboard-id dashboard-id})
(update :viz-settings (fn [viz] (merge viz dash-viz)))
(update :middleware (fn [middleware]
(merge
- {:js-int-to-string? true, :ignore-cached-results? ignore-cache}
- middleware))))
+ {:js-int-to-string? true, :ignore-cached-results? ignore-cache}
+ middleware))))
info (cond-> {:executed-by api/*current-user-id*
:context context
:card-id card-id
@@ -226,7 +234,4 @@
(log/tracef "Running query for Card %d:\n%s" card-id
(u/pprint-to-str query))
(binding [qp.perms/*card-id* card-id]
- (let [run (if (= :pivot (:display card))
- (process-query-for-card-default-run-fn qp.pivot/run-pivot-query export-format)
- run)]
- (run query info)))))
+ (runner query info))))
diff --git a/src/metabase/query_processor/dashboard.clj b/src/metabase/query_processor/dashboard.clj
index 568a8a47b5b..f5f00762aae 100644
--- a/src/metabase/query_processor/dashboard.clj
+++ b/src/metabase/query_processor/dashboard.clj
@@ -163,7 +163,7 @@
(defn process-query-for-dashcard
"Like [[metabase.query-processor.card/process-query-for-card]], but runs the query for a `DashboardCard` with
`parameters` and `constraints`. By default, returns a `metabase.async.streaming_response.StreamingResponse` (see
- [[metabase.async.streaming-response]]), but this may vary if you pass in a different `:run` function. Will throw an
+ [[metabase.async.streaming-response]]), but this may vary if you pass in a different `:make-run` function. Will throw an
Exception if preconditions such as proper permissions are not met *before* returning the `StreamingResponse`.
See [[metabase.query-processor.card/process-query-for-card]] for more information about the various parameters."
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index a615361ba4f..3246e565100 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -2381,8 +2381,8 @@
(let [orig qp.card/process-query-for-card]
(with-redefs [qp.card/process-query-for-card (fn [card-id export-format & options]
(apply orig card-id export-format
- :run (fn [{:keys [constraints]} _]
- {:constraints constraints})
+ :make-run (constantly (fn [{:keys [constraints]} _]
+ {:constraints constraints}))
options))]
(testing "Sanity check: this CSV download should not be subject to C O N S T R A I N T S"
(is (= {:constraints nil}
diff --git a/test/metabase/query_processor/card_test.clj b/test/metabase/query_processor/card_test.clj
index 2cab76d5609..2db499c78e5 100644
--- a/test/metabase/query_processor/card_test.clj
+++ b/test/metabase/query_processor/card_test.clj
@@ -21,9 +21,10 @@
;; stuff doesn't belong in the Dashboard QP namespace
(binding [api/*current-user-permissions-set* (atom #{"/"})]
(qp.card/process-query-for-card
- card-id :api
- :run (fn [query info]
- (qp/process-query (assoc query :info info))))))
+ card-id :api
+ :make-run (constantly
+ (fn [query info]
+ (qp/process-query (assoc query :info info)))))))
(defn field-filter-query
"A query with a Field Filter parameter"
@@ -170,6 +171,17 @@
(is (= [[100]]
(mt/rows (run-query-for-card card-id)))))))
+(deftest ^:parallel pivot-tables-should-not-override-the-run-function
+ (testing "Pivot tables should not override the run function (#44160)"
+ (t2.with-temp/with-temp [:model/Card {card-id :id} {:dataset_query
+ (mt/mbql-query venues
+ {:aggregation [[:count]]})
+ :display :pivot}]
+ (let [result (run-query-for-card card-id)]
+ (is (=? {:status :completed}
+ result))
+ (is (= [[100]] (mt/rows result)))))))
+
(deftest nested-query-permissions-test
(testing "Should be able to run a Card with another Card as its source query with just perms for the former (#15131)"
(mt/with-no-data-perms-for-all-users!
@@ -190,9 +202,10 @@
(letfn [(process-query-for-card [card]
(qp.card/process-query-for-card
(u/the-id card) :api
- :run (fn [query info]
- (let [info (assoc info :query-hash (byte-array 0))]
- (qp/process-query (assoc query :info info))))))]
+ :make-run (constantly
+ (fn [query info]
+ (let [info (assoc info :query-hash (byte-array 0))]
+ (qp/process-query (assoc query :info info)))))))]
(testing "Should not be able to run the parent Card"
(is (not (mi/can-read? disallowed-collection)))
(is (not (mi/can-read? parent-card)))
diff --git a/test/metabase/query_processor/dashboard_test.clj b/test/metabase/query_processor/dashboard_test.clj
index ae5d8e3da8b..3739eac91ed 100644
--- a/test/metabase/query_processor/dashboard_test.clj
+++ b/test/metabase/query_processor/dashboard_test.clj
@@ -21,8 +21,9 @@
:dashboard-id dashboard-id
:card-id card-id
:dashcard-id dashcard-id
- :run (fn run [query info]
- (qp/process-query (assoc query :info info)))
+ :make-run (constantly
+ (fn run [query info]
+ (qp/process-query (assoc query :info info))))
options)))
(deftest resolve-parameters-validation-test
--
GitLab