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

Import ErrorWebFluxAutoConfiguration with @WebFluxTest #16266

Conversation

alimate
Copy link
Contributor

@alimate alimate commented Mar 19, 2019

It's worth mentioning that the imported auto-configuration will register a bean of type ErrorWebExceptionHandler if there is no such bean already in the application context. So in order to provide a custom handler, we need to register a bean of type ErrorWebExceptionHandler, not the WebExceptionHandler.

Closes #16258
Related Issue #13627

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 19, 2019
*/
@Component
public class ExampleWebExceptionHandler implements WebExceptionHandler {
@ConditionalOnProperty(name = "custom-error-handler.enable")
Copy link
Contributor

Choose a reason for hiding this comment

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

@alimate Could you explain why this change is necessary?

Copy link
Contributor Author

@alimate alimate Mar 22, 2019

Choose a reason for hiding this comment

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

@mbhave Apparently, As part of the #13627, We support the WebExceptionHandlers in WebFluxTests. As far as I know, this behavior is inconsistent (Please correct me if I'm wrong) with when we run Spring Boot applications manually. In the latter scenario, we should register a bean of type ErrorWebExceptionHandler (Not of type WebExceptionHandler) to disable the default error handling and use a custom approach for error handling.

Basically, here I'm implementing the ErrorWebExceptionHandler interface to align error handling in automatic and manual testing scenarios.

Also, I'm registering this new handler conditionally to test both of the following:

  • When there is no ErrorWebExceptionHandler registered, the default one provided by the ErrorWebFluxAutoConfiguration should be used.
  • Otherwise, Spring Boot discards the default one and registers our custom ErrorWebExceptionHandler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. I think we'd still want to keep the WebExceptionHandler to make sure that those get picked up by @WebFluxTest. For testing that the ErrorWebFluxAutoConfiguration is imported, the test that you've added in WebFluxTestAutoConfigurationIntegrationTests should suffice.

Let's see what @bclozel thinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created a sample repo for this inconsistent behaviour
https://github.com/alimate/webflux-Inconsistency

Copy link
Member

Choose a reason for hiding this comment

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

This is not a case of ErrorWebExceptionHandler vs. WebExceptionHandler, since ErrorWebExceptionHandler is extending WebExceptionHandler after all.

This is more about the ordering of the beans - by default Spring Boot configures the ErrorWebExceptionHandler instance at order -1 to make sure to handle errors before the WebExceptionHandler provided by Spring WebFlux.

An application can still configure a WebExceptionHandler earlier to process errors before the handler provided by Spring Boot.

In short, I agree with @mbhave and the @ConditionalOnProperty change is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bclozel Thanks. So instead of registering the ExampleWebExceptionHandler as a Component, we're better off registering it manually using a @Bean with different Orders, right?

Copy link
Contributor Author

@alimate alimate Mar 25, 2019

Choose a reason for hiding this comment

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

Anyway, I removed the Component and ConditionalOnProperty annotations from the ExampleWebExceptionHandler.

Instead, I registered the exception handler using a @Bean with different priorities to test the fact that WebExceptionHandlers with appropriate Orders would be called before the default handler provided by the ErrorWebFluxAutoConfiguration.

What do you think? @bclozel @mbhave

Copy link
Contributor

Choose a reason for hiding this comment

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

@alimate I don't think we need to change anything related to ExampleWebExceptionHandler. The fact that a WebExceptionHandler can be configured with a higher precedence than the one provided by Spring Boot doesn't need to be tested in a test for @WebFluxTest. This issue is about ensuring that ErrorWebFluxAutoConfiguration is imported by @WebFluxTest and I think the only tests you need to change for that are WebFluxTestAutoConfigurationIntegrationTests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbhave OK. I'm reverting back some of the changes 👍

@mbhave mbhave added the type: bug A general bug label Mar 22, 2019
@mbhave mbhave added this to the 2.1.x milestone Mar 22, 2019
@alimate alimate force-pushed the webfluxtest-import-error-autoconfig branch from f8ef26c to 6f77b9e Compare March 26, 2019 04:20
@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged label Mar 28, 2019
@snicoll snicoll self-assigned this Mar 29, 2019
@snicoll snicoll changed the title Importing the ErrorWebFluxAutoConfiguration in @WebFluxTest Import ErrorWebFluxAutoConfiguration with @WebFluxTest Mar 29, 2019
@snicoll snicoll modified the milestones: 2.1.x, 2.1.4 Mar 29, 2019
snicoll pushed a commit that referenced this pull request Mar 29, 2019
snicoll added a commit that referenced this pull request Mar 29, 2019
* pr/16266:
  Polish "Add error rendering support with @WebFluxTest"
  Add error rendering support with @WebFluxTest
@snicoll snicoll closed this in 090f5f5 Mar 29, 2019
@snicoll
Copy link
Member

snicoll commented Mar 29, 2019

@alimate thank you for making your first contribution to Spring Boot. This is now merged in 2.1.x and master with a polish commit as one test was still failing.

@alimate alimate deleted the webfluxtest-import-error-autoconfig branch April 2, 2019 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants