yogthos / migratus

MIGRATE ALL THE THINGS!
651 stars 95 forks source link

Migratus rollback doesn't bring down the last migration, but the first migration. #248

Open philoskim opened 1 year ago

philoskim commented 1 year ago

migratus version: 1.5.1

The following is the sample sqls in resources/migrations folder. I intentionally used incremental integers for brevity.

;; resources/migrations/1-create-foo-table.up.sql
CREATE TABLE IF NOT EXISTS foo(id BIGINT);
;; resources/migrations/1-create-foo-table.down.sql
DROP TABLE IF EXISTS foo;

;; resources/migrations/2-create-bar-table.up.sql
CREATE TABLE IF NOT EXISTS bar(id BIGINT);
;; resources/migrations/2-create-bar-table.down.sql
DROP TABLE IF EXISTS bar;

;; resources/migrations/3-create-baz-table.up.sql
CREATE TABLE IF NOT EXISTS baz(id BIGINT);
;; resources/migrations/3-create-baz-table.down.sql
DROP TABLE IF EXISTS baz;

The following is my project.clj related to Migratus.

  :plugins [[migratus-lein "0.7.3"]]
  :migratus {:store :database
             :migration-dir "resources/migrations/"
             :db {:dbtype "mysql"
                  :dbname "hcp_safety"
                  :user "root"
                  :password "my_password"}}

lein migratus migrate worked as I expected.

$ lein migratus migrate
migrating all outstanding migrations
$ mysql
mysql> use hcp_safety;

mysql> show tables;
+----------------------+
| Tables_in_hcp_safety |
+----------------------+
| bar                  |
| baz                  |
| foo                  |
| schema_migrations    |
+----------------------+
4 rows in set (0.00 sec)

mysql> select * from schema_migrations;
+----+---------------------+------------------+
| id | applied             | description      |
+----+---------------------+------------------+
|  1 | 2023-08-02 20:11:38 | create-foo-table |
|  2 | 2023-08-02 20:11:38 | create-bar-table |
|  3 | 2023-08-02 20:11:38 | create-baz-table |
+----+---------------------+------------------+
3 rows in set (0.00 sec)

However, lein migratus rollback didn't work as I expected.

$ lein migratus rollback
rolling back last migration
$ mysql
mysql> use hcp_safety;

mysql> show tables;
+----------------------+
| Tables_in_hcp_safety |
+----------------------+
| bar                  |
| baz                  |
| schema_migrations    |
+----------------------+
3 rows in set (0.01 sec)

mysql> select * from schema_migrations;
+----+---------------------+------------------+
| id | applied             | description      |
+----+---------------------+------------------+
|  2 | 2023-08-02 20:11:38 | create-bar-table |
|  3 | 2023-08-02 20:11:38 | create-baz-table |
+----+---------------------+------------------+
2 rows in set (0.00 sec)

Migratus rollback doesn't bring down the last migration (create-baz-table), but the first migration (create-foo-table).

yogthos commented 1 year ago

hmm, I'm not able to reproduce with h2 in the REPL

(ns migtest.core
  (:require
   [jdbc.core :as jdbc]
   [migratus.core :as m]))

(def config {:store                :database
             :migration-dir        "migrations/"
             :init-script          "init.sql" ;script should be located in the :migration-dir path
             ;defaults to true, some databases do not support
             ;schema initialization in a transaction
             :init-in-transaction? false
             :db {:dbtype "h2"
                  :dbname "site.db"}})

(def conn (jdbc/connection {:subprotocol "h2"
                            :subname "file:./site.db"}))

(map :table_name (jdbc/fetch conn "show tables"))
;;()

(m/create config "create-foo")
(m/create config "create-bar")
(m/create config "create-baz")

(m/migrate config)
(prn (map :table_name (jdbc/fetch conn "show tables")))
;; ("BAR" "BAZ" "FOO" "SCHEMA_MIGRATIONS")

(dotimes [_ 3]
  (m/rollback config)
  (prn (map :table_name (jdbc/fetch conn "show tables"))))
;; ("BAR" "FOO" "SCHEMA_MIGRATIONS")
;; ("FOO" "SCHEMA_MIGRATIONS")
;; ("SCHEMA_MIGRATIONS")
philoskim commented 1 year ago

I tested your migtest.core code and confirmed that it worked as you did.

However, my mysql test code still didn't work.

I don't know why, but I suspect that the following code might have some problem.

;; migratus/core.clj
(defn- rollback* [config store _]
  (run-down
    config
    store
    (->> (proto/completed-ids store)
         first  ;; <-- In my opinion, this should be changed to `last`.
         vector)))

I changed the above first to last and confirmed that it worked on my mysql test code as I expected.

ieugen commented 1 year ago

Could it be that results are ordered in different ways on h2 vs mysql?

We should probably be using test-containers more for integration tests.

philoskim commented 1 year ago

Yes, the results are ordered in different ways on h2 vs mysql.

Further findings:

The followings are migrations files for my migration-up test.

;; resources/migrations/
20230101120000-create-foo.up.sql
20230101130000-create-bar.up.sql
20230101140000-create-baz.up.sql

On mysql, the ids of schema_migartions table are exactly the same as the above prefixed numbers on files after lein migratus migrate.

mysql> select * from schema_migrations;
+----------------+---------------------+-------------+
| id             | applied             | description |
+----------------+---------------------+-------------+
| 20230101120000 | 2023-09-17 19:40:09 | create-foo  |
| 20230101130000 | 2023-09-17 19:40:09 | create-bar  |
| 20230101140000 | 2023-09-17 19:40:09 | create-baz  |

However, the ids of schema_migartions table on h2 are different as follows.

(jdbc/fetch conn "select * from schema_migrations") =>
; => [{:id 20111202110600,
;      :applied #inst "2023-09-16T19:57:54.763000000-00:00",
;      :description "create-foo-table"}
;     {:id 20111202113000,
;      :applied #inst "2023-09-16T19:57:54.764000000-00:00",
;      :description "create-bar-table"}
;     {:id 20120827170200,
;      :applied #inst "2023-09-17T10:43:21.711000000-00:00",
;      :description "multiple-statements"}]
yogthos commented 1 year ago

@ieugen agree that test-containers would be a good idea. H2 driver converts the inst into a timestamp, but that shouldn't affect ordering. The actual logic for deciding which migration to rollback is here, and it just does a reverse sort-by the migration id.