Merge pull request #3284 from metabase/a-little-bit-of-cleanup

A tiny bit of cleanup 

src/metabase/config.clj

@@ -35,6 +35,7 @@
   "Retrieve value for a single configuration key.  Accepts either a keyword or a string.
 
    We resolve properties from these places:
+
    1.  environment variables (ex: MB_DB_TYPE -> :mb-db-type)
    2.  jvm options (ex: -Dmb.db.type -> :mb-db-type)
    3.  hard coded `app-defaults`"

src/metabase/middleware.clj

@@ -3,6 +3,7 @@
   (:require [clojure.tools.logging :as log]
             (cheshire factory
                       [generate :refer [add-encoder encode-str encode-nil]])
+            monger.json ; Monger provides custom JSON encoders for Cheshire if you load this namespace -- see http://clojuremongodb.info/articles/integration.html
             [metabase.api.common :refer [*current-user* *current-user-id* *is-superuser?*]]
             (metabase [config :as config]
                       [db :as db])
@@ -32,9 +33,9 @@
 
 ;;; # ------------------------------------------------------------ AUTH & SESSION MANAGEMENT ------------------------------------------------------------
 
-(def ^:private ^:const metabase-session-cookie "metabase.SESSION_ID")
-(def ^:private ^:const metabase-session-header "x-metabase-session")
-(def ^:private ^:const metabase-api-key-header "x-metabase-apikey")
+(def ^:private ^:const ^String metabase-session-cookie "metabase.SESSION_ID")
+(def ^:private ^:const ^String metabase-session-header "x-metabase-session")
+(def ^:private ^:const ^String metabase-api-key-header "x-metabase-apikey")
 
 (def ^:const response-unauthentic "Generic `401 (Unauthenticated)` Ring response map." {:status 401, :body "Unauthenticated"})
 (def ^:const response-forbidden   "Generic `403 (Forbidden)` Ring response map."       {:status 403, :body "Forbidden"})
@@ -46,33 +47,33 @@
    We first check the request :cookies for `metabase.SESSION_ID`, then if no cookie is found we look in the
    http headers for `X-METABASE-SESSION`.  If neither is found then then no keyword is bound to the request."
   [handler]
-  (fn [{:keys [cookies headers] :as request}]
-    (if-let [session-id (or (get-in cookies [metabase-session-cookie :value])
-                            (headers metabase-session-header))]
-      ;; alternatively we could always associate the keyword and just let it be nil if there is no value
-      (handler (assoc request :metabase-session-id session-id))
-      (handler request))))
-
+  (comp handler (fn [{:keys [cookies headers] :as request}]
+                  (if-let [session-id (or (get-in cookies [metabase-session-cookie :value])
+                                          (headers metabase-session-header))]
+                    (assoc request :metabase-session-id session-id)
+                    request))))
+
+
+(defn- add-current-user-id [{:keys [metabase-session-id] :as request}]
+  (or (when (and metabase-session-id ((resolve 'metabase.core/initialized?)))
+        (when-let [session (db/select-one [Session :created_at :user_id (db/qualify User :is_superuser)]
+                             (db/join [Session :user_id] [User :id])
+                             (db/qualify User :is_active) true
+                             (db/qualify Session :id) metabase-session-id)]
+          (let [session-age-ms (- (System/currentTimeMillis) (or (when-let [^java.util.Date created-at (:created_at session)]
+                                                                   (.getTime created-at))
+                                                                 0))]
+            ;; If the session exists and is not expired (max-session-age > session-age) then validation is good
+            (when (and session (> (config/config-int :max-session-age) (quot session-age-ms 60000)))
+              (assoc request
+                :metabase-user-id (:user_id session)
+                :is-superuser?    (:is_superuser session))))))
+      request))
 
 (defn wrap-current-user-id
   "Add `:metabase-user-id` to the request if a valid session token was passed."
   [handler]
-  (fn [{:keys [metabase-session-id] :as request}]
-    ;; TODO - what kind of validations can we do on the sessionid to make sure it's safe to handle?  str?  alphanumeric?
-    (handler (or (when (and metabase-session-id ((resolve 'metabase.core/initialized?)))
-                   (when-let [session (db/select-one [Session :created_at :user_id (db/qualify User :is_superuser)]
-                                        (db/join [Session :user_id] [User :id])
-                                        (db/qualify User :is_active) true
-                                        (db/qualify Session :id) metabase-session-id)]
-                     (let [session-age-ms (- (System/currentTimeMillis) (or (when-let [^java.util.Date created-at (:created_at session)]
-                                                                              (.getTime created-at))
-                                                                            0))]
-                       ;; If the session exists and is not expired (max-session-age > session-age) then validation is good
-                       (when (and session (> (config/config-int :max-session-age) (quot session-age-ms 60000)))
-                         (assoc request
-                           :metabase-user-id (:user_id session)
-                           :is-superuser?    (:is_superuser session))))))
-                 request))))
+  (comp handler add-current-user-id))
 
 
 (defn enforce-authentication
@@ -105,10 +106,10 @@
   "Middleware that sets the `:metabase-api-key` keyword on the request if a valid API Key can be found.
    We check the request headers for `X-METABASE-APIKEY` and if it's not found then then no keyword is bound to the request."
   [handler]
-  (fn [{:keys [headers] :as request}]
-    (handler (if-let [api-key (headers metabase-api-key-header)]
-               (assoc request :metabase-api-key api-key)
-               request))))
+  (comp handler (fn [{:keys [headers] :as request}]
+                  (if-let [api-key (headers metabase-api-key-header)]
+                    (assoc request :metabase-api-key api-key)
+                    request))))
 
 
 (defn enforce-api-key
@@ -179,21 +180,20 @@
   "Base-64 encoded public key for this site's SSL certificate. Specify this to enable HTTP Public Key Pinning.
    See http://mzl.la/1EnfqBf for more information.") ; TODO - it would be nice if we could make this a proper link in the UI; consider enabling markdown parsing
 
-;(defn- public-key-pins-header []
-;  (when-let [k (ssl-certificate-public-key)]
-;    {"Public-Key-Pins" (format "pin-sha256=\"base64==%s\"; max-age=31536000" k)}))
+#_(defn- public-key-pins-header []
+  (when-let [k (ssl-certificate-public-key)]
+    {"Public-Key-Pins" (format "pin-sha256=\"base64==%s\"; max-age=31536000" k)}))
 
 (defn- api-security-headers [] ; don't need to include all the nonsense we include with index.html
   (merge (cache-prevention-headers)
          strict-transport-security-header
-         ;(public-key-pins-header)
-         ))
+         #_(public-key-pins-header)))
 
 (defn- index-page-security-headers []
   (merge (cache-prevention-headers)
          strict-transport-security-header
          content-security-policy-header
-         ;(public-key-pins-header)
+         #_(public-key-pins-header)
          {"X-Frame-Options"                   "DENY"          ; Tell browsers not to render our site as an iframe (prevent clickjacking)
           "X-XSS-Protection"                  "1; mode=block" ; Tell browser to block suspected XSS attacks
           "X-Permitted-Cross-Domain-Policies" "none"          ; Prevent Flash / PDF files from including content from site.

src/metabase/query_processor.clj

@@ -127,57 +127,56 @@
            (fail query e)))))
 
 
-(defn- pre-add-settings
+(defn- add-settings
   "Adds the `:settings` map to the query which can contain any fixed properties that would be useful at execution time.
    Currently supports a settings object like:
 
-       {:report-timezone \"US/Pacific\"}
-   "
-  [qp]
-  (fn [{:keys [driver] :as query}]
-    (let [settings {:report-timezone (when (driver/driver-supports? driver :set-timezone)
-                                       (let [report-tz (driver/report-timezone)]
-                                         (when-not (empty? report-tz)
-                                           report-tz)))}]
-      (qp (assoc query :settings (m/filter-vals (complement nil?) settings))))))
+       {:report-timezone \"US/Pacific\"}"
+  [{:keys [driver] :as query}]
+  (let [settings {:report-timezone (when (driver/driver-supports? driver :set-timezone)
+                                     (let [report-tz (driver/report-timezone)]
+                                       (when-not (empty? report-tz)
+                                         report-tz)))}]
+    (assoc query :settings (m/filter-vals (complement nil?) settings))))
+
+(defn- pre-add-settings [qp] (comp qp add-settings))
 
 
-(defn- pre-expand-macros
+(defn- expand-macros
   "Looks for macros in a structured (unexpanded) query and substitutes the macros for their contents."
-  [qp]
-  (fn [query]
-    ;; if necessary, handle macro substitution
-    (let [query (if-not (mbql-query? query)
-                  query
-                  ;; for MBQL queries run our macro expansion
-                  (u/prog1 (macros/expand-macros query)
-                    (when (and (not *disable-qp-logging*)
-                               (not= <> query))
-                      (log/debug (u/format-color 'cyan "\n\nMACRO/SUBSTITUTED: 😻\n%s" (u/pprint-to-str <>))))))]
-      (qp query))))
+  [query]
+  (if-not (mbql-query? query)
+    query
+    (u/prog1 (macros/expand-macros query)
+      (when (and (not *disable-qp-logging*)
+                 (not= <> query))
+        (log/debug (u/format-color 'cyan "\n\nMACRO/SUBSTITUTED: 😻\n%s" (u/pprint-to-str <>)))))))
+
+(defn- pre-expand-macros [qp] (comp qp expand-macros))
 
-(defn- pre-substitute-parameters
+
+(defn- substitute-parameters
   "If any parameters were supplied then substitute them into the query."
-  [qp]
-  (fn [query]
-    ;; if necessary, handle parameters substitution
-    (let [query (u/prog1 (params/expand-parameters query)
-                  (when (and (not *disable-qp-logging*)
-                             (not= <> query))
-                    (log/debug (u/format-color 'cyan "\n\nPARAMS/SUBSTITUTED: 😻\n%s" (u/pprint-to-str <>)))))]
-      (qp query))))
+  [query]
+  (u/prog1 (params/expand-parameters query)
+    (when (and (not *disable-qp-logging*)
+               (not= <> query))
+      (log/debug (u/format-color 'cyan "\n\nPARAMS/SUBSTITUTED: 😻\n%s" (u/pprint-to-str <>))))))
+
+(defn- pre-substitute-parameters [qp] (comp qp substitute-parameters))
+
 
-(defn- pre-expand-resolve
+(defn- expand-resolve
   "Transforms an MBQL into an expanded form with more information and structure. Also resolves references to fields, tables,
    etc, into their concrete details which are necessary for query formation by the executing driver."
-  [qp]
-  (fn [{database-id :database, :as query}]
-    (let [resolved-db (db/select-one [database/Database :name :id :engine :details], :id database-id)
-          query       (if-not (mbql-query? query)
-                        query
-                        ;; for MBQL queries we expand first, then resolve
-                        (resolve/resolve (expand/expand query)))]
-      (qp (assoc query :database resolved-db)))))
+  [{database-id :database, :as query}]
+  (let [resolved-db (db/select-one [database/Database :name :id :engine :details], :id database-id)
+        query       (if-not (mbql-query? query)
+                      query
+                      (resolve/resolve (expand/expand query)))]
+    (assoc query :database resolved-db)))
+
+(defn- pre-expand-resolve [qp] (comp qp expand-resolve))
 
 
 (defn- post-add-row-count-and-status
@@ -243,37 +242,37 @@
         (map->DateTimeField {:field field, :unit :default})
         field))))
 
-(defn- pre-add-implicit-fields
+(defn- add-implicit-fields
   "Add an implicit `fields` clause to queries with `rows` aggregations."
-  [qp]
-  (fn [{{:keys [source-table]} :query, :as query}]
-    (let [query (if-not (should-add-implicit-fields? query)
-                  query
-                  ;; this is a structured `:rows` query, so lets add a `:fields` clause with all fields from the source table + expressions
-                  (let [fields      (fields-for-source-table source-table)
-                        expressions (for [[expression-name] (get-in query [:query :expressions])]
-                                      (strict-map->ExpressionRef {:expression-name (name expression-name)}))]
-                    (when-not (seq fields)
-                      (log/warn (format "Table '%s' has no Fields associated with it." (:name source-table))))
-                    (-> query
-                        (assoc-in [:query :fields-is-implicit] true)
-                        (assoc-in [:query :fields] (concat fields expressions)))))]
-      (qp query))))
-
-
-(defn- pre-add-implicit-breakout-order-by
+  [{{:keys [source-table]} :query, :as query}]
+  (if-not (should-add-implicit-fields? query)
+    query
+    ;; this is a structured `:rows` query, so lets add a `:fields` clause with all fields from the source table + expressions
+    (let [fields      (fields-for-source-table source-table)
+          expressions (for [[expression-name] (get-in query [:query :expressions])]
+                        (strict-map->ExpressionRef {:expression-name (name expression-name)}))]
+      (when-not (seq fields)
+        (log/warn (format "Table '%s' has no Fields associated with it." (:name source-table))))
+      (-> query
+          (assoc-in [:query :fields-is-implicit] true)
+          (assoc-in [:query :fields] (concat fields expressions))))))
+
+(defn- pre-add-implicit-fields [qp] (comp qp add-implicit-fields))
+
+
+(defn- add-implicit-breakout-order-by
   "`Fields` specified in `breakout` should add an implicit ascending `order-by` subclause *unless* that field is *explicitly* referenced in `order-by`."
-  [qp]
-  (fn [{{breakout-fields :breakout, order-by :order-by} :query, :as query}]
-    (if (mbql-query? query)
-      (let [order-by-fields                   (set (map :field order-by))
-            implicit-breakout-order-by-fields (filter (partial (complement contains?) order-by-fields)
-                                                      breakout-fields)]
-        (qp (cond-> query
-              (seq implicit-breakout-order-by-fields) (update-in [:query :order-by] concat (for [field implicit-breakout-order-by-fields]
-                                                                                             {:field field, :direction :ascending})))))
-      ;; for non-MBQL queries we do nothing
-      (qp query))))
+  [{{breakout-fields :breakout, order-by :order-by} :query, :as query}]
+  (if-not (mbql-query? query)
+    query
+    (let [order-by-fields                   (set (map :field order-by))
+          implicit-breakout-order-by-fields (filter (partial (complement contains?) order-by-fields)
+                                                    breakout-fields)]
+      (cond-> query
+        (seq implicit-breakout-order-by-fields) (update-in [:query :order-by] concat (for [field implicit-breakout-order-by-fields]
+                                                                                       {:field field, :direction :ascending}))))))
+
+(defn- pre-add-implicit-breakout-order-by [qp] (comp qp add-implicit-breakout-order-by))
 
 
 (defn- pre-cumulative-sum
@@ -402,9 +401,8 @@
       (update results :rows (partial take (or max-results
                                               absolute-max-results))))))
 
-
-(defn- pre-log-query [qp]
-  (fn [query]
+(defn- log-query [query]
+  (u/prog1 query
     (when (and (mbql-query? query)
                (not *disable-qp-logging*))
       (log/debug (u/format-color 'magenta "\nPREPROCESSED/EXPANDED: 😻\n%s"
@@ -417,8 +415,9 @@
                      ;; obscure DB details when logging. Just log the name of driver because we don't care about its properties
                      (-> query
                          (assoc-in [:database :details] "😋 ") ; :yum:
-                         (update :driver name)))))))
-    (qp query)))
+                         (update :driver name)))))))))
+
+(defn- pre-log-query [qp] (comp qp log-query))
 
 ;; The following are just assertions that check the behavior of the QP. It doesn't make sense to run them on prod because at best they
 ;; just waste CPU cycles and at worst cause a query to fail when it would otherwise succeed.
@@ -431,11 +430,11 @@
   (if config/is-prod?
     identity
     (fn [qp]
-      (let [called? (atom false)]
-        (fn [query]
-          (assert (not @called?) "(QP QUERY) IS BEING CALLED MORE THAN ONCE!")
-          (reset! called? true)
-          (qp query))))))
+      (comp qp (let [called? (atom false)]
+                 (fn [query]
+                   (u/prog1 query
+                     (assert (not @called?) "(QP QUERY) IS BEING CALLED MORE THAN ONCE!")
+                     (reset! called? true))))))))
 
 
 (def ^:private post-check-results-format
@@ -445,9 +444,7 @@
   (if config/is-prod?
     identity
     (fn [qp]
-      (fn [query]
-        (u/prog1 (qp query)
-          (validate-results <>))))))
+      (comp validate-results qp))))
 
 (defn query->remark
   "Genarate an approparite REMARK to be prepended to a query to give DBAs additional information about the query being executed.