Use classloader/require everywhere; un-HACK driver test HACKs (#10177)

* Use classloader/require everywhere; un-HACK driver test HACKs
[ci drivers]

* Test fixes [ci drivers]

* Test fixes  [ci drivers]

* Oracle Test fixes [ci oracle]

lein-plugins/include-drivers/project.clj

-(defproject metabase/lein-include-drivers "1.0.7"
+(defproject metabase/lein-include-drivers "1.0.8"
   :min-lein-version "2.5.0"
   :eval-in-leiningen true
-  :deploy-repositories [["clojars" {:sign-releases false}]])
+  :deploy-repositories [["clojars" {:sign-releases false}]]
+  :dependencies [[colorize "0.1.1" :exclusions [org.clojure/clojure]]])

lein-plugins/include-drivers/src/leiningen/include_drivers.clj

 (ns leiningen.include-drivers
   (:require [clojure.string :as str]
+            [colorize.core :as colorize]
             [leiningen.core.project :as p])
   (:import java.io.File))
 
@@ -31,11 +32,18 @@
 
   Make sure a file matching that name pattern exists in the `/plugins` directory."
   [driver]
-  (when-let [{:keys [include-drivers-dependencies]} (driver->project driver)]
+  {:pre [(string? driver) (seq driver)]}
+  (if-let [{:keys [include-drivers-dependencies]} (driver->project driver)]
     (or (every? plugins-file-exists? include-drivers-dependencies)
         (println
-         (format "Not including %s because not all dependencies matching %s found in /plugins"
-                 driver include-drivers-dependencies)))))
+         (colorize/color
+          :red
+          (format " [include-drivers middleware]Not including %s because not all dependencies matching %s found in /plugins"
+                  driver (set include-drivers-dependencies)))))
+    (println
+     (colorize/color
+      :red
+      (format "[include-drivers middleware] Not including %s because we could not its project.clj" driver)))))
 
 ;; if :include-drivers is specified in the project, and its value is `:all`, include all drivers; if it's a collection
 ;; of driver names, include the specified drivers; otherwise include whatever was set in the `DRIVERS` env var
@@ -49,13 +57,20 @@
           include-drivers
 
           :else
