Fixing a problem with manual migrations (#12023)

Co-authored-by: Marcelo Gimenes de Oliveira <marcelo.gimenes.oliveira@gmail.com>

resources/migrations/000_migrations.yaml

@@ -6196,3 +6196,19 @@ databaseChangeLog:
             tableName: query_cache
             columnName: updated_at
             newDataType: ${cache_timestamp_type}
+
+# Drop the old query_queryexecution table if present. This was replaced by query_execution in 0.23.0. This was
+# formerly a data migration but was converted to a Liquibase migration so people running migrations manually will
+# still have the Table dropped.
+
+  - changeSet:
+      id: 162
+      author: camsaul
+      comment: 'Added 0.23.0 as a data migration; converted to Liquibase migration in 0.35.0'
+      preConditions:
+        - onFail: MARK_RAN
+        - tableExists:
+            tableName: query_queryexecution
+      changes:
+        - dropTable:
+            tableName: query_queryexecution

src/metabase/db.clj

@@ -152,6 +152,23 @@
 ;;; |                                                    MIGRATE!                                                    |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defn- print-migrations-and-quit-if-needed!
+  "If we are not doing auto migrations then print out migration SQL for user to run manually.
+   Then throw an exception to short circuit the setup process and make it clear we can't proceed."
+  [liquibase]
+  (when (liquibase/has-unrun-migrations? liquibase)
+    (log/info (str (trs "Database Upgrade Required")
+                   "\n\n"
+                   (trs "NOTICE: Your database requires updates to work with this version of Metabase.")
+                   "\n"
+                   (trs "Please execute the following sql commands on your database before proceeding.")
+                   "\n\n"
+                   (liquibase/migrations-sql liquibase)
+                   "\n\n"
+                   (trs "Once your database is updated try running the application again.")
+                   "\n"))
+    (throw (Exception. (trs "Database requires manual upgrade.")))))
+
 (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`):
@@ -190,7 +207,7 @@
            :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))
+           :print         (print-migrations-and-quit-if-needed! 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)
@@ -289,30 +306,13 @@
   entries for the \"magic\" groups and permissions entries. "
   false)
 
-(defn- print-migrations-and-quit!
-  "If we are not doing auto migrations then print out migration SQL for user to run manually.
-   Then throw an exception to short circuit the setup process and make it clear we can't proceed."
-  [db-details]
-  (let [sql (migrate! (jdbc-spec db-details) :print)]
-    (log/info (str "Database Upgrade Required\n\n"
-                   "NOTICE: Your database requires updates to work with this version of Metabase.  "
-                   "Please execute the following sql commands on your database before proceeding.\n\n"
-                   sql
-                   "\n\n"
-                   "Once your database is updated try running the application again.\n"))
-    (throw (Exception. "Database requires manual upgrade."))))
-
 (defn- run-schema-migrations!
   "Run through our DB migration process and make sure DB is fully prepared"
   [auto-migrate? db-details]
   (log/info (trs "Running Database Migrations..."))
-  (if auto-migrate?
-    (migrate! (jdbc-spec 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 (liquibase/with-liquibase [liquibase (jdbc-spec db-details)]
-            (liquibase/has-unrun-migrations? liquibase))
-      (print-migrations-and-quit! db-details)))
+  ;; if `MB_DB_AUTOMIGRATE` is false, and we have migrations that need to be ran, print and quit. Otherwise continue
+  ;; to start normally
+  (migrate! (jdbc-spec db-details) (if auto-migrate? :up :print))
   (log/info (trs "Database Migrations Current ... ") (u/emoji "✅")))
 
 (defn- run-data-migrations!

src/metabase/db/migrations.clj

@@ -7,7 +7,6 @@
      CREATE TABLE IF NOT EXISTS ... -- Good
      CREATE TABLE ...               -- Bad"
   (:require [cemerick.friend.credentials :as creds]
-            [clojure.java.jdbc :as jdbc]
             [clojure.string :as str]
             [clojure.tools.logging :as log]
             [metabase
@@ -197,21 +196,9 @@
   (when-let [site-url (db/select-one-field :value Setting :key "-site-url")]
     (public-settings/site-url site-url)))
 
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; |                                           Migrating QueryExecutions                                            |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-;; drop the legacy QueryExecution table now that we don't need it anymore
-(defmigration ^{:author "camsaul", :added "0.23.0"} drop-old-query-execution-table
-  ;; DROP TABLE IF EXISTS should work on Postgres, MySQL, and H2
-  (jdbc/execute! (db/connection) [(format "DROP TABLE IF EXISTS %s;" ((db/quote-fn) "query_queryexecution"))]))
-
-;; There's a window on in the 0.23.0 and 0.23.1 releases that the
-;; site-url could be persisted without a protocol specified. Other
-;; areas of the application expect that site-url will always include
-;; http/https. This migration ensures that if we have a site-url
-;; stored it has the current defaulting logic applied to it
+;; There's a window on in the 0.23.0 and 0.23.1 releases that the site-url could be persisted without a protocol
+;; specified. Other areas of the application expect that site-url will always include http/https. This migration
+;; ensures that if we have a site-url stored it has the current defaulting logic applied to it
 (defmigration ^{:author "senior", :added "0.25.1"} ensure-protocol-specified-in-site-url
   (let [stored-site-url (db/select-one-field :value Setting :key "site-url")
         defaulted-site-url (public-settings/site-url stored-site-url)]
@@ -219,9 +206,8 @@
                (not= stored-site-url defaulted-site-url))
       (setting/set! "site-url" stored-site-url))))
 
-;; There was a bug (#5998) preventing database_id from being persisted with
-;; native query type cards. This migration populates all of the Cards
-;; missing those database ids
+;; There was a bug (#5998) preventing database_id from being persisted with native query type cards. This migration
+;; populates all of the Cards missing those database ids
 (defmigration ^{:author "senior", :added "0.27.0"} populate-card-database-id
   (doseq [[db-id cards] (group-by #(get-in % [:dataset_query :database])
                                   (db/select [Card :dataset_query :id :name] :database_id [:= nil]))