Skip to content

BATCH-2294: Don't replace my PlatformTransactionManager with a new one #609

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

Conversation

elreydetodo
Copy link

For some reason DefaultBatchConfigurer decides that you can't possibly
already have a PlatformTransactionManager and makes a new one for you.
Because of the way Spring's bean factory works, this new one is
sometimes created after the one you actually wanted, and it replaces
the real one in your application with a lame one that is useless.

This patch changes the implementation of DefaultBatchConfigurer so it
tries to autowire an existing bean and only makes a new one if one
doesn't exist already.

For some reason DefaultBatchConfigurer decides that you can't possibly
already have a PlatformTransactionManager and makes a new one for you.
Because of the way Spring's bean factory works, this new one is
sometimes created after the one you _actually_ wanted, and it _replaces_
the real one in your application with a lame one that is useless.

This patch changes the implementation of DefaultBatchConfigurer so it
tries to autowire an existing bean and only makes a new one if one
doesn't exist already.
@elreydetodo
Copy link
Author

This PR tries to fix the same issue as #415, which has merge conflicts. I took what seems like the simpler route.

I was unsure how to create a test for this situation. I'm open to suggestions. I'm also happy to make adjustments to the way I fixed it if anyone has a better suggestion.

logger.warn("No datasource was provided...using a Map based JobRepository");

if(this.transactionManager == null) {
if(this.transactionManager == null) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formatting seems inconsistent with the Spring Framework Code Style. There should be a space between "if" and the paren. I mimicked what was already in the file because I felt it would be weird to mix the style within a single file. I'm happy to fix the formatting if you'd like me to, but I want to make sure the approach I took is acceptable before I go to that effort.

@elreydetodo
Copy link
Author

@mminella can you take a look at this PR? It's doing the same thing as #415, but it doesn't have a merge conflict and it doesn't add that getter you didn't like.

@mminella
Copy link
Member

@elreydetodo We actually reviewed these PRs internally yesterday. We liked the structure of the if statements in the init method better in #415 but we liked your logging additions. If you want to apply the changes he made to your PR and add a test, we'd be happy to merge it. cc: @benas

@fmbenhassine
Copy link
Contributor

fmbenhassine commented Aug 28, 2018

I was unsure how to create a test for this situation.

I wrote a test suite here: fmbenhassine@b6fa592

While testing this PR, I found a couple of issues:

  • When I specify a data source but not a transaction manager, I was expecting a DataSourceTransactionManager to be used but it is not the case (this test is failing)
  • When I don't specify a data source and no transaction manager, I'm expecting a ResourcelessTransactionManager to be used but this is not the case (this test is failing)

Those tests are failing because of the following sequence:

  1. SimpleBatchConfiguration is loaded, the 5 @Bean annotated methods are invoked and beans are added to the application context. One of them is the transaction manager which is a lazy proxy with null reference.
  2. The setter DefaultBatchConfigurer#setTransactionManager is invoked and gets injected the transaction manager bean created in 1)
  3. The DefaultBatchConfigurer#initialize is invoked where the condition if(this.transactionManager == null) is never true.

As mentioned in #415, I think we should not try to autowire the transaction manager. If the user wants a custom transaction manager, he has to provide a custom BatchConfigurer (subclassing DefaultBatchConfigurer and overriding the getter should be sufficient). Note that this is possible if we use the getter in initialize and createJobRepository methods (this approach is tested here: fmbenhassine@b6fa592, and all tests are passing (except the ones that are ignored because we don't want to address them)).

@mminella
Copy link
Member

Based on the comments @benas made as well as the PR he submitted to address this issue, I'm closing this PR. #634 should address the functionality discussed in this issue. Thank you for the contribution!

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

Successfully merging this pull request may close these issues.

3 participants