Skip to content

Caching of ProducesRequestCondition in EndpointHandlerMapping may break custom HandlerMapping or ContentTypeResolver arrangements #20150

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

Closed
ykoyano opened this issue Feb 13, 2020 · 3 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@ykoyano
Copy link

ykoyano commented Feb 13, 2020

Due to the optimization of ProducesRequestCondition, HandlerMapping caches MediaTypesAttribute.
However, WebFluxEndpointHandlerMapping and CloudFoundryWebFluxEndpointHandlerMapping do not delete the cached MediaTypesAttribute and affect the resolution of MediaTypes of other HandlerMappings. ( On the other hand, the class that inherits RequestMappingHandlerMapping deletes the cache.)

These issues are occurring not only in Spring Webflux's HandlerMapping but also in Spring MVC's HandlerMapping.

To solve this problem, I think you need one of the following measures:
Add a process to delete the cache at the end of HandlerMapping to AbstractWebFluxEndpointHandlerMapping and AbstractWebMvcEndpointHandlerMapping.
Move the process of deleting the cache at the end of HandlerMapping from RequestMappingHandlerMapping to RequestMappingInfoHandlerMapping of the parent class.

If my proposed solution is correct, I am ready to create a PR.

Related URL

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 13, 2020
@bclozel
Copy link
Member

bclozel commented Feb 13, 2020

Hello @ykoyano

I can see how we might need to align here with Spring Framework - but the issue you've created only mentions the solution but not the actual problem. Did your application fail because of this problem? What was the problem, what were you trying to do and what was the expected behavior?

We need this type of information to properly triage this issue and assign the fix to a milestone.

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

ykoyano commented Feb 17, 2020

Helllo @bclozel, thank you for the reaction.

In our team, we provided our own custom RequestedContentTypeResolver for RequestMappingHandlerMapping to convert our own accept header and content-type header. (This is because the accept header controls versioning.)

Usually, HandlerMapping is executed in the following order.

  1. WebFluxEndpointHandlerMapping (HeaderContentTypeResolver)
  2. ControllerEndpointHandlerMapping (HeaderContentTypeResolver)
  3. RequestMappingHandlerMapping (CustomRequestedContentTypeResolver)

The value of MediaTypesAttribute cached by HeaderContentTypeResolver of WebFluxEndpointHandlerMapping is cleared when ControllerEndpointHandlerMapping ends.

However, for some reason, our team executed HandlerMapping in the following order. (I am aware that this is a rare case.)

  1. WebFluxEndpointHandlerMapping (HeaderContentTypeResolver)
  2. RequestMappingHandlerMapping (CustomRequestedContentTypeResolver)
  3. ControllerEndpointHandlerMapping (HeaderContentTypeResolver)

In this case, the MediaTypesAttribute value cached by the HeaderContentTypeResolver of WebFluxEndpointHandlerMapping is inherited by RequestMappingHandlerMapping without being cleared. Therefore, we can no longer resolve the MediaTypesAttribute with the CustomRequestedContentTypeResolver.

Manipulating the HandlerMapping order can solve the above problem. However, if WebFluxEndpointHandlerMapping alone can clear the value of MediaTypesAttribute, can the value of MediaTypesAttribute be handled without depending on the order of HandlerMapping?
What do you think?

@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 Feb 17, 2020
@bclozel bclozel self-assigned this Feb 17, 2020
@bclozel bclozel added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Feb 17, 2020
@bclozel bclozel added this to the 2.2.5 milestone Feb 17, 2020
@bclozel bclozel changed the title Side effects of ProducesRequestCondition by WebFluxEndpointHandlerMapping Clear ProducesRequestCondition cache attribute in EndpointHandlerMapping infrastructure Feb 17, 2020
bclozel added a commit that referenced this issue Feb 17, 2020
As of spring-projects/spring-framework#22644, Spring Framework caches
the "produces" condition when matching for endpoints in the
`HandlerMapping` infrastructure. This has been improved in
spring-projects/spring-framework#23091 to prevent side-effects in other
implementations.

Prior to this commit, the Spring Boot actuator infrastructure for
`EndpointHandlerMapping` would not clear the cached attribute,
presenting the same issue as Spring Framework's infrastructure. This
means that a custom arrangement with custom `HandlerMapping` or
`ContentTypeResolver` would not work properly and reuse the cached
produced conditions for other, unintented, parts of the handler mapping
process.

This commit clears the cached data and ensures that other handler
mapping implementations are free of that side-effect.

Fixes gh-20150
@bclozel
Copy link
Member

bclozel commented Feb 17, 2020

Thanks @ykoyano for your contribution - your analysis is spot on and your use case helped triaging this issue!

@bclozel bclozel closed this as completed Feb 17, 2020
@wilkinsona wilkinsona changed the title Clear ProducesRequestCondition cache attribute in EndpointHandlerMapping infrastructure Caching of ProducesRequestCondition in EndpointHandlerMapping may break custom HandlerMapping or ContentTypeResolver arrangements Feb 27, 2020
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

No branches or pull requests

3 participants