move tests around a bit

src/metabase/db.clj

@@ -50,13 +50,13 @@
   []
   (case (config/config-kw :mb-db-type)
     :h2 (h2 {:db (db-file)
-             :naming {:keys str/lower-case
+             :naming {:keys   str/lower-case
                       :fields str/upper-case}})
-    :postgres (postgres {:db (config/config-str :mb-db-dbname)
-                         :port (config/config-int :mb-db-port)
-                         :user (config/config-str :mb-db-user)
+    :postgres (postgres {:db       (config/config-str :mb-db-dbname)
+                         :port     (config/config-int :mb-db-port)
+                         :user     (config/config-str :mb-db-user)
                          :password (config/config-str :mb-db-pass)
-                         :host (config/config-str :mb-db-host)})))
+                         :host     (config/config-str :mb-db-host)})))
 
 
 ;; ## CONNECTION
@@ -88,8 +88,8 @@
   [jdbc-db direction]
   (let [conn (jdbc/get-connection jdbc-db)]
     (case direction
-      :up (com.metabase.corvus.migrations.LiquibaseMigrations/setupDatabase conn)
-      :down (com.metabase.corvus.migrations.LiquibaseMigrations/teardownDatabase conn)
+      :up    (com.metabase.corvus.migrations.LiquibaseMigrations/setupDatabase conn)
+      :down  (com.metabase.corvus.migrations.LiquibaseMigrations/teardownDatabase conn)
       :print (com.metabase.corvus.migrations.LiquibaseMigrations/genSqlDatabase conn))))
 
 

src/metabase/driver.clj

@@ -5,7 +5,7 @@
             [cheshire.core :as cheshire]
             [medley.core :refer :all]
             [metabase.db :refer [exists? ins sel upd]]
