weavejester / ragtime

Database-independent migration library
Eclipse Public License 1.0
610 stars 85 forks source link

Fix #149 unquote ragtime table name #150

Closed holyjak closed 2 years ago

holyjak commented 2 years ago

... before comparing it with the DB metadata - fixes #149

weavejester commented 2 years ago

Thanks for the PR, but can you explain what you're doing here? Isn't fixing #149 just a case of updating metadata-matches-table??

holyjak commented 2 years ago

Well, yes and no. You need to talk to the DB to find out what is the quoting character so that you can ignore it in metadata-matches-table. And to be able to get the DB MetaData, I need a Connection - but what I actually have might be a datasource or a wrapped connection. So the new macro ensures I get a connection. (It could also be a function taking a callback that takes the connection...)

holyjak commented 2 years ago

To make it more concrete: at my project I have the schema 7eleven and want the migration table there. Thus I need to give it the name quoted, i.e. - in postgres sytnax - "\"7eleven\".ragtime_migrations". Currently that will cause Ragtime to never understand that the table has been created because it will be comparing 7eleven (from metadata) against "7eleven" (from the configured table name).

Somebody might also want to use special chars in the table name and thus quote that as well: "myschema"."mytable".

weavejester commented 2 years ago

Okay, so let's remove the with-connection. We don't want a macro here, and certainly not a public one.

Instead, let's change get-table-metadata to get-db-metadata and have it return a map with :quote and :tables, so all the information we need is returned by one trip.

holyjak commented 2 years ago

Done!

weavejester commented 2 years ago

Thanks. Can you squash down your changes and give it a commit message like:

Ensure table lookup works for quoted identifiers

Fixes #149.
holyjak commented 2 years ago

done

holyjak commented 2 years ago

Hi @weavejester ! I believe I have addressed all your comments. Please let me know if there is anything more I should do. Thank you.

weavejester commented 2 years ago

Apologies for the delay. I forgot about tests last time. Can you extend test-migrations-table with a test for this? e.g.

(let [table "`myschema`.migrations"
      db    (jdbc/sql-database datasource {:migrations-table table})]
    (p/add-migration-id db "30")
    (is (= ["30"]
           (->> ["SELECT * FROM `myschema`.migrations"]
                (n.j/execute! (:datasource db))
                (map :MIGRATIONS/ID)))))
weavejester commented 2 years ago

H2 returns a quote character but obviously doesn't sort quoting schema names in create table.

Sorry, I'm not sure I understand this. Can you explain a little further?

holyjak commented 2 years ago

I have added the test now

holyjak commented 2 years ago

@weavejester sorry to bother but is there anything else I should do? Thank you!

weavejester commented 2 years ago

Thanks - merged!