Merge pull request metabase#6543 from metabase/code-cleanup-3000

Code cleanup 3000

src/metabase/api/card.clj

@@ -327,7 +327,7 @@
   "You must be a superuser to change the value of `enable_embedding` or `embedding_params`. Embedding must be
   enabled."
   [card enable-embedding? embedding-params]
-  (when (or (and (not (nil? enable-embedding?))
+  (when (or (and (some? enable-embedding?)
                  (not= enable-embedding? (:enable_embedding card)))
             (and embedding-params
                  (not= embedding-params (:embedding_params card))))

src/metabase/api/dashboard.clj

@@ -194,7 +194,7 @@
    archived                (s/maybe s/Bool)}
   (let [dash (api/write-check Dashboard id)]
     ;; you must be a superuser to change the value of `enable_embedding` or `embedding_params`. Embedding must be enabled
-    (when (or (and (not (nil? enable_embedding))
+    (when (or (and (some? enable_embedding)
                    (not= enable_embedding (:enable_embedding dash)))
               (and embedding_params
                    (not= embedding_params (:embedding_params dash))))

src/metabase/api/embed.clj

@@ -79,7 +79,7 @@
 (defn- valid-param?
   "Is V a valid param value? (Is it non-`nil`, and, if a String, non-blank?)"
   [v]
-  (and (not (nil? v))
+  (and (some? v)
        (or (not (string? v))
            (not (str/blank? v)))))
 
@@ -151,7 +151,7 @@
   [parameters parameter-values]
   (for [param parameters
         :let  [value (get parameter-values (keyword (:slug param)))]
-        :when (not (nil? value))]
+        :when (some? value)]
     (assoc (select-keys param [:type :target])
       :value value)))
 

src/metabase/driver.clj

@@ -16,7 +16,9 @@
             [metabase.config :as config]
             [metabase.models
              [database :refer [Database]]
-             [setting :refer [defsetting]]]
+             field
+             [setting :refer [defsetting]]
+             table]
             [metabase.sync.interface :as si]
             [metabase.util :as u]
             [schema.core :as s]

src/metabase/driver/generic_sql.clj

@@ -362,12 +362,16 @@
                                      (assoc field :pk? true))))))))
 
 (defn describe-database
-  "Default implementation of `describe-database` for JDBC-based drivers."
+  "Default implementation of `describe-database` for JDBC-based drivers. Uses various `ISQLDriver` methods and JDBC
+   metadata."
   [driver database]
   (with-metadata [metadata driver database]
     {:tables (active-tables driver, ^DatabaseMetaData metadata)}))
 