-            (metabase.driver [interface :as i]
+            (metabase.driver [interface :refer [IDriver] :as i]
                              [result :as result])
             (metabase.models [database :refer [Database]]
                              [query-execution :refer [QueryExecution]])
@@ -23,41 +23,67 @@
 ;; ## Driver Lookup
 
 (def ^{:arglists '([engine])} engine->driver
+  "Return the driver instance that should be used for given ENGINE.
+   This loads the corresponding driver if needed; it is expected that it resides in a var named
+
+     metabase.driver.<engine>/driver
+
+   i.e., the `:postgres` driver should be bound interned at `metabase.driver.postgres/driver`.
+
+     (require ['metabase.driver.interface :as i])
+     (i/active-table-names (engine->driver :postgres) some-pg-database)"
   (memoize
    (fn [engine]
+     {:pre [(keyword? engine)]}
      (let [ns-symb (symbol (format "metabase.driver.%s" (name engine)))]
        (require ns-symb)
-       (var-get (ns-resolve ns-symb 'driver))))))
+       (let [driver (some-> (ns-resolve ns-symb 'driver)
+                            var-get)]
+         (assert driver)
+         driver)))))
 
 ;; Can the type of a DB change?
 (def ^{:arglists '([database-id])} database-id->driver
+  "Memoized function that returns the driver instance that should be used for `Database` with ID.
+   (Databases aren't expected to change their types, and this optimization makes things a lot faster).
+
+   This loads the corresponding driver if needed."
   (memoize
    (fn [database-id]
+     {:pre [(integer? database-id)]}
      (engine->driver (sel :one :field [Database :engine] :id database-id)))))
 
 
 ;; ## Implementation-Agnostic Driver API
 
-(defn can-connect? [database]
-  (i/can-connect? ^i/IDriver (engine->driver (:engine database)) database))
+(defn can-connect?
+  "Check whether we can connect to DATABASE and perform a basic query (such as `SELECT 1`)."
+  [database]
+  (i/can-connect? ^IDriver (engine->driver (:engine database)) database))
+
+(defn can-connect-with-details?
+  "Check whether we can connect to a database with ENGINE and DETAILS-MAP and perform a basic query.
 
-(defn can-connect-with-details? [engine details-map]
-  (i/can-connect-with-details? ^i/IDriver (engine->driver engine) details-map))
+     (can-connect-with-details? :postgres {:host \"localhost\", :port 5432, ...})"
+  [engine details-map]
+  (i/can-connect-with-details? ^IDriver (engine->driver engine) details-map))
 
 (def ^{:arglists '([database])} sync-database!
   "Sync a `Database`, its `Tables`, and `Fields`."
   (let [-sync-database! (u/runtime-resolved-fn 'metabase.driver.sync 'sync-database!)] ; these need to be resolved at runtime to avoid circular deps
     (fn [database]
-      (-sync-database! ^i/IDriver (engine->driver (:engine database)) database))))
+      (-sync-database! ^IDriver (engine->driver (:engine database)) database))))
 
 (def ^{:arglists '([table])} sync-table!
   "Sync a `Table` and its `Fields`."
   (let [-sync-table! (u/runtime-resolved-fn 'metabase.driver.sync 'sync-table!)]
     (fn [table]
-      (-sync-table! ^i/IDriver (database-id->driver (:db_id table)) table))))
+      (-sync-table! ^IDriver (database-id->driver (:db_id table)) table))))
 
-(defn process-query [query]
-  (i/process-query ^i/IDriver (database-id->driver (:database query)) query))
+(defn process-query
+  "Process a structured or native query, and return the result."
+  [query]
+  (i/process-query ^IDriver (database-id->driver (:database query)) query))
 
 
 ;; ## Query Execution Stuff
@@ -184,3 +210,12 @@
       query-execution)
     ;; first time saving execution, so insert it
     (mapply ins QueryExecution query-execution)))
+
+(defn x [db-id]
+  (let [db (sel :one Database :id db-id)
+        driver (database-id->driver db-id)
+        connection-details ((:database->connection-details driver) db)
+        connection-spec ((:connection-details->connection-spec driver) connection-details)]
+    (clojure.pprint/pprint connection-details)
+    (clojure.pprint/pprint connection-spec)
+    (i/active-table-names driver db)))

src/metabase/driver/generic_sql/util.clj

@@ -20,8 +20,9 @@
         connection-details->connection-spec (:connection-details->connection-spec driver)]
     (-> database database->connection-details connection-details->connection-spec)))
 
-#_(def ^{:arglists '([database])} db->korma-db
-  "Return a Korma database definition for DATABASE."
+(def ^{:arglists '([database])} db->korma-db
+  "Return a Korma database definition for DATABASE.
+   This does a little bit of smart caching (for 60 seconds) to avoid creating new connections when unneeded."
   (let [-db->korma-db (memo/ttl (fn [database]
                                   (log/debug (color/red "Creating a new DB connection..."))
                                   (kdb/create-db (db->connection-spec database)))
@@ -30,13 +31,8 @@
     (fn [database]
       (-db->korma-db (select-keys database [:engine :details])))))
 
-(defn db->korma-db [database]
-  (kdb/create-db (db->connection-spec database)))
-
-(def -db (sel :one Database :id 1))
-(defn x []
-  (-> (clojure.set/rename-keys (:details -db) {:conn_str :db})
-      korma.db/h2))
+#_(defn db->korma-db [database]
+    (kdb/create-db (db->connection-spec database)))
 
 (def ^:dynamic ^java.sql.DatabaseMetaData *jdbc-metadata*
   "JDBC metadata object for a database. This is set by `with-jdbc-metadata`."

src/metabase/driver/h2.clj

@@ -7,7 +7,9 @@
 ;; ## CONNECTION
 
 (defn- connection-details->connection-spec [details-map]
-  (korma.db/h2 details-map))
+  (korma.db/h2 (assoc details-map
+                      :db-type :h2          ; what are we using this for again (?)
+                      :make-pool? false)))
 
 (defn- database->connection-details [database]
   (set/rename-keys (:details database) {:conn_str :db}))

src/metabase/driver/interface.clj

-(ns metabase.driver.interface)
+(ns metabase.driver.interface
+  "Protocols that DB drivers implement. Thus, the interface such drivers provide.")
 
 ;; ## IDriver Protocol
 

src/metabase/driver/postgres.clj

@@ -87,7 +87,7 @@
     (-> details
         (assoc :host host                                     ; e.g. "localhost"
                :make-pool? false
-               :db-type :postgres                             ; HACK hardcoded to postgres for time being until API has a way to choose DB type !
+               :db-type :postgres
                :port (Integer/parseInt port))                 ; convert :port to an Integer
         (cond-> (config/config-bool :mb-postgres-ssl) (assoc :ssl true :sslfactory "org.postgresql.ssl.NonValidatingFactory"))
         (rename-keys {:dbname :db}))))

test/metabase/driver/generic_sql_test.clj

+(ns metabase.driver.generic-sql-test
+  (:require [expectations :refer :all]
+            [korma.core :refer :all]
+            [metabase.db :refer :all]
+            [metabase.driver :as driver]
+            (metabase.driver [h2 :as h2]
+                             [interface :as i])
+            [metabase.driver.generic-sql.util :refer [korma-entity]]
+            (metabase.models [field :refer [Field]]
+                             [foreign-key :refer [ForeignKey]]
+                             [table :refer [Table]])
+            [metabase.test-data :refer :all]
+            [metabase.test.util :refer [resolve-private-fns]]))
+
+(def users-table
+  (delay (sel :one Table :name "USERS")))
+
+(def venues-table
+  (delay (sel :one Table :name "VENUES")))
+
+(def korma-users-table
+  (delay (korma-entity @users-table)))
+
+(def users-name-field
+  (delay (sel :one Field :id (field->id :users :name))))
+
+;; ACTIVE-TABLE-NAMES
+(expect
+    #{"CATEGORIES" "VENUES" "CHECKINS" "USERS"}
+  (i/active-table-names h2/driver @test-db))
+
+;; ACTIVE-COLUMN-NAMES->TYPE
+(expect
+    #{{:type_name "INTEGER", :column_name "CATEGORY_ID"}
+      {:type_name "DOUBLE", :column_name "LONGITUDE"}
+      {:type_name "INTEGER", :column_name "PRICE"}
+      {:type_name "BIGINT", :column_name "ID"}
+      {:type_name "VARCHAR", :column_name "NAME"}
+      {:type_name "DOUBLE", :column_name "LATITUDE"}}
+  (i/active-column-names->type h2/driver @venues-table))
+
+
+;; ## TEST TABLE-PK-NAMES
+;; Pretty straightforward
+(expect #{"ID"}
+  (i/table-pks h2/driver @venues-table))
+
+
+;; ## TEST FIELD-AVG-LENGTH
+(expect 13
+  (i/field-avg-length h2/driver @users-name-field))
+
+
+;; ## TEST CHECK-FOR-URLS
+(expect 0.375
+  (with-temp-table [table {:url "VARCHAR(254)"}]
+    (insert table
+            (values [{:url "http://www.google.com"}   ; 1/1 *
+                     {:url nil}                       ; 1/1 (ignored)
+                     {:url "https://amazon.co.uk"}    ; 2/2 *
+                     {:url "http://what.com?ok=true"} ; 3/3 *
+                     {:url "http://missing-period"}   ; 3/4
+                     {:url "ftp://not-http"}          ; 3/5
+                     {:url "http//amazon.com.uk"}     ; 3/6
+                     {:url "Not a URL"}               ; 3/7
+                     {:url "Not-a-url"}]))            ; 3/8
+    (i/field-percent-urls h2/driver {:name "URL"
+                                     :table (delay (assoc table
+                                                          :db test-db))})))

