Skip to content

Commit

Permalink
Add properties to query_execution (metabase#35508)
Browse files Browse the repository at this point in the history
  • Loading branch information
qwef authored Nov 17, 2023
1 parent a1cee62 commit 3827a2b
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,10 @@
(defenterprise merge-sandboxing-metadata
"Post-processing middleware. Merges in column metadata from the original, unsandboxed version of the query."
:feature :sandboxes
[{::keys [original-metadata]} rff]
(if original-metadata
(fn merge-sandboxing-metadata-rff* [metadata]
(rff (merge-metadata original-metadata metadata)))
rff))
[{::keys [original-metadata] :as query} rff]
(fn merge-sandboxing-metadata-rff* [metadata]
(let [metadata (assoc metadata :is_sandboxed (some? (get-in query [::qp.perms/perms :gtaps])))
metadata (if original-metadata
(merge-metadata original-metadata metadata)
metadata)]
(rff metadata))))
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
[clojure.test :refer :all]
[metabase-enterprise.test :as met]
[metabase.api.card-test :as api.card-test]
[metabase.models :refer [Card Collection Database PermissionsGroup PermissionsGroupMembership Table]]
[metabase.models :refer [Card Collection Database PermissionsGroup
PermissionsGroupMembership Table]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
[metabase.query-processor :as qp]
[metabase.test :as mt]
[metabase.util :as u]))
[metabase.util :as u]
[toucan2.tools.with-temp :as t2.with-temp]))

(deftest users-with-segmented-perms-test
(testing "Users with segmented permissions should be able to save cards"
Expand Down Expand Up @@ -83,3 +86,11 @@
(is (=? {:values []
:has_more_values false}
(search :rasta)))))))))

(deftest is-sandboxed-test
(testing "Adding a GTAP to the all users group to a table makes it such that is_sandboxed returns true."
(met/with-gtaps {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:<= $id 3]})}}}
(t2.with-temp/with-temp [Card card {:database_id (mt/id)
:table_id (mt/id :categories)
:dataset_query (mt/mbql-query categories)}]
(is (get-in (qp/process-userland-query (:dataset_query card)) [:data :is_sandboxed]))))))
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
[metabase.query-processor :as qp]
[metabase.query-processor.middleware.cache-test :as cache-test]
[metabase.query-processor.middleware.permissions :as qp.perms]
[metabase.query-processor.middleware.process-userland-query-test :as process-userland-query-test]
[metabase.query-processor.pivot :as qp.pivot]
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.util :as qp.util]
Expand Down Expand Up @@ -1113,3 +1114,14 @@
(is (not (str/includes? (-> sandboxed-result :data :native_form :query)
(:table_name persisted-info)))
"Erroneously used the persisted model cache")))))))))))

(deftest is-sandboxed-success-test
(testing "Integration test that checks that is_sandboxed is recorded in query_execution correctly for a sandboxed query"
(met/with-gtaps {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:<= $id 3]})}}}
(t2.with-temp/with-temp [Card card {:database_id (mt/id)
:table_id (mt/id :categories)
:dataset_query (mt/mbql-query categories)}]
(let [query (:dataset_query card)]
(process-userland-query-test/with-query-execution [qe query]
(qp/process-userland-query query)
(is (:is_sandboxed (qe)))))))))
15 changes: 15 additions & 0 deletions resources/migrations/001_update_migrations.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3901,6 +3901,21 @@ databaseChangeLog:
- sql:
sql: drop view if exists v_view_log;

- changeSet:
id: v48.00-045
author: qwef
comment: Added 0.48.0 - add is_sandboxed to query_execution
changes:
- addColumn:
tableName: query_execution
columns:
- column:
name: is_sandboxed
type: boolean
remarks: "Is query from a sandboxed user"
constraints:
nullable: true

- changeSet:
id: v48.00-046
author: noahmoss
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,11 @@
(catch Throwable e
(log/error e (trs "Error saving query execution info"))))))))

(defn- save-successful-query-execution! [cached? query-execution result-rows]
(let [qe-map (assoc query-execution :cache_hit (boolean cached?) :result_rows result-rows)]
(defn- save-successful-query-execution! [cached? is_sandboxed? query-execution result-rows]
(let [qe-map (assoc query-execution
:cache_hit (boolean cached?)
:result_rows result-rows
:is_sandboxed (boolean is_sandboxed?))]
(save-query-execution! qe-map)))

(defn- save-failed-query-execution! [query-execution message]
Expand All @@ -75,7 +78,7 @@
(merge
(-> query-execution
add-running-time
(dissoc :error :hash :executor_id :action_id :card_id :dashboard_id :pulse_id :result_rows :native))
(dissoc :error :hash :executor_id :action_id :is_sandboxed :card_id :dashboard_id :pulse_id :result_rows :native))
result
{:status :completed
:average_execution_time (when cached?
Expand All @@ -94,7 +97,7 @@
(events/publish-event! :event/card-query {:user-id (:executor_id execution-info)
:card-id (:card_id execution-info)
:context (:context execution-info)}))
(save-successful-query-execution! (:cached acc) execution-info @row-count)
(save-successful-query-execution! (:cached acc) (get-in acc [:data :is_sandboxed]) execution-info @row-count)
(rf (if (map? acc)
(success-response execution-info acc)
acc)))
Expand Down
1 change: 1 addition & 0 deletions test/metabase/api/dataset_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
:native false
:pulse_id nil
:card_id nil
:is_sandboxed false
:dashboard_id nil
:error nil
:id true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

(set! *warn-on-reflection* true)

(defn- do-with-query-execution [query run]
(defn do-with-query-execution [query run]
(mt/with-clock #t "2020-02-04T12:22-08:00[US/Pacific]"
(let [original-hash (qp.util/query-hash query)
result (promise)]
Expand All @@ -31,7 +31,7 @@
(:hash qe) (update :hash (fn [^bytes a-hash]
(some-> a-hash codecs/bytes->hex)))))))))))

(defmacro ^:private with-query-execution {:style/indent 1} [[qe-result-binding query] & body]
(defmacro with-query-execution {:style/indent 1} [[qe-result-binding query] & body]
`(do-with-query-execution ~query (fn [~qe-result-binding] ~@body)))

(defn- process-userland-query
Expand Down Expand Up @@ -71,6 +71,7 @@
:pulse_id nil
:card_id nil
:action_id nil
:is_sandboxed false
:context nil
:running_time true
:cache_hit false
Expand Down

0 comments on commit 3827a2b

Please sign in to comment.