From f986af7c90aeef932ce21b49799d1bf30a376b92 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Fri, 17 Jan 2020 14:58:25 -0800
Subject: [PATCH] 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]
---
.../MetabaseMySqlCreateTableSqlGenerator.java | 57 +++++
.../oracle/src/metabase/driver/oracle.clj | 12 +-
.../src/metabase/driver/snowflake.clj | 16 +-
.../src/metabase/driver/hive_like.clj | 9 +-
.../sqlite/src/metabase/driver/sqlite.clj | 6 +-
.../vertica/src/metabase/driver/vertica.clj | 16 +-
project.clj | 3 +
src/metabase/cmd/dump_to_h2.clj | 6 +-
src/metabase/cmd/load_from_h2.clj | 6 +-
src/metabase/db.clj | 204 ++----------------
src/metabase/db/jdbc_protocols.clj | 4 +-
src/metabase/db/liquibase.clj | 192 +++++++++++++++++
src/metabase/driver.clj | 1 +
src/metabase/driver/mysql.clj | 2 +-
.../driver/sql_jdbc/execute/legacy_impl.clj | 24 +--
src/metabase/query_processor/timezone.clj | 4 +-
src/metabase/util.clj | 2 +-
test/metabase/db/liquibase_test.clj | 34 +++
18 files changed, 366 insertions(+), 232 deletions(-)
create mode 100644 java/metabase/db/liquibase/MetabaseMySqlCreateTableSqlGenerator.java
create mode 100644 src/metabase/db/liquibase.clj
create mode 100644 test/metabase/db/liquibase_test.clj
diff --git a/java/metabase/db/liquibase/MetabaseMySqlCreateTableSqlGenerator.java b/java/metabase/db/liquibase/MetabaseMySqlCreateTableSqlGenerator.java
new file mode 100644
index 00000000000..88a9cf32f8d
--- /dev/null
+++ b/java/metabase/db/liquibase/MetabaseMySqlCreateTableSqlGenerator.java
@@ -0,0 +1,57 @@
+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);
+ }
+}
diff --git a/modules/drivers/oracle/src/metabase/driver/oracle.clj b/modules/drivers/oracle/src/metabase/driver/oracle.clj
index 80468b41e44..27f4be986e5 100644
--- a/modules/drivers/oracle/src/metabase/driver/oracle.clj
+++ b/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]
diff --git a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
index 56630e9cd98..5bf66119366 100644
--- a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
+++ b/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?
diff --git a/modules/drivers/sparksql/src/metabase/driver/hive_like.clj b/modules/drivers/sparksql/src/metabase/driver/hive_like.clj
index f1ae2c197d4..6b679330ab8 100644
--- a/modules/drivers/sparksql/src/metabase/driver/hive_like.clj
+++ b/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"))))
diff --git a/modules/drivers/sqlite/src/metabase/driver/sqlite.clj b/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
index 1c51ab662f1..c4ce0e037f8 100644
--- a/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
+++ b/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))))))
diff --git a/modules/drivers/vertica/src/metabase/driver/vertica.clj b/modules/drivers/vertica/src/metabase/driver/vertica.clj
index 66eff36421c..fbec1bde969 100644
--- a/modules/drivers/vertica/src/metabase/driver/vertica.clj
+++ b/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)))
diff --git a/project.clj b/project.clj
index 122f1b766a7..4fcb66191a9 100644
--- a/project.clj
+++ b/project.clj
@@ -156,6 +156,9 @@
:javac-options
["-target" "1.8", "-source" "1.8"]
+ :java-source-paths
+ ["java"]
+
:uberjar-name
"metabase.jar"
diff --git a/src/metabase/cmd/dump_to_h2.clj b/src/metabase/cmd/dump_to_h2.clj
index d330d63b30b..c733fb5f461 100644
--- a/src/metabase/cmd/dump_to_h2.clj
+++ b/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)]
diff --git a/src/metabase/cmd/load_from_h2.clj b/src/metabase/cmd/load_from_h2.clj
index 3bbbf85c810..1bef4f00389 100644
--- a/src/metabase/cmd/load_from_h2.clj
+++ b/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..."))
diff --git a/src/metabase/db.clj b/src/metabase/db.clj
index bd105307fc6..94acb70de1c 100644
--- a/src/metabase/db.clj
+++ b/src/metabase/db.clj
@@ -1,18 +1,17 @@
(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)
diff --git a/src/metabase/db/jdbc_protocols.clj b/src/metabase/db/jdbc_protocols.clj
index 1ff4c4cfa40..1ff8d3a8c6b 100644
--- a/src/metabase/db/jdbc_protocols.clj
+++ b/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))))
diff --git a/src/metabase/db/liquibase.clj b/src/metabase/db/liquibase.clj
new file mode 100644
index 00000000000..8723a85c530
--- /dev/null
+++ b/src/metabase/db/liquibase.clj
@@ -0,0 +1,192 @@
+(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"))))))
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 89b4f6d81f4..7a7933e944c 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -69,6 +69,7 @@
[driver & body]
`(do-with-driver ~driver (fn [] ~@body)))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Driver Registration / Hierarchy / Multimethod Dispatch |
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/src/metabase/driver/mysql.clj b/src/metabase/driver/mysql.clj
index d7138df84d9..401058a9702 100644
--- a/src/metabase/driver/mysql.clj
+++ b/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)))
diff --git a/src/metabase/driver/sql_jdbc/execute/legacy_impl.clj b/src/metabase/driver/sql_jdbc/execute/legacy_impl.clj
index c3910befb38..8e47e51d6f2 100644
--- a/src/metabase/driver/sql_jdbc/execute/legacy_impl.clj
+++ b/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)))
diff --git a/src/metabase/query_processor/timezone.clj b/src/metabase/query_processor/timezone.clj
index 67069a9f440..c92b05c89ce 100644
--- a/src/metabase/query_processor/timezone.clj
+++ b/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))
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index baf6d1afed7..34e36bbfff9 100644
--- a/src/metabase/util.clj
+++ b/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)))))
diff --git a/test/metabase/db/liquibase_test.clj b/test/metabase/db/liquibase_test.clj
new file mode 100644
index 00000000000..a54a22e115b
--- /dev/null
+++ b/test/metabase/db/liquibase_test.clj
@@ -0,0 +1,34 @@
+(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)))))))))
--
GitLab