From 1a611340418d98feaadc0a070b118af3ae0ae208 Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Wed, 6 Jun 2018 16:03:37 -0700
Subject: [PATCH] Code cleanup etc.
---
src/metabase/api/common.clj | 68 +++++++++++++------
src/metabase/api/common/internal.clj | 62 +----------------
src/metabase/api/field.clj | 4 +-
src/metabase/api/permissions.clj | 8 ++-
src/metabase/api/user.clj | 7 +-
src/metabase/core.clj | 6 ++
src/metabase/middleware.clj | 56 ++++++++++++++-
src/metabase/models/card.clj | 3 +-
src/metabase/models/interface.clj | 20 ++++--
src/metabase/models/user.clj | 2 +-
src/metabase/plugins.clj | 24 +++++++
src/metabase/query_processor.clj | 18 ++---
.../middleware/add_implicit_clauses.clj | 17 ++---
.../query_processor/middleware/cache.clj | 30 ++++----
.../query_processor/middleware/resolve.clj | 37 +++++-----
.../middleware/source_table.clj | 27 ++++----
test/metabase/api/common_test.clj | 51 ++++++++------
test/metabase/api/dataset_test.clj | 3 +-
test/metabase/models/card_test.clj | 3 +-
.../middleware/permissions_test.clj | 4 +-
test/metabase/test/data/users.clj | 12 +++-
test/metabase/test_setup.clj | 3 +-
22 files changed, 278 insertions(+), 187 deletions(-)
diff --git a/src/metabase/api/common.clj b/src/metabase/api/common.clj
index 6b6391fb694..f5fa5bb9c50 100644
--- a/src/metabase/api/common.clj
+++ b/src/metabase/api/common.clj
@@ -6,7 +6,7 @@
[clojure.java.io :as io]
[clojure.string :as str]
[clojure.tools.logging :as log]
- [compojure.core :refer [defroutes]]
+ [compojure.core :as compojure]
[medley.core :as m]
[metabase
[public-settings :as public-settings]
@@ -227,6 +227,11 @@
[& body]
`(api-let ~generic-403 ~@body))
+(defn throw-403
+ "Throw a generic 403 (no permissions) error response."
+ []
+ (throw (ex-info (tru "You don''t have permissions to do that.") {:status-code 403})))
+
;; #### GENERIC 500 RESPONSE HELPERS
;; For when you don't feel like writing something useful
(def ^:private generic-500
@@ -251,7 +256,7 @@
;;; --------------------------------------- DEFENDPOINT AND RELATED FUNCTIONS ----------------------------------------
;; TODO - several of the things `defendpoint` does could and should just be done by custom Ring middleware instead
-;; e.g. `catch-api-exceptions` and `auto-parse`
+;; e.g. `auto-parse`
(defmacro defendpoint
"Define an API function.
This automatically does several things:
@@ -283,25 +288,50 @@
:doc (route-dox method route docstr args (m/map-vals eval arg->schema) body)
:is-endpoint? true)
(~method ~route ~args
- (catch-api-exceptions
- (auto-parse ~args
- ~@validate-param-calls
- (wrap-response-if-needed (do ~@body))))))))
-
+ (auto-parse ~args
+ ~@validate-param-calls
+ (wrap-response-if-needed (do ~@body)))))))
+
+(defn- namespace->api-route-fns
+ "Return a sequence of all API endpoint functions defined by `defendpoint` in a namespace."
+ [nmspace]
+ (for [[symb varr] (ns-publics nmspace)
+ :when (:is-endpoint? (meta varr))]
+ symb))
+
+(defn- api-routes-docstring [nmspace route-fns middleware]
+ (str
+ (format "Ring routes for %s:\n%s"
+ (-> (ns-name nmspace)
+ (str/replace #"^metabase\." "")
+ (str/replace #"\." "/"))
+ (u/pprint-to-str route-fns))
+ (when (seq middleware)
+ (str "\nMiddleware applied to all endpoints in this namespace:\n"
+ (u/pprint-to-str middleware)))))
(defmacro define-routes
- "Create a `(defroutes routes ...)` form that automatically includes all functions created with
- `defendpoint` in the current namespace."
- [& additional-routes]
- (let [api-routes (for [[symb varr] (ns-publics *ns*)
- :when (:is-endpoint? (meta varr))]
- symb)]
- `(defroutes ~(vary-meta 'routes assoc :doc (format "Ring routes for %s:\n%s"
- (-> (ns-name *ns*)
- (str/replace #"^metabase\." "")
- (str/replace #"\." "/"))
- (u/pprint-to-str (concat api-routes additional-routes))))
- ~@additional-routes ~@api-routes)))
+ "Create a `(defroutes routes ...)` form that automatically includes all functions created with `defendpoint` in the
+ current namespace. Optionally specify middleware that will apply to all of the endpoints in the current namespace.
+
+ (api/define-routes api/+check-superuser) ; all API endpoints in this namespace will require superuser access"
+ {:style/indent 0}
+ [& middleware]
+ (let [api-route-fns (namespace->api-route-fns *ns*)
+ routes `(compojure/routes ~@api-route-fns)]
+ `(def ~(vary-meta 'routes assoc :doc (api-routes-docstring *ns* api-route-fns middleware))
+ ~(if (seq middleware)
+ `(-> ~routes ~@middleware)
+ routes))))
+
+(defn +check-superuser
+ "Wrap a Ring handler to make sure the current user is a superuser before handling any requests.
+
+ (api/+check-superuser routes)"
+ [handler]
+ (fn [request]
+ (check-superuser)
+ (handler request)))
;;; ---------------------------------------- PERMISSIONS CHECKING HELPER FNS -----------------------------------------
diff --git a/src/metabase/api/common/internal.clj b/src/metabase/api/common/internal.clj
index 5862498c567..b3b5125437c 100644
--- a/src/metabase/api/common/internal.clj
+++ b/src/metabase/api/common/internal.clj
@@ -1,15 +1,13 @@
(ns metabase.api.common.internal
"Internal functions used by `metabase.api.common`.
These are primarily used as the internal implementation of `defendpoint`."
- (:require [clojure.java.jdbc :as jdbc]
- [clojure.string :as str]
+ (:require [clojure.string :as str]
[clojure.tools.logging :as log]
[medley.core :as m]
[metabase.util :as u]
[metabase.util.schema :as su]
[puppetlabs.i18n.core :refer [trs tru]]
- [schema.core :as s])
- (:import java.sql.SQLException))
+ [schema.core :as s]))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | DOCSTRING GENERATION |
@@ -206,62 +204,6 @@
~@body)))
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | EXCEPTION HANDLING |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-;; TODO - this SHOULD all be implemented as middleware instead
-(defn- api-exception-response
- "Convert an exception from an API endpoint into an appropriate HTTP response."
- [^Throwable e]
- (let [{:keys [status-code], :as info} (ex-data e)
- other-info (dissoc info :status-code)
- message (.getMessage e)]
- {:status (or status-code 500)
- :body (cond
- ;; Exceptions that include a status code *and* other info are things like Field validation exceptions.
- ;; Return those as is
- (and status-code
- (seq other-info))
- other-info
- ;; If status code was specified but other data wasn't, it's something like a 404. Return message as the
- ;; body.
- status-code
- message
- ;; Otherwise it's a 500. Return a body that includes exception & filtered stacktrace for debugging
- ;; purposes
- :else
- (let [stacktrace (u/filtered-stacktrace e)]
- (merge (assoc other-info
- :message message
- :type (class e)
- :stacktrace stacktrace)
- (when (instance? SQLException e)
- {:sql-exception-chain (str/split (with-out-str (jdbc/print-sql-exception-chain e))
- #"\s*\n\s*")}))))}))
-
-(def ^:dynamic ^Boolean *automatically-catch-api-exceptions*
- "Should API exceptions automatically be caught? By default, this is `true`, but this can be disabled when we want to
- catch Exceptions and return something generic to avoid leaking information, e.g. with the `api/public` and
- `api/embed` endpoints. generic exceptions"
- true)
-
-(defn do-with-caught-api-exceptions
- "Execute F with and catch any exceptions, converting them to the appropriate HTTP response."
- [f]
- (if-not *automatically-catch-api-exceptions*
- (f)
- (try (f)
- (catch Throwable e
- (api-exception-response e)))))
-
-(defmacro catch-api-exceptions
- "Execute BODY, and if an exception is thrown, return the appropriate HTTP response."
- {:style/indent 0}
- [& body]
- `(do-with-caught-api-exceptions (fn [] ~@body)))
-
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PARAM VALIDATION |
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index a1ea3eec15b..c3409d53635 100644
--- a/src/metabase/api/field.clj
+++ b/src/metabase/api/field.clj
@@ -164,8 +164,8 @@
{:values [], :field_id (:id field)}))
(api/defendpoint GET "/:id/values"
- "If `Field`'s special type derives from `type/Category`, or its base type is `type/Boolean`, return all distinct
- values of the field, and a map of human-readable values defined by the user."
+ "If a Field's value of `has_field_values` is `list`, return a list of all the distinct values of the Field, and (if
+ defined by a User) a map of human-readable remapped values."
[id]
(field->values (api/read-check Field id)))
diff --git a/src/metabase/api/permissions.clj b/src/metabase/api/permissions.clj
index 063c3cdbbde..ea52c7da685 100644
--- a/src/metabase/api/permissions.clj
+++ b/src/metabase/api/permissions.clj
@@ -22,11 +22,17 @@
(defn- ->int [id] (Integer/parseInt (name id)))
+(defn- dejsonify-table-perms [table-perms]
+ (if-not (map? table-perms)
+ (keyword table-perms)
+ (into {} (for [[k v] table-perms]
+ [(keyword k) (keyword v)]))))
+
(defn- dejsonify-tables [tables]
(if (string? tables)
(keyword tables)
(into {} (for [[table-id perms] tables]
- {(->int table-id) (keyword perms)}))))
+ {(->int table-id) (dejsonify-table-perms perms)}))))
(defn- dejsonify-schemas [schemas]
(if (string? schemas)
diff --git a/src/metabase/api/user.clj b/src/metabase/api/user.clj
index dd9c083c884..86d4a5436f0 100644
--- a/src/metabase/api/user.clj
+++ b/src/metabase/api/user.clj
@@ -1,4 +1,5 @@
(ns metabase.api.user
+ "/api/user endpoints"
(:require [cemerick.friend.credentials :as creds]
[compojure.core :refer [DELETE GET POST PUT]]
[metabase.api
@@ -78,7 +79,6 @@
(check-self-or-superuser id)
(api/check-404 (fetch-user :id id, :is_active true)))
-
(api/defendpoint PUT "/:id"
"Update an existing, active `User`."
[id :as {{:keys [email first_name last_name is_superuser login_attributes] :as body} :body}]
@@ -95,9 +95,10 @@
(api/check-500
(db/update! User id
(u/select-keys-when body
- :present #{:login_attributes}
+ :present (when api/*is-superuser?*
+ #{:login_attributes})
:non-nil (set (concat [:first_name :last_name :email]
- (when (:is_superuser @api/*current-user*)
+ (when api/*is-superuser?*
[:is_superuser]))))))
(fetch-user :id id))
diff --git a/src/metabase/core.clj b/src/metabase/core.clj
index fb0a84479c6..dcaa9cc47ed 100644
--- a/src/metabase/core.clj
+++ b/src/metabase/core.clj
@@ -86,7 +86,9 @@
(def ^:private app
"The primary entry point to the Ring HTTP server."
+ ;; ▼▼▼ POST-PROCESSING ▼▼▼ happens from TOP-TO-BOTTOM
(-> #'routes/routes ; the #' is to allow tests to redefine endpoints
+ mb-middleware/catch-api-exceptions ; catch exceptions and return them in our expected format
(mb-middleware/log-api-call
jetty-stats)
mb-middleware/add-security-headers ; Add HTTP headers to API responses to prevent them from being cached
@@ -104,6 +106,7 @@
wrap-cookies ; Parses cookies in the request map and assocs as :cookies
wrap-session ; reads in current HTTP session and sets :session/key
wrap-gzip)) ; GZIP response if client can handle it
+;; ▲▲▲ PRE-PROCESSING ▲▲▲ happens from BOTTOM-TO-TOP
;;; --------------------------------------------------- Lifecycle ----------------------------------------------------
@@ -131,6 +134,7 @@
(task/stop-scheduler!)
(log/info (trs "Metabase Shutdown COMPLETE")))
+
(defn init!
"General application initialization function which should be run once at application startup."
[]
@@ -145,6 +149,8 @@
;; load any plugins as needed
(plugins/load-plugins!)
(init-status/set-progress! 0.3)
+ (plugins/setup-plugins!)
+ (init-status/set-progress! 0.35)
;; Load up all of our Database drivers, which are used for app db work
(driver/find-and-load-drivers!)
diff --git a/src/metabase/middleware.clj b/src/metabase/middleware.clj
index 0debf25c5d1..54c7150d565 100644
--- a/src/metabase/middleware.clj
+++ b/src/metabase/middleware.clj
@@ -1,6 +1,8 @@
(ns metabase.middleware
"Metabase-specific middleware functions & configuration."
(:require [cheshire.generate :refer [add-encoder encode-nil encode-str]]
+ [clojure.java.jdbc :as jdbc]
+ [clojure.string :as str]
[clojure.tools.logging :as log]
[metabase
[config :as config]
@@ -8,7 +10,6 @@
[public-settings :as public-settings]
[util :as u]]
[metabase.api.common :refer [*current-user* *current-user-id* *current-user-permissions-set* *is-superuser?*]]
- [metabase.api.common.internal :refer [*automatically-catch-api-exceptions*]]
[metabase.core.initialization-status :as init-status]
[metabase.models
[session :refer [Session]]
@@ -17,7 +18,8 @@
[metabase.util.date :as du]
[puppetlabs.i18n.core :refer [tru]]
[toucan.db :as db])
- (:import com.fasterxml.jackson.core.JsonGenerator))
+ (:import com.fasterxml.jackson.core.JsonGenerator
+ java.sql.SQLException))
;;; ---------------------------------------------------- UTIL FNS ----------------------------------------------------
@@ -117,6 +119,8 @@
(def ^:private current-user-fields
(vec (cons User user/all-user-fields)))
+(defn- find-user [user-id]
+ (db/select-one current-user-fields, :id user-id))
(defn bind-current-user
"Middleware that binds `metabase.api.common/*current-user*`, `*current-user-id*`, `*is-superuser?*`, and
@@ -131,7 +135,7 @@
(if-let [current-user-id (:metabase-user-id request)]
(binding [*current-user-id* current-user-id
*is-superuser?* (:is-superuser? request)
- *current-user* (delay (db/select-one current-user-fields, :id current-user-id))
+ *current-user* (delay (find-user current-user-id))
*current-user-permissions-set* (delay (user/permissions-set current-user-id))]
(handler request))
(handler request))))
@@ -349,6 +353,12 @@
;;; ----------------------------------------------- EXCEPTION HANDLING -----------------------------------------------
+(def ^:dynamic ^:private ^Boolean *automatically-catch-api-exceptions*
+ "Should API exceptions automatically be caught? By default, this is `true`, but this can be disabled when we want to
+ catch Exceptions and return something generic to avoid leaking information, e.g. with the `api/public` and
+ `api/embed` endpoints. generic exceptions"
+ true)
+
(defn genericize-exceptions
"Catch any exceptions thrown in the request handler body and rethrow a generic 400 exception instead. This minimizes
information available to bad actors when exceptions occur on public endpoints."
@@ -370,3 +380,43 @@
(handler request))
(catch Throwable e
{:status 400, :body (.getMessage e)}))))
+
+(defn- api-exception-response
+ "Convert an exception from an API endpoint into an appropriate HTTP response."
+ [^Throwable e]
+ (let [{:keys [status-code], :as info} (ex-data e)
+ other-info (dissoc info :status-code)
+ message (.getMessage e)]
+ {:status (or status-code 500)
+ :body (cond
+ ;; Exceptions that include a status code *and* other info are things like Field validation exceptions.
+ ;; Return those as is
+ (and status-code
+ (seq other-info))
+ other-info
+ ;; If status code was specified but other data wasn't, it's something like a 404. Return message as the
+ ;; body.
+ status-code
+ message
+ ;; Otherwise it's a 500. Return a body that includes exception & filtered stacktrace for debugging
+ ;; purposes
+ :else
+ (let [stacktrace (u/filtered-stacktrace e)]
+ (merge (assoc other-info
+ :message message
+ :type (class e)
+ :stacktrace stacktrace)
+ (when (instance? SQLException e)
+ {:sql-exception-chain (str/split (with-out-str (jdbc/print-sql-exception-chain e))
+ #"\s*\n\s*")}))))}))
+
+(defn catch-api-exceptions
+ "Middleware that catches API Exceptions and returns them in our normal-style format rather than the Jetty 500
+ Stacktrace page, which is not so useful for our frontend."
+ [handler]
+ (fn [request]
+ (if *automatically-catch-api-exceptions*
+ (try (handler request)
+ (catch Throwable e
+ (api-exception-response e)))
+ (handler request))))
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index befccd05a4b..dc2bbe1f138 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -16,8 +16,8 @@
[query :as query]
[revision :as revision]]
[metabase.models.query.permissions :as query-perms]
- [metabase.util.query :as q]
[metabase.query-processor.util :as qputil]
+ [metabase.util.query :as q]
[puppetlabs.i18n.core :refer [tru]]
[toucan
[db :as db]
@@ -33,7 +33,6 @@
[{:keys [id]}]
(db/count 'DashboardCard, :card_id id))
-
;;; -------------------------------------------------- Dependencies --------------------------------------------------
(defn card-dependencies
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index db7adec1dfc..d8ebb1d2383 100644
--- a/src/metabase/models/interface.clj
+++ b/src/metabase/models/interface.clj
@@ -27,28 +27,38 @@
obj
(json/generate-string obj)))
-(defn- json-out [obj]
+(defn- json-out [obj keywordize-keys?]
(let [s (u/jdbc-clob->str obj)]
(if (string? s)
- (json/parse-string s keyword)
+ (json/parse-string s keywordize-keys?)
obj)))
+(defn- json-out-with-keywordization [obj]
+ (json-out obj true))
+
+(defn- json-out-without-keywordization [obj]
+ (json-out obj false))
+
(models/add-type! :json
:in json-in
- :out json-out)
+ :out json-out-with-keywordization)
+
+(models/add-type! :json-no-keywordization
+ :in json-in
+ :out json-out-without-keywordization)
;; json-set is just like json but calls `set` on it when coming out of the DB. Intended for storing things like a
;; permissions set
(models/add-type! :json-set
:in json-in
- :out #(when % (set (json-out %))))
+ :out #(when % (set (json-out-with-keywordization %))))
(models/add-type! :clob
:in identity
:out u/jdbc-clob->str)
(def ^:private encrypted-json-in (comp encryption/maybe-encrypt json-in))
-(def ^:private encrypted-json-out (comp json-out encryption/maybe-decrypt))
+(def ^:private encrypted-json-out (comp json-out-with-keywordization encryption/maybe-decrypt))
;; cache the decryption/JSON parsing because it's somewhat slow (~500µs vs ~100µs on a *fast* computer)
;; cache the decrypted JSON for one hour
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index 50e9076814c..b25deb3d125 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -118,7 +118,7 @@
:pre-update pre-update
:post-select post-select
:pre-delete pre-delete
- :types (constantly {:login_attributes :json})}))
+ :types (constantly {:login_attributes :json-no-keywordization})}))
;;; --------------------------------------------------- Helper Fns ---------------------------------------------------
diff --git a/src/metabase/plugins.clj b/src/metabase/plugins.clj
index 3ce3d4e67f5..0ee78b8d8fd 100644
--- a/src/metabase/plugins.clj
+++ b/src/metabase/plugins.clj
@@ -77,3 +77,27 @@
(if (u/is-java-9-or-higher?)
(show-java-9-message)
(dynamically-add-jars!)))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Running Plugin Setup Fns |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn setup-plugins!
+ "Look for any namespaces on the classpath with the pattern `metabase.*plugin-setup` For each matching namespace, load
+ it. If the namespace has a function named `-init-plugin!`, call that function with no arguments.
+
+ This is intended as a startup hook for Metabase plugins to run any startup logic that needs to be done. This
+ function is normally called once in Metabase startup, after `load-plugins!` runs above (which simply adds JARs to
+ the classpath in the `plugins` directory.)"
+ []
+ ;; find each namespace ending in `plugin-setup`
+ (doseq [ns-symb @u/metabase-namespace-symbols
+ :when (re-find #"plugin-setup$" (name ns-symb))]
+ ;; load the matching ns
+ (log/info (u/format-color 'magenta "Loading plugin setup namespace %s... %s" (name ns-symb) (u/emoji "🔌")))
+ (require ns-symb)
+ ;; now look for a fn in that namespace called `-init-plugin!`. If it exists, call it
+ (when-let [init-fn-var (ns-resolve ns-symb '-init-plugin!)]
+ (log/info (u/format-color 'magenta "Running plugin init fn %s/-init-plugin!... %s" (name ns-symb) (u/emoji "🔌")))
+ (init-fn-var))))
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 6f65bcdb304..a49bb992b78 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -14,8 +14,8 @@
[add-row-count-and-status :as row-count-and-status]
[add-settings :as add-settings]
[annotate-and-sort :as annotate-and-sort]
- [binning :as binning]
[bind-effective-timezone :as bind-timezone]
+ [binning :as binning]
[cache :as cache]
[catch-exceptions :as catch-exceptions]
[cumulative-aggregations :as cumulative-ags]
@@ -30,9 +30,9 @@
[mbql-to-native :as mbql-to-native]
[parameters :as parameters]
[permissions :as perms]
- [results-metadata :as results-metadata]
- [resolve-driver :as resolve-driver]
[resolve :as resolve]
+ [resolve-driver :as resolve-driver]
+ [results-metadata :as results-metadata]
[source-table :as source-table]]
[metabase.query-processor.util :as qputil]
[metabase.util
@@ -41,9 +41,9 @@
[schema.core :as s]
[toucan.db :as db]))
-;;; +-------------------------------------------------------------------------------------------------------+
-;;; | QUERY PROCESSOR |
-;;; +-------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | QUERY PROCESSOR |
+;;; +----------------------------------------------------------------------------------------------------------------+
(defn- execute-query
"The pivotal stage of the `process-query` pipeline where the query is actually executed by the driver's Query
@@ -151,9 +151,9 @@
;; instead of running it
-;;; +----------------------------------------------------------------------------------------------------+
-;;; | DATASET-QUERY PUBLIC API |
-;;; +----------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | DATASET-QUERY PUBLIC API |
+;;; +----------------------------------------------------------------------------------------------------------------+
;; The only difference between `process-query` and `process-query-and-save-execution!` (below) is that the
;; latter records a `QueryExecution` (inserts a new row) recording some stats about this Query run including
diff --git a/src/metabase/query_processor/middleware/add_implicit_clauses.clj b/src/metabase/query_processor/middleware/add_implicit_clauses.clj
index 820409c2e09..77ff8cc38a4 100644
--- a/src/metabase/query_processor/middleware/add_implicit_clauses.clj
+++ b/src/metabase/query_processor/middleware/add_implicit_clauses.clj
@@ -26,16 +26,17 @@
(defn- fields-for-source-table
"Return the all fields for SOURCE-TABLE, for use as an implicit `:fields` clause."
- [{{source-table-id :id, :as source-table} :source-table, :as inner-query}]
+ [inner-query]
;; Sort the implicit FIELDS so the SQL (or other native query) that gets generated (mostly) approximates the 'magic' sorting
;; we do on the results. This is done so when the outer query we generate is a `SELECT *` the order doesn't change
- (for [field (sort/sort-fields inner-query (fetch-fields-for-souce-table-id source-table-id))
- :let [field (-> field
- resolve/convert-db-field
- (resolve/resolve-table {[nil source-table-id] source-table}))]]
- (if (qputil/datetime-field? field)
- (i/map->DateTimeField {:field field, :unit :default})
- field)))
+ (let [{source-table-id :id, :as source-table} (qputil/get-normalized inner-query :source-table)]
+ (for [field (sort/sort-fields inner-query (fetch-fields-for-souce-table-id source-table-id))
+ :let [field (-> field
+ resolve/convert-db-field
+ (resolve/resolve-table {[nil source-table-id] source-table}))]]
+ (if (qputil/datetime-field? field)
+ (i/map->DateTimeField {:field field, :unit :default})
+ field))))
(defn- should-add-implicit-fields? [{:keys [fields breakout source-table], aggregations :aggregation}]
(and source-table ; if query is using another query as its source then there will be no table to add nested fields for
diff --git a/src/metabase/query_processor/middleware/cache.clj b/src/metabase/query_processor/middleware/cache.clj
index ce8db38cea6..eedf5b2e2bd 100644
--- a/src/metabase/query_processor/middleware/cache.clj
+++ b/src/metabase/query_processor/middleware/cache.clj
@@ -1,17 +1,18 @@
(ns metabase.query-processor.middleware.cache
"Middleware that returns cached results for queries when applicable.
- If caching is enabled (`enable-query-caching` is `true`) cached results will be returned for Cards if possible. There's
- a global default TTL defined by the setting `query-caching-default-ttl`, but individual Cards can override this value
- with custom TTLs with a value for `:cache_ttl`.
+ If caching is enabled (`enable-query-caching` is `true`) cached results will be returned for Cards if possible.
+ There's a global default TTL defined by the setting `query-caching-default-ttl`, but individual Cards can override
+ this value with custom TTLs with a value for `:cache_ttl`.
- For all other queries, caching is skipped.
+ For all other queries, caching is skipped.
- Various caching backends are defined in `metabase.query-processor.middleware.cache-backend` namespaces.
- The default backend is `db`, which uses the application database; this value can be changed by setting the env var
- `MB_QP_CACHE_BACKEND`.
+ Various caching backends are defined in `metabase.query-processor.middleware.cache-backend` namespaces. The default
+ backend is `db`, which uses the application database; this value can be changed by setting the env var
+ `MB_QP_CACHE_BACKEND`.
- Refer to `metabase.query-processor.middleware.cache-backend.interface` for more details about how the cache backends themselves."
+ Refer to `metabase.query-processor.middleware.cache-backend.interface` for more details about how the cache
+ backends themselves."
(:require [clojure.tools.logging :as log]
[metabase
[config :as config]
@@ -23,11 +24,11 @@
(def ^:dynamic ^Boolean *ignore-cached-results*
"Should we force the query to run, ignoring cached results even if they're available?
- Setting this to `true` will run the query again and will still save the updated results."
+ Setting this to `true` will run the query again and will still save the updated results."
false)
-;;; ------------------------------------------------------------ Backend ------------------------------------------------------------
+;;; ---------------------------------------------------- Backend -----------------------------------------------------
(def ^:private backend-instance
(atom nil))
@@ -49,7 +50,8 @@
(valid-backend? @varr) @varr
allow-reload? (do (require backend-ns-symb :reload)
(get-backend-instance-in-namespace backend-ns-symb false))
- :else (throw (Exception. (format "%s/instance doesn't satisfy IQueryProcessorCacheBackend" backend-ns-symb)))))))
+ :else (throw (Exception. (format "%s/instance doesn't satisfy IQueryProcessorCacheBackend"
+ backend-ns-symb)))))))
(defn- set-backend!
"Set the cache backend to the cache defined by the keyword BACKEND.
@@ -66,7 +68,7 @@
-;;; ------------------------------------------------------------ Cache Operations ------------------------------------------------------------
+;;; ------------------------------------------------ Cache Operations ------------------------------------------------
(defn- cached-results [query-hash max-age-seconds]
(when-not *ignore-cached-results*
@@ -81,7 +83,7 @@
(i/save-results! @backend-instance query-hash results))
-;;; ------------------------------------------------------------ Middleware ------------------------------------------------------------
+;;; --------------------------------------------------- Middleware ---------------------------------------------------
(defn- is-cacheable? ^Boolean [{cache-ttl :cache_ttl}]
(boolean (and (public-settings/enable-query-caching)
@@ -127,7 +129,7 @@
(defn maybe-return-cached-results
"Middleware for caching results of a query if applicable.
- In order for a query to be eligible for caching:
+ In order for a query to be eligible for caching:
* Caching (the `enable-query-caching` Setting) must be enabled
* The query must pass a `:cache_ttl` value. For Cards, this can be the value of `:cache_ttl`,
diff --git a/src/metabase/query_processor/middleware/resolve.clj b/src/metabase/query_processor/middleware/resolve.clj
index 7ccc329c97c..78cf8066fce 100644
--- a/src/metabase/query_processor/middleware/resolve.clj
+++ b/src/metabase/query_processor/middleware/resolve.clj
@@ -379,24 +379,25 @@
(defn- resolve-tables
"Resolve the `Tables` in an EXPANDED-QUERY-DICT."
- [{{{ source-table-id :id :as source-table} :source-table} :query, :keys [table-ids fk-field-ids], :as expanded-query-dict}]
- (if-not source-table-id
- ;; if we have a `source-query`, recurse and resolve tables in that
- (update-in expanded-query-dict [:query :source-query] (fn [source-query]
- (if (:native source-query)
- source-query
- (:query (resolve-tables (assoc expanded-query-dict
- :query source-query))))))
- ;; otherwise we can resolve tables in the (current) top-level
- (let [table-ids (conj table-ids source-table-id)
- joined-tables (fk-field-ids->joined-tables source-table-id fk-field-ids)
- fk-id+table-id->table (into {[nil source-table-id] source-table}
- (for [{:keys [source-field table-id join-alias]} joined-tables]
- {[(:field-id source-field) table-id] {:name join-alias
- :id table-id}}))]
- (as-> expanded-query-dict <>
- (assoc-in <> [:query :join-tables] joined-tables)
- (walk/postwalk #(resolve-table % fk-id+table-id->table) <>)))))
+ [{:keys [table-ids fk-field-ids], :as expanded-query-dict}]
+ (let [{source-table-id :id :as source-table} (qputil/get-in-normalized expanded-query-dict [:query :source-table])]
+ (if-not source-table-id
+ ;; if we have a `source-query`, recurse and resolve tables in that
+ (update-in expanded-query-dict [:query :source-query] (fn [source-query]
+ (if (:native source-query)
+ source-query
+ (:query (resolve-tables (assoc expanded-query-dict
+ :query source-query))))))
+ ;; otherwise we can resolve tables in the (current) top-level
+ (let [table-ids (conj table-ids source-table-id)
+ joined-tables (fk-field-ids->joined-tables source-table-id fk-field-ids)
+ fk-id+table-id->table (into {[nil source-table-id] source-table}
+ (for [{:keys [source-field table-id join-alias]} joined-tables]
+ {[(:field-id source-field) table-id] {:name join-alias
+ :id table-id}}))]
+ (as-> expanded-query-dict <>
+ (assoc-in <> [:query :join-tables] joined-tables)
+ (walk/postwalk #(resolve-table % fk-id+table-id->table) <>))))))
;;; ------------------------------------------------ PUBLIC INTERFACE ------------------------------------------------
diff --git a/src/metabase/query_processor/middleware/source_table.clj b/src/metabase/query_processor/middleware/source_table.clj
index 047323ade34..56718f02cff 100644
--- a/src/metabase/query_processor/middleware/source_table.clj
+++ b/src/metabase/query_processor/middleware/source_table.clj
@@ -4,21 +4,22 @@
[toucan.db :as db]))
(defn- resolve-source-table
- [{{source-table-id :source-table} :query :as expanded-query-dict}]
- (cond
- (not (qputil/mbql-query? expanded-query-dict))
- expanded-query-dict
+ [expanded-query-dict]
+ (let [source-table-id (qputil/get-in-normalized expanded-query-dict [:query :source-table])]
+ (cond
+ (not (qputil/mbql-query? expanded-query-dict))
+ expanded-query-dict
- (nil? source-table-id)
- (update-in expanded-query-dict [:query :source-query] (fn [source-query]
- (if (:native source-query)
- source-query
- (:query (resolve-source-table (assoc expanded-query-dict :query source-query))))))
+ (nil? source-table-id)
+ (update-in expanded-query-dict [:query :source-query] (fn [source-query]
+ (if (:native source-query)
+ source-query
+ (:query (resolve-source-table (assoc expanded-query-dict :query source-query))))))
- :else
- (let [source-table (or (db/select-one [Table :schema :name :id], :id source-table-id)
- (throw (Exception. (format "Query expansion failed: could not find source table %d." source-table-id))))]
- (assoc-in expanded-query-dict [:query :source-table] source-table))))
+ :else
+ (let [source-table (or (db/select-one [Table :schema :name :id], :id source-table-id)
+ (throw (Exception. (format "Query expansion failed: could not find source table %d." source-table-id))))]
+ (assoc-in expanded-query-dict [:query :source-table] source-table)))))
(defn resolve-source-table-middleware
"Middleware that will take the source-table (an integer) and hydrate
diff --git a/test/metabase/api/common_test.clj b/test/metabase/api/common_test.clj
index c738b4ed259..335b67793f8 100644
--- a/test/metabase/api/common_test.clj
+++ b/test/metabase/api/common_test.clj
@@ -1,8 +1,9 @@
(ns metabase.api.common-test
(:require [clojure.core.async :as async]
[expectations :refer :all]
- [metabase.api.common :refer :all :as api]
+ [metabase.api.common :as api :refer :all]
[metabase.api.common.internal :refer :all]
+ [metabase.middleware :as mb-middleware]
[metabase.test.data :refer :all]
[metabase.util.schema :as su]))
@@ -13,33 +14,41 @@
{:status 404
:body "Not found."})
-(defn ^:private my-mock-api-fn [_]
- (catch-api-exceptions
- (check-404 @*current-user*)
- {:status 200
- :body @*current-user*}))
+(defn ^:private my-mock-api-fn []
+ ((mb-middleware/catch-api-exceptions
+ (fn [_]
+ (check-404 @*current-user*)
+ {:status 200
+ :body @*current-user*}))
+ nil))
; check that `check-404` doesn't throw an exception if TEST is true
(expect {:status 200
:body "Cam Saul"}
(binding [*current-user* (atom "Cam Saul")]
- (my-mock-api-fn nil)))
+ (my-mock-api-fn)))
; check that 404 is returned otherwise
(expect four-oh-four
- (my-mock-api-fn nil))
+ (my-mock-api-fn))
;;let-404 should return nil if test fails
-(expect four-oh-four
- (catch-api-exceptions
- (let-404 [user nil]
- {:user user})))
+(expect
+ four-oh-four
+ ((mb-middleware/catch-api-exceptions
+ (fn [_]
+ (let-404 [user nil]
+ {:user user})))
+ nil))
;; otherwise let-404 should bind as expected
-(expect {:user {:name "Cam"}}
- (catch-api-exceptions
- (let-404 [user {:name "Cam"}]
- {:user user})))
+(expect
+ {:user {:name "Cam"}}
+ ((mb-middleware/catch-api-exceptions
+ (fn [_]
+ (let-404 [user {:name "Cam"}]
+ {:user user})))
+ nil))
(defmacro ^:private expect-expansion
@@ -53,7 +62,8 @@
;;; TESTS FOR AUTO-PARSE
-;; TODO - these need to be moved to `metabase.api.common.internal-test`. But first `expect-expansion` needs to be put somewhere central
+;; TODO - these need to be moved to `metabase.api.common.internal-test`. But first `expect-expansion` needs to be put
+;; somewhere central
;; when auto-parse gets an args form where arg is present in *autoparse-types*
;; the appropriate let binding should be generated
@@ -88,10 +98,9 @@
(expect-expansion
(def GET_:id
(GET ["/:id" :id "#[0-9]+"] [id]
- (metabase.api.common.internal/catch-api-exceptions
- (metabase.api.common.internal/auto-parse [id]
- (metabase.api.common.internal/validate-param 'id id su/IntGreaterThanZero)
- (metabase.api.common.internal/wrap-response-if-needed (do (select-one Card :id id)))))))
+ (metabase.api.common.internal/auto-parse [id]
+ (metabase.api.common.internal/validate-param 'id id su/IntGreaterThanZero)
+ (metabase.api.common.internal/wrap-response-if-needed (do (select-one Card :id id))))))
(defendpoint GET "/:id" [id]
{id su/IntGreaterThanZero}
(select-one Card :id id))))
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index 2063529d875..2854fd26825 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -113,7 +113,8 @@
:pulse_id nil
:card_id nil
:dashboard_id nil}]
- ;; Error message's format can differ a bit depending on DB version and the comment we prepend to it, so check that it exists and contains the substring "Syntax error in SQL statement"
+ ;; Error message's format can differ a bit depending on DB version and the comment we prepend to it, so check that
+ ;; it exists and contains the substring "Syntax error in SQL statement"
(let [check-error-message (fn [output]
(update output :error (fn [error-message]
(boolean (re-find #"Syntax error in SQL statement" error-message)))))
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index e67f016da8a..9edb80f9eae 100644
--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -4,8 +4,7 @@
[metabase.models
[card :as card :refer :all]
[dashboard :refer [Dashboard]]
- [dashboard-card :refer [DashboardCard]]
- [permissions :as perms]]
+ [dashboard-card :refer [DashboardCard]]]
[metabase.test
[data :as data]
[util :as tu]]
diff --git a/test/metabase/query_processor/middleware/permissions_test.clj b/test/metabase/query_processor/middleware/permissions_test.clj
index f926a718052..be3515731f5 100644
--- a/test/metabase/query_processor/middleware/permissions_test.clj
+++ b/test/metabase/query_processor/middleware/permissions_test.clj
@@ -18,9 +18,7 @@
(defn- do-with-rasta
"Call F with rasta as the current user."
[f]
- (binding [api/*current-user-id* (users/user->id :rasta)
- api/*current-user-permissions-set* (atom (user/permissions-set (users/user->id :rasta)))]
- (f)))
+ (users/call-with-api-vars :rasta f))
(defn- check-perms-for-rasta
"Check permissions for QUERY with rasta as the current user."
diff --git a/test/metabase/test/data/users.clj b/test/metabase/test/data/users.clj
index c0839c85183..4de1ebf5b27 100644
--- a/test/metabase/test/data/users.clj
+++ b/test/metabase/test/data/users.clj
@@ -5,8 +5,9 @@
[config :as config]
[http-client :as http]
[util :as u]]
+ [metabase.api.common :as api]
[metabase.core.initialization-status :as init-status]
- [metabase.models.user :refer [User]]
+ [metabase.models.user :as user :refer [User]]
[toucan.db :as db])
(:import clojure.lang.ExceptionInfo))
@@ -161,3 +162,12 @@
remove this. (TODO)"
[]
(db/delete! User :id [:not-in (map user->id [:crowberto :lucky :rasta :trashbird])]))
+
+(defn call-with-api-vars
+ "Call `f` with various `metabase.api.common` dynamic vars bound to the test User named by `user-kwd`."
+ [user-kwd f]
+ (binding [api/*current-user* (delay (User (user->id user-kwd)))
+ api/*current-user-id* (user->id user-kwd)
+ api/*is-superuser?* (db/select-one-field :is_superuser User :id (user->id user-kwd))
+ api/*current-user-permissions-set* (delay (user/permissions-set (user->id user-kwd)))]
+ (f)))
diff --git a/test/metabase/test_setup.clj b/test/metabase/test_setup.clj
index cf5cd773d70..67243da80c3 100644
--- a/test/metabase/test_setup.clj
+++ b/test/metabase/test_setup.clj
@@ -9,6 +9,7 @@
[core :as core]
[db :as mdb]
[driver :as driver]
+ [plugins :as plugins]
[util :as u]]
[metabase.core.initialization-status :as init-status]
[metabase.models.setting :as setting]))
@@ -70,8 +71,8 @@
;; Start Jetty in the BG so if test setup fails we have an easier time debugging it -- it's trickier to debug things
;; on a BG thread
(let [start-jetty! (future (core/start-jetty!))]
-
(try
+ (plugins/setup-plugins!)
(log/info (format "Setting up %s test DB and running migrations..." (name (mdb/db-type))))
(mdb/setup-db! :auto-migrate true)
(setting/set! :site-name "Metabase Test")
--
GitLab