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: * https://github.com/spring-cloud/spring-cloud-netflix/issues/466 * https://stackoverflow.com/questions/29284911/can-a-spring-cloud-feign-client-share-interface-with-an-spring-web-controller

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

Comment From: spring-projects-issues

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?

Comment From: spring-projects-issues

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.

Comment From: spring-projects-issues

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.

Comment From: rstoyanchev

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.

Comment From: remal

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. 1. 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.

Comment From: TannnnnnnnnnnnnnnnK

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

Comment From: glockbender

This issue is still relevant. Any progress with that?

Comment From: odrotbohm

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.

Comment From: mothinx

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

Comment From: odrotbohm

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

Comment From: rstoyanchev

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.

Comment From: remal

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

Comment From: rstoyanchev

@remal, yes you can follow here https://github.com/spring-cloud/spring-cloud-openfeign/issues/547.

Comment From: OrangeDog

Just to note that the previous behaviour allowed declaring and constructing the controller with a @Bean method. Now that you have to add @Controller to the class, they are automatically declared by @ComponentScan, which will now need additional exclusion filters.