test/metabase/driver/generic_sql/sync_test.clj → test/metabase/driver/sync_test.clj

-(ns metabase.driver.generic-sql.sync-test
+(ns metabase.driver.sync-test
   (:require [expectations :refer :all]
             [korma.core :refer :all]
             [metabase.db :refer :all]
-            (metabase.driver.generic-sql [sync :refer :all]
-                                         [util :refer [korma-entity]])
+            [metabase.driver :as driver]
+            (metabase.driver [h2 :as h2]
+                             [interface :as i]
+                             [sync :as sync])
+            [metabase.driver.generic-sql.util :refer [korma-entity]]
             (metabase.models [field :refer [Field]]
                              [foreign-key :refer [ForeignKey]]
                              [table :refer [Table]])
             [metabase.test-data :refer :all]
             [metabase.test.util :refer [resolve-private-fns]]))
 
-
-(resolve-private-fns metabase.driver.generic-sql.sync determine-fk-type field-avg-length field-percent-urls set-table-pks-if-needed!)
-
 (def users-table
   (delay (sel :one Table :name "USERS")))
 
+(def venues-table
+  (delay (sel :one Table :name "VENUES")))
+
 (def korma-users-table
   (delay (korma-entity @users-table)))
 
 (def users-name-field
   (delay (sel :one Field :id (field->id :users :name))))
 
-;; TABLE-NAMES
-(expect
-    #{"CATEGORIES" "VENUES" "CHECKINS" "USERS"}
-  (table-names @test-db))
-
-;; JDBC-COLUMNS
-(expect
-    #{{:type_name "INTEGER", :column_name "CATEGORY_ID"}
-      {:type_name "DOUBLE", :column_name "LONGITUDE"}
-      {:type_name "INTEGER", :column_name "PRICE"}
-      {:type_name "BIGINT", :column_name "ID"}
-      {:type_name "VARCHAR", :column_name "NAME"}
-      {:type_name "DOUBLE", :column_name "LATITUDE"}}
-    (jdbc-columns @test-db "VENUES"))
 
