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

Allow default migration resolvers to be overriden #1078

Closed
martin-grofcik opened this issue Aug 11, 2015 · 5 comments
Closed

Allow default migration resolvers to be overriden #1078

martin-grofcik opened this issue Aug 11, 2015 · 5 comments

Comments

@martin-grofcik
Copy link

@martin-grofcik martin-grofcik commented Aug 11, 2015

Current behavior:

  • custom resolvers are added in CompositeMigrationResolver after the default one. (SqlMigrationResolver, JdbcMigrationResolver, SpringJdbcMigrationResolver)
            for (Location location : locations.getLocations()) {
                migrationResolvers.add(new SqlMigrationResolver(dbSupport, classLoader, location, placeholderReplacer,
                        encoding, sqlMigrationPrefix, sqlMigrationSeparator, sqlMigrationSuffix));
                migrationResolvers.add(new JdbcMigrationResolver(classLoader, location));

                if (new FeatureDetector(classLoader).isSpringJdbcAvailable()) {
                    migrationResolvers.add(new SpringJdbcMigrationResolver(classLoader, location));
                }
            }
            migrationResolvers.addAll(Arrays.asList(customMigrationResolvers));

Problem description:
When we want to override SpringJdbcMigrationResolver, we have no possibility to do that. Because SpringJdbcMigrationResolver is added to the resolvers by default and it is not possible to remove it. Spring JDBC migrations will be resolved twice (he first time from default resolver the second time from the default one) The usecase is not rare e.g.:
http://stackoverflow.com/questions/27105681/flyway-db-need-access-to-spring-environment-for-migration
#1062

Solution proposals:

  • add default migration resolvers only in the case when there is no custom migration resolver
  • another possibility is to do not add default migration resolver when there is already subclass of default migration resolver in the custom migration resolvers (little bit more complicated but allows backward comapatibility)
martin-grofcik added a commit to martin-grofcik/flyway that referenced this issue Aug 11, 2015
martin-grofcik added a commit to martin-grofcik/flyway that referenced this issue Aug 11, 2015
@pauxus
Copy link
Contributor

@pauxus pauxus commented Jan 13, 2016

A third proposal:

provide an additional flag parameter (flyway.skipDefaultResolvers) to allow to skip the default resolvers. This would also be completly backward compatible.

pauxus added a commit to pauxus/flyway that referenced this issue Jan 13, 2016
Introduces a new parameter flyway.skipDefaultResolvers and its corresponding property to skip usage of default built-in resolvers.
pauxus added a commit to pauxus/flyway that referenced this issue Jan 15, 2016
Introduces a new parameter flyway.skipDefaultResolvers and its corresponding property to skip usage of default built-in resolvers.
@pauxus
Copy link
Contributor

@pauxus pauxus commented Jan 15, 2016

skipDefaultResolvers is actually not enough, since we need a couple things more:

  • Own resolvers need to have access to Locations, Placeholders and DbSupport instances to fully take the place of the built-in resolvers.

I implemented this, along with the skipping in a PR: #811 (see #805)

@pauxus
Copy link
Contributor

@pauxus pauxus commented Jan 26, 2016

created a new PR base on my previous patch:

#1183

@pauxus
Copy link
Contributor

@pauxus pauxus commented Feb 3, 2016

Implementation of skipDefaultResolvers is merged with #1191. This allows to replace default resolvers, however custom subclasses of the resolvers are not yet supported.

@axelfontaine axelfontaine added this to the Flyway 4.0 milestone Feb 3, 2016
axelfontaine added a commit to flyway/flywaydb.org that referenced this issue Feb 3, 2016
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Feb 3, 2016

Thanks @pauxus for the PR!

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.