Auto-configured OpenEntityManagerInViewInterceptor does not back off when an OpenEntityManagerInViewFilter is only defined as a registration bean

[wilkinsona]

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

If you have contributed before, consider leaving this one for someone new, and looking through our general ideal-for-contribution issues. Thanks!

Background

Spring Boot auto-configures an OpenEntityManagerInViewInterceptor in JpaWebConfiguration. When the user provides their own equivalent bean, the auto-configured interceptor should back off.

Problem

The auto-configuration is conditional on there being no OpenEntityManagerInViewInterceptor or OpenEntityManagerInViewFilter:

spring-boot/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/orm/jpa/JpaBaseConfiguration.java

Lines 212 to 213 in 99656b9

	@ConditionalOnMissingBean({ OpenEntityManagerInViewInterceptor.class,
	OpenEntityManagerInViewFilter.class })

This means that it correctly backs off when the user defines a bean of either type, however it does not back off if the user defines a FilterRegistrationBean<OpenEntityManagerInViewFilter> bean unless they also define an OpenEntityManagerInViewFilter bean. Defining only a FilterRegistrationBean<OpenEntityManagerInViewFilter> bean should be sufficient to cause the auto-configuration of the interceptor to back off.

Solution

Rather than considering OpenEntityManagerInViewFilter in the @ConditionalOnMissingBean, @ConditionalOnMissingFilterBean(OpenEntityManagerInViewFilter.class) should be used instead. This will cause the auto-configured filter to back off when there is a user-defined OpenEntityManagerInViewFilter or FilterRegistrationBean<OpenEntityManagerInViewFilter> bean.

There is an existing test that checks the OpenEntityManagerInViewFilter case:

spring-boot/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/orm/jpa/AbstractJpaAutoConfigurationTests.java

Lines 149 to 159 in 99656b9

	@Test
	public void openEntityManagerInViewInterceptorIsNotRegisteredWhenFilterPresent() {
	new WebApplicationContextRunner()
	.withPropertyValues("spring.datasource.generate-unique-name=true")
	.withUserConfiguration(TestFilterConfiguration.class)
	.withConfiguration(AutoConfigurations.of(
	DataSourceAutoConfiguration.class,
	TransactionAutoConfiguration.class, this.autoConfiguredClass))
	.run((context) -> assertThat(context)
	.doesNotHaveBean(OpenEntityManagerInViewInterceptor.class));
	}

A new test for the filter registration bean case should be added. Rather than using TestFilterConfiguration it should use a new TestFilterRegistrationConfiguration class that defines a FilterRegistrationBean<OpenEntityManagerInViewFilter> bean.

Steps to Fix

• Claim this issue with a comment below and ask any clarifying questions you need
• Set up a repository locally following the Contributing Guidelines
• Try to fix the issue following the steps above
• Commit your changes and start a pull request.

[acemoo]

I would love to see if I can get this done this weekend.

[wilkinsona]

Great! Thank you. Please let us know if you have any questions.

[xafarr]

Thanks @wilkinsona. I am looking into this now.

[mbhave]

@xafarr Thanks for offering to work on this but @acemoo is already doing so. Please keep an eye out for other first-timers-only issues in the future and we'll be happy to guide you through the process once you've claimed an issue.

[xafarr]

@mbhave Thanks. I missed that. Will keep an eye on first-timers-only.

[rajagopalmani]

I will pick it, please, or something else which I could start with if it is assigned already ( Just read that someone else also commented to pick this issue)

[rajagopalmani]

I have done below changes:
JpaBaseConfiguration.class

@Configuration
	@ConditionalOnWebApplication(type = Type.SERVLET)
	@ConditionalOnClass(WebMvcConfigurer.class)
	@ConditionalOnMissingFilterBean(OpenEntityManagerInViewFilter.class)
	@ConditionalOnMissingBean(OpenEntityManagerInViewFilter.class)
	@ConditionalOnProperty(prefix = "spring.jpa", name = "open-in-view", havingValue = "true", matchIfMissing = true)
	protected static class JpaWebConfiguration {

AbstractJpaAutoConfigurationTests.class

@Test
	public void openEntityManagerInViewInterceptorIsNotRegisteredWhenFilterRegistrationPresent() {
		new WebApplicationContextRunner()
				.withPropertyValues("spring.datasource.generate-unique-name=true")
				.withUserConfiguration(TestFilterRegistrationConfiguration.class)
				.withConfiguration(AutoConfigurations.of(
						DataSourceAutoConfiguration.class,
						TransactionAutoConfiguration.class, this.autoConfiguredClass))
				.run((context) -> assertThat(context)
						.doesNotHaveBean(OpenEntityManagerInViewInterceptor.class));
	}

        @Configuration
	@TestAutoConfigurationPackage(City.class)
	protected static class TestFilterRegistrationConfiguration {
		
		@Bean
		public FilterRegistrationBean<OpenEntityManagerInViewFilter> openEntityManagerInViewFilterRegistration(){
		    FilterRegistrationBean<OpenEntityManagerInViewFilter> registrationBean 
		      = new FilterRegistrationBean<>();
		    registrationBean.setFilter(new OpenEntityManagerInViewFilter());
		    return registrationBean; 		    
		}

	}

Please verify if all look good and help me on what to do next.

[snicoll]

@rajagopalmani as you've figured out yourself, this issue is already taken and a comment above provides already everything you need to know.

[rajagopalmani]

@rajagopalmani as you've figured out yourself, this issue is already taken and a comment above provides already everything you need to know.

True, I have been trying to contribute and never knew how or where to start. So thought of dropping a message to see if something else is open. Sorry if I have disturbed the thread.

[acemoo]

The ticket mentions leaving out the @ConditionalOnMissingBean(OpenEntityManagerInViewFilter.class) but this seems necessary for the tests to succeed.
Also, I think there is a test missing for the @ConditionalOnMissingBean(OpenEntityManagerInViewInterceptor.class)
The auto-configuration should also backoff when there is already an interceptor class right?

[wilkinsona]

Thanks for this, @acemoo.

@ConditionalOnMissingBean(OpenEntityManagerInViewFilter.class) but this seems necessary for the tests to succeed.

It should be possible to omit it, but due to, I think, a bug in the condition's implementation, you're right that it's needed at the moment.

Also, I think there is a test missing for the @ConditionalOnMissingBean(OpenEntityManagerInViewInterceptor.class)

Well spotted. We should add a test for that case.

The auto-configuration should also backoff when there is already an interceptor class right?

It should. I think the implementation will do that at the moment, but, as you've noticed, it's not tested.