-(defn- describe-table [driver database table]
+(defn describe-table
+  "Default implementation of `describe-table` for JDBC-based drivers. Uses various `ISQLDriver` methods and JDBC
+   metadata."
+  [driver database table]
   (with-metadata [metadata driver database]
     (->> (assoc (select-keys table [:name :schema]) :fields (describe-table-fields metadata driver table))
          ;; find PKs and mark them

src/metabase/driver/mongo.clj

@@ -124,7 +124,7 @@
              :base-type     (driver/class->base-type most-common-object-type)}
       (= :_id field-kw)           (assoc :pk? true)
       (:special-types field-info) (assoc :special-type (->> (vec (:special-types field-info))
-                                                            (filter #(not (nil? (first %))))
+                                                            (filter #(some? (first %)))
                                                             (sort-by second)
                                                             last
                                                             first))

src/metabase/driver/postgres.clj

@@ -1,4 +1,6 @@
 (ns metabase.driver.postgres
+  "Database driver for PostgreSQL databases. Builds on top of the 'Generic SQL' driver, which implements most
+  functionality for JDBC-based drivers."
   (:require [clojure
              [set :as set :refer [rename-keys]]
              [string :as s]]
@@ -13,7 +15,7 @@
              [ssh :as ssh]])
   (:import java.util.UUID))
 
-(def ^:private ^:const column->base-type
+(def ^:private column->base-type
   "Map of Postgres column types -> Field base types.
    Add more mappings here as you come across them."
   {:bigint        :type/BigInteger
@@ -171,7 +173,10 @@
     #".*" ; default
     message))
 
-(defn- prepare-value [{value :value, {:keys [base-type]} :field}]
+(defn- prepare-value
+  "Prepare a value for compilation to SQL. This should return an appropriate HoneySQL form. See description in
+  `ISQLDriver` protocol for details."
+  [{value :value, {:keys [base-type database-type]} :field}]
   (if-not value
     value
     (cond

src/metabase/query_processor/annotate.clj

@@ -1,6 +1,9 @@
 (ns metabase.query-processor.annotate
-  "Code that analyzes the results of running a query and adds relevant type information about results (including foreign key information)."
-  ;; TODO - The code in this namespace could definitely use a little cleanup to make it a little easier to wrap one's head around :)
+  "Code that analyzes the results of running a query and adds relevant type information about results (including
+  foreign key information). This also does things like taking lisp-case keys used in the QP and converting them back
+  to snake_case ones used in the frontend."
+  ;; TODO - The code in this namespace could definitely use a little cleanup to make it a little easier to wrap one's
+  ;;        head around :)
   ;; TODO - This namespace should be called something like `metabase.query-processor.middleware.annotate`
   (:require [clojure
              [set :as set]
@@ -23,11 +26,15 @@
 ;;; ## Field Resolution
 
 (defn- valid-collected-field? [keep-date-time-fields? f]
-  (or (instance? metabase.query_processor.interface.Field f)
-      (instance? metabase.query_processor.interface.FieldLiteral f)
-      (instance? metabase.query_processor.interface.ExpressionRef f)
-      (when keep-date-time-fields?
-        (instance? metabase.query_processor.interface.DateTimeField f))))
+  (or
+   ;; is `f` an instance of `Field`, `FieldLiteral`, or `ExpressionRef`?
+   (some (u/rpartial instance? f)
+         [metabase.query_processor.interface.Field
+          metabase.query_processor.interface.FieldLiteral
+          metabase.query_processor.interface.ExpressionRef])
+   ;; or if we're keeping DateTimeFields, is is an instance of `DateTimeField`?
+   (when keep-date-time-fields?
+     (instance? metabase.query_processor.interface.DateTimeField f))))
 
 (defn collect-fields
   "Return a sequence of all the `Fields` inside THIS, recursing as needed for collections.
@@ -74,21 +81,25 @@
        :base-type          :type/Float
        :special-type       :type/Number)]
 
-    ;; for every value in a map in the query we'll descend into the map and find all the fields contained therein and mark the key as each field's source.
-    ;; e.g. if we descend into the `:breakout` columns for a query each field returned will get a `:source` of `:breakout`
-    ;; The source is important since it is used to determine sort order for for columns
+    ;; for every value in a map in the query we'll descend into the map and find all the fields contained therein and
+    ;; mark the key as each field's source. e.g. if we descend into the `:breakout` columns for a query each field
+    ;; returned will get a `:source` of `:breakout` The source is important since it is used to determine sort order
+    ;; for for columns
     clojure.lang.IPersistentMap
     (for [[k v] (seq this)
           field (collect-fields v keep-date-time-fields?)
           :when field]
       (if (= k :source-query)
         ;; For columns collected from a source query...
-        ;; 1) Make sure they didn't accidentally pick up an integer ID if the fields clause was added implicitly. If it does
-        ;;    the frontend won't know how to use the field since it won't match up with the same field in the "virtual" table metadata.
-        ;; 2) Keep the original `:source` rather than replacing it with `:source-query` since the frontend doesn't know what to do with that.
+        ;; 1) Make sure they didn't accidentally pick up an integer ID if the fields clause was added implicitly. If
+        ;;     it does the frontend won't know how to use the field since it won't match up with the same field in the
+        ;;     "virtual" table metadata.
+        ;; 2) Keep the original `:source` rather than replacing it with `:source-query` since the frontend doesn't
+        ;;    know what to do with that.
         (if (= (:unit field) :year)
-          ;; if the field is broken out by year we don't want to advertise it as type/DateTime because you can't do a datetime breakout on the years that come back
-          ;; (they come back as text). So instead just tell people it's a Text column
+          ;; if the field is broken out by year we don't want to advertise it as type/DateTime because you can't do a
+          ;; datetime breakout on the years that come back (they come back as text). So instead just tell people it's
+          ;; a Text column
           (assoc field
             :field-id [:field-literal (:field-name field) :type/Text]
             :base-type :type/Text
@@ -126,7 +137,8 @@
                                             (if (instance? Expression arg)
                                               (str "(" (aggregation-name arg) ")")
                                               (aggregation-name arg)))))
-    ;; for unnamed normal aggregations, the column alias is always the same as the ag type except for `:distinct` with is called `:count` (WHY?)
+    ;; for unnamed normal aggregations, the column alias is always the same as the ag type except for `:distinct` with
+    ;; is called `:count` (WHY?)
     aggregation-type          (if (= (keyword aggregation-type) :distinct)
                                 "count"
                                 (name aggregation-type))))
@@ -148,13 +160,15 @@
             :field-display-name field-name
             :base-type          (:base-type ag-field)
             :special-type       (:special-type ag-field)})
-         ;; Always treat count or distinct count as an integer even if the DB in question returns it as something wacky like a BigDecimal or Float
+         ;; Always treat count or distinct count as an integer even if the DB in question returns it as something
+         ;; wacky like a BigDecimal or Float
          (when (contains? #{:count :distinct} ag-type)
            {:base-type    :type/Integer
             :special-type :type/Number})
-         ;; For the time being every Expression is an arithmetic operator and returns a floating-point number, so hardcoding these types is fine;
-         ;; In the future when we extend Expressions to handle more functionality we'll want to introduce logic that associates a return type with a given expression.
-         ;; But this will work for the purposes of a patch release.
+         ;; For the time being every Expression is an arithmetic operator and returns a floating-point number, so
+         ;; hardcoding these types is fine; In the future when we extend Expressions to handle more functionality
+         ;; we'll want to introduce logic that associates a return type with a given expression. But this will work
+         ;; for the purposes of a patch release.
          (when (instance? ExpressionRef ag-field)
            {:base-type    :type/Float
             :special-type :type/Number})))
@@ -163,7 +177,8 @@
   "Does QUERY have an aggregation?"
   [{aggregations :aggregation}]
   (or (empty? aggregations)
-      ;; TODO - Not sure this needs to be checked anymore since `:rows` is a legacy way to specifiy "no aggregations" and should be stripped out during preprocessing
+      ;; TODO - Not sure this needs to be checked anymore since `:rows` is a legacy way to specifiy "no aggregations"
+      ;; and should be stripped out during preprocessing
       (= (:aggregation-type (first aggregations)) :rows)))
 
 (defn- add-aggregate-fields-if-needed
@@ -188,8 +203,8 @@
    :field-name         k
    :field-display-name (humanization/name->human-readable-name (name k))})
 
-;; TODO - I'm not 100% sure the code reaches this point any more since the `collect-fields` logic now handles nested queries
-;; maybe this is used for queries where the source query is native?
+;; TODO - I'm not 100% sure the code reaches this point any more since the `collect-fields` logic now handles nested
+;; queries maybe this is used for queries where the source query is native?
 (defn- info-for-column-from-source-query
   "Return information about a column that comes back when we're using a source query.
    (This is basically the same as the generic information, but we also add `:id` and `:source`
@@ -202,9 +217,10 @@
 
 
 (defn- info-for-duplicate-field
-  "The Clojure JDBC driver automatically appends suffixes like `count_2` to duplicate columns if multiple columns come back with the same name;
-   since at this time we can't resolve those normally (#1786) fall back to using the metadata for the first column (e.g., `count`).
-   This is definitely a HACK, but in most cases this should be correct (or at least better than the generic info) for the important things like type information."
+  "The Clojure JDBC driver automatically appends suffixes like `count_2` to duplicate columns if multiple columns come
+  back with the same name; since at this time we can't resolve those normally (#1786) fall back to using the metadata
+  for the first column (e.g., `count`). This is definitely a HACK, but in most cases this should be correct (or at
+  least better than the generic info) for the important things like type information."
   [fields k]
   (when-let [[_ field-name-without-suffix] (re-matches #"^(.*)_\d+$" (name k))]
     (some (fn [{field-name :field-name, :as field}]
@@ -230,22 +246,19 @@
                         (assert (every? keyword? <>)))
         missing-keys  (set/difference (set actual-keys) expected-keys)]
     (when (seq missing-keys)
-      (log/warn (u/format-color 'yellow "There are fields we (maybe) weren't expecting in the results: %s\nExpected: %s\nActual: %s"
+      (log/warn (u/format-color 'yellow (str "There are fields we (maybe) weren't expecting in the results: %s\n"
+                                             "Expected: %s\nActual: %s")
                   missing-keys expected-keys (set actual-keys))))
     (concat fields (for [k     actual-keys
                          :when (contains? missing-keys k)]
                      (info-for-missing-key inner-query fields k (map k initial-rows))))))
 
 (defn- fixup-renamed-fields
-  "After executing the query, it's possible that java.jdbc changed the
-  name of the column that was originally in the query. This can happen
-  when java.jdbc finds two columns with the same name, it will append
-  an integer (like _2) on the end. When this is done on an existing
-  column in the query, this function fixes that up, updating the
-  column information we have with the new name that java.jdbc assigned
-  the column. The `add-unknown-fields-if-needed` function above is
-  similar, but is used when we don't have existing information on that
-  column and need to infer it."
+  "After executing the query, it's possible that java.jdbc changed the name of the column that was originally in the
+  query. This can happen when java.jdbc finds two columns with the same name, it will append an integer (like _2) on
+  the end. When this is done on an existing column in the query, this function fixes that up, updating the column
+  information we have with the new name that java.jdbc assigned the column. The `add-unknown-fields-if-needed`
+  function above is similar, but is used when we don't have existing information on that column and need to infer it."
   [query actual-keys]
   (let [expected-field-names (set (map (comp keyword name) (:fields query)))]
     (if (= expected-field-names (set actual-keys))
@@ -290,7 +303,8 @@
                                                    (integer? id))]
                                    id))
        (constantly nil)))
-  ;; Fetch the foreign key fields whose origin is in the returned Fields, create a map of origin-field-id->destination-field-id
+  ;; Fetch the foreign key fields whose origin is in the returned Fields, create a map of
+  ;; origin-field-id->destination-field-id
   ([fields fk-ids]
    (when (seq fk-ids)
      (fk-field->dest-fn fields fk-ids (db/select-id->field :fk_target_field_id Field
@@ -320,8 +334,8 @@
                         {}))))))
 
 (defn- resolve-sort-and-format-columns
-  "Collect the Fields referenced in INNER-QUERY, sort them according to the rules at the top
-   of this page, format them as expected by the frontend, and return the results."
+  "Collect the Fields referenced in INNER-QUERY, sort them according to the rules at the top of this page, format them
+  as expected by the frontend, and return the results."
   [inner-query result-keys initial-rows]
   {:pre [(sequential? result-keys)]}
   (when (seq result-keys)
@@ -352,8 +366,8 @@
   "Post-process a structured query to add metadata to the results. This stage:
 
   1.  Sorts the results according to the rules at the top of this page
-  2.  Resolves the Fields returned in the results and adds information like `:columns` and `:cols`
-      expected by the frontend."
+  2.  Resolves the Fields returned in the results and adds information like `:columns` and `:cols` expected by the
+      frontend."
   [query {:keys [columns rows], :as results}]
   (let [row-maps (for [row rows]
                    (zipmap columns row))