From 0dd4013dc0bf43f508cf0c59a77295891bda202e Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Mon, 26 Aug 2024 11:27:05 -0400
Subject: [PATCH] Use more precise query contexts when recording execution of
public downloads (#47201)
---
src/metabase/api/public.clj | 11 ++++++-
src/metabase/lib/schema/info.cljc | 3 ++
.../middleware/process_userland_query.clj | 4 +--
test/metabase/api/public_test.clj | 30 +++++++++++++++++++
4 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index 4e1bea08214..d97860262f1 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -15,6 +15,7 @@
[metabase.db.query :as mdb.query]
[metabase.events :as events]
[metabase.lib.schema.id :as lib.schema.id]
+ [metabase.lib.schema.info :as lib.schema.info]
[metabase.lib.util.match :as lib.util.match]
[metabase.models.action :as action]
[metabase.models.card :as card :refer [Card]]
@@ -138,6 +139,14 @@
(mw.session/as-admin
(qp (update query :info merge info) rff))))))
+(mu/defn- export-format->context :- ::lib.schema.info/context
+ [export-format]
+ (case export-format
+ "csv" :public-csv-download
+ "xlsx" :public-xlsx-download
+ "json" :public-json-download
+ :public-question))
+
(mu/defn process-query-for-card-with-id
"Run the query belonging to Card with `card-id` with `parameters` and other query options (e.g. `:constraints`).
Returns a `StreamingResponse` object that should be returned as the result of an API endpoint."
@@ -155,7 +164,7 @@
(mw.session/as-admin
(m/mapply qp.card/process-query-for-card card-id export-format
:parameters parameters
- :context :public-question
+ :context (export-format->context export-format)
:qp qp
:make-run process-query-for-card-with-id-run-fn
options)))
diff --git a/src/metabase/lib/schema/info.cljc b/src/metabase/lib/schema/info.cljc
index 3665118c047..0d9bd7bedf4 100644
--- a/src/metabase/lib/schema/info.cljc
+++ b/src/metabase/lib/schema/info.cljc
@@ -25,6 +25,9 @@
:json-download
:public-dashboard
:public-question
+ :public-csv-download
+ :public-xlsx-download
+ :public-json-download
:embedded-dashboard
:embedded-question
:embedded-csv-download
diff --git a/src/metabase/query_processor/middleware/process_userland_query.clj b/src/metabase/query_processor/middleware/process_userland_query.clj
index afb96ec491e..03d3f5b7b09 100644
--- a/src/metabase/query_processor/middleware/process_userland_query.clj
+++ b/src/metabase/query_processor/middleware/process_userland_query.clj
@@ -64,9 +64,7 @@
(defn- save-execution-metadata!
"Save a `QueryExecution` row containing `execution-info`. Done asynchronously when a query is finished."
[execution-info field-usages]
- (let [execution-info' (sdk/include-analytics execution-info)
- ;; `sdk/assoc-analytics` reads values from dynamic vars, so we need to set them here, on the same thread:
- ]
+ (let [execution-info' (sdk/include-analytics execution-info)]
(qp.util/with-execute-async
;; 1. Asynchronously save QueryExecution, update query average execution time etc. using the Agent/pooledExecutor
;; pool, which is a fixed pool of size `nthreads + 2`. This way we don't spin up a ton of threads doing unimportant
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index 2983cab4e13..ee0324f17f5 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -21,6 +21,7 @@
[metabase.models.params.chain-filter-test :as chain-filter-test]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
+ [metabase.query-processor.middleware.process-userland-query-test :as process-userland-query-test]
[metabase.test :as mt]
[metabase.util :as u]
[throttle.core :as throttle]
@@ -507,6 +508,35 @@
keys
set))))))
+(deftest query-execution-context test
+ (testing "Make sure we record the correct context for each export format (#45147)"
+ (mt/with-temporary-setting-values [enable-public-sharing true]
+ (let [query (merge (mt/mbql-query venues)
+ ;; Add these constraints for the API query so that the query hash matches in `with-query-execution!`
+ {:constraints {:max-results 10000, :max-results-bare-rows 2000}})]
+ (with-temp-public-card [{uuid :public_uuid} {:dataset_query query}]
+ (testing "Default :api response format"
+ (process-userland-query-test/with-query-execution! [qe query]
+ (client/client :get 202 (str "public/card/" uuid "/query"))
+ (is (= :public-question (:context (qe))))))))
+
+ (let [query (merge (mt/mbql-query venues))]
+ (with-temp-public-card [{uuid :public_uuid} {:dataset_query query}]
+ (testing ":json download response format"
+ (process-userland-query-test/with-query-execution! [qe query]
+ (client/client :get 200 (str "public/card/" uuid "/query/json"))
+ (is (= :public-json-download (:context (qe))))))
+
+ (testing ":xlsx download response format"
+ (process-userland-query-test/with-query-execution! [qe query]
+ (client/client :get 200 (str "public/card/" uuid "/query/xlsx"))
+ (is (= :public-xlsx-download (:context (qe))))))
+
+ (testing ":csv download response format"
+ (process-userland-query-test/with-query-execution! [qe query]
+ (client/client :get 200 (str "public/card/" uuid "/query/csv"), :format :csv)
+ (is (= :public-csv-download (:context (qe)))))))))))
+
;;; ---------------------------------------- GET /api/public/dashboard/:uuid -----------------------------------------
(deftest get-public-dashboard-errors-test
--
GitLab