weavejester / ragtime

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

Check for table metadata instead of attempting to create it #101

Closed venantius closed 8 years ago

venantius commented 8 years ago

This commit uses the java.sql getMetaData and getTables methods to check for the migration table's existence rather than trying to create it and just handling the exception.

This approach is preferable to the previous approach in that it does not invalidate an ongoing transaction that might contain Ragtime migrations.

weavejester commented 8 years ago

Currently seems to be failing the tests...

venantius commented 8 years ago

Let me tuck into this a bit. I didn't actually run the tests locally before pushing this up 😜

venantius commented 8 years ago

Tests now pass. Two concerns are outstanding:

  1. Tests passing is not representative of logic being correct. Specifically, the precise casing of table names w/r/t metadata lookups is inconsistent. I believe that only PostgreSQL is the outlier here, but I need to look into it a bit more.
  2. Your point about db-spec not being guaranteed to have a connection is well taken. I can add some appropriate logic to provide one.
weavejester commented 8 years ago

Thanks! Could you change the commit message to something a little more in line with the repo? So perhaps:

Check migrations table exists before creation

Rather than attempting to create the migration table and ignoring the
exception, instead use the .getTables method to check the migration
table exists first.
venantius commented 8 years ago

Updated!

weavejester commented 8 years ago

Thanks! If you get rid of that additional space it should be good to merge.

I should probably add cljfmt to my CI scripts...

venantius commented 8 years ago

Fixed!

weavejester commented 8 years ago

Perfect, thanks!

aroemers commented 8 years ago

@weavejester Do you have any ETA of a 0.6.1 release , with this in? This actually solves an issue I'm having with transactions around ragtime migrations: ignoring a failed CREATE TABLE ragtime_migrations ... (which the current source does) can bring the connection in a bad state, breaking the surrounding transaction.

weavejester commented 8 years ago

Done. Sorry for the delay.

aroemers commented 8 years ago

No problem at all, thank you for the quick release!