yogthos / migratus

MIGRATE ALL THE THINGS!
651 stars 95 forks source link

Unable to run migrations when `next.jdbc.transaction/*nested-tx*` is set to `:prohibit` #260

Closed p-himik closed 9 months ago

p-himik commented 9 months ago

Pretty much what the title says. An cleaned up exception data:

{:clojure.main/message "Execution error (IllegalStateException) at next.jdbc.transaction/eval3813$fn (transaction.clj:129).\nNested transactions are prohibited\n"
 :clojure.main/triage  {:clojure.error/class  java.lang.IllegalStateException
                        :clojure.error/line   129
                        :clojure.error/cause  "Nested transactions are prohibited"
                        :clojure.error/symbol next.jdbc.transaction/eval3813$fn
                        :clojure.error/source "transaction.clj"
                        :clojure.error/phase  :execution}
 :clojure.main/trace   {:via   [{:type    java.lang.IllegalStateException
                                 :message "Nested transactions are prohibited"
                                 :at      [next.jdbc.transaction$eval3813$fn__3814 invoke "transaction.clj" 129]}]
                        :trace [[next.jdbc.transaction$eval3813$fn__3814 invoke "transaction.clj" 129]
                                [next.jdbc.protocols$eval2784$fn__2785$G__2775__2794 invoke "protocols.clj" 58]
                                [next.jdbc$transact invokeStatic "jdbc.clj" 420]
                                [next.jdbc$transact invoke "jdbc.clj" 412]
                                [migratus.migration.sql$run_sql invokeStatic "sql.clj" 106]
                                [migratus.migration.sql$run_sql invoke "sql.clj" 102]
                                [migratus.migration.sql.SqlMigration up "sql.clj" 122]
                                [migratus.database$migrate_up_STAR_ invokeStatic "database.clj" 76]
                                [migratus.database$migrate_up_STAR_ invoke "database.clj" 70]
                                [migratus.database.Database$fn__4418 invoke "database.clj" 310]
                                [next.jdbc.transaction$transact_STAR_ invokeStatic "transaction.clj" 72]
                                [next.jdbc.transaction$transact_STAR_ invoke "transaction.clj" 51]
                                [next.jdbc.transaction$eval3813$fn__3814 invoke "transaction.clj" 126]
                                [next.jdbc.protocols$eval2784$fn__2785$G__2775__2794 invoke "protocols.clj" 58]
                                [next.jdbc$transact invokeStatic "jdbc.clj" 420]
                                [next.jdbc$transact invoke "jdbc.clj" 412]
                                [migratus.database.Database migrate_up "database.clj" 309]
                                [migratus.core$up_STAR_ invokeStatic "core.clj" 103]
                                [migratus.core$up_STAR_ invoke "core.clj" 101]
                                [migratus.core$migrate_up_STAR_ invokeStatic "core.clj" 114]
                                [migratus.core$migrate_up_STAR_ invoke "core.clj" 105]
                                [migratus.core$migrate_STAR_ invokeStatic "core.clj" 127]
                                [migratus.core$migrate_STAR_ invoke "core.clj" 123]
                                [clojure.core$partial$fn__5908 invoke "core.clj" 2642]
                                [migratus.core$run invokeStatic "core.clj" 54]
                                [migratus.core$run invoke "core.clj" 50]
                                [migratus.core$migrate invokeStatic "core.clj" 134]
                                [migratus.core$migrate invoke "core.clj" 129]
                                [some.wrapper.ns.that.sets.the.var.to.prohibit.nested.txs]]
                        :cause "Nested transactions are prohibited"}}
p-himik commented 9 months ago

It seems that whenever this block gets executed, the transaction has already been started by the wrapping functionality: https://github.com/yogthos/migratus/blob/master/src/migratus/migration/sql.clj#L106-L107

But in the case where the relevant functions below migratus.core/migrate are considered a public API, just removing that extra transaction would be a breaking change. Perhaps a check for an existing transaction could be introduced? Or a local binding of next.jdbc.transaction/*nested-tx* to :ignore (doesn't seem like :allow should be a preferred choice).

yogthos commented 9 months ago

I think using :ignore here would probably be the way to resolve this. Would you be ok to try it out and do a pr for the fix?

yogthos commented 9 months ago

Thanks for the fix, just pushed up 1.5.5 to Clojars with the changes.