Merge pull request #343 from metabase/cleanup

General cleanup

src/metabase/api/annotation.clj

@@ -22,25 +22,30 @@
   (-> (checkp-contains? (set (keys object-models)) symb (keyword value))
       object-models))
 
-(defannotation AnnotationType [symb value :nillable]
+(defannotation AnnotationType
+  "Param must be either `0` (`general`) or `1` (`description`)."
+  [symb value :nillable]
   (annotation:Integer symb value)
   (checkp-contains? (set [annotation-description annotation-general]) symb value))
 
 
-(defendpoint GET "/" [org object_model object_id]
+(defendpoint GET "/"
+  "Fetch `Annotations` for an org. When `object_model` and `object_id` are specified, only return annotations for that object."
+  [org object_model object_id]
   {org Required, object_model AnnotationObjectModel->ID}
   (read-check Org org)
-  (-> (if (and object_model object_id)
-        ;; caller wants annotations about a specific entity
-        (sel :many Annotation :organization_id org :object_type_id object_model :object_id object_id (korma/order :start :DESC))
-        ;; default is to return all annotations
-        (sel :many Annotation :organization_id org (korma/order :start :DESC)))
+  (-> (sel :many Annotation :organization_id org (korma/order :start :DESC) (korma/where (and {:organization_id org}
+                                                                                              (when (and object_model object_id)
+                                                                                                {:object_type_id object_model
+                                                                                                 :object_id object_id}))))
       (hydrate :author)))
 
 
-(defendpoint POST "/" [:as {{:keys [organization start end title body annotation_type object_model object_id]
-                             :or {annotation_type annotation-general}
-                             :as request-body} :body}]
+(defendpoint POST "/"
+  "Create a new `Annotation`."
+  [:as {{:keys [organization start end title body annotation_type object_model object_id]
+         :or {annotation_type annotation-general}
+         :as request-body} :body}]
   {organization    [Required Integer]
    start           [Required Date]
    end             [Required Date]
@@ -65,7 +70,9 @@
     (hydrate :author)))
 
 
-(defendpoint GET "/:id" [id]
+(defendpoint GET "/:id"
+  "Fetch `Annotation` with ID."
+  [id]
   (let-404 [annotation (sel :one Annotation :id id)]
     (read-check Org (:organization_id annotation))
     (hydrate annotation :author)))
@@ -89,7 +96,9 @@
     (sel :one Annotation :id id)))
 
 
-(defendpoint DELETE "/:id" [id]
+(defendpoint DELETE "/:id"
+  "Delete `Annotation` with ID."
+  [id]
   (let-404 [annotation (sel :one Annotation :id id)]
     (read-check Org (:organization_id annotation))
     (del Annotation :id id)))

src/metabase/api/emailreport.clj

@@ -21,7 +21,9 @@
   (annotation:Integer symb value)
   (checkp-contains? (set (map :id (vals model/modes))) symb value))
 
-(defendpoint GET "/form_input" [org]
+(defendpoint GET "/form_input"
+  "Values of options for the create/edit `EmailReport` UI."
+  [org]
   {org Required}
   (read-check Org org)
   (let [dbs (databases-for-org org)
@@ -109,18 +111,24 @@
       (hydrate :creator :database :can_read :can_write)))
 
 
-(defendpoint DELETE "/:id" [id]
+(defendpoint DELETE "/:id"
+  "Delete an `EmailReport`."
+  [id]
   (write-check EmailReport id)
   (cascade-delete EmailReport :id id))
 
 
-(defendpoint POST "/:id" [id]
+(defendpoint POST "/:id"
+  "Execute and send an `EmailReport`."
+  [id]
   (read-check EmailReport id)
   (->> (report/execute-and-send id)
        (sel :one EmailReportExecutions :id)))
 
 
-(defendpoint GET "/:id/executions" [id]
+(defendpoint GET "/:id/executions"
+  "Get the `EmailReportExecutions` for an `EmailReport`."
+  [id]
   (read-check EmailReport id)
   (-> (sel :many EmailReportExecutions :report_id id (order :created_at :DESC) (limit 25))
       (hydrate :organization)))

src/metabase/api/meta/db.clj

@@ -45,7 +45,9 @@
     ;; make sure we return the newly created db object
     new-db))
 
-(defendpoint GET "/form_input" []
+(defendpoint GET "/form_input"
+  "Values of options for the create/edit `Database` UI."
+  []
   {:timezones metabase.models.common/timezones
    :engines driver/available-drivers})
 

src/metabase/api/meta/table.clj

@@ -51,7 +51,10 @@
   (read-check Table id)
   (sel :many Field :table_id id :active true (order :name :ASC)))
 
-(defendpoint GET "/:id/query_metadata" [id]
+(defendpoint GET "/:id/query_metadata"
+  "Get metadata about a `Table` useful for running queries.
+   Returns DB, fields, field FKs, and field values."
+  [id]
   (->404 (sel :one Table :id id)
          read-check
          (hydrate :db [:fields [:target]] :field_values)))

src/metabase/api/qs.clj

 (ns metabase.api.qs
+  "/api/qs endpoints."
   (:require [clojure.data.csv :as csv]
             [compojure.core :refer [defroutes GET POST]]
             [metabase.api.common :refer :all]
@@ -10,7 +11,9 @@
             [metabase.util :refer [contains-many? now-iso8601]]))
 
 
-(defendpoint POST "/" [:as {{:keys [timezone database sql] :as body} :body}]
+(defendpoint POST "/"
+  "Create a new `Query`, and start it asynchronously."
+  [:as {{:keys [timezone database sql] :as body} :body}]
   {database [Required Integer]
    sql      [Required NonEmptyString]} ; TODO - check timezone
   (read-check Database database)
@@ -24,12 +27,16 @@
     (driver/dataset-query dataset-query options)))
 
 
-(defendpoint GET "/:uuid" [uuid]
+(defendpoint GET "/:uuid"
+  "Fetch the results of a `Query` with UUID."
+  [uuid]
   (let-404 [query-execution (sel :one all-fields :uuid uuid)]
     (build-response query-execution)))
 
 
-(defendpoint GET "/:uuid/csv" [uuid]
+(defendpoint GET "/:uuid/csv"
+  "Fetch the results of a `Query` with UUID as CSV."
+  [uuid]
   (let-404 [{{:keys [columns rows]} :result_data} (sel :one all-fields :uuid uuid)]
     {:status 200
      :body (with-out-str

src/metabase/api/query.clj

@@ -15,7 +15,9 @@
                              [query-execution :refer [QueryExecution all-fields]])
             [metabase.util :as util]))
 
-(defendpoint GET "/form_input" [org]
+(defendpoint GET "/form_input"
+  "Values of options for the create/edit `Query` UI."
+  [org]
   {org Required}
   (read-check Org org)
   {:permissions common/permissions

src/metabase/api/result.clj

@@ -12,8 +12,9 @@
                              [query-execution :refer [QueryExecution all-fields build-response]])))
 
 
-;; Returns the basic information about a given query result
-(defendpoint GET "/:id" [id]
+(defendpoint GET "/:id"
+  "Returns the basic information about a given query result."
+  [id]
   (let-404 [{:keys [query_id] :as query-execution} (sel :one QueryExecution :id id)]
     ;; NOTE - this endpoint requires there to be a saved query associated with this execution
     (check-404 query_id)
@@ -22,8 +23,9 @@
       query-execution)))
 
 
-;; Returns the actual data response for a given query result (as if the query was just executed)
-(defendpoint GET "/:id/response" [id]
+(defendpoint GET "/:id/response"
+  "Returns the actual data response for a given query result (as if the query was just executed)."
+  [id]
   (let-404 [{:keys [query_id] :as query-execution} (sel :one all-fields :id id)]
     ;; NOTE - this endpoint requires there to be a saved query associated with this execution
     (check-404 query_id)
@@ -32,8 +34,9 @@
       (build-response query-execution))))
 
 
-;; Returns the data response for a given query result as a CSV file
-(defendpoint GET "/:id/csv" [id]
+(defendpoint GET "/:id/csv"
+  "Returns the data response for a given query result as a CSV file."
+  [id]
   (let-404 [{:keys [result_data query_id] :as query-execution} (sel :one all-fields :id id)]
     ;; NOTE - this endpoint requires there to be a saved query associated with this execution
     (check-404 query_id)

src/metabase/api/setup.clj

@@ -12,9 +12,11 @@
   [symb value]
   (checkp-with setup/token-match? symb value [403 "Token does not match the setup token."]))
 
-;; special endpoint for creating the first user during setup
-;; this endpoint both creates the user AND logs them in and returns a session id
-(defendpoint POST "/user" [:as {{:keys [token first_name last_name email password] :as body} :body}]
+
+(defendpoint POST "/user"
+  "Special endpoint for creating the first user during setup.
+   This endpoint both creates the user AND logs them in and returns a session ID."
+  [:as {{:keys [token first_name last_name email password] :as body} :body}]
   {first_name [Required NonEmptyString]
    last_name  [Required NonEmptyString]
    email      [Required Email]

src/metabase/api/tiles.clj

@@ -3,16 +3,18 @@
             [compojure.core :refer [GET]]
             [metabase.api.common :refer :all]
             [metabase.db :refer :all]
-            [metabase.driver :as driver]))
+            [metabase.driver :as driver])
+  (:import (java.util ArrayList
+                      Collection)))
 
 
 (def ^:const tile-size 256)
 (def ^:const pixel-origin (float (/ tile-size 2)))
 (def ^:const pixel-per-lon-degree (float (/ tile-size 360.0)))
-(def ^:const pixel-per-lon-radian (float (/ tile-size (* 2 java.lang.Math/PI))))
+(def ^:const pixel-per-lon-radian (float (/ tile-size (* 2 Math/PI))))
 
 (defn- radians->degrees [rad]
-  (/ rad (float (/ java.lang.Math/PI 180))))
+  (/ rad (float (/ Math/PI 180))))
 
 (defn- tile-lat-lon
   "Get the Latitude & Longitude of the upper left corner of a given tile"
@@ -22,8 +24,8 @@
         corner-y    (float (/ (* y tile-size) num-tiles))
         lon         (float (/ (- corner-x pixel-origin) pixel-per-lon-degree))
         lat-radians (/ (- corner-y pixel-origin) (* pixel-per-lon-radian -1))
-        lat          (radians->degrees (- (* 2 (java.lang.Math/atan (java.lang.Math/exp lat-radians)))
-                                          (/ java.lang.Math/PI 2)))]
+        lat         (radians->degrees (- (* 2 (Math/atan (Math/exp lat-radians)))
+                                         (/ Math/PI 2)))]
     {:lat lat
      :lon lon}))
 
@@ -47,10 +49,11 @@
   [lat-col-idx lon-col-idx {{:keys [rows cols]} :data}]
   (if-not (> (count rows) 0)
     ;; if we have no rows then return an empty list of points
-    (java.util.ArrayList. [])
+    (ArrayList. (ArrayList.))
     ;; otherwise we go over the data, pull out the lat/lon columns, and convert them to ArrayLists
-    (->> (map (fn [row] (java.util.ArrayList. [(nth row lat-col-idx) (nth row lon-col-idx)])) rows)
-         (java.util.ArrayList.))))
+    (ArrayList. ^Collection (map (fn [row]
+                                   (ArrayList. ^Collection (vector (nth row lat-col-idx) (nth row lon-col-idx))))
+                                 rows))))
 
 
 (defendpoint GET "/:zoom/:x/:y/:lat-field/:lon-field/:lat-col-idx/:lon-col-idx/"

src/metabase/driver/generic_sql/sync.clj

@@ -67,22 +67,24 @@
   "Fetch a list of table names for DATABASE."
   [database]
   (with-jdbc-metadata database
-    (fn [md] (->> (-> md
-                     (.getTables nil nil nil (into-array String ["TABLE"])) ; ResultSet getTables(String catalog, String schemaPattern, String tableNamePattern, String[] types)
-                     jdbc/result-set-seq)
-                 (map :table_name)
-                 doall))))
+    (fn [^java.sql.DatabaseMetaData md]
+      (->> (-> md
+               (.getTables nil nil nil (into-array String ["TABLE"])) ; ResultSet getTables(String catalog, String schemaPattern, String tableNamePattern, String[] types)
+               jdbc/result-set-seq)
+           (map :table_name)
+           doall))))
 
 (defn jdbc-columns
   "Fetch information about the various columns for Table with TABLE-NAME by getting JDBC metadata for DATABASE."
   [database table-name]
   (with-jdbc-metadata database
-    (fn [md] (->> (-> md
-                    (.getColumns nil nil table-name nil) ; ResultSet getColumns(String catalog, String schemaPattern, String tableNamePattern, String columnNamePattern)
-                    jdbc/result-set-seq)
-                 (filter #(not= (:table_schem %) "INFORMATION_SCHEMA")) ; filter out internal DB columns. This works for H2; does it work for *other*
-                 (map #(select-keys % [:column_name :type_name]))       ; databases?
-                 doall))))
+    (fn [^java.sql.DatabaseMetaData md]
+      (->> (-> md
+               (.getColumns nil nil table-name nil) ; ResultSet getColumns(String catalog, String schemaPattern, String tableNamePattern, String columnNamePattern)
+               jdbc/result-set-seq)
+           (filter #(not= (:table_schem %) "INFORMATION_SCHEMA")) ; filter out internal DB columns. This works for H2; does it work for *other*
+           (map #(select-keys % [:column_name :type_name])) ; databases?
+           doall))))
 
 
 ;; # IMPLEMENTATION

src/metabase/driver/generic_sql/util.clj

@@ -27,7 +27,7 @@
   (connection->korma-db (driver/connection database)))
 
 
-(def ^:dynamic *jdbc-metadata*
+(def ^:dynamic ^java.sql.DatabaseMetaData *jdbc-metadata*
   "JDBC metadata object for a database. This is set by `with-jdbc-metadata`."
   nil)
 

src/metabase/util.clj

@@ -178,30 +178,32 @@
   "Is STRING a valid HTTP/HTTPS URL?"
   [^String string]
   (boolean (when string
-             (when-let [url (try (java.net.URL. string)
-                                 (catch java.net.MalformedURLException _
-                                   nil))]
+             (when-let [^java.net.URL url (try (java.net.URL. string)
+                                               (catch java.net.MalformedURLException _
+                                                 nil))]
                (and (re-matches #"^https?$" (.getProtocol url))          ; these are both automatically downcased
                     (re-matches #"^.+\..{2,}$" (.getAuthority url))))))) ; this is the part like 'google.com'. Make sure it contains at least one period and 2+ letter TLD
 
+(def ^:private ^:const host-up-timeout
+  "Timeout (in ms) for checking if a host is available with `host-up?` and `host-port-up?`."
+  5000)
+
 (defn host-port-up?
   "Returns true if the port is active on a given host, false otherwise"
-  [hostname port]
+  [^String hostname ^Integer port]
   (try
-    (let [sock-addr (InetSocketAddress. hostname port)
-          timeout 5000]
+    (let [sock-addr (InetSocketAddress. hostname port)]
       (with-open [sock (Socket.)]
-        (. sock connect sock-addr timeout)
+        (. sock connect sock-addr host-up-timeout)
         true))
     (catch Exception _ false)))
 
 (defn host-up?
   "Returns true if the host given by hostname is reachable, false otherwise "
-  [hostname]
+  [^String hostname]
   (try
-    (let [host-addr (. InetAddress getByName hostname)
-          timeout 5000]
-      (. host-addr isReachable timeout))
+    (let [host-addr (InetAddress/getByName hostname)]
+      (.isReachable host-addr host-up-timeout))
     (catch Exception _ false)))
 
 (defn rpartial

src/metabase/util/password.clj

@@ -10,23 +10,18 @@
          (string? s)]}
   (reduce + (map #(if (true? (f %)) 1 0) s)))
 
-
 (defn is-complex?
   "Check if a given password meets complexity standards for the application."
   [password]
   {:pre [(string? password)]}
   (let [complexity (config/config-kw :mb-password-complexity)
-        length (config/config-int :mb-password-length)
-        lowers (count-occurrences #(Character/isLowerCase %) password)
-        uppers (count-occurrences #(Character/isUpperCase %) password)
-        digits (count-occurrences #(Character/isDigit %) password)
-        specials (count-occurrences #(not (Character/isLetterOrDigit %)) password)]
-    (if-not (>= (count password) length)
-      false
+        length     (config/config-int :mb-password-length)
+        lowers     (count-occurrences #(Character/isLowerCase ^Character %) password)
+        uppers     (count-occurrences #(Character/isUpperCase ^Character %) password)
+        digits     (count-occurrences #(Character/isDigit ^Character %) password)
+        specials   (count-occurrences #(not (Character/isLetterOrDigit ^Character %)) password)]
+    (if-not (>= (count password) length) false
       (case complexity
-        ;; weak = 1 lower, 1 digit, 1 uppercase
-        :weak (and (> lowers 0) (> digits 0) (> uppers 0))
-        ;; normal = 1 lower, 1 digit, 1 uppercase, 1 special
-        :normal (and (> lowers 0) (> digits 0) (> uppers 0) (> specials 0))
-        ;; strong = 2 lower, 1 digit, 2 uppercase, 1 special
-        :strong (and (> lowers 1) (> digits 0) (> uppers 1) (> specials 0))))))
+        :weak   (and (> lowers 0) (> digits 0) (> uppers 0))                    ; weak   = 1 lower, 1 digit, 1 uppercase
+        :normal (and (> lowers 0) (> digits 0) (> uppers 0) (> specials 0))     ; normal = 1 lower, 1 digit, 1 uppercase, 1 special
+        :strong (and (> lowers 1) (> digits 0) (> uppers 1) (> specials 0)))))) ; strong = 2 lower, 1 digit, 2 uppercase, 1 special