More cleanup

src/metabase/driver/generic_sql.clj

@@ -147,15 +147,9 @@
       (swap! connection-pools assoc id <>))))
 
 (defn db->jdbc-connection-spec
-  "Return a JDBC connection spec for DATABASE. Normally this will have a C3P0 pool as its datasource, unless the database is `short-lived`."
-  ;; TODO - I don't think short-lived? key is really needed anymore. It's only used by unit tests, and its original purpose was for creating temporary DBs;
-  ;; since we don't destroy databases at the end of each test anymore, it's probably time to remove this
+  "Return a JDBC connection spec for DATABASE. This will have a C3P0 pool as its datasource."
   [{:keys [engine details], :as database}]
-  (if (:short-lived? details)
-    ;; short-lived connections are not pooled, so just return a non-pooled spec
-    (connection-details->spec (driver/engine->driver engine) details)
-    ;; default behavior is to use a pooled connection
-    (db->pooled-connection-spec database)))
+  (db->pooled-connection-spec database))
 
 
 (defn escape-field-name

test/metabase/api/table_test.clj

@@ -331,7 +331,7 @@
 (tu/expect-with-temp [Table [table {:rows 15}]]
   (merge (-> (table-defaults)
              (dissoc :segments :field_values :metrics)
-             (assoc-in [:db :details] {:short-lived? nil, :db "mem:test-data;USER=GUEST;PASSWORD=guest"}))
+             (assoc-in [:db :details] {:db "mem:test-data;USER=GUEST;PASSWORD=guest"}))
          (match-$ table
            {:description     "What a nice table!"
             :entity_type     "person"

test/metabase/driver/postgres_test.clj

@@ -161,7 +161,7 @@
                    ["DROP DATABASE IF EXISTS materialized_views_test;
                      CREATE DATABASE materialized_views_test;"]
                    {:transaction? false})
-    (let [details (i/database->connection-details pg-driver :db {:database-name "materialized_views_test", :short-lived? true})]
+    (let [details (i/database->connection-details pg-driver :db {:database-name "materialized_views_test"})]
       (jdbc/execute! (sql/connection-details->spec pg-driver details)
                      ["DROP MATERIALIZED VIEW IF EXISTS test_mview;
                        CREATE MATERIALIZED VIEW test_mview AS
@@ -177,7 +177,7 @@
                    ["DROP DATABASE IF EXISTS fdw_test;
                      CREATE DATABASE fdw_test;"]
                    {:transaction? false})
-    (let [details (i/database->connection-details pg-driver :db {:database-name "fdw_test", :short-lived? true})]
+    (let [details (i/database->connection-details pg-driver :db {:database-name "fdw_test"})]
       (jdbc/execute! (sql/connection-details->spec pg-driver details)
                      [(str "CREATE EXTENSION IF NOT EXISTS postgres_fdw;
                             CREATE SERVER foreign_server

test/metabase/test/data.clj

@@ -243,7 +243,7 @@
   "Execute F with DBDEF loaded as the current dataset. F takes a single argument, the `DatabaseInstance` that was loaded and synced from DBDEF."
   [^DatabaseDefinition dbdef, f]
   (let [loader *driver*
-        dbdef  (i/map->DatabaseDefinition (assoc dbdef :short-lived? true))]
+        dbdef  (i/map->DatabaseDefinition dbdef)]
     (swap! loader->loaded-db-def conj [loader dbdef])
     (binding [db/*disable-db-logging* true]
       (let [db (get-or-create-database! loader dbdef)]

test/metabase/test/data/generic_sql.clj

@@ -153,7 +153,7 @@
 
 (defn- default-database->spec [driver context dbdef]
   (let [spec (sql/connection-details->spec driver (i/database->connection-details driver context dbdef))]
-    (assoc spec :make-pool? (not (:short-lived? spec)))))
+    (assoc spec :make-pool? true)))
 
 
 ;;; Loading Table Data

test/metabase/test/data/h2.clj

 (ns metabase.test.data.h2
   "Code for creating / destroying an H2 database from a `DatabaseDefinition`."
-  ;; TODO - rework this namespace to use `u/drop-first-arg` where appropriate
   (:require [clojure.core.reducers :as r]
             [clojure.string :as s]
             [metabase.db.spec :as dbspec]
@@ -21,20 +20,14 @@
    :type/Text       "VARCHAR"
    :type/Time       "TIME"})
 
-;; ## DatabaseDefinition helper functions
 
-(def ^:private ^:dynamic *dbdef*
-  nil)
+(defn- database->connection-details [context dbdef]
+  {:db (str "mem:" (i/escaped-name dbdef) (when (= context :db)
+                                            ;; Return details with the GUEST user added so SQL queries are allowed.
+                                            ";USER=GUEST;PASSWORD=guest"))})
 
-(defn- database->connection-details
-  [_ context {:keys [short-lived?], :as dbdef}]
-  {:short-lived? short-lived?
-   :db           (str "mem:" (i/escaped-name dbdef) (when (= context :db)
-                                                      ;; Return details with the GUEST user added so SQL queries are allowed.
-                                                      ";USER=GUEST;PASSWORD=guest"))})
 
-
-(defn quote-name [_ nm]
+(defn- quote-name [nm]
   (str \" (s/upper-case nm) \"))
 
 (def ^:private ^:const ^String create-db-sql
@@ -55,7 +48,7 @@
    (generic/default-create-table-sql this dbdef tabledef) ";\n"
 
    ;; Grant the GUEST account r/w permissions for this table
-   (format "GRANT ALL ON %s TO GUEST;" (quote-name this table-name))))
+   (format "GRANT ALL ON %s TO GUEST;" (quote-name table-name))))
 
 
 (u/strict-extend H2Driver
@@ -75,11 +68,11 @@
             :pk-field-name             (constantly "ID")
             :pk-sql-type               (constantly "BIGINT AUTO_INCREMENT")
             :prepare-identifier        (u/drop-first-arg s/upper-case)
-            :quote-name                quote-name}))
+            :quote-name                (u/drop-first-arg quote-name)}))
 
   i/IDatasetLoader
   (merge generic/IDatasetLoaderMixin
-         {:database->connection-details       database->connection-details
+         {:database->connection-details       (u/drop-first-arg database->connection-details)
           :default-schema                     (constantly "PUBLIC")
           :engine                             (constantly :h2)
           :format-name                        (u/drop-first-arg s/upper-case)

test/metabase/test/data/interface.clj

@@ -26,11 +26,7 @@
                               rows              :- [[s/Any]]])
 
 (s/defrecord DatabaseDefinition [database-name     :- su/NonBlankString
-                                 table-definitions :- [TableDefinition]
-                                 ;; Optional. Set this to non-nil to let dataset loaders know that we don't intend to keep it
-                                 ;; for long -- they can adjust connection behavior, e.g. choosing simple connections instead of creating pools.
-                                 ;; TODO - not sure this is still used now that we create connection pools directly via C3P0; we might be able to remove it
-                                 short-lived?      :- (s/maybe s/Bool)])
+                                 table-definitions :- [TableDefinition]])
 
 (defn escaped-name
   "Return escaped version of database name suitable for use as a filename / database name / etc."

test/metabase/test/data/mysql.clj

@@ -19,11 +19,10 @@
    :type/Text       "TEXT"
    :type/Time       "TIME"})
 
-(defn- database->connection-details [context {:keys [database-name short-lived?]}]
+(defn- database->connection-details [context {:keys [database-name]}]
   (merge {:host         "localhost"
           :port         3306
           :timezone     :America/Los_Angeles
-          :short-lived? short-lived?
           :user         (if (env :circleci) "ubuntu"
                             "root")}
          (when (= context :db)

test/metabase/test/data/oracle.clj

@@ -36,12 +36,11 @@
 
 
 (def ^:private db-connection-details
-  (delay {:host         (get-db-env-var :host)
-          :port         (Integer/parseInt (get-db-env-var :port "1521"))
-          :user         (get-db-env-var :user)
-          :password     (get-db-env-var :password)
-          :sid          (get-db-env-var :sid)
-          :short-lived? false}))
+  (delay {:host     (get-db-env-var :host)
+          :port     (Integer/parseInt (get-db-env-var :port "1521"))
+          :user     (get-db-env-var :user)
+          :password (get-db-env-var :password)
+          :sid      (get-db-env-var :sid)}))
 
 
 (def ^:private ^:const field-base-type->sql-type
@@ -71,6 +70,13 @@
   ([db-name table-name]            [session-schema (i/db-qualified-table-name db-name table-name)])
   ([db-name table-name field-name] [session-schema (i/db-qualified-table-name db-name table-name) field-name]))
 
+(defn- expected-base-type->actual [base-type]
+  ;; Oracle doesn't have INTEGERs
+  (if (isa? base-type :type/Integer)
+    :type/Decimal
+    base-type))
+
+
 (extend OracleDriver
   generic/IGenericSQLDatasetLoader
   (merge generic/DefaultsMixin
@@ -88,11 +94,7 @@
          {:database->connection-details (fn [& _] @db-connection-details)
           :default-schema               (constantly session-schema)
           :engine                       (constantly :oracle)
-          :expected-base-type->actual   (fn [_ base-type]
-                                          ;; Oracle doesn't have INTEGERs
-                                          (if (isa? base-type :type/Integer)
-                                            :type/Decimal
-                                            base-type))
+          :expected-base-type->actual   (u/drop-first-arg expected-base-type->actual)
           :id-field-type                (constantly :type/Decimal)}))
 
 (defn- dbspec []

test/metabase/test/data/postgres.clj

@@ -21,16 +21,16 @@
    :type/Time       "TIME"
    :type/UUID       "UUID"})
 
-(defn- database->connection-details [context {:keys [database-name short-lived?]}]
-  (merge {:host         "localhost"
-          :port         5432
-          :timezone     :America/Los_Angeles
-          :short-lived? short-lived?}
+(defn- database->connection-details [context {:keys [database-name]}]
+  (merge {:host     "localhost"
+          :port     5432
+          :timezone :America/Los_Angeles}
          (when (env :circleci)
            {:user "ubuntu"})
          (when (= context :db)
            {:db database-name})))
 
+
 (u/strict-extend PostgresDriver
   generic/IGenericSQLDatasetLoader
   (merge generic/DefaultsMixin
@@ -40,12 +40,13 @@
           :pk-sql-type               (constantly "SERIAL")})
   i/IDatasetLoader
   (merge generic/IDatasetLoaderMixin
-         {:database->connection-details (u/drop-first-arg database->connection-details)
-          :default-schema               (constantly "public")
-          :engine                       (constantly :postgres)
+         {:database->connection-details       (u/drop-first-arg database->connection-details)
+          :default-schema                     (constantly "public")
+          :engine                             (constantly :postgres)
           ;; TODO: this is suspect, but it works
           :has-questionable-timezone-support? (constantly true)}))
 
+
 ;; it's super obnoxious when testing locally to have tests fail because someone is already connected to the test-data DB (meaning we can't drop it), so close all connections to it beforehand
 (defn- kill-connections-to-test-data-db!
   {:expectations-options :before-run}

test/metabase/test/data/sqlite.clj

@@ -8,9 +8,8 @@
             [metabase.util.honeysql-extensions :as hx])
   (:import metabase.driver.sqlite.SQLiteDriver))
 
-(defn- database->connection-details [context {:keys [short-lived?], :as dbdef}]
-  {:short-lived? short-lived?
-   :db           (str (i/escaped-name dbdef) ".sqlite")})
+(defn- database->connection-details [context dbdef]
+  {:db (str (i/escaped-name dbdef) ".sqlite")})
 
 (def ^:private ^:const field-base-type->sql-type
   {:type/BigInteger "BIGINT"

test/metabase/test/data/sqlserver.clj

@@ -46,14 +46,13 @@
       (throw (Exception. (format "In order to test SQL Server, you must specify the env var MB_SQL_SERVER_%s."
                                  (s/upper-case (name env-var)))))))
 
-(defn- database->connection-details [context {:keys [database-name short-lived?]}]
+(defn- database->connection-details [context {:keys [database-name]}]
   {:host         (get-db-env-var :host)
    :port         (Integer/parseInt (get-db-env-var :port "1433"))
    :user         (get-db-env-var :user)
    :password     (get-db-env-var :password)
    :db           (when (= context :db)
-                   (+suffix database-name))
-   :short-lived? short-lived?})
+                   (+suffix database-name))})
 
 
 (defn- drop-db-if-exists-sql [{:keys [database-name]}]