Use UTF8/InnoDB for new MySQL tables (metabase#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

@@ -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);
+    }
+}

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..."))