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

@RequestMapping without @Controller registered as handler [SPR-17622] #22154

Closed
spring-projects-issues opened this issue Dec 23, 2018 · 13 comments
Closed
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 23, 2018

Eugene Tenkaev opened SPR-17622 and commented

Following this approach here http://projects.spring.io/spring-cloud/spring-cloud.html#spring-cloud-feign-inheritance

If you add root @RequestMapping to the UserService it will be registered as handler in Spring MVC application.

Example project to reproduce here https://github.com/Hronom/test-shared-mapping-interface

Related discussions:

To handle this properly need to avoid registration of controller that has only @RequestMapping annotation.

Proposed solution:
Register handler only if it has annotation @Controller or @RestController.


Affects: 5.1.3

Issue Links:

  • #16747 Introduce proxy-based REST client similar to HttpInvokerProxyFactoryBean

1 votes, 3 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 24, 2018

Sébastien Deleuze commented

I understand the rationale behind what you ask, but class level @Component + @RequestMapping is supported for a long time, so removing that support would break a lot of applications.

Rossen Stoyanchev Arjen Poutsma Could you share your point of view on this change request?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 24, 2018

Eugene Tenkaev commented

Sébastien Deleuze I have edit description the idea is next:
Make Spring MVC register endpoint only if it has @Controller or @RestController on the class level.

My example shows that:
Right now, if class has only @RequestMapping - this class will be registered as the endpoint in Spring MVC.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 25, 2018

Sébastien Deleuze commented

I understand, but what I wanted to clarify is that @RequestMapping alone does not expose endpoints automatically, but beans with class level @RequestMapping annotations does and this is used by developers in various ways, one of these ways being my @Component + @RequestMapping example.

@FeignClient itself is not meta annotated with @Component, but I guess it is registered as a bean by FeignClientFactoryBean (I have not verified) which is from Spring Framework POV similar to @Component + @RequestMapping or programmatic bean registration of classes annotated by @RequestMapping.

I understand your request for being more restrictive in how we identify REST endpoints, and the issue raised for @FeignClient could apply for #16747. But I am also concern by such breaking change, that's why I am asking Rossen and Arjen feedback who have more knowledge and context than me on that topic.

@spring-projects-issues spring-projects-issues added type: bug status: waiting-for-triage in: web and removed type: bug labels Jan 11, 2019
@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Jan 18, 2019

Class level @RequestMapping is used as a hint, independent of @Controller, because @RequestMapping can be used on an interface, in which case the controller can be an AOP proxy and the @Controller annotation is not accessible to Spring MVC through the proxy.

@jhoeller do you see any options to refine the checks, e.g. if type-level @RequestMapping is found without @Controller and the bean is a proxy, then introspect further to see if we can find the @Controller annotation?

Note also that Spring Data REST has a similar situation for REST endpoints, and it solves that through an additional RequestMappingHandlerMapping instance ordered earlier + a special stereotype to identify such endpoints, which works but is probably more involved than it needs to be. We could also try and suppress Spring MVC from treating certain types (based on some criteria) as controllers but again that doesn't seem ideal and requires extra config.

@remal
Copy link

@remal remal commented Jul 5, 2019

This issue leads to a lot of different problems that are very hard to debug. Please fix it. I can suggest these solutions:

  1. Do not treat classes annotated by @RequestMapping as handlers. Only @Controller annotation should be taken into consideration.
  2. Spring Data has @NoRepositoryBean. A similar annotation can be created for request handlers. For example: @NoRequestHandler.
    • In this case @FeignClient annotation can be annotated by this @NoRequestHandler annotation.

I'd suggest implementing the first solution.

@sbrannen sbrannen added the for: team-attention label Jul 6, 2019
@TannnnnnnnnnnnnnnnK
Copy link

@TannnnnnnnnnnnnnnnK TannnnnnnnnnnnnnnnK commented Apr 16, 2020

regist handler and regist requestmapping
two different things, should be separate from each other, even they are related

@glockbender
Copy link

@glockbender glockbender commented Jun 16, 2020

This issue is still relevant. Any progress with that?

@odrotbohm
Copy link
Member

@odrotbohm odrotbohm commented Jul 14, 2020

Copying the description of #25386 here for reference:

tl;dr – The current behavior is problematic for folks trying to customize Spring Data REST using custom controllers, too, as it subtley makes controllers using @RequestMapping on the type level end up in the wrong handler mapping

Rossen mentioned that here already, but it just recently came up in StackOverflow questions again.

RequestMappingHandlerMapping.isHandler(…) not only picks up types annotated with @Controller but also ones that are annotated with @RequestMapping on the type level. This is problematic in cases in which other HandlerMapping instances are registered that might be supposed to handle those controllers.

A prominent example of this is Spring Data REST, which registers a dedicated mapping to expose HTTP resources for Spring Data repositories. Users can selectively override those resources by declaring a controller themselves and just declare a handler method for e.g. the URI of an item resource and an HTTP verb of choice. If that controller now declares an @RequestMapping on the type level, the Spring MVC registered one will pick up that class, and not see any other mappings defined for the same URI pattern but exposing support for other HTTP methods potentially available in subsequent HanderMapping implementations.

This is a pretty common error scenario reported by users (see this StackOverflow question for example). It's also pretty hard to explain to users as it involves talking about quite a few implementation details.

Removing the explicit handling of @RequestMapping on the type level bears the risk that controller implementations not also being annotated with @Controller would not be picked up automatically anymore. I haven't found any Spring MVC related documentation that actually shows an example of code not using the annotations in combination when used at the type level. A fix for that issue would be to also annotate the affected controller type with @Controller. I can see this being suboptimal for a release in a minor version but for 6.0 we should at least reevaluate.

@mothinx
Copy link

@mothinx mothinx commented Nov 12, 2020

Just get this issue with a feign client and spent a lot of hours to locate it. Is someone on that issue ?

@odrotbohm
Copy link
Member

@odrotbohm odrotbohm commented Dec 18, 2020

This just came up in yet another Spring Data REST issue: https://jira.spring.io/browse/DATAREST-1591.

@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Oct 6, 2021

Team Decision: after some cross-team discussions, for the short term the issue will be addressed on the side of Spring Data REST and Spring Cloud.

For Spring Framework 6.0, we will also address this in the Spring Framework by no longer considering a class with a type-level @RequestMapping as a candidate for detection, unless there is also @Controller.

@rstoyanchev rstoyanchev added type: enhancement and removed for: team-attention status: waiting-for-triage labels Oct 6, 2021
@rstoyanchev rstoyanchev added this to the 6.0 M1 milestone Oct 6, 2021
@remal
Copy link

@remal remal commented Oct 6, 2021

@rstoyanchev do you know if there is a corresponding ticket in Spring Cloud that can be subscribed to?

@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Oct 14, 2021

@remal, yes you can follow here spring-cloud/spring-cloud-openfeign#547.

wilkinsona added a commit to wilkinsona/spring-framework that referenced this issue May 11, 2022
See spring-projectsgh-22154 which removed support for a type-level @RequestMapping
annotation alone being sufficient for handler detection.
sbrannen pushed a commit that referenced this issue May 11, 2022
See gh-22154 which removed support for a type-level @RequestMapping
annotation alone being sufficient for handler detection.

Closes gh-28448
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

8 participants