-
-;; ## TEST TABLE-PK-NAMES
-;; Pretty straightforward
-(expect #{"ID"}
-  (table-pk-names @test-db "VENUES"))
-
-;; ## TEST SET-TABLE-PK-IF-NEEDED!
+;; ## TEST PK SYNCING
 (expect [:id
          nil
          :id
          :latitude
          :id]
-  (let [table (sel :one Table :id (table->id :venues))
-        get-special-type (fn [] (sel :one :field [Field :special_type] :id (field->id :venues :id)))]
+  (let [get-special-type (fn [] (sel :one :field [Field :special_type] :id (field->id :venues :id)))]
     [;; Special type should be :id to begin with
      (get-special-type)
      ;; Clear out the special type
      (do (upd Field (field->id :venues :id) :special_type nil)
          (get-special-type))
-     ;; Calling set-table-pks-if-needed! Should set the special type again
-     (do (set-table-pks-if-needed! table)
+     ;; Calling sync-table! should set the special type again
+     (do (driver/sync-table! @venues-table)
          (get-special-type))
-     ;; set-table-pks-if-needed! should *not* change the special type of fields that are marked with a different type
+     ;; sync-table! should *not* change the special type of fields that are marked with a different type
      (do (upd Field (field->id :venues :id) :special_type :latitude)
          (get-special-type))
      ;; Make sure that sync-table runs set-table-pks-if-needed!
      (do (upd Field (field->id :venues :id) :special_type nil)
-         (sync-table table)
+         (driver/sync-table! @venues-table)
          (get-special-type))]))
 
-;; ## TEST TABLE-FKs
-
-(expect [#{}
-         #{{:fk-column-name "VENUE_ID", :dest-table-name "VENUES", :dest-column-name "ID"}
-           {:fk-column-name "USER_ID", :dest-table-name "USERS", :dest-column-name "ID"}}
-         #{}
-         #{{:fk-column-name "CATEGORY_ID", :dest-table-name "CATEGORIES", :dest-column-name "ID"}}]
-  (map (partial table-fks @test-db)
-       ["CATEGORIES" "CHECKINS" "USERS" "VENUES"]))
+;; ## FK SYNCING
 
 ;; Check that Foreign Key relationships were created on sync as we expect
 
@@ -88,7 +62,7 @@
 (expect (field->id :categories :id)
   (sel :one :field [ForeignKey :destination_id] :origin_id (field->id :venues :category_id)))
 
-;; Check that sync-table causes FKs to be set like we'd expect
+;; Check that sync-table! causes FKs to be set like we'd expect
 (expect [[:fk true]
          [nil false]
          [:fk true]]
@@ -96,7 +70,7 @@
         get-special-type-and-fk-exists? (fn []
                                           [(sel :one :field [Field :special_type] :id field-id)
                                            (exists? ForeignKey :origin_id field-id)])]
-    [;; FK should exist to start with
+    [ ;; FK should exist to start with
      (get-special-type-and-fk-exists?)
      ;; Clear out FK / special_type
      (do (del ForeignKey :origin_id field-id)
@@ -104,45 +78,15 @@
          (get-special-type-and-fk-exists?))
      ;; Run sync-table and they should be set again
      (let [table (sel :one Table :id (table->id :checkins))]
-       (sync-table table)
+       (driver/sync-table! table)
        (get-special-type-and-fk-exists?))]))
 
 ;; ## Tests for DETERMINE-FK-TYPE
 ;; Since COUNT(category_id) > COUNT(DISTINCT(category_id)) the FK relationship should be Mt1
 (expect :Mt1
-  (determine-fk-type (korma-entity (sel :one Table :id (table->id :venues))) "CATEGORY_ID"))
+  (sync/determine-fk-type (korma-entity (sel :one Table :id (table->id :venues))) "CATEGORY_ID"))
 
 ;; Since COUNT(id) == COUNT(DISTINCT(id)) the FK relationship should be 1t1
 ;; (yes, ID isn't really a FK field, but determine-fk-type doesn't need to know that)
 (expect :1t1
-  (determine-fk-type (korma-entity (sel :one Table :id (table->id :venues))) "ID"))
-
-
-;; ## TEST FIELD-AVG-LENGTH
-;; Test that this works if *sql-string-length-fn* is bound as exected
-;; NOTE - This assumes test DB is H2
-(expect 13
-  (binding [*sql-string-length-fn* :LENGTH]
-    (field-avg-length @korma-users-table @users-name-field)))
-
-
-;; The fallback manual count should work as well
-(expect 13
-  (field-avg-length @korma-users-table @users-name-field))
-
-
-
-;; ## TEST CHECK-FOR-URLS
-(expect 0.375
-  (with-temp-table [table {:url "VARCHAR(254)"}]
-    (insert table
-            (values [{:url "http://www.google.com"}   ; 1/1 *
-                     {:url nil}                       ; 1/1 (ignored)
-                     {:url "https://amazon.co.uk"}    ; 2/2 *
-                     {:url "http://what.com?ok=true"} ; 3/3 *
-                     {:url "http://missing-period"}   ; 3/4
-                     {:url "ftp://not-http"}          ; 3/5
-                     {:url "http//amazon.com.uk"}     ; 3/6
-                     {:url "Not a URL"}               ; 3/7
-                     {:url "Not-a-url"}]))            ; 3/8
-    (field-percent-urls table {:name :url})))
+  (sync/determine-fk-type (korma-entity (sel :one Table :id (table->id :venues))) "ID"))