-          (some-> (System/getenv "DRIVERS") (str/split #",")))
+          (some-> (System/getenv "DRIVERS") (str/split #",") set (disj "h2" "postgres" "mysql")))
 
-        drivers
+        _ (println
+           (colorize/color
+            :magenta
+            (format "[include-drivers middleware] Attempting to include these drivers: %s" (set drivers))))
+
+        available-drivers
         (for [driver drivers
               :when (driver-dependencies-satisfied? driver)]
           driver)]
-    (concat drivers (set (mapcat driver-parents drivers)))))
+    (concat
+     available-drivers
+     (set (mapcat driver-parents available-drivers)))))
 
 (defn- test-drivers-source-paths [test-drivers]
   (vec
@@ -79,10 +94,7 @@
      test-path)))
 
 (defn- test-drivers-projects [test-drivers]
-  (for [driver test-drivers
-        :let   [project (driver->project driver)]
-        :when  project]
-    project))
+  (filter some? (map driver->project test-drivers)))
 
 (defn- test-drivers-dependencies [test-projects]
   (vec
@@ -108,6 +120,10 @@
 (defn- test-drivers-profile [project]
   (let [test-drivers  (test-drivers project)
         test-projects (test-drivers-projects test-drivers)]
+    (println
+     (colorize/color
+      :magenta
+      (format "[include-drivers middleware] including these drivers: %s" (set test-drivers))))
     {:repositories (test-drivers-repositories test-projects)
      :dependencies (test-drivers-dependencies test-projects)
      :aot          (test-drivers-aot          test-projects)

modules/drivers/bigquery/src/metabase/driver/bigquery.clj

@@ -10,7 +10,6 @@
              [core :as hsql]
              [helpers :as h]]
             [metabase
-             [config :as config]
              [driver :as driver]
              [util :as u]]
             [metabase.driver
@@ -467,14 +466,7 @@
 
 (defmethod driver/supports? [:bigquery :expressions] [_ _] false)
 
-;; Don't enable foreign keys when testing because BigQuery *doesn't* have a notion of foreign keys. Joins are still
-;; allowed, which puts us in a weird position, however; people can manually specifiy "foreign key" relationships in
-;; admin and everything should work correctly. Since we can't infer any "FK" relationships during sync our normal FK
-;; tests are not appropriate for BigQuery, so they're disabled for the time being.
-;;
-;; TODO - either write BigQuery-speciifc tests for FK functionality or add additional code to manually set up these FK
-;; relationships for FK tables
-(defmethod driver/supports? [:bigquery :foreign-keys] [_ _] (not config/is-test?))
+(defmethod driver/supports? [:bigquery :foreign-keys] [_ _] true)
 
 ;; BigQuery doesn't return a timezone with it's time strings as it's always UTC, JodaTime parsing also defaults to UTC
 (defmethod driver.common/current-db-time-date-formatters :bigquery [_]

modules/drivers/bigquery/test/metabase/test/data/bigquery.clj

@@ -4,6 +4,10 @@
              [format :as tformat]]
             [clojure.string :as str]
             [medley.core :as m]
+            [metabase
+             [config :as config]
+             [driver :as driver]
+             [util :as u]]
             [metabase.driver
              [bigquery :as bigquery]
              [google :as google]]
@@ -11,7 +15,6 @@
              [datasets :as datasets]
              [interface :as tx]
              [sql :as sql.tx]]
-            [metabase.util :as u]
             [metabase.util
              [date :as du]
              [schema :as su]]
@@ -24,6 +27,16 @@
 
 (sql.tx/add-test-extensions! :bigquery)
 
+;; Don't enable foreign keys when testing because BigQuery *doesn't* have a notion of foreign keys. Joins are still
+;; allowed, which puts us in a weird position, however; people can manually specifiy "foreign key" relationships in
+;; admin and everything should work correctly. Since we can't infer any "FK" relationships during sync our normal FK
+;; tests are not appropriate for BigQuery, so they're disabled for the time being.
+;;
+;; TODO - either write BigQuery-speciifc tests for FK functionality or add additional code to manually set up these FK
+;; relationships for FK tables
+(defmethod driver/supports? [:bigquery :foreign-keys] [_ _] (not config/is-test?))
+
+
 ;;; ----------------------------------------------- Connection Details -----------------------------------------------
 
 (def ^:private ^String normalize-name (comp #(str/replace % #"-" "_") name))

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

@@ -13,6 +13,7 @@
              [schema :as mbql.s]
              [util :as mbql.u]]
             [metabase.models.field :refer [Field]]
+            [metabase.plugins.classloader :as classloader]
             [metabase.query-processor
              [interface :as i]
              [store :as qp.store]]
@@ -38,8 +39,8 @@
 ;; These are loaded here and not in the `:require` above because they tend to get automatically removed by
 ;; `cljr-clean-ns` and also cause Eastwood to complain about unused namespaces
 (when-not *compile-files*
-  (require 'monger.joda-time
-           'monger.json))
+  (classloader/require 'monger.joda-time
+                       'monger.json))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                     Schema                                                     |

modules/drivers/oracle/src/metabase/driver/oracle.clj

 (ns metabase.driver.oracle
-  (:require [clojure
-             [set :as set]
-             [string :as str]]
-            [clojure.java.jdbc :as jdbc]
+  (:require [clojure.java.jdbc :as jdbc]
+            [clojure.string :as str]
             [honeysql.core :as hsql]
-            [metabase
-             [config :as config]
-             [driver :as driver]]
+            [metabase.driver :as driver]
             [metabase.driver.common :as driver.common]
             [metabase.driver.sql
              [query-processor :as sql.qp]
@@ -248,8 +244,7 @@
   (apply driver.common/current-db-time args))
 
 (defmethod sql-jdbc.sync/excluded-schemas :oracle [_]
-  (set/union
-   #{"ANONYMOUS"
+  #{"ANONYMOUS"
      ;; TODO - are there othere APEX tables we want to skip? Maybe we should make this a pattern instead? (#"^APEX_")
      "APEX_040200"
      "APPQOSSYS"
@@ -274,12 +269,7 @@
      "SYSTEM"
      "WMSYS"
      "XDB"
-     "XS$NULL"}
-   (when config/is-test?
-     ;; DIRTY HACK (!) This is similar hack we do for Redshift, see the explanation there we just want to ignore all
-     ;; the test "session schemas" that don't match the current test
-     (require 'metabase.test.data.oracle)
-     ((resolve 'metabase.test.data.oracle/non-session-schemas)))))
+     "XS$NULL"})
 
 (defmethod sql-jdbc.execute/set-timezone-sql :oracle [_]
   "ALTER session SET time_zone = %s")

modules/drivers/oracle/test/metabase/test/data/oracle.clj

 (ns metabase.test.data.oracle
   (:require [clojure.java.jdbc :as jdbc]
-            [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+            [clojure.set :as set]
+            [metabase
+             [config :as config]
+             [util :as u]]
+            [metabase.driver.sql-jdbc
+             [connection :as sql-jdbc.conn]
+             [sync :as sql-jdbc.sync]]
             [metabase.test.data
              [interface :as tx]
              [sql :as sql.tx]
              [sql-jdbc :as sql-jdbc.tx]]
             [metabase.test.data.sql-jdbc
              [execute :as execute]
-             [load-data :as load-data]]
-            [metabase.util :as u]))
+             [load-data :as load-data]]))
 
 (sql-jdbc.tx/add-test-extensions! :oracle)
 
@@ -103,6 +108,15 @@
   []
   (set (map :username (jdbc/query (dbspec) ["SELECT username FROM dba_users WHERE username <> ?" session-schema]))))
 
+(let [orig (get-method sql-jdbc.sync/excluded-schemas :oracle)]
+  (defmethod sql-jdbc.sync/excluded-schemas :oracle [driver]
+    (set/union
+     (orig driver)
+     (when config/is-test?
+       ;; This is similar hack we do for Redshift, see the explanation there we just want to ignore all the test
+       ;; "session schemas" that don't match the current test
+       (non-session-schemas)))))
+
 
 ;;; Clear out the sesion schema before and after tests run
 ;; TL;DR Oracle schema == Oracle user. Create new user for session-schema

modules/drivers/presto/src/metabase/driver/presto.clj

@@ -13,7 +13,6 @@
              [core :as hsql]
              [helpers :as h]]
             [metabase
-             [config :as config]
              [driver :as driver]
              [util :as u]]
             [metabase.driver.common :as driver.common]
@@ -363,5 +362,4 @@
 (defmethod driver/supports? [:presto :expression-aggregations]         [_ _] true)
 (defmethod driver/supports? [:presto :binning]                         [_ _] true)
 
-;; during unit tests don't treat presto as having FK support
-(defmethod driver/supports? [:presto :foreign-keys] [_ _] (not config/is-test?))
+(defmethod driver/supports? [:presto :foreign-keys] [_ _] true)

modules/drivers/presto/test/metabase/test/data/presto.clj

@@ -4,6 +4,9 @@
             [honeysql
              [core :as hsql]
              [helpers :as h]]
+            [metabase
+             [config :as config]
+             [driver :as driver]]
             [metabase.driver.presto :as presto]
             [metabase.driver.sql.util :as sql.u]
             [metabase.driver.sql.util.unprepare :as unprepare]
@@ -14,6 +17,9 @@
 
 (sql.tx/add-test-extensions! :presto)
 
+;; during unit tests don't treat presto as having FK support
+(defmethod driver/supports? [:presto :foreign-keys] [_ _] (not config/is-test?))
+
 ;;; IDriverTestExtensions implementation
 
 ;; in the past, we had to manually update our Docker image and add a new catalog for every new dataset definition we

modules/drivers/redshift/src/metabase/driver/redshift.clj

@@ -3,14 +3,11 @@
   (:require [clojure.java.jdbc :as jdbc]
             [clojure.string :as str]
             [honeysql.core :as hsql]
-            [metabase
-             [config :as config]
-             [driver :as driver]]
+            [metabase.driver :as driver]
             [metabase.driver.common :as driver.common]
             [metabase.driver.sql-jdbc
              [connection :as sql-jdbc.conn]
-             [execute :as sql-jdbc.execute]
-             [sync :as sql-jdbc.sync]]
+             [execute :as sql-jdbc.execute]]
             [metabase.driver.sql.query-processor :as sql.qp]
             [metabase.util.honeysql-extensions :as hx]))
 
@@ -101,20 +98,3 @@
     :ssl                           true
     :OpenSourceSubProtocolOverride false}
    (dissoc opts :host :port :db)))
-
-;; HACK ! When we test against Redshift we use a session-unique schema so we can run simultaneous tests
-;; against a single remote host; when running tests tell the sync process to ignore all the other schemas
-(def ^:private excluded-schemas
-  (if-not config/is-test?
-    (constantly nil)
-    (memoize
-     (fn []
-       (require 'metabase.test.data.redshift)
-       (let [session-schema-number @(resolve 'metabase.test.data.redshift/session-schema-number)]
-         (set (conj (for [i     (range 240)
-                          :when (not= i session-schema-number)]
-                      (str "schema_" i))
-                    "public")))))))
-
-(defmethod sql-jdbc.sync/excluded-schemas :redshift [_]
-  (excluded-schemas))

modules/drivers/redshift/test/metabase/test/data/redshift.clj

 (ns metabase.test.data.redshift
   (:require [clojure.java.jdbc :as jdbc]
-            [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+            [metabase
+             [config :as config]
+             [util :as u]]
+            [metabase.driver.sql-jdbc
+             [connection :as sql-jdbc.conn]
+             [sync :as sql-jdbc.sync]]
             [metabase.test.data
              [interface :as tx]
              [sql :as sql.tx]]
-            [metabase.test.data.sql.ddl :as ddl]
-            [metabase.util :as u]))
+            [metabase.test.data.sql.ddl :as ddl]))
 
 ;; we don't need to add test extensions here because redshift derives from Postgres and thus already has test
 ;; extensions
@@ -45,6 +49,21 @@
 (defonce ^:private session-schema-name
   (str "schema_" session-schema-number))
 
+;; When we test against Redshift we use a session-unique schema so we can run simultaneous tests
+;; against a single remote host; when running tests tell the sync process to ignore all the other schemas
+(def ^:private excluded-schemas
+  (if-not config/is-test?
+    (constantly nil)
+    (memoize
+     (fn []
+       (set (conj (for [i     (range 240)
+                        :when (not= i session-schema-number)]
+                    (str "schema_" i))
+                  "public"))))))
+
+(defmethod sql-jdbc.sync/excluded-schemas :redshift [_]
+  (excluded-schemas))
+
 (defmethod sql.tx/create-db-sql         :redshift [& _] nil)
 (defmethod sql.tx/drop-db-if-exists-sql :redshift [& _] nil)
 

modules/drivers/sparksql/src/metabase/driver/sparksql.clj

@@ -6,9 +6,7 @@
             [honeysql
              [core :as hsql]
              [helpers :as h]]
-            [metabase
-             [config :as config]
-             [driver :as driver]]
+            [metabase.driver :as driver]
             [metabase.driver.hive-like :as hive-like]
             [metabase.driver.sql
              [query-processor :as sql.qp]
@@ -142,7 +140,6 @@
 (defmethod driver/supports? [:sparksql :nested-queries]                  [_ _] true)
 (defmethod driver/supports? [:sparksql :standard-deviation-aggregations] [_ _] true)
 
-;; during unit tests don't treat Spark SQL as having FK support
-(defmethod driver/supports? [:sparksql :foreign-keys] [_ _] (not config/is-test?))
+(defmethod driver/supports? [:sparksql :foreign-keys] [_ _] true)
 
 (defmethod sql.qp/quote-style :sparksql [_] :mysql)

modules/drivers/sparksql/test/metabase/test/data/sparksql.clj

@@ -3,9 +3,11 @@
             [clojure.string :as str]
             [honeysql
              [core :as hsql]
-             [format :as hformat]
-             [helpers :as h]]
-            [metabase.driver.hive-like :as hive-like]
+             [format :as hformat]]
+            [metabase
+             [config :as config]
+             [driver :as driver]
+             [util :as u]]
             [metabase.driver.sql
              [query-processor :as sql.qp]
              [util :as sql.u]]
@@ -14,16 +16,16 @@
              [interface :as tx]
              [sql :as sql.tx]
              [sql-jdbc :as sql-jdbc.tx]]
-            [metabase.test.data.sql.ddl :as ddl]
             [metabase.test.data.sql-jdbc
              [execute :as execute]
-             [load-data :as load-data]
-             [spec :as spec]]
-            [metabase.util :as u]
-            [metabase.util.honeysql-extensions :as hx]))
+             [load-data :as load-data]]
+            [metabase.test.data.sql.ddl :as ddl]))
 
 (sql-jdbc.tx/add-test-extensions! :sparksql)
 
+;; during unit tests don't treat Spark SQL as having FK support
+(defmethod driver/supports? [:sparksql :foreign-keys] [_ _] (not config/is-test?))
+
 (defmethod sql.tx/field-base-type->sql-type [:sparksql :type/BigInteger] [_ _] "BIGINT")
 (defmethod sql.tx/field-base-type->sql-type [:sparksql :type/Boolean]    [_ _] "BOOLEAN")
 (defmethod sql.tx/field-base-type->sql-type [:sparksql :type/Date]       [_ _] "DATE")

project.clj

@@ -196,7 +196,7 @@
 
    :with-include-drivers-middleware
    {:plugins
-    [[metabase/lein-include-drivers "1.0.7"]]
+    [[metabase/lein-include-drivers "1.0.8"]]
 
     :middleware
     [leiningen.include-drivers/middleware]}

src/metabase/api/email.clj

@@ -75,6 +75,8 @@
   (let [email-settings (select-keys settings (keys mb-to-smtp-settings))
         smtp-settings  (-> (set/rename-keys email-settings mb-to-smtp-settings)
                            (assoc :port (some-> (:email-smtp-port settings) Integer/parseInt)))
+        ;; TODO - this is :thumbs_down`, we should just use `with-redefs` for `email/test-smtp-connection` where
+        ;; needed in the tests. FIXME!
         response       (if-not config/is-test?
                          ;; in normal conditions, validate connection
                          (email/test-smtp-connection smtp-settings)

src/metabase/cmd.clj

@@ -20,6 +20,7 @@
              [config :as config]
              [db :as mdb]
              [util :as u]]
+            [metabase.plugins.classloader :as classloader]
             [metabase.util.date :as du]))
 
 (defn ^:command migrate
@@ -32,7 +33,7 @@
   ([]
    (load-from-h2 nil))
   ([h2-connection-string]
-   (require 'metabase.cmd.load-from-h2)
+   (classloader/require 'metabase.cmd.load-from-h2)
    (binding [mdb/*disable-data-migrations* true]
      ((resolve 'metabase.cmd.load-from-h2/load-from-h2!) h2-connection-string))))
 
@@ -40,20 +41,20 @@
   "Start Metabase the usual way and exit. Useful for profiling Metabase launch time."
   []
   ;; override env var that would normally make Jetty block forever
-  (require 'environ.core 'metabase.core)
+  (classloader/require 'environ.core 'metabase.core)
   (alter-var-root #'environ.core/env assoc :mb-jetty-join "false")
   (du/profile "start-normally" ((resolve 'metabase.core/start-normally))))
 
 (defn ^:command reset-password
   "Reset the password for a user with `email-address`."
   [email-address]
-  (require 'metabase.cmd.reset-password)
+  (classloader/require 'metabase.cmd.reset-password)
   ((resolve 'metabase.cmd.reset-password/reset-password!) email-address))
 
 (defn ^:command refresh-integration-test-db-metadata
   "Re-sync the frontend integration test DB's metadata for the Sample Dataset."
   []
-  (require 'metabase.cmd.refresh-integration-test-db-metadata)
+  (classloader/require 'metabase.cmd.refresh-integration-test-db-metadata)
   ((resolve 'metabase.cmd.refresh-integration-test-db-metadata/refresh-integration-test-db-metadata)))
 
 (defn ^:command help
@@ -89,14 +90,14 @@
   "Generate a markdown file containing documentation for all API endpoints. This is written to a file called
   `docs/api-documentation.md`."
   []
-  (require 'metabase.cmd.endpoint-dox)
+  (classloader/require 'metabase.cmd.endpoint-dox)
   ((resolve 'metabase.cmd.endpoint-dox/generate-dox!)))
 
 (defn ^:command driver-methods
   "Print a list of all multimethods a available for a driver to implement. A useful reference when implementing a new
   driver."
   []
-  (require 'metabase.cmd.driver-methods)
+  (classloader/require 'metabase.cmd.driver-methods)
   ((resolve 'metabase.cmd.driver-methods/print-available-multimethods)))
 
 

src/metabase/cmd/driver_methods.clj

@@ -2,6 +2,7 @@
   (:require [clojure.java.classpath :as classpath]
             [clojure.string :as str]
             [clojure.tools.namespace.find :as ns-find]
+            [metabase.plugins.classloader :as classloader]
             [metabase.util :as u]))
 
 (defn- driver-ns-symbs []
@@ -11,7 +12,7 @@
          :when   (and (or (starts-with? "metabase.driver")
                           (starts-with? "metabase.test.data"))
                       (do
-                        (u/ignore-exceptions (require ns-symb))
+                        (u/ignore-exceptions (classloader/require ns-symb))
                         (find-ns ns-symb)))]
      ns-symb)))
 

src/metabase/cmd/endpoint_dox.clj

@@ -4,7 +4,8 @@
             [clojure.string :as str]
             [metabase
              [config :as config]
-             [util :as u]]))
+             [util :as u]]
+            [metabase.plugins.classloader :as classloader]))
 
 (defn- dox
   "Generate a Markdown string containing documentation for all Metabase API endpoints."
@@ -14,7 +15,7 @@
        "\n\n"
        (str/join "\n\n\n" (for [ns-symb     @u/metabase-namespace-symbols
                                 :when       (.startsWith (name ns-symb) "metabase.api.")
-                                [symb varr] (do (require ns-symb)
+                                [symb varr] (do (classloader/require ns-symb)
                                                 (sort (ns-interns ns-symb)))
                                 :when       (:is-endpoint? (meta varr))]
                             (:doc (meta varr))))))

src/metabase/config.clj

@@ -3,7 +3,8 @@
              [io :as io]
              [shell :as shell]]
             [clojure.string :as str]
-            [environ.core :as environ])
+            [environ.core :as environ]
+            [metabase.plugins.classloader :as classloader])
   (:import clojure.lang.Keyword))
 
 (def ^Boolean is-windows?
@@ -106,7 +107,7 @@
 ;; If for some wacky reason the test namespaces are getting loaded (e.g. when running via
 ;; `lein ring` or `lein ring sever`, DO NOT RUN THE EXPECTATIONS TESTS AT SHUTDOWN! THIS WILL NUKE YOUR APPLICATION DB
 (try
-  (require 'expectations)
+  (classloader/require 'expectations)
   ((resolve 'expectations/disable-run-on-shutdown))
   ;; This will fail if the test dependencies aren't present (e.g. in a JAR situation) which is totally fine
   (catch Throwable _))

src/metabase/core.clj

@@ -19,6 +19,7 @@
             [metabase.models
              [setting :as setting]
              [user :refer [User]]]
+            [metabase.plugins.classloader :as classloader]
             [metabase.util.i18n :refer [set-locale trs]]
             [toucan.db :as db]))
 
@@ -127,7 +128,7 @@
       (System/exit 1))))
 
 (defn- run-cmd [cmd args]
-  (require 'metabase.cmd)
+  (classloader/require 'metabase.cmd)
   ((resolve 'metabase.cmd/run-cmd) cmd args))