From 86f4e6327ca9c90f12dc155dfc5186156c65b8f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cam=20Sa=C3=BCl?= <cammsaul@gmail.com>
Date: Tue, 10 Jan 2017 12:42:39 -0800
Subject: [PATCH] Code cleanup :shower:
---
src/metabase/api/card.clj | 11 ++-
src/metabase/api/common.clj | 9 +-
src/metabase/api/dataset.clj | 6 +-
src/metabase/public_settings.clj | 26 ++---
src/metabase/query_processor.clj | 154 ++++++++++++++++-------------
test/metabase/api/dataset_test.clj | 8 +-
6 files changed, 119 insertions(+), 95 deletions(-)
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 02ba3e9dfa2..2d3c1582a9c 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -324,7 +324,10 @@
;;; ------------------------------------------------------------ Running a Query ------------------------------------------------------------
-(defn- run-query-for-card [card-id parameters constraints]
+(defn run-query-for-card
+ "Run the query for Card with PARAMETERS and CONSTRAINTS, and return results in the usual format."
+ [card-id & {:keys [parameters constraints]
+ :or {constraints dataset-api/default-query-constraints}}]
{:pre [(u/maybe? sequential? parameters)]}
(let [card (read-check Card card-id)
query (assoc (:dataset_query card)
@@ -337,19 +340,19 @@
(defendpoint POST "/:card-id/query"
"Run the query associated with a Card."
[card-id :as {{:keys [parameters]} :body}]
- (run-query-for-card card-id parameters dataset-api/query-constraints))
+ (run-query-for-card card-id, :parameters parameters))
(defendpoint POST "/:card-id/query/csv"
"Run the query associated with a Card, and return its results as CSV. Note that this expects the parameters as serialized JSON in the 'parameters' parameter"
[card-id parameters]
{parameters (s/maybe su/JSONString)}
- (dataset-api/as-csv (run-query-for-card card-id (json/parse-string parameters keyword) nil)))
+ (dataset-api/as-csv (run-query-for-card card-id, :parameters (json/parse-string parameters keyword), :constraints nil)))
(defendpoint POST "/:card-id/query/json"
"Run the query associated with a Card, and return its results as JSON. Note that this expects the parameters as serialized JSON in the 'parameters' parameter"
[card-id parameters]
{parameters (s/maybe su/JSONString)}
- (dataset-api/as-json (run-query-for-card card-id (json/parse-string parameters keyword) nil)))
+ (dataset-api/as-json (run-query-for-card card-id, :parameters (json/parse-string parameters keyword), :constraints nil)))
(define-routes)
diff --git a/src/metabase/api/common.clj b/src/metabase/api/common.clj
index c6597276982..9ece0a03712 100644
--- a/src/metabase/api/common.clj
+++ b/src/metabase/api/common.clj
@@ -64,9 +64,11 @@
(recur (first rest-args) (second rest-args) (drop 2 rest-args))))))
(defn check-exists?
- "Check that object with ID exists in the DB, or throw a 404."
- [entity id]
- (check-404 (db/exists? entity, :id id)))
+ "Check that object with ID (or other key/values) exists in the DB, or throw a 404."
+ ([entity id]
+ (check-exists? entity :id id))
+ ([entity k v & more]
+ (check-404 (apply db/exists? entity k v more))))
(defn check-superuser
"Check that `*current-user*` is a superuser or throw a 403. This doesn't require a DB call."
@@ -210,6 +212,7 @@
;;; ------------------------------------------------------------ DEFENDPOINT AND RELATED FUNCTIONS ------------------------------------------------------------
+;; TODO - several of the things `defendpoint` does could and should just be done by custom Ring middleware instead
(defmacro defendpoint
"Define an API function.
This automatically does several things:
diff --git a/src/metabase/api/dataset.clj b/src/metabase/api/dataset.clj
index 9fc602ee412..bed6de0afbe 100644
--- a/src/metabase/api/dataset.clj
+++ b/src/metabase/api/dataset.clj
@@ -21,7 +21,7 @@
"General maximum number of rows to return from an API query."
10000)
-(def ^:const query-constraints
+(def ^:const default-query-constraints
"Default map of constraints that we apply on dataset queries executed by the api."
{:max-results max-results
:max-results-bare-rows max-results-bare-rows})
@@ -31,7 +31,7 @@
[:as {{:keys [database] :as body} :body}]
(read-check Database database)
;; add sensible constraints for results limits on our query
- (let [query (assoc body :constraints query-constraints)]
+ (let [query (assoc body :constraints default-query-constraints)]
(qp/dataset-query query {:executed-by *current-user-id*})))
(defendpoint POST "/duration"
@@ -39,7 +39,7 @@
[:as {{:keys [database] :as query} :body}]
(read-check Database database)
;; add sensible constraints for results limits on our query
- (let [query (assoc query :constraints query-constraints)
+ (let [query (assoc query :constraints default-query-constraints)
running-times (db/select-field :running_time QueryExecution
:query_hash (hash query)
{:order-by [[:started_at :desc]]
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index afc5bc5b03e..c176840ef48 100644
--- a/src/metabase/public_settings.clj
+++ b/src/metabase/public_settings.clj
@@ -68,20 +68,20 @@
(defn public-settings
"Return a simple map of key/value pairs which represent the public settings (`MetabaseBootstrap`) for the front-end application."
[]
- {:ga_code "UA-60817802-1"
- :password_complexity password/active-password-complexity
- :timezones common/timezones
- :version config/mb-version-info
- :engines ((resolve 'metabase.driver/available-drivers))
- :setup_token ((resolve 'metabase.setup/token-value))
+ {:admin_email (admin-email)
:anon_tracking_enabled (anon-tracking-enabled)
- :site_name (site-name)
+ :custom_geojson (setting/get :custom-geojson)
:email_configured ((resolve 'metabase.email/email-configured?))
- :admin_email (admin-email)
- :report_timezone (setting/get :report-timezone)
- :timezone_short (short-timezone-name (setting/get :report-timezone))
- :has_sample_dataset (db/exists? 'Database, :is_sample true)
+ :engines ((resolve 'metabase.driver/available-drivers))
+ :ga_code "UA-60817802-1"
:google_auth_client_id (setting/get :google-auth-client-id)
+ :has_sample_dataset (db/exists? 'Database, :is_sample true)
:map_tile_server_url (map-tile-server-url)
- :custom_geojson (setting/get :custom-geojson)
- :types (types/types->parents)})
+ :password_complexity password/active-password-complexity
+ :report_timezone (setting/get :report-timezone)
+ :setup_token ((resolve 'metabase.setup/token-value))
+ :site_name (site-name)
+ :timezone_short (short-timezone-name (setting/get :report-timezone))
+ :timezones common/timezones
+ :types (types/types->parents)
+ :version config/mb-version-info})
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 3d2e62ce547..531c161fb3d 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -508,65 +508,17 @@
;;; | DATASET-QUERY PUBLIC API |
;;; +----------------------------------------------------------------------------------------------------+
-(declare query-fail query-complete save-query-execution!)
-
-(defn- assert-valid-query-result [query-result]
- (when-not (contains? query-result :status)
- (throw (Exception. "invalid response from database driver. no :status provided")))
- (when (= :failed (:status query-result))
- (log/error (u/pprint-to-str 'red query-result))
- (throw (Exception. (str (get query-result :error "general error"))))))
-
-(defn dataset-query
- "Process and run a json based dataset query and return results.
-
- Takes 2 arguments:
-
- 1. the json query as a dictionary
- 2. query execution options specified in a dictionary
-
- Depending on the database specified in the query this function will delegate to a driver specific implementation.
- For the purposes of tracking we record each call to this function as a QueryExecution in the database.
-
- Possible caller-options include:
-
- :executed-by [int] (User ID of caller)
- :card-id [int] (ID of Card associated with this execution)"
- {:arglists '([query options])}
- [query {:keys [executed-by card-id]}]
- {:pre [(integer? executed-by) (u/maybe? integer? card-id)]}
- (let [query-uuid (str (java.util.UUID/randomUUID))
- query-hash (hash query)
- query-execution {:uuid query-uuid
- :executor_id executed-by
- :json_query query
- :query_hash query-hash
- :version 0
- :status :starting
- :error ""
- :started_at (u/new-sql-timestamp)
- :finished_at (u/new-sql-timestamp)
- :running_time 0
- :result_rows 0
- :result_file ""
- :result_data "{}"
- :raw_query ""
- :additional_info ""
- :start_time_millis (System/currentTimeMillis)}
- query (assoc query :info {:executed-by executed-by
- :card-id card-id
- :uuid query-uuid
- :query-hash query-hash
- :query-type (if (mbql-query? query) "MBQL" "native")})]
- (try
- (let [result (process-query query)]
- (assert-valid-query-result result)
- (query-complete query-execution result))
- (catch Throwable e
- (log/error (u/format-color 'red "Query failure: %s\n%s" (.getMessage e) (u/pprint-to-str (u/filtered-stacktrace e))))
- (query-fail query-execution (.getMessage e))))))
+(defn- save-query-execution!
+ "Save (or update) a `QueryExecution`."
+ [{:keys [id], :as query-execution}]
+ (if id
+ ;; execution has already been saved, so update it
+ (u/prog1 query-execution
+ (db/update! QueryExecution id query-execution))
+ ;; first time saving execution, so insert it
+ (db/insert! QueryExecution query-execution)))
-(defn- query-fail
+(defn- save-and-return-failed-query!
"Save QueryExecution state and construct a failed query response"
[query-execution error-message]
(let [updates {:status :failed
@@ -586,7 +538,7 @@
:cols []
:columns []}))))
-(defn- query-complete
+(defn- save-and-return-successful-query!
"Save QueryExecution state and construct a completed (successful) query response"
[query-execution query-result]
;; record our query execution and format response
@@ -602,12 +554,78 @@
(dissoc :error :raw_query :result_rows :version)
(merge query-result)))
-(defn- save-query-execution!
- "Save (or update) a `QueryExecution`."
- [{:keys [id], :as query-execution}]
- (if id
- ;; execution has already been saved, so update it
- (u/prog1 query-execution
- (db/update! QueryExecution id query-execution))
- ;; first time saving execution, so insert it
- (db/insert! QueryExecution query-execution)))
+
+(defn- assert-query-status-successful
+ "Make sure QUERY-RESULT `:status` is something other than `nil`or `:failed`, or throw an Exception."
+ [query-result]
+ (when-not (contains? query-result :status)
+ (throw (Exception. "invalid response from database driver. no :status provided")))
+ (when (= :failed (:status query-result))
+ (log/error (u/pprint-to-str 'red query-result))
+ (throw (Exception. (str (get query-result :error "general error"))))))
+
+(def ^:dynamic ^Boolean *allow-queries-with-no-executor-id*
+ "Should we allow running queries (via `dataset-query`) without specifying the `executed-by` User ID?
+ By default this is `false`, but this constraint can be disabled for running queries not executed by a specific user
+ (e.g., public Cards)."
+ false)
+
+(defn- query-execution-info
+ "Return the info for the `QueryExecution` entry for this QUERY."
+ [{{:keys [uuid executed-by query-hash]} :info, :as query}]
+ {:uuid (or uuid (throw (Exception. "Missing query UUID!")))
+ :executor_id executed-by
+ :json_query (dissoc query :info)
+ :query_hash (or query-hash (throw (Exception. "Missing query hash!")))
+ :version 0
+ :error ""
+ :started_at (u/new-sql-timestamp)
+ :finished_at (u/new-sql-timestamp)
+ :running_time 0
+ :result_rows 0
+ :result_file ""
+ :result_data "{}"
+ :raw_query ""
+ :additional_info ""
+ :start_time_millis (System/currentTimeMillis)})
+
+(defn- run-and-save-query!
+ "Run QUERY and save appropriate `QueryExecution` info, and then return results (or an error message) in the usual format."
+ [query]
+ (let [query-execution (query-execution-info query)]
+ (try
+ (let [result (process-query query)]
+ (assert-query-status-successful result)
+ (save-and-return-successful-query! query-execution result))
+ (catch Throwable e
+ (log/error (u/format-color 'red "Query failure: %s\n%s" (.getMessage e) (u/pprint-to-str (u/filtered-stacktrace e))))
+ (save-and-return-failed-query! query-execution (.getMessage e))))))
+
+(defn dataset-query
+ "Process and run a json based dataset query and return results.
+
+ Takes 2 arguments:
+
+ 1. the json query as a dictionary
+ 2. query execution options specified in a dictionary
+
+ Depending on the database specified in the query this function will delegate to a driver specific implementation.
+ For the purposes of tracking we record each call to this function as a QueryExecution in the database.
+
+ Possible caller-options include:
+
+ :executed-by [int] (User ID of caller)
+ :card-id [int] (ID of Card associated with this execution)"
+ {:arglists '([query options])}
+ [query {:keys [executed-by card-id]}]
+ {:pre [(or (integer? executed-by)
+ *allow-queries-with-no-executor-id*)
+ (u/maybe? integer? card-id)]}
+ (let [query-uuid (str (java.util.UUID/randomUUID))
+ query-hash (hash query)
+ query (assoc query :info {:executed-by executed-by
+ :card-id card-id
+ :uuid query-uuid
+ :query-hash query-hash
+ :query-type (if (mbql-query? query) "MBQL" "native")})]
+ (run-and-save-query! query)))
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index c6f2459457d..36221ffa2fe 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -2,7 +2,7 @@
"Unit tests for /api/dataset endpoints."
(:require [clojure.string :as s]
[expectations :refer :all]
- [metabase.api.dataset :refer [query-constraints]]
+ [metabase.api.dataset :refer [default-query-constraints]]
[metabase.db :as db]
(metabase.models [card :refer [Card]]
[query-execution :refer [QueryExecution]])
@@ -65,7 +65,7 @@
(ql/aggregation (ql/count))))
(assoc :type "query")
(assoc-in [:query :aggregation] [{:aggregation-type "count", :custom-name nil}])
- (assoc :constraints query-constraints))
+ (assoc :constraints default-query-constraints))
:started_at true
:finished_at true
:running_time true}
@@ -81,7 +81,7 @@
(ql/aggregation (ql/count))))
(assoc :type "query")
(assoc-in [:query :aggregation] [{:aggregation-type "count", :custom-name nil}])
- (assoc :constraints query-constraints))
+ (assoc :constraints default-query-constraints))
:started_at true
:finished_at true
:running_time true
@@ -108,7 +108,7 @@
:json_query {:database (id)
:type "native"
:native {:query "foobar"}
- :constraints query-constraints}
+ :constraints default-query-constraints}
:started_at true
:finished_at true
:running_time true}]
--
GitLab