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

Database migrations may not have run before JdbcTemplate is used when creating Flyway @Bean #21436

Closed
jordanms opened this issue May 13, 2020 · 6 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@jordanms
Copy link

jordanms commented May 13, 2020

Using Spring Boot 2.2.4.RELEASE.

This is the same problem as in #13155, the difference is that a Flyway bean is being provided using a @Configuration class.

@Configuration
public class FlywayConfiguration {

  @Bean
  public Flyway myFlyway(DataSource dataSource) {
    return new Flyway(...);
  }

  // This is needed so we can use FlywayMigrationStrategy
  @Bean
  public FlywayMigrationInitializer myFlywayInitializer(Flyway myFlyway,
      ObjectProvider<FlywayMigrationStrategy> migrationStrategy) {
    return new FlywayMigrationInitializer(myFlyway, migrationStrategy.getIfAvailable());
  }
}

I checked the code and the problem is that in FlywayAutoConfiguration the FlywayMigrationInitializer*DependsOnPostProcessor classes are only imported if Flyway is not defined:

	@Configuration(proxyBeanMethods = false)
	@ConditionalOnMissingBean(Flyway.class)
	@EnableConfigurationProperties({ DataSourceProperties.class, FlywayProperties.class })
	@Import({ FlywayMigrationInitializerEntityManagerFactoryDependsOnPostProcessor.class,
			FlywayMigrationInitializerJdbcOperationsDependsOnPostProcessor.class,
			FlywayMigrationInitializerNamedParameterJdbcOperationsDependsOnPostProcessor.class })
	public static class FlywayConfiguration {

As a workaround we can define our own DependsOnPostProcessor class and import it on the @Configuration class.

  static class FlywayMigrationInitializerJdbcOperationsDependsOnPostProcessor
      extends JdbcOperationsDependsOnPostProcessor {

    FlywayMigrationInitializerJdbcOperationsDependsOnPostProcessor() {
      super(FlywayMigrationInitializer.class);
    }
  }

However, this is not ideal because those things are supposed to work out of the box.

The desired behavior is that we would be able to declare our own Flyway bean and everything would work the same way as using the auto-config declared bean.

The solution, in my opinion, is to declare flywayInitializer bean outside FlywayConfiguration class and import FlywayMigrationInitializer*DependsOnPostProcessor in FlywayAutoConfiguration class.

In the attached demo application, uncomment the commented out code in FlywayConfiguration to start the application without error.

demo.zip

@wilkinsona
Copy link
Member

wilkinsona commented May 13, 2020

Thanks for the analysis.

It's not clear why you've defined both a Flyway bean and a FlywayMigrationInitializer. I believe you could combine things and avoid the problem that you're seeing:

@Bean
public Flyway myFlyway(DataSource dataSource, ObjectProvider<FlywayMigrationStrategy> migrationStrategy) {
	Flyway customFlyway = new Flyway();
	migrationStrategy.getIfAvailable(() -> (flyway) -> flyway.migrate()).migrate(customFlyway);
	return customFlyway;
}

However, this is not ideal because those things are supposed to work out of the box.

The current intent of the auto-configuration is that it will back off if you provide your own Flyway bean. Backing off includes not instructing Flyway to perform its migration as it's expected that this will be done as part of the bean being defined. This is why the FlywayMigrationInitializer and the dependencies upon it are defined in FlywayConfiguration so that they back of in the presence of a Flyway bean.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label May 13, 2020
@jordanms
Copy link
Author

Thanks for the reply, @wilkinsona.

It's not clear why you've defined both a Flyway bean and a FlywayMigrationInitializer

As I said, I define it because I want to use FlywayMigrationStrategy. In my current project, we use it for testing so we can clean the db before running tests (don't judge, it's not my call not to use in-memory db for testing :) ).

  @Bean
  public FlywayMigrationStrategy cleanMigrationStrategy() {
    return flyway -> {
      flyway.clean();
      flyway.migrate();
    };
  }

I understand the current intent, but for me, they are two different concerns. One is to configure Flyway and the other is to run the migration. Your proposed solution is mixing those two concerns as both are done in the same method -- and a bean declaration is not supposed to do anything else. The other point is that it is basically duplicating code from FlywayMigrationInitializer.

I do think that separating the concerns is the best approach as it is the cleaner solution and FlywayMigrationStrategy does it very well. I just think that they should be separated in the configuration as well.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 13, 2020
@jordanms
Copy link
Author

Putting it in another way, it should be possible to configure flyway just like that:

@Configuration
public class FlywayConfiguration {

  @Bean
  public Flyway myFlyway(DataSource dataSource) {
    return new Flyway();
  }
}

This is currently not possible. The minimum you have to do is:

@Configuration
public class FlywayConfiguration {

  @Bean(initMethod = "migrate")
  public Flyway myFlyway(DataSource dataSource) {
    return new Flyway();
  }
}

@wilkinsona
Copy link
Member

This is really a question of how and when the auto-configuration should back off. I am not sure that I agree that it should be possible to configure Flyway with just a Flyway @Bean and rely upon the auto-configuration to trigger the migration. That would inconvenience users who want to take complete control over how and when the migration is performed as they'd have to exclude FlywayAutoConfiguration explicitly rather than relying on it backing off.

@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review and removed status: feedback-provided Feedback has been provided labels May 19, 2020
@jordanms
Copy link
Author

That would inconvenience users who want to take complete control over how and when the migration is performed as they'd have to exclude FlywayAutoConfiguration explicitly rather than relying on it backing off

We all agree that any setup we choose will be inconvenient for some use cases. Therefore, we have to choose the one that is convenient for the majority of the use cases. Spring boot definitely does that with auto-configuration the way it is, using application.propperties to configure Flyway. Then we have the less common use cases (the ones we are discussing here):

  1. Manually configure Flyway.
  2. Manually trigger the migration.
  3. Manually configure Flyway and trigger the migration.

Solution with the current implementation:

  @Bean(initMethod = "migrate")
  public Flyway myFlyway(DataSource dataSource) {
    return new Flyway();
  }

The problem with that solution is that while your intent is to manually configure Flyway, you are forced to trigger the migration too. You also lose the flywayMigrationStrategy feature (unless you configure it manually too). Basically, you are forced into use case 3.

  1. See here
    @Bean
    public FlywayMigrationStrategy flywayMigrationStrategy() {
        return flyway -> {
            // do nothing  
        };
    }

Exactly what you expect.

  @Bean
  public Flyway myFlyway(DataSource dataSource) {
    return new Flyway();
  }

  // More code somewhere else to do what's needed.

While the developers get the behaviour they want, the mechanism for disabling auto migration is hidden in the auto-configuration logic. If the developer is not aware of the details, one might spend a considerable amount of time to figure out why the FlywayMigrationStrategy that was defined doesn't work.

Solution with the proposed implementation:

  @Bean
  public Flyway myFlyway(DataSource dataSource) {
    return new Flyway();
  }

Exactly what you expect.

    @Bean
    public FlywayMigrationStrategy flywayMigrationStrategy() {
        return flyway -> {
            // do nothing  
        };
    }

Exactly what you expect.

  @Bean
  public Flyway myFlyway(DataSource dataSource) {
    return new Flyway();
  }

  @Bean
  public FlywayMigrationStrategy flywayMigrationStrategy() {
      return flyway -> {
          // do nothing  
      };
  }

  // More code somewhere else to do what's needed.

Few lines more of code, but both concerns are clearly overridden (no hidden magic), which reduces the chances of a mistake from the developer, makes the code cleaner and easier to maintain.

Last but not least, among the three use cases we discussed here, the third is clearly the least frequent.

@wilkinsona
Copy link
Member

We've discussed this one today and concluded that things should be left as they are. FlywayMigrationStrategy is intended for customization of the migration of the auto-configured Flyway bean. If you are defining your own Flyway bean that is a signal that you wish to take control of how the Flyway bean is defined and migrated. If the behaviour was changed as you have proposed, we may break existing applications that are relying on the definition of a Flyway bean allowing them to take this control.

@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

3 participants