yogthos / migratus

MIGRATE ALL THE THINGS!
640 stars 95 forks source link

Table name should be keyword for next.jdbc #241

Open conan opened 1 year ago

conan commented 1 year ago

This was a fun one!

The friendly functions in next.jdbc expect table names as keywords, but Migratus passes them as strings.

Normally this is fine, because the friendly functions delegate to next.jdbc.sql.builder/for-insert which calls the next.jdbc.sql.builder/safe-name function that turns the table name into a string anyway. So usually there isn't a problem. Usually!

I use clojure.spec and had enabled next.jdbc's spec instrumentation, which checks arguments passed to next.jdbc functions.

In the case of migratus, it identifies that table names should be keywords, and throws a spec validation error. This first occurs when marking the table as reserved prior to running migrations, and exceptions thrown at this point are swallowed; migratus is unable to mark the migrations table as reserved, and assumes this is because somebody else has already reserved the table, so it skips running the migrations, and marks the table as unreserved again (which also fails spec validation).

The expected behaviour is:

The observed behaviour is:

To reproduce:

Run (next.jdbc.specs/instrument) prior to running migrations with Migratus.

A workaround is not to instrument next.jdbc functions for spec, but that's functionality that I'd like to have elsewhere in my codebase, so it's a shame to lose it. An error in the logs when Migratus fails to mark a table as reserved for a non-sql reason would help with debugging this issue.

yogthos commented 1 year ago

Any chance you'd be up to do a pr for the fix? :)

conan commented 1 year ago

Possibly, I'll see if I can find time next week!