Spring-cloud-netflix: @FeignClient with top level @RequestMapping annotation is also registered as Spring MVC handler

Created on 28 Jul 2015 · 25Comments · Source: spring-cloud/spring-cloud-netflix

This is follow up to the discussion from this issue: https://github.com/spring-cloud/spring-cloud-netflix/issues/457

Generally the problem is that: when you annotate an @FeignClient interface with @RequestMapping annotation the handler will be also registered within Spring MVC.

Example:

@FeignClient("localapp")
 @RequestMapping(value = "/users") 
public interface UsersClient extends CrudClient<User> {
  }

In this test case: https://github.com/jmnarloch/spring-cloud-netflix/commit/2d24bf630e57f6aec012983b4465c8cc8875dea7 this acctually causes the test to fail with error:

Caused by: java.lang.IllegalStateException: Ambiguous mapping. Cannot map 'feignHttpClientTests$UserClient' method 
public abstract T org.springframework.cloud.netflix.feign.valid.FeignHttpClientTests$CrudClient.get(long)
to {[/users/{id}],methods=[GET]}: There is already 'feignHttpClientTests.Application' bean method
public java.lang.Object org.springframework.cloud.netflix.feign.valid.FeignHttpClientTests$Application.get(long) mapped.
    at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping$MappingRegistry.assertUniqueMethodMapping(AbstractHandlerMethodMapping.java:565)
    at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping$MappingRegistry.register(AbstractHandlerMethodMapping.java:529)
    at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.registerHandlerMethod(AbstractHandlerMethodMapping.java:252)
    at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.detectHandlerMethods(AbstractHandlerMethodMapping.java:223)
    at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.initHandlerMethods(AbstractHandlerMethodMapping.java:183)
    at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.afterPropertiesSet(AbstractHandlerMethodMapping.java:164)
    at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping.afterPropertiesSet(RequestMappingHandlerMapping.java:134)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.invokeInitMethods(AbstractAutowireCapableBeanFactory.java:1637)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1574)
    ... 44 more

Due to duplicated Spring MVC - FeignClient mappings, but this how I found this issue.

Removing the top @RequestMapping fixes the tests, but disallows some advances usages like interface inheritance and path overriding.

wontfix

Source

jmnarloch

Most helpful comment

What about doing:

@Configuration
@ConditionalOnClass({Feign.class})
public class FeignMappingDefaultConfiguration {
    @Bean
    public WebMvcRegistrations feignWebRegistrations() {
        return new WebMvcRegistrationsAdapter() {
            @Override
            public RequestMappingHandlerMapping getRequestMappingHandlerMapping() {
                return new FeignFilterRequestMappingHandlerMapping();
            }
        };
    }

    private static class FeignFilterRequestMappingHandlerMapping extends RequestMappingHandlerMapping {
        @Override
        protected boolean isHandler(Class<?> beanType) {
            return super.isHandler(beanType) && (AnnotationUtils.findAnnotation(beanType, FeignClient.class) == null);
        }
    }
}

kadaan on 29 Oct 2016

👍23 ❤3

All 25 comments

So is there some neat way to hide the @RequestMapping annotation on the interface type?
I guess that it's being exposed through FeignClientFactoryBean#getObjectType method.

jmnarloch on 28 Jul 2015

There's no way to hide it other than not be in a web application. Or I guess put it in a child context with no handler mappings. I don't expect this will be explicitly supportable in the framework, but if you try it and it works it might be good to share your experience here.

dsyer on 31 Jul 2015

Closing, this would need to change in the framework.

spencergibb on 11 Nov 2015

Does it have any new progress?

succour on 3 Feb 2016

@succour this issue is closed and we cannot fix it as this how spring framework works.

spencergibb on 3 Feb 2016

could we not get around this by just extending the EnableWebMvcConfiguration to ignore classes annotated with @RequestMapping AND @FeignClient? Something like this? It appears to be working.

@Configuration
public class WebConfiguration extends EnableWebMvcConfiguration {

    @Override
    protected RequestMappingHandlerMapping createRequestMappingHandlerMapping() {
    RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping(){
            @Override
            protected boolean isHandler(Class<?> beanType) {
                return super.isHandler(beanType) && (AnnotationUtils.findAnnotation(beanType, FeignClient.class) == null);
            }
    };
    return mapping;
    }
}

joevalerio on 20 Jul 2016

That's a decent workaround. I'm not sure I want that code to be in Spring Cloud though. IMO Spring MVC should not consider a bean without a @Controller annotation to be a handler. You'd have to take that up in JIRA though.

dsyer on 20 Jul 2016

I agree this should not exist in spring-cloud but only as a specific workaround for this issue. I also agree with your other point about @Controller but that ship has sail and it would be a major breaking change to revert that now. In the mean time, we need to manage clients. IMO they are easier to maintain with the @RequestMapping annotations, and applying it at the Class level reduces the need for each method's @RequestMapping to set its consumes/produces property, making the file much easier to read and maintain.

Long term, it would be great if we could delegate the isHandler method to a Bean, then spring-cloud-netflix could supply a version that takes @FeignClient into consideration to avoid the bindings.

joevalerio on 20 Jul 2016

We have worked around it by using anotation on method only

joevalerio [email protected] schrieb am Mi., 20. Juli 2016, 18:18:

I agree this should not exist in spring-cloud but only as a specific
workaround for this issue. I also agree with your other point about
@Controller https://github.com/Controller but that ship has sail and it
would be a major breaking change to revert that now. In the mean time, we
need to manage clients. IMO they are easier to maintain with the
@RequestMapping annotations, and applying it at the Class level reduces the
need for each method's @ReqeustMapping to set its consumes/produces
property, making the file much easier to read and maintain.

Long term, it would be great if we could delegate the isHandler method to
a Bean, then spring-cloud-netflix could supply a version that does take the
@FeignClient into consideration to avoid the bindings.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/spring-cloud/spring-cloud-netflix/issues/466#issuecomment-234000603,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAYARoXzsDgJ-tBQoTmLSjlpZhl1NCvcks5qXkpEgaJpZM4FhB-m
.

cforce on 20 Jul 2016

What about doing:

@Configuration
@ConditionalOnClass({Feign.class})
public class FeignMappingDefaultConfiguration {
    @Bean
    public WebMvcRegistrations feignWebRegistrations() {
        return new WebMvcRegistrationsAdapter() {
            @Override
            public RequestMappingHandlerMapping getRequestMappingHandlerMapping() {
                return new FeignFilterRequestMappingHandlerMapping();
            }
        };
    }

    private static class FeignFilterRequestMappingHandlerMapping extends RequestMappingHandlerMapping {
        @Override
        protected boolean isHandler(Class<?> beanType) {
            return super.isHandler(beanType) && (AnnotationUtils.findAnnotation(beanType, FeignClient.class) == null);
        }
    }
}

kadaan on 29 Oct 2016

👍23 ❤3

@kadaan thank for your code, it's working great :)

DevWantJustHaveFun on 21 Dec 2016

@kadaan thank for your code.

zhijun714 on 4 Aug 2017

@kadaan thank for your code.

mengxiangrui007 on 1 Sep 2017

@kadaan thank for your code.

chuguoren on 27 Dec 2017

@Configuration
@ConditionalOnClass({ Feign.class })
public class FeignMappingDefaultConfiguration {
    @Bean
    public WebMvcRegistrations feignWebRegistrations() {
        return new WebMvcRegistrationsAdapter() {
            @Override
            public RequestMappingHandlerMapping getRequestMappingHandlerMapping() {
                return new FeignFilterRequestMappingHandlerMapping();
            }
        };
    }

    @Slf4j
    private static class FeignFilterRequestMappingHandlerMapping extends RequestMappingHandlerMapping {
        @Override
        protected boolean isHandler(Class<?> beanType) {
            return super.isHandler(beanType) && !beanType.isInterface();
        }
    }
}

@kadaan

qiujiayu on 15 Mar 2018

👍1

In my opinion this is a major security issue in spring. My application is exposing services which I call internally just because the team of the other service annotated their interfaces with @ RequestMapping on top level.
This is not what I would expect. If I want a Controller to behave like a Controller it should be annotated as Controller.
``` @Override
protected boolean isHandler(Class beanType) {
return (AnnotatedElementUtils.hasAnnotation(beanType, Controller.class));
}

```

ARadauer on 27 Mar 2018

👍4

I guess you need to open an issue in the Spring JIRA for that?

dsyer on 27 Mar 2018

Can you maybe throw an error if a FeignClient also has a RequestMapping? This would at least help people who get bitten by this.

asgh on 12 Jul 2018

Workaround provided by @kadaan works well when running the server but fails in Spring boot tests (FeignFilterRequestMappingHandlerMapping.isHandler is not called).
Any clue ?

sparqueur on 18 Sep 2018

What the progress on this?
Why not just add rule to the Spring MVC that will register handler only if it annotated by @Controller or @RestController? Looks very oblivious and will not bring to the Spring MVC any additional dependency.