Skip to content
Snippets Groups Projects
Commit ac60e0b7 authored by Ryan Senior's avatar Ryan Senior
Browse files

Ensure queries to compute metadata include user information

When saving a question and the included metadata is incorrect or
different we will execute another query to compute that metadata. This
commit adds the `:info` map needed to include the user and query hash
information we use in the SQL remark.

Fixes #8825
parent 43e0a504
Branches
Tags
No related merge requests found
......@@ -187,7 +187,7 @@
This is obviously a bit wasteful so hopefully we can avoid having to do this."
[query]
(binding [qpi/*disable-qp-logging* true]
(let [{:keys [status], :as results} (qp/process-query query)]
(let [{:keys [status], :as results} (qp/process-query-without-save! api/*current-user-id* query)]
(if (= status :failed)
(log/error (trs "Error running query to determine Card result metadata:")
(u/pprint-to-str 'red results))
......
......@@ -327,6 +327,11 @@
(u/pprint-to-str (u/filtered-stacktrace e))))
(save-and-return-failed-query! query-execution e)))))))
(s/defn ^:private assoc-query-info [query, options :- mbql.s/Info]
(assoc query :info (assoc options
:query-hash (qputil/query-hash query)
:query-type (if (qputil/mbql-query? query) "MBQL" "native"))))
;; TODO - couldn't saving the query execution be done by MIDDLEWARE?
(s/defn process-query-and-save-execution!
"Process and run a json based dataset query and return results.
......@@ -342,9 +347,7 @@
OPTIONS must conform to the `mbql.s/Info` schema; refer to that for more details."
{:style/indent 1}
[query, options :- mbql.s/Info]
(run-and-save-query! (assoc query :info (assoc options
:query-hash (qputil/query-hash query)
:query-type (if (qputil/mbql-query? query) "MBQL" "native")))))
(run-and-save-query! (assoc-query-info query options)))
(def ^:private ^:const max-results-bare-rows
"Maximum number of rows to return specifically on :rows type queries via the API."
......@@ -364,3 +367,8 @@
{:style/indent 1}
[query, options :- mbql.s/Info]
(process-query-and-save-execution! (assoc query :constraints default-query-constraints) options))
(s/defn process-query-without-save!
"Invokes `process-query` with info needed for the included remark."
[user query]
(process-query (assoc-query-info query {:executed-by user})))
......@@ -390,6 +390,45 @@
;; now check the correct metadata was fetched and was saved in the DB
(db/select-one-field :result_metadata Card :name card-name))))))
;; Check that the generated query to fetch the query result metadata includes user information in the generated query
(expect
{:metadata-results [{:base_type "type/Integer"
:display_name "count"
:name "count"
:special_type "type/Quantity"
:fingerprint {:global {:distinct-count 1
:nil% 0.0},
:type {:type/Number {:min 100.0, :max 100.0, :avg 100.0, :q1 100.0, :q3 100.0 :sd nil}}}}]
:has-user-id-remark? true}
(tu/with-non-admin-groups-no-root-collection-perms
(let [metadata [{:base_type :type/Integer
:display_name "Count Chocula"
:name "count_chocula"
:special_type :type/Quantity}]
card-name (tu/random-name)]
(tt/with-temp Collection [collection]
(perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
(tu/with-model-cleanup [Card]
;; Rebind the `cancellable-run-query` function so that we can capture the generated SQL and inspect it
(let [orig-fn (var-get #'metabase.driver.generic-sql.query-processor/cancellable-run-query)
sql-result (atom [])]
(with-redefs [metabase.driver.generic-sql.query-processor/cancellable-run-query (fn [db sql params opts]
(swap! sql-result conj sql)
(orig-fn db sql params opts))]
;; create a card with the metadata
((user->client :rasta) :post 200 "card"
(assoc (card-with-name-and-query card-name)
:collection_id (u/get-id collection)
:result_metadata metadata
:metadata_checksum "ABCDEF"))) ; bad checksum
;; now check the correct metadata was fetched and was saved in the DB
{:metadata-results (db/select-one-field :result_metadata Card :name card-name)
;; Was the user id found in the generated SQL?
:has-user-id-remark? (-> (str "userID: " (user->id :rasta))
re-pattern
(re-find (first @sql-result))
boolean)}))))))
;; Make sure we can create a Card with a Collection position
(expect
#metabase.models.card.CardInstance{:collection_id true, :collection_position 1}
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment