Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Table name should be keyword for next.jdbc #241

Open
conan opened this issue Jun 15, 2023 · 2 comments
Open

Table name should be keyword for next.jdbc #241

conan opened this issue Jun 15, 2023 · 2 comments
Labels

Comments

@conan
Copy link

conan commented Jun 15, 2023

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:

  • Migratus marks table as reserved
  • Migratus attempts to run migrations
  • Migratus marks table as unreserved

The observed behaviour is:

  • Migratus fails to mark table as reserved
  • Migratus doesn't attempt migrations
  • Migratus fails to mark table as unreserved
  • Migratus logs an INFO message (not an ERROR) which says Migration reserved by another instance. Ignoring.

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 yogthos added the bug label Jun 15, 2023
@yogthos
Copy link
Owner

yogthos commented Jun 15, 2023

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

@conan
Copy link
Author

conan commented Jun 16, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants