Use UTF8/InnoDB for new MySQL tables (#11742)

* Move Liquibase code to different namespace

* Append ENGINE/CHARACTER SET/COLLATE to all MySQL CREATE TABLE statements
[ci mysql]

* Test fixes; make sure read-column impls check for nil [ci drivers]

java/metabase/db/liquibase/MetabaseMySqlCreateTableSqlGenerator.java

+package metabase.db.liquibase;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import liquibase.database.Database;
+import liquibase.database.core.MySQLDatabase;
+import liquibase.exception.ValidationErrors;
+import liquibase.sql.Sql;
+import liquibase.sql.UnparsedSql;
+import liquibase.sqlgenerator.SqlGeneratorChain;
+import liquibase.sqlgenerator.core.AbstractSqlGenerator;
+import liquibase.sqlgenerator.core.CreateTableGenerator;
+import liquibase.statement.core.CreateTableStatement;
+import liquibase.structure.DatabaseObject;
+
+// This class is a simple wrapper around a CreateTableGenerator that appends ENGINE InnoDB CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci to the generated SQL
+public class MetabaseMySqlCreateTableSqlGenerator extends AbstractSqlGenerator<CreateTableStatement> {
+    private CreateTableGenerator parentGenerator;
+
+    public MetabaseMySqlCreateTableSqlGenerator() {
+        this.parentGenerator = new CreateTableGenerator();
+    }
+
+    @Override
+    public boolean supports(CreateTableStatement statement, Database database) {
+        return parentGenerator.supports(statement, database) && (database instanceof MySQLDatabase);
+    }
+
+    @Override
+    public int getPriority() {
+        return parentGenerator.getPriority() + 1;
+    }
+
+    @Override
+    public Sql[] generateSql(CreateTableStatement statement, Database database, SqlGeneratorChain sqlGeneratorChain) {
+        Sql[] sqls = this.parentGenerator.generateSql(statement, database, sqlGeneratorChain);
+        for (int i = 0; i < sqls.length; i++) {
+            Sql sql = sqls[i];
+            if (!sql.toSql().startsWith("CREATE TABLE")) continue;
+
+            Collection<? extends DatabaseObject> affectedObjects = sql.getAffectedDatabaseObjects();
+
+            sqls[i] = new UnparsedSql(sql.toSql() + " ENGINE InnoDB CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci",
+                                      sql.getEndDelimiter(),
+                                      affectedObjects.toArray(new DatabaseObject[affectedObjects.size()]));
+        }
+        return sqls;
+    }
+
+    @Override
+    public ValidationErrors validate(CreateTableStatement statement, Database database, SqlGeneratorChain sqlGeneratorChain) {
+        return this.parentGenerator.validate(statement, database, sqlGeneratorChain);
+    }
+}

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

@@ -294,12 +294,12 @@
   ;; try both and wrap the first in a try-catch. As far as I know there's now way to tell whether the value has a zone
   ;; offset or ID without first fetching a `TIMESTAMPTZ` object. So to avoid the try-catch we can fetch the
   ;; `TIMESTAMPTZ` and use `.offsetDateTimeValue` instead.
-  (let [^TIMESTAMPTZ t                  (.getObject rs i TIMESTAMPTZ)
-        ^C3P0ProxyConnection proxy-conn (.. rs getStatement getConnection)
-        conn                            (.unwrap proxy-conn OracleConnection)]
-    ;; TIMEZONE FIXME - we need to warn if the Oracle JDBC driver is `ojdbc7.jar`, which probably won't have this method
-    ;; I think we can call `(oracle.jdbc.OracleDriver/getJDBCVersion)` and check whether it returns 4.2+
-    (.offsetDateTimeValue t conn)))
+  (when-let [^TIMESTAMPTZ t (.getObject rs i TIMESTAMPTZ)]
+    (let [^C3P0ProxyConnection proxy-conn (.. rs getStatement getConnection)
+          conn                            (.unwrap proxy-conn OracleConnection)]
+      ;; TIMEZONE FIXME - we need to warn if the Oracle JDBC driver is `ojdbc7.jar`, which probably won't have this method
+      ;; I think we can call `(oracle.jdbc.OracleDriver/getJDBCVersion)` and check whether it returns 4.2+
+      (.offsetDateTimeValue t conn))))
 
 (defmethod unprepare/unprepare-value [:oracle OffsetDateTime]
   [_ t]

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

@@ -274,17 +274,17 @@
 ;; return a String that we can parse
 (defmethod sql-jdbc.execute/read-column [:snowflake Types/TIME]
   [_ _ ^ResultSet rs _ ^Integer i]
-  (let [s (.getString rs i)
-        t (u.date/parse s)]
-    (log/tracef "(.getString rs %d) [TIME] -> %s -> %s" i s t)
-    t))
+  (when-let [s (.getString rs i)]
+    (let [t (u.date/parse s)]
+      (log/tracef "(.getString rs %d) [TIME] -> %s -> %s" i s t)
+      t)))
 
 (defmethod sql-jdbc.execute/read-column [:snowflake Types/TIME_WITH_TIMEZONE]
   [_ _ ^ResultSet rs _ ^Integer i]
-  (let [s (.getString rs i)
-        t (u.date/parse s)]
-    (log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i s t)
-    t))
+  (when-let [s (.getString rs i)]
+    (let [t (u.date/parse s)]
+      (log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i s t)
+      t)))
 
 ;; TODO ­ would it make more sense to use functions like `timestamp_tz_from_parts` directly instead of JDBC parameters?
 

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

@@ -172,12 +172,15 @@
 ;; TIMEZONE FIXME — not sure what timezone the results actually come back as
 (defmethod sql-jdbc.execute/read-column [:hive-like Types/TIME]
   [_ _ ^ResultSet rs rsmeta ^Integer i]
-  (t/offset-time (t/local-time (.getTimestamp rs i)) (t/zone-offset 0)))
+  (when-let [t (.getTimestamp rs i)]
+    (t/offset-time (t/local-time t) (t/zone-offset 0))))
 
 (defmethod sql-jdbc.execute/read-column [:hive-like Types/DATE]
   [_ _ ^ResultSet rs rsmeta ^Integer i]
-  (t/zoned-date-time (t/local-date (.getDate rs i)) (t/local-time 0) (t/zone-id "UTC")))
+  (when-let [t (.getDate rs i)]
+    (t/zoned-date-time (t/local-date t) (t/local-time 0) (t/zone-id "UTC"))))
 
 (defmethod sql-jdbc.execute/read-column [:hive-like Types/TIMESTAMP]
   [_ _ ^ResultSet rs rsmeta ^Integer i]
-  (t/zoned-date-time (t/local-date-time (.getTimestamp rs i)) (t/zone-id "UTC")))
+  (when-let [t (.getTimestamp rs i)]
+    (t/zoned-date-time (t/local-date-time t) (t/zone-id "UTC"))))

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

@@ -288,6 +288,8 @@
 (defmethod sql-jdbc.execute/read-column [:sqlite Types/DATE]
   [_ _ ^ResultSet rs _ ^Integer i]
   (try
-    (t/local-date (.getDate rs i))
+    (when-let [t (.getDate rs i)]
+      (t/local-date t))
     (catch Throwable _
-      (u.date/parse (.getString rs i) (qp.timezone/results-timezone-id)))))
+      (when-let [s (.getString rs i)]
+        (u.date/parse s (qp.timezone/results-timezone-id))))))

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

@@ -129,14 +129,14 @@
 
 (defmethod sql-jdbc.execute/read-column [:vertica Types/TIME]
   [_ _ ^ResultSet rs _ ^Integer i]
-  (let [s (.getString rs i)
-        t (u.date/parse s)]
-    (log/tracef "(.getString rs %d) [TIME] -> %s -> %s" i s t)
-    t))
+  (when-let [s (.getString rs i)]
+    (let [t (u.date/parse s)]
+      (log/tracef "(.getString rs %d) [TIME] -> %s -> %s" i s t)
+      t)))
 
 (defmethod sql-jdbc.execute/read-column [:vertica Types/TIME_WITH_TIMEZONE]
   [_ _ ^ResultSet rs _ ^Integer i]
-  (let [s (.getString rs i)
-        t (u.date/parse s)]
-    (log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i s t)
-    t))
+  (when-let [s (.getString rs i)]
+    (let [t (u.date/parse s)]
+      (log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i s t)
+      t)))

project.clj

@@ -156,6 +156,9 @@
   :javac-options
   ["-target" "1.8", "-source" "1.8"]
 
+  :java-source-paths
+  ["java"]
+
   :uberjar-name
   "metabase.jar"
 

src/metabase/cmd/dump_to_h2.clj

@@ -106,7 +106,7 @@
 
 (defn- h2-details [h2-connection-string-or-nil]
   (let [h2-filename (add-file-prefix-if-needed h2-connection-string-or-nil)]
-    (mdb/jdbc-details {:type :h2, :db h2-filename})))
+    (mdb/jdbc-spec {:type :h2, :db h2-filename})))
 
 
 ;;; ------------------------------------------- Fetching & Inserting Rows --------------------------------------------
@@ -151,8 +151,8 @@
   (println-ok))
 
 (defn- load-data! [target-db-conn]
-  (println "Source db:" (mdb/jdbc-details))
-  (jdbc/with-db-connection [db-conn (mdb/jdbc-details)]
+  (println "Source db:" (mdb/jdbc-spec))
+  (jdbc/with-db-connection [db-conn (mdb/jdbc-spec)]
     (doseq [{table-name :table, :as e} entities
             :let [rows (jdbc/query db-conn [(str "SELECT * FROM " (name table-name))])]
             :when (seq rows)]

src/metabase/cmd/load_from_h2.clj

@@ -113,7 +113,7 @@
 
 (defn- h2-details [h2-connection-string-or-nil]
   (let [h2-filename (add-file-prefix-if-needed (or h2-connection-string-or-nil @metabase.db/db-file))]
-    (mdb/jdbc-details {:type :h2, :db (str h2-filename ";IFEXISTS=TRUE")})))
+    (mdb/jdbc-spec {:type :h2, :db (str h2-filename ";IFEXISTS=TRUE")})))
 
 
 ;;; ------------------------------------------- Fetching & Inserting Rows --------------------------------------------
@@ -214,7 +214,7 @@
 
 ;; Update the sequence nextvals.
 (defmethod update-sequence-values! :postgres []
-  (jdbc/with-db-transaction [target-db-conn (mdb/jdbc-details)]
+  (jdbc/with-db-transaction [target-db-conn (mdb/jdbc-spec)]
     (println (u/format-color 'blue "Setting postgres sequence ids to proper values..."))
     (doseq [e     entities
             :when (not (contains? entities-without-autoinc-ids e))
@@ -242,7 +242,7 @@
   (assert (#{:postgres :mysql} (mdb/db-type))
     (trs "Metabase can only transfer data from H2 to Postgres or MySQL/MariaDB."))
 
-  (jdbc/with-db-transaction [target-db-conn (mdb/jdbc-details)]
+  (jdbc/with-db-transaction [target-db-conn (mdb/jdbc-spec)]
     (jdbc/db-set-rollback-only! target-db-conn)
 
     (println (u/format-color 'blue "Testing if target DB is already populated..."))

src/metabase/db.clj

 (ns metabase.db
   "Application database definition, and setup logic, and helper functions for interacting with it."
-  (:require [clojure
-             [string :as str]
-             [walk :as walk]]
-            [clojure.java
+  (:require [clojure.java
              [io :as io]
              [jdbc :as jdbc]]
             [clojure.tools.logging :as log]
+            [clojure.walk :as walk]
             [metabase
              [config :as config]
              [connection-pool :as connection-pool]
              [util :as u]]
             [metabase.db
              [jdbc-protocols :as db.jdbc-protocols]
+             [liquibase :as liquibase]
              [spec :as db.spec]]
             [metabase.plugins.classloader :as classloader]
             [metabase.util
@@ -21,12 +20,7 @@
             [ring.util.codec :as codec]
             [schema.core :as s]
             [toucan.db :as db])
-  (:import java.io.StringWriter
-           [liquibase Contexts Liquibase]
-           [liquibase.database Database DatabaseFactory]
-           liquibase.database.jvm.JdbcConnection
-           liquibase.exception.LockException
-           liquibase.resource.ClassLoaderResourceAccessor))
+  (:import liquibase.exception.LockException))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                          DB FILE & CONNECTION DETAILS                                          |
@@ -124,8 +118,9 @@
            "https://metabase.com/docs/latest/operations-guide/migrating-from-h2.html"))))
    (or @connection-string-details
        (case (db-type)
-         :h2       {:type     :h2 ; TODO - we probably don't need to specifc `:type` here since we can just call (db-type)
-                    :db       @db-file}
+         ;; TODO - we probably don't need to specifc `:type` here since we can just call (db-type)
+         :h2       {:type :h2
+                    :db   @db-file}
          :mysql    {:type     :mysql
                     :host     (config/config-str :mb-db-host)
                     :port     (config/config-int :mb-db-port)
@@ -139,10 +134,11 @@
                     :user     (config/config-str :mb-db-user)
                     :password (config/config-str :mb-db-pass)}))))
 
-(defn jdbc-details
+(defn jdbc-spec
   "Takes our own MB details map and formats them properly for connection details for JDBC."
   ([]
-   (jdbc-details @db-connection-details))
+   (jdbc-spec @db-connection-details))
+
   ([db-details]
    {:pre [(map? db-details)]}
    ;; TODO: it's probably a good idea to put some more validation here and be really strict about what's in `db-details`
@@ -156,147 +152,6 @@
 ;;; |                                                    MIGRATE!                                                    |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(def ^:private ^String changelog-file "liquibase.yaml")
-
-(defn- migrations-sql
-  "Return a string of SQL containing the DDL statements needed to perform unrun `liquibase` migrations."
-  ^String [^Liquibase liquibase]
-  (let [writer (StringWriter.)]
-    (.update liquibase "" writer)
-    (.toString writer)))
-
-(defn- migrations-lines
-  "Return a sequnce of DDL statements that should be used to perform migrations for `liquibase`.
-
-  MySQL gets snippy if we try to run the entire DB migration as one single string; it seems to only like it if we run
-  one statement at a time; Liquibase puts each DDL statement on its own line automatically so just split by lines and
-  filter out blank / comment lines. Even though this is not necessary for H2 or Postgres go ahead and do it anyway
-  because it keeps the code simple and doesn't make a significant performance difference.
-
-  As of 0.31.1 this is only used for printing the migrations without running or using force migrating."
-  [^Liquibase liquibase]
-  (for [line  (str/split-lines (migrations-sql liquibase))
-        :when (not (or (str/blank? line)
-                       (re-find #"^--" line)))]
-    line))
-
-(defn- has-unrun-migrations?
-  "Does `liquibase` have migration change sets that haven't been run yet?
-
-  It's a good idea to check to make sure there's actually something to do before running `(migrate :up)` so we can
-  skip creating and releasing migration locks, which is both slightly dangerous and a waste of time when we won't be
-  using them.
-
-  (I'm not 100% sure whether `Liquibase.update()` still acquires locks if the database is already up-to-date, but
-  `migrations-lines` certainly does; duplicating the skipping logic doesn't hurt anything.)"
-  ^Boolean [^Liquibase liquibase]
-  (boolean (seq (.listUnrunChangeSets liquibase nil))))
-
-(defn- migration-lock-exists?
-  "Is a migration lock in place for `liquibase`?"
-  ^Boolean [^Liquibase liquibase]
-  (boolean (seq (.listLocks liquibase))))
-
-(defn- wait-for-migration-lock-to-be-cleared
-  "Check and make sure the database isn't locked. If it is, sleep for 2 seconds and then retry several times. There's a
-  chance the lock will end up clearing up so we can run migrations normally."
-  [^Liquibase liquibase]
-  (u/auto-retry 5
-    (when (migration-lock-exists? liquibase)
-      (Thread/sleep 2000)
-      (throw
-       (LockException.
-        (str
-         (trs "Database has migration lock; cannot run migrations.")
-         " "
-         (trs "You can force-release these locks by running `java -jar metabase.jar migrate release-locks`.")))))))
-
-(defn- migrate-up-if-needed!
-  "Run any unrun `liquibase` migrations, if needed."
-  [conn, ^Liquibase liquibase]
-  (log/info (trs "Checking if Database has unrun migrations..."))
-  (when (has-unrun-migrations? liquibase)
-    (log/info (trs "Database has unrun migrations. Waiting for migration lock to be cleared..."))
-    (wait-for-migration-lock-to-be-cleared liquibase)
-    ;; while we were waiting for the lock, it was possible that another instance finished the migration(s), so make
-    ;; sure something still needs to be done...
-    (if (has-unrun-migrations? liquibase)
-      (do
-        (log/info (trs "Migration lock is cleared. Running migrations..."))
-        (u/auto-retry 3 (let [^Contexts contexts nil] (.update liquibase contexts))))
-      (log/info
-        (trs "Migration lock cleared, but nothing to do here! Migrations were finished by another instance.")))))
-
-(defn- force-migrate-up-if-needed!
-  "Force migrating up. This does two things differently from `migrate-up-if-needed!`:
-
-  1.  This doesn't check to make sure the DB locks are cleared
-  2.  This generates a sequence of individual DDL statements with `migrations-lines` and runs them each in turn
-  3.  Any DDL statements that fail are ignored
-
-  It can be used to fix situations where the database got into a weird state, as was common before the fixes made in
-  #3295.
-
-  Each DDL statement is ran inside a nested transaction; that way if the nested transaction fails we can roll it back
-  without rolling back the entirety of changes that were made. (If a single statement in a transaction fails you can't
-  do anything futher until you clear the error state by doing something like calling `.rollback`.)"
-  [conn, ^Liquibase liquibase]
-  (.clearCheckSums liquibase)
-  (when (has-unrun-migrations? liquibase)
-    (doseq [line (migrations-lines liquibase)]
-      (log/info line)
-      (jdbc/with-db-transaction [nested-transaction-connection conn]
-        (try (jdbc/execute! nested-transaction-connection [line])
-             (log/info (u/format-color 'green "[SUCCESS]"))
-             (catch Throwable e
-               (.rollback (jdbc/get-connection nested-transaction-connection))
-               (log/error (u/format-color 'red "[ERROR] %s" (.getMessage e)))))))))
-
-(def ^{:arglists '([])} ^DatabaseFactory database-factory
-  "Return an instance of the Liquibase `DatabaseFactory`. This is done on a background thread at launch because
-  otherwise it adds 2 seconds to startup time."
-  (when-not *compile-files*
-    (partial deref (future (DatabaseFactory/getInstance)))))
-
-(defn- conn->liquibase
-  "Get a `Liquibase` object from JDBC `conn`."
-  (^Liquibase []
-   (conn->liquibase (jdbc-details)))
-
-  (^Liquibase [conn]
-   (let [^JdbcConnection liquibase-conn (JdbcConnection. (jdbc/get-connection conn))
-         ^Database       database       (.findCorrectDatabaseImplementation (database-factory) liquibase-conn)]
-     (Liquibase. changelog-file (ClassLoaderResourceAccessor.) database))))
-
-(defn- consolidate-liquibase-changesets!
-  "Consolidate all previous DB migrations so they come from single file.
-
-  Previously migrations where stored in many small files which added seconds per file to the startup time because
-  liquibase was checking the jar signature for each file. This function is required to correct the liquibase tables to
-  reflect that these migrations where moved to a single file.
-
-  see https://github.com/metabase/metabase/issues/3715"
-  [conn]
-  (let [liquibase-table-name (if (#{:h2 :mysql} (:type conn))
-                               "DATABASECHANGELOG"
-                               "databasechangelog")
-        fresh-install?       (jdbc/with-db-metadata [meta (jdbc-details conn)] ;; don't migrate on fresh install
-                               (empty? (jdbc/metadata-query
-                                        (.getTables meta nil nil liquibase-table-name (u/varargs String ["TABLE"])))))
-        statement            (format "UPDATE %s SET FILENAME = ?" liquibase-table-name)]
-    (when-not fresh-install?
-      (jdbc/execute! conn [statement "migrations/000_migrations.yaml"]))))
-
-(defn- release-lock-if-needed!
-  "Attempts to release the liquibase lock if present. Logs but does not bubble up the exception if one occurs as it's
-  intended to be used when a failure has occurred and bubbling up this exception would hide the real exception."
-  [^Liquibase liquibase]
-  (when (migration-lock-exists? liquibase)
-    (try
-      (.forceReleaseLocks liquibase)
-      (catch Exception e
-        (log/error e (trs "Unable to release the Liquibase lock after a migration failure"))))))
-
 (defn migrate!
   "Migrate the database (this can also be ran via command line like `java -jar metabase.jar migrate up` or `lein run
   migrate up`):
@@ -318,23 +173,23 @@
    (migrate! @db-connection-details direction))
 
   ([db-details direction]
-   (jdbc/with-db-transaction [conn (jdbc-details db-details)]
+   (jdbc/with-db-transaction [conn (jdbc-spec db-details)]
      ;; Tell transaction to automatically `.rollback` instead of `.commit` when the transaction finishes
      (jdbc/db-set-rollback-only! conn)
      ;; Disable auto-commit. This should already be off but set it just to be safe
      (.setAutoCommit (jdbc/get-connection conn) false)
      ;; Set up liquibase and let it do its thing
      (log/info (trs "Setting up Liquibase..."))
-     (let [liquibase (conn->liquibase conn)]
+     (liquibase/with-liquibase [liquibase conn]
        (try
-         (consolidate-liquibase-changesets! conn)
+         (liquibase/consolidate-liquibase-changesets! conn)
          (log/info (trs "Liquibase is ready."))
          (case direction
-           :up            (migrate-up-if-needed! conn liquibase)
-           :force         (force-migrate-up-if-needed! conn liquibase)
-           :down-one      (.rollback liquibase 1 "")
-           :print         (println (migrations-sql liquibase))
-           :release-locks (.forceReleaseLocks liquibase))
+           :up            (liquibase/migrate-up-if-needed! conn liquibase)
+           :force         (liquibase/force-migrate-up-if-needed! conn liquibase)
+           :down-one      (liquibase/rollback-one liquibase)
+           :print         (println (liquibase/migrations-sql liquibase))
+           :release-locks (liquibase/force-release-locks! liquibase))
          ;; Migrations were successful; disable rollback-only so `.commit` will be called instead of `.rollback`
          (jdbc/db-unset-rollback-only! conn)
          :done
@@ -353,7 +208,7 @@
            (.rollback (jdbc/get-connection conn))
            ;; With some failures, it's possible that the lock won't be released. To make this worse, if we retry the
            ;; operation without releasing the lock first, the real error will get hidden behind a lock error
-           (release-lock-if-needed! liquibase)
+           (liquibase/release-lock-if-needed! liquibase)
            (throw e)))))))
 
 
@@ -438,23 +293,11 @@
   [auto-migrate? db-details]
   (log/info (trs "Running Database Migrations..."))
   (if auto-migrate?
-    ;; There is a weird situation where running the migrations can cause a race condition: if two (or more) instances
-    ;; in a horizontal cluster are started at the exact same time, they can all start running migrations (and all
-    ;; acquire a lock) at the exact same moment. Since they all acquire a lock at the same time, none of them would
-    ;; have been blocked from starting by the lock being in place. (Yes, this not working sort of defeats the whole
-    ;; purpose of the lock in the first place, but this *is* Liquibase.)
-    ;;
-    ;; So what happens is one instance will ultimately end up commiting the transaction first (and succeed), while the
-    ;; others will fail due to duplicate tables or the like and have their transactions rolled back.
-    ;;
-    ;; However, we don't want to have that instance killed because its migrations failed for that reason, so retry a
-    ;; second time; this time, it will either run into the lock, or see that there are no migrations to run in the
-    ;; first place, and launch normally.
-    (u/auto-retry 1
-      (migrate! db-details :up))
+    (migrate! db-details :up)
     ;; if `MB_DB_AUTOMIGRATE` is false, and we have migrations that need to be ran, print and quit. Otherwise continue
     ;; to start normally
-    (when (has-unrun-migrations? (conn->liquibase))
+    (when (liquibase/with-liquibase [liquibase (jdbc-spec)]
+            (liquibase/has-unrun-migrations? liquibase))
       (print-migrations-and-quit! db-details)))
   (log/info (trs "Database Migrations Current ... ") (u/emoji "✅")))
 
@@ -465,7 +308,6 @@
     (classloader/require 'metabase.db.migrations)
     ((resolve 'metabase.db.migrations/run-all!))))
 
-
 (defn setup-db!*
   "Connects to db and runs migrations."
   [db-details auto-migrate]
@@ -473,7 +315,7 @@
     (u/with-us-locale
       (verify-db-connection db-details)
       (run-schema-migrations! auto-migrate db-details)
-      (create-connection-pool! (jdbc-details db-details))
+      (create-connection-pool! (jdbc-spec db-details))
       (run-data-migrations!)))
   nil)
 

src/metabase/db/jdbc_protocols.clj

@@ -110,7 +110,7 @@
     ;;
     ;; Check and see if the column type is `TIMESTAMP` (as opposed to `DATETIME`, which is the equivalent of
     ;; LocalDateTime), and normalize it to a UTC timestamp if so.
-    (let [t (.getObject rs i LocalDateTime)]
+    (when-let [t (.getObject rs i LocalDateTime)]
       (if (= (.getColumnTypeName rsmeta i) "TIMESTAMP")
         (t/with-offset-same-instant (t/offset-date-time t (t/zone-id)) (t/zone-offset 0))
         t))
@@ -135,7 +135,7 @@
     (try
       (.getObject rs i LocalTime)
       (catch Throwable _
-        (let [s (.getString rs i)]
+        (when-let [s (.getString rs i)]
           (log/tracef "Error in Postgres JDBC driver reading TIME value, fetching as string '%s'" s)
           (u.date/parse s))))
 

src/metabase/db/liquibase.clj

+(ns metabase.db.liquibase
+  (:require [clojure.java.jdbc :as jdbc]
+            [clojure.string :as str]
+            [clojure.tools.logging :as log]
+            [metabase.util :as u]
+            [metabase.util.i18n :refer [trs]])
+  (:import java.io.StringWriter
+           [liquibase Contexts Liquibase]
+           [liquibase.database Database DatabaseFactory]
+           liquibase.database.jvm.JdbcConnection
+           liquibase.exception.LockException
+           liquibase.resource.ClassLoaderResourceAccessor
+           liquibase.sqlgenerator.SqlGeneratorFactory
+           metabase.db.liquibase.MetabaseMySqlCreateTableSqlGenerator))
+
+(.register (SqlGeneratorFactory/getInstance) (MetabaseMySqlCreateTableSqlGenerator.))
+
+(def ^:private ^String changelog-file "liquibase.yaml")
+
+(defn- liquibase-connection ^JdbcConnection [^java.sql.Connection jdbc-connection]
+  (JdbcConnection. jdbc-connection))
+
+(defn- database ^Database [^JdbcConnection liquibase-conn]
+  (.findCorrectDatabaseImplementation (DatabaseFactory/getInstance) liquibase-conn))
+
+(defn- liquibase ^Liquibase [^Database database]
+  (Liquibase. changelog-file (ClassLoaderResourceAccessor.) database))
+
+(defn do-with-liquibase
+  "Impl for `with-liquibase-macro`."
+  [jdbc-spec-or-conn f]
+  ;; closing the `LiquibaseConnection`/`Database` closes the parent JDBC `Connection`, so only use it in combination
+  ;; with `with-open` *if* we are opening a new JDBC `Connection` from a JDBC spec. If we're passed in a `Connection`,
+  ;; it's safe to assume the caller is managing its lifecycle.
+  (letfn []
+    (cond
+      (instance? java.sql.Connection jdbc-spec-or-conn)
+      (f (-> jdbc-spec-or-conn liquibase-connection database liquibase))
+
+      (:connection jdbc-spec-or-conn)
+      (recur (:connection jdbc-spec-or-conn) f)
+
+      :else
+      (with-open [jdbc-conn      (jdbc/get-connection jdbc-spec-or-conn)
+                  liquibase-conn (liquibase-connection jdbc-conn)
+                  database       (database liquibase-conn)]
+        (f (liquibase database))))))
+
+(defmacro with-liquibase
+  "Execute body with an instance of a `Liquibase` bound to `liquibase-binding`.
+
+    (liquibase/with-liquibase [liquibase {:subname :postgres, ...}]
+      (liquibase/migrate-up-if-needed! liquibase))"
+  {:style/indent 1}
+  [[liquibase-binding jdbc-spec-or-conn] & body]
+  `(do-with-liquibase
+    ~jdbc-spec-or-conn
+    (fn [~(vary-meta liquibase-binding assoc :tag (symbol (.getCanonicalName Liquibase)))]
+      ~@body)))
+
+(defn migrations-sql
+  "Return a string of SQL containing the DDL statements needed to perform unrun `liquibase` migrations."
+  ^String [^Liquibase liquibase]
+  (let [writer (StringWriter.)]
+    (.update liquibase "" writer)
+    (.toString writer)))
+
+(defn migrations-lines
+  "Return a sequnce of DDL statements that should be used to perform migrations for `liquibase`.
+
+  MySQL gets snippy if we try to run the entire DB migration as one single string; it seems to only like it if we run
+  one statement at a time; Liquibase puts each DDL statement on its own line automatically so just split by lines and
+  filter out blank / comment lines. Even though this is not necessary for H2 or Postgres go ahead and do it anyway
+  because it keeps the code simple and doesn't make a significant performance difference.
+
+  As of 0.31.1 this is only used for printing the migrations without running or using force migrating."
+  [^Liquibase liquibase]
+  (for [line  (str/split-lines (migrations-sql liquibase))
+        :when (not (or (str/blank? line)
+                       (re-find #"^--" line)))]
+    line))
+
+(defn has-unrun-migrations?
+  "Does `liquibase` have migration change sets that haven't been run yet?
+
+  It's a good idea to check to make sure there's actually something to do before running `(migrate :up)` so we can
+  skip creating and releasing migration locks, which is both slightly dangerous and a waste of time when we won't be
+  using them.
+
+  (I'm not 100% sure whether `Liquibase.update()` still acquires locks if the database is already up-to-date, but
+  `migrations-lines` certainly does; duplicating the skipping logic doesn't hurt anything.)"
+  ^Boolean [^Liquibase liquibase]
+  (boolean (seq (.listUnrunChangeSets liquibase nil))))
+
+(defn- migration-lock-exists?
+  "Is a migration lock in place for `liquibase`?"
+  ^Boolean [^Liquibase liquibase]
+  (boolean (seq (.listLocks liquibase))))
+
+(defn- wait-for-migration-lock-to-be-cleared
+  "Check and make sure the database isn't locked. If it is, sleep for 2 seconds and then retry several times. There's a
+  chance the lock will end up clearing up so we can run migrations normally."
+  [^Liquibase liquibase]
+  (u/auto-retry 5
+    (when (migration-lock-exists? liquibase)
+      (Thread/sleep 2000)
+      (throw
+       (LockException.
+        (str
+         (trs "Database has migration lock; cannot run migrations.")
+         " "
+         (trs "You can force-release these locks by running `java -jar metabase.jar migrate release-locks`.")))))))
+
+(defn migrate-up-if-needed!
+  "Run any unrun `liquibase` migrations, if needed."
+  [conn, ^Liquibase liquibase]
+  (log/info (trs "Checking if Database has unrun migrations..."))
+  (when (has-unrun-migrations? liquibase)
+    (log/info (trs "Database has unrun migrations. Waiting for migration lock to be cleared..."))
+    (wait-for-migration-lock-to-be-cleared liquibase)
+    ;; while we were waiting for the lock, it was possible that another instance finished the migration(s), so make
+    ;; sure something still needs to be done...
+    (if (has-unrun-migrations? liquibase)
+      (do
+        (log/info (trs "Migration lock is cleared. Running migrations..."))
+        (u/auto-retry 3 (let [^Contexts contexts nil] (.update liquibase contexts))))
+      (log/info
+        (trs "Migration lock cleared, but nothing to do here! Migrations were finished by another instance.")))))
+
+(defn force-migrate-up-if-needed!
+  "Force migrating up. This does two things differently from `migrate-up-if-needed!`:
+
+  1.  This doesn't check to make sure the DB locks are cleared
+  2.  This generates a sequence of individual DDL statements with `migrations-lines` and runs them each in turn
+  3.  Any DDL statements that fail are ignored
+
+  It can be used to fix situations where the database got into a weird state, as was common before the fixes made in
+  #3295.
+
+  Each DDL statement is ran inside a nested transaction; that way if the nested transaction fails we can roll it back
+  without rolling back the entirety of changes that were made. (If a single statement in a transaction fails you can't
+  do anything futher until you clear the error state by doing something like calling `.rollback`.)"
+  [conn, ^Liquibase liquibase]
+  (.clearCheckSums liquibase)
+  (when (has-unrun-migrations? liquibase)
+    (doseq [line (migrations-lines liquibase)]
+      (log/info line)
+      (jdbc/with-db-transaction [nested-transaction-connection conn]
+        (try (jdbc/execute! nested-transaction-connection [line])
+             (log/info (u/format-color 'green "[SUCCESS]"))
+             (catch Throwable e
+               (.rollback (jdbc/get-connection nested-transaction-connection))
+               (log/error (u/format-color 'red "[ERROR] %s" (.getMessage e)))))))))
+
+(defn consolidate-liquibase-changesets!
+  "Consolidate all previous DB migrations so they come from single file.
+
+  Previously migrations where stored in many small files which added seconds per file to the startup time because
+  liquibase was checking the jar signature for each file. This function is required to correct the liquibase tables to
+  reflect that these migrations where moved to a single file.
+
+  see https://github.com/metabase/metabase/issues/3715"
+  [jdbc-spec]
+  (let [liquibase-table-name (if (#{:h2 :mysql} (:type jdbc-spec))
+                               "DATABASECHANGELOG"
+                               "databasechangelog")
+        fresh-install?       (jdbc/with-db-metadata [meta jdbc-spec] ;; don't migrate on fresh install
+                               (empty? (jdbc/metadata-query
+                                        (.getTables meta nil nil liquibase-table-name (u/varargs String ["TABLE"])))))
+        statement            (format "UPDATE %s SET FILENAME = ?" liquibase-table-name)]
+    (when-not fresh-install?
+      (jdbc/execute! jdbc-spec [statement "migrations/000_migrations.yaml"]))))
+
+(defn rollback-one
+  "Roll back the last migration."
+  [^Liquibase liquibase]
+  (.rollback liquibase 1 ""))
+
+(defn force-release-locks!
+  "(Attempt to) force release Liquibase migration locks."
+  [^Liquibase liquibase]
+  (.forceReleaseLocks liquibase))
+
+(defn release-lock-if-needed!
+  "Attempts to release the liquibase lock if present. Logs but does not bubble up the exception if one occurs as it's
+  intended to be used when a failure has occurred and bubbling up this exception would hide the real exception."
+  [^Liquibase liquibase]
+  (when (migration-lock-exists? liquibase)
+    (try
+      (force-release-locks! liquibase)
+      (catch Exception e
+        (log/error e (trs "Unable to release the Liquibase lock after a migration failure"))))))

src/metabase/driver.clj

@@ -69,6 +69,7 @@
   [driver & body]
   `(do-with-driver ~driver (fn [] ~@body)))
 
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                             Driver Registration / Hierarchy / Multimethod Dispatch                             |
 ;;; +----------------------------------------------------------------------------------------------------------------+

src/metabase/driver/mysql.clj

@@ -282,7 +282,7 @@
 ;; LocalDateTime), and normalize it to a UTC timestamp if so.
 (defmethod sql-jdbc.execute/read-column [:mysql Types/TIMESTAMP]
   [_ _ ^ResultSet rs ^ResultSetMetaData rsmeta ^Integer i]
-  (let [t (.getObject rs i LocalDateTime)]
+  (when-let [t (.getObject rs i LocalDateTime)]
     (if (= (.getColumnTypeName rsmeta i) "TIMESTAMP")
       (t/with-offset-same-instant (t/offset-date-time t (t/zone-id (qp.timezone/results-timezone-id))) (t/zone-offset 0))
       t)))

src/metabase/driver/sql_jdbc/execute/legacy_impl.clj

@@ -55,21 +55,21 @@
 
 (defmethod sql-jdbc.execute/read-column [:use-legacy-classes-for-read-and-set Types/TIME]
   [_ _ ^ResultSet rs _ ^Integer i]
-  (let [s (.getString rs i)
-        t (u.date/parse s)]
-    (log/tracef "(.getString rs i) [TIME] -> %s -> %s" (pr-str s) (pr-str t))
-    t))
+  (when-let [s (.getString rs i)]
+    (let [t (u.date/parse s)]
+      (log/tracef "(.getString rs i) [TIME] -> %s -> %s" (pr-str s) (pr-str t))
+      t)))
 
 (defmethod sql-jdbc.execute/read-column [::use-legacy-classes-for-read-and-set Types/DATE]
   [_ _ ^ResultSet rs _ ^Integer i]
-  (let [s (.getString rs i)
-        t (u.date/parse s)]
-    (log/tracef "(.getString rs i) [DATE] -> %s -> %s" (pr-str s) (pr-str t))
-    t))
+  (when-let [s (.getString rs i)]
+    (let [t (u.date/parse s)]
+      (log/tracef "(.getString rs i) [DATE] -> %s -> %s" (pr-str s) (pr-str t))
+      t)))
 
 (defmethod sql-jdbc.execute/read-column [::use-legacy-classes-for-read-and-set Types/TIMESTAMP]
   [_ _ ^ResultSet rs _ ^Integer i]
-  (let [s (.getString rs i)
-        t (u.date/parse s)]
-    (log/tracef "(.getString rs i) [TIMESTAMP] -> %s -> %s" (pr-str s) (pr-str t))
-    t))
+  (when-let [s (.getString rs i)]
+    (let [t (u.date/parse s)]
+      (log/tracef "(.getString rs i) [TIMESTAMP] -> %s -> %s" (pr-str s) (pr-str t))
+      t)))

src/metabase/query_processor/timezone.clj

@@ -68,9 +68,9 @@
   (valid-timezone-id (report-timezone-id*)))
 
 (defn results-timezone-id
-  "The timezone that a query is actually ran in -- report timezone, if set and supported by the current driver;
+  "The timezone that a query is actually ran in ­ report timezone, if set and supported by the current driver;
   otherwise the timezone of the database (if known), otherwise the system timezone. Guaranteed to always return a
-  timezone ID — never returns `nil`."
+  timezone ID ­ never returns `nil`."
   (^String []
    (results-timezone-id driver/*driver* ::db-from-store))
 

src/metabase/util.clj

@@ -85,7 +85,7 @@
 
     (u/varargs String)
     (u/varargs String [\"A\" \"B\"])"
-  {:style/indent 1}
+  {:style/indent 1, :arglists '([klass] [klass xs])}
   [klass & [objects]]
   (vary-meta `(into-array ~klass ~objects)
              assoc :tag (format "[L%s;" (.getCanonicalName ^Class (ns-resolve *ns* klass)))))

test/metabase/db/liquibase_test.clj

+(ns metabase.db.liquibase-test
+  (:require [clojure
+             [string :as str]
+             [test :refer :all]]
+            [clojure.java.jdbc :as jdbc]
+            [metabase.db.liquibase :as liquibase]
+            [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+            [metabase.test :as mt]))
+
+(deftest mysql-engine-charset-test
+  (mt/test-driver :mysql
+    (testing "Make sure MySQL CREATE DATABASE statements have ENGINE/CHARACTER SET appended to them (#10691)"
+      (jdbc/with-db-connection [conn (sql-jdbc.conn/connection-details->spec :mysql
+                                       (mt/dbdef->connection-details :mysql :server nil))]
+        (doseq [statement ["DROP DATABASE IF EXISTS liquibase_test;"
+                           "CREATE DATABASE liquibase_test;"]]
+          (jdbc/execute! conn statement)))
+      (liquibase/with-liquibase [liquibase (sql-jdbc.conn/connection-details->spec :mysql
+                                             (mt/dbdef->connection-details :mysql :db {:database-name "liquibase_test"}))]
+        (testing "Make sure the first line actually matches the shape we're testing against"
+          (is (= (str "CREATE TABLE liquibase_test.DATABASECHANGELOGLOCK ("
+                      "ID INT NOT NULL, "
+                      "`LOCKED` BIT(1) NOT NULL, "
+                      "LOCKGRANTED datetime NULL, "
+                      "LOCKEDBY VARCHAR(255) NULL, "
+                      "CONSTRAINT PK_DATABASECHANGELOGLOCK PRIMARY KEY (ID)"
+                      ") ENGINE InnoDB CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;")
+                 (first (liquibase/migrations-lines liquibase)))))
+        (testing "Make sure *every* line contains ENGINE ... CHARACTER SET ... COLLATE"
+          (doseq [line  (liquibase/migrations-lines liquibase)
+                  :when (str/starts-with? line "CREATE TABLE")]
+            (is (= true
+                   (str/includes? line "ENGINE InnoDB CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci"))
+                (format "%s should include ENGINE ... CHARACTER SET ... COLLATE ..." (pr-str line)))))))))