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

Fix an error where MigrationVersion wouldn't parse correctly #903

Merged
merged 2 commits into from Dec 5, 2014

Conversation

andrewlmurray
Copy link
Contributor

Presently MigrationVersion parses to a List of Longs internally. If any of the pieces of a version string overflow a long it will fail to parse with a misleading error message. This is problematic for two reasons:

  1. It can be useful to have version numbers longer than longs.
  2. The error message provided to the user is incorrect in the case of overflow and is confusing.

Take for example a migration with the name:

V20141203182115912052_00001__create_user.sql

(Here we're using the date/time from a shared ntp server as the prefix of a version number, which overflows a long) upon being parsed in trunk we'd get this error:

org.flywaydb.core.api.FlywayException: Invalid version containing non-numeric characters. Only 0..9 and . are allowed. Invalid version: 20141203182115912052.00001

Which is obviously incorrect.

This patch changes the internal storage of the migration version to use a BigInteger instead of a Long, thus allowing version sections to be of arbitrary length. I ran this with the non-commercial test suite and everything passed, but I haven't run with the commercial stuff.

@axelfontaine
Copy link
Contributor

Thanks! Could you also extend the unit test to verify this?

@andrewlmurray
Copy link
Contributor Author

@axelfontaine Sure thing - I added a test. I verified that it throws the above mentioned exception prior to my fix. (and doesn't now)

What is the release schedule for flyway? (Just trying to get a sense of when this will be in the released versions as it breaks a scheme I was planning on using) No rush or anything as I can just use a custom version for now of course.

@andrewlmurray
Copy link
Contributor Author

@axelfontaine Note the build failed after my last changeset but it doesn't seem to have anything to do with my changes - is the gradle stuff non-deterministic?

@axelfontaine
Copy link
Contributor

Hmm that test is reliable to the best of my knowledge. Does the issue happen on your machine too?

@andrewlmurray
Copy link
Contributor Author

So I ran these locally again and they work for me.
What is odd in the travisci pull request view

https://travis-ci.org/flyway/flyway/pull_requests

You can see my first changeset ran just fine. The second one (which only adds a passing test) failed. Looking at the error (a null pointer exception in like 52 of GradleLargeTest) this seems to be some type of spurious error. (As this would indicate that runGradle returned null)

I can't seem to find a button to re-run the tests on travis. Am I missing something?

axelfontaine pushed a commit that referenced this pull request Dec 5, 2014
Fix an error where MigrationVersion wouldn't parse correctly
@axelfontaine axelfontaine merged commit cf7397a into flyway:master Dec 5, 2014
axelfontaine pushed a commit to flyway/flywaydb.org that referenced this pull request Dec 5, 2014
@axelfontaine
Copy link
Contributor

Thanks for doublechecking the Gradle issue! Merged. I've also listed you on the Hall of Fame page: http://flywaydb.org/contribute/hallOfFame.html

Cheers
Axel

@axelfontaine
Copy link
Contributor

@andrewlmurray Could you please get in touch with me via email (axel at boxfuse.com)? Thanks!

@axelfontaine
Copy link
Contributor

@andrewlmurray Did you get my previous message? Could you please get in touch with me via email (axel at boxfuse.com)? Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants