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

@@ -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

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])

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))))

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."

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}

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)))

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