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

@FeignClient with top level @RequestMapping annotation is also registered as Spring MVC handler #466

Closed
jmnarloch opened this issue Jul 28, 2015 · 25 comments
Labels

Comments

@jmnarloch
Copy link
Contributor

@jmnarloch jmnarloch commented Jul 28, 2015

This is follow up to the discussion from this issue: #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: jmnarloch@2d24bf6 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.

@jmnarloch

This comment has been minimized.

Copy link
Contributor Author

@jmnarloch jmnarloch commented Jul 28, 2015

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.

@dsyer

This comment has been minimized.

Copy link
Contributor

@dsyer dsyer commented Jul 31, 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.

@spencergibb

This comment has been minimized.

Copy link
Member

@spencergibb spencergibb commented Nov 11, 2015

Closing, this would need to change in the framework.

@succour

This comment has been minimized.

Copy link

@succour succour commented Feb 3, 2016

Does it have any new progress?

@spencergibb spencergibb added the wontfix label Feb 3, 2016
@spencergibb

This comment has been minimized.

Copy link
Member

@spencergibb spencergibb commented Feb 3, 2016

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

@joevalerio

This comment has been minimized.

Copy link

@joevalerio joevalerio commented Jul 20, 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;
    }
}
@dsyer

This comment has been minimized.

Copy link
Contributor

@dsyer dsyer commented Jul 20, 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.

@joevalerio

This comment has been minimized.

Copy link

@joevalerio joevalerio commented Jul 20, 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.

@cforce

This comment has been minimized.

Copy link

@cforce cforce commented Jul 20, 2016

We have worked around it by using anotation on method only

joevalerio notifications@github.com 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
#466 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAYARoXzsDgJ-tBQoTmLSjlpZhl1NCvcks5qXkpEgaJpZM4FhB-m
.

@kadaan

This comment has been minimized.

Copy link

@kadaan kadaan commented Oct 28, 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);
        }
    }
}
@DevWantJustHaveFun

This comment has been minimized.

Copy link

@DevWantJustHaveFun DevWantJustHaveFun commented Dec 21, 2016

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

@zhijun714

This comment has been minimized.

Copy link

@zhijun714 zhijun714 commented Aug 4, 2017

@kadaan thank for your code.

@mengxiangrui007

This comment has been minimized.

Copy link

@mengxiangrui007 mengxiangrui007 commented Sep 1, 2017

@kadaan thank for your code.

@chuguoren

This comment has been minimized.

Copy link

@chuguoren chuguoren commented Dec 27, 2017

@kadaan thank for your code.

@qiujiayu

This comment has been minimized.

Copy link

@qiujiayu qiujiayu commented Mar 15, 2018

@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

@ARadauer

This comment has been minimized.

Copy link

@ARadauer ARadauer commented Mar 27, 2018

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.

        protected boolean isHandler(Class<?> beanType) {
            return (AnnotatedElementUtils.hasAnnotation(beanType, Controller.class));
        }
  
@dsyer

This comment has been minimized.

Copy link
Contributor

@dsyer dsyer commented Mar 27, 2018

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

@asgh

This comment has been minimized.

Copy link

@asgh asgh commented Jul 12, 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.

@sparqueur

This comment has been minimized.

Copy link

@sparqueur sparqueur commented Sep 18, 2018

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

@Hronom

This comment has been minimized.

Copy link

@Hronom Hronom commented Dec 23, 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.

@spencergibb

This comment has been minimized.

Copy link
Member

@spencergibb spencergibb commented Dec 23, 2018

That's in spring Framework and not handled here, hence it was closed. Open in issue in http://jira.spring.io if you'd like

@Hronom

This comment has been minimized.

Copy link

@Hronom Hronom commented Dec 23, 2018

@spencergibb Was able to reproduce this, will create issue for this on jira. Reproducible at Greenwich.RC2 and Spring Boot 2.1.1.RELEASE.

Here is my test project for this https://github.com/Hronom/test-shared-mapping-interface

@Hronom

This comment has been minimized.

Copy link

@Hronom Hronom commented Dec 23, 2018

@storm-coding

This comment has been minimized.

Copy link

@storm-coding storm-coding commented Jan 3, 2019

Why not use path. Such as @FeignClient(name="localapp", path="users")

@storm-coding

This comment has been minimized.

Copy link

@storm-coding storm-coding commented Jan 3, 2019

@spring-cloud spring-cloud locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.