Fix db migration breakage and add force migrate command

src/metabase/db.clj

@@ -24,7 +24,10 @@
            liquibase.database.jvm.JdbcConnection
            liquibase.resource.ClassLoaderResourceAccessor))
 
-;; ## DB FILE, JDBC/KORMA DEFINITONS
+
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; |                                              DB FILE & CONNECTION DETAILS                                              |
+;;; +------------------------------------------------------------------------------------------------------------------------+
 
 (def db-file
   "Path to our H2 DB file from env var or app config."
@@ -88,49 +91,148 @@
 
 (defn jdbc-details
   "Takes our own MB details map and formats them properly for connection details for JDBC."
-  [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`
-  (case (:type db-details)
-    :h2       (dbspec/h2       db-details)
-    :mysql    (dbspec/mysql    (assoc db-details :db (:dbname db-details)))
-    :postgres (dbspec/postgres (assoc db-details :db (:dbname db-details)))))
+  ([]
+   (jdbc-details @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`
+   (case (:type db-details)
+     :h2       (dbspec/h2       db-details)
+     :mysql    (dbspec/mysql    (assoc db-details :db (:dbname db-details)))
+     :postgres (dbspec/postgres (assoc db-details :db (:dbname db-details))))))
 
 
-;; ## MIGRATE
+;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; |                                                        MIGRATE                                                         |
+;;; +------------------------------------------------------------------------------------------------------------------------+
 
-(def ^:private ^:const ^String changelog-file
-  "migrations/liquibase.json")
+(def ^:private ^:const ^String changelog-file "migrations/liquibase.json")
+
+(defn- migrations-sql
+  "Return a string of SQL containing the DDL statements needed to perform unran 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 neccesary for H2 or Postgres go ahead and do it anyway because it keeps the code simple and doesn't make a significant performance difference."
+  [^Liquibase liquibase]
+  (for [line  (s/split-lines (migrations-sql liquibase))
+        :when (not (or (s/blank? line)
+                       (re-find #"^--" line)))]
+    line))
+
+(defn- has-unran-migrations?
+  "Does LIQUIBASE have migration change sets that haven't been ran yet?
+
+   It's a good idea to Check to make sure there's actually something to do before running `(migrate :up)` because `migrations-sql` will
+   always contain SQL to create and release migration locks, which is both slightly dangerous and a waste of time when we won't be using them."
+  ^Boolean [^Liquibase liquibase]
+  (boolean (seq (.listUnrunChangeSets liquibase nil))))
+
+(defn- has-migration-lock?
+  "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 (has-migration-lock? liquibase)
+      (Thread/sleep 2000)
+      (throw (Exception. "Database has migration lock; cannot run migrations. You can force-release these locks by running `java -jar metabase.jar migrate release-locks`.")))))
+
+(defn- migrate-up-if-needed
+  "Run any unran LIQUIBASE migrations, if needed.
+
+   This creates SQL for the migrations to be performed, then executes each DDL statement.
+   Running `.update` directly doesn't seem to work as we'd expect; it ends up commiting the changes made and they can't be rolled back at
+   the end of the transaction block. Converting the migration to SQL string and running that via `jdbc/execute!` seems to do the trick."
+  [conn, ^Liquibase liquibase]
+  (when (has-unran-migrations? liquibase)
+    (wait-for-migration-lock-to-be-cleared liquibase)
+    (doseq [line (migrations-lines liquibase)]
+      (jdbc/execute! conn [line]))))
+
+(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.  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]
+  (when (has-unran-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- 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 (DatabaseFactory/getInstance) liquibase-conn)]
+     (Liquibase. changelog-file (ClassLoaderResourceAccessor.) database))))
 
 (defn migrate
-  "Migrate the database (this can also be ran via command line like `lein run migrate down-one`):
+  "Migrate the database (this can also be ran via command line like `java -jar metabase.jar migrate up` or `lein run migrate up`):
 
    *  `:up`            - Migrate up
+   *  `:force`         - Force migrate up, ignoring locks and any DDL statements that fail.
    *  `:down`          - Rollback *all* migrations
    *  `:down-one`      - Rollback a single migration
    *  `:print`         - Just print the SQL for running the migrations, don't actually run them.
    *  `:release-locks` - Manually release migration locks left by an earlier failed migration.
-                         (This shouldn't be necessary now that we run migrations inside a transaction,
-                         but is available just in case)."
+                         (This shouldn't be necessary now that we run migrations inside a transaction, but is available just in case)."
+  ([]
+   (migrate :up))
   ([direction]
    (migrate @db-connection-details direction))
   ([db-details direction]
    (jdbc/with-db-transaction [conn (jdbc-details db-details)]
-     (let [^Database database (-> (DatabaseFactory/getInstance)
-                                  (.findCorrectDatabaseImplementation (JdbcConnection. (jdbc/get-connection conn))))
-           ^Liquibase liquibase (Liquibase. changelog-file (ClassLoaderResourceAccessor.) database)]
-       (case direction
-         :up            (.update liquibase "")
-         :down          (.rollback liquibase 10000 "")
-         :down-one      (.rollback liquibase 1 "")
-         :print         (let [writer (StringWriter.)]
-                          (.update liquibase "" writer)
-                          (.toString writer))
-         :release-locks (.forceReleaseLocks liquibase))))))
+     ;; 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
+     (try
+       (let [liquibase (conn->liquibase conn)]
+         (case direction
+           :up            (migrate-up-if-needed conn liquibase)
+           :force         (force-migrate-up-if-needed conn liquibase)
+           :down          (.rollback liquibase 10000 "")
+           :down-one      (.rollback liquibase 1 "")
+           :print         (println (migrations-sql liquibase))
+           :release-locks (.forceReleaseLocks liquibase)))
+       ;; Migrations were successful; disable rollback-only so `.commit` will be called instead of `.rollback`
+       (jdbc/db-unset-rollback-only! conn)
+       :done
+       ;; If for any reason any part of the migrations fail then rollback all changes
+       (catch Throwable e
+         ;; This should already be happening as a result of `db-set-rollback-only!` but running it twice won't hurt so better safe than sorry
+         (.rollback (jdbc/get-connection conn))
+         (throw e))))))
 
 
 ;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; |                                           DB CONNECTION / TRANSACTION STUFF                                            |
+;;; |                                          CONNECTION POOLS & TRANSACTION STUFF                                          |
 ;;; +------------------------------------------------------------------------------------------------------------------------+
 
 (defn connection-pool
@@ -193,7 +295,7 @@
 
 
 ;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; |                                                 DB SETUP & MIGRATIONS                                                  |
+;;; |                                                       DB SETUP                                                         |
 ;;; +------------------------------------------------------------------------------------------------------------------------+
 
 

src/metabase/driver/generic_sql/query_processor.clj

@@ -227,14 +227,14 @@
   [sql]
   (when sql
     (-> sql
-        (s/replace #"\sFROM" "\nFROM")
+        (s/replace #"\sFROM"      "\nFROM")
         (s/replace #"\sLEFT JOIN" "\nLEFT JOIN")
-        (s/replace #"\sWHERE" "\nWHERE")
-        (s/replace #"\sGROUP BY" "\nGROUP BY")
-        (s/replace #"\sORDER BY" "\nORDER BY")
-        (s/replace #"\sLIMIT" "\nLIMIT")
-        (s/replace #"\sAND\s" "\n   AND ")
-        (s/replace #"\sOR\s" "\n    OR "))))
+        (s/replace #"\sWHERE"     "\nWHERE")
+        (s/replace #"\sGROUP BY"  "\nGROUP BY")
+        (s/replace #"\sORDER BY"  "\nORDER BY")
+        (s/replace #"\sLIMIT"     "\nLIMIT")
+        (s/replace #"\sAND\s"     "\n   AND ")
+        (s/replace #"\sOR\s"      "\n    OR "))))
 
 
 ;; TODO - make this a protocol method ?
@@ -304,13 +304,15 @@
          (throw (Exception. (exception->nice-error-message e))))))
 
 (defn- do-with-auto-commit-disabled
-  "Disable auto-commit for this transaction, that way shady queries are unable to modify the database; execute F in a try-finally block.
-   In the `finally`, rollback any changes made during this transaction just to be extra-double-sure JDBC doesn't try to commit them automatically for us."
+  "Disable auto-commit for this transaction, and make the transaction `rollback-only`, which means when the transaction finishes `.rollback` will be called instead of `.commit`.
+   Furthermore, execute F in a try-finally block; in the `finally`, manually call `.rollback` just to be extra-double-sure JDBC any changes made by the transaction aren't committed."
   {:style/indent 1}
-  [{^java.sql.Connection connection :connection}, f]
-  (.setAutoCommit connection false)
+  [conn f]
+  (jdbc/db-set-rollback-only! conn)
+  (.setAutoCommit (jdbc/get-connection conn) false)
+  ;; TODO - it would be nice if we could also `.setReadOnly` on the transaction as well, but that breaks setting the timezone. Is there some way we can have our cake and eat it too?
   (try (f)
-       (finally (.rollback connection))))
+       (finally (.rollback (jdbc/get-connection conn)))))
 
 (defn- do-in-transaction [connection f]
   (jdbc/with-db-transaction [transaction-connection connection]