Fix check vendor location in FlywayAutoConfiguration

[eddumelendez]

flyway.locations supports vendor resolution. But, if
flyway.check-location=true is added and there is just one location
with vendor location the current implementation fails. This commit adds
validation for vendor locations.

[snicoll]

Thanks for the PR @eddumelendez

I don't understand why the code was moved to a separate auto-configuration. Can you expand on that please? I am not sure checking vendor location's location is a good thing. You may run your upgrades on a database that do not need a vendor specific script, in which case using the regular one only would make perfect sense?

Perhaps @vpavic has an opinion about that?

[eddumelendez]

I don't understand why the code was moved to a separate auto-configuration. Can you expand on that please?

At the beginning I thought that previous validation (FlywayValidatorAutoConfiguration) would be right but in the process I just realized that if I am using the almost the same datasource configuration for Flyway I should throw a BeanCreationException maybe in the same class FlywayAutoConfiguration. I can rework this if we agreed to merge it.

I am not sure checking vendor location's location is a good thing.

Of course this is not required, we can use the default configuration o a custom one without need for vendor location. But, since this feature about resolve {vendor} was added, if users want to handle scripts just by vendors I think a proper validation for scripts location would be useful.

[snicoll]

if users want to handle scripts just by vendors I think a proper validation for scripts location would be useful.

Yes but the problem is that you're not providing a path in your configuration, you are providing a placeholder that's going to be replaced with the database that you are using. If you are using two databases and one of them does not need specific scripts, the startup will fail because the directory does not exist. It means that the use case of this feature is quite narrowed IMO: all the databases that you support must require database specific scripts.

Does that make sense?

[vpavic]

Thanks for the ping @snicoll.

If you are using two databases and one of them does not need specific scripts, the startup will fail because the directory does not exist.

Unless I'm missing something from the scenario you're describing, there shouldn't be a problem like this since the location validation is still based on finding at least one match in provided locations. The only difference is that the location under validation are now having {vendor} placeholder replaced with an actual value. So having something like flyway.locations=classpath:db/migration/common,classpath:db/migration/{vendor} shouldn't run into problems when running for vendor that doesn't have a specific migration, right? OTOH with only flyway.locations=classpath:db/migration/{vendor} this provides validation capabilities that are currently not possible.

[vpavic]

If one uses only vendor specific locations things are currently broken by default in 2.0.0.M6 due to #10807 being merged. So IMO either this needs to be addressed or #10807 need to be reverted.

I've just faced this problem while upgrading from 2.0.0.M5 to 2.0.0.M6 with spring.flyway.locations set to db/migration/{vendor}. Initialization fails with:

java.lang.IllegalStateException: Cannot find migrations location in: [db/migration/{vendor}] (please add migrations or check your Flyway configuration)

Setting spring.flyway.check-location to false works around the problem but it's not the greatest user experience.

[wilkinsona]

@eddumelendez It looks like me like the code may be simpler if the validation was performed during the creation of the Flyway bean. That would provide a single place for both replacing the {vendor} placeholder and for validating that at least one of the locations exists. It would also remove the need for the SpringBootFlyway subclass. What do you think?

[eddumelendez]

@wilkinsona I will rework this later today 😉

[eddumelendez]

@wilkinsona I'm doing so because @ ConfigurationProperties is overriding the value. May the SpringBootFlyway class approach looks better? WDYT?

[wilkinsona]

Ah, yes. Of course. I'd forgotten that we still bind directly to the Flyway instance. You're right. Let's keep the SpringBootFlyway class please.

[philwebb]

Thanks for updates. I've extracted the resolve logic into its own class and pushed this to master.