Skip to content

Don't use Spring MVC annotations on Feign by default #659

Closed
@spencergibb

Description

@spencergibb
Member

I did it originally for familiarity. Developers are trying to share the interface between the client and server. I'd rather use Feigns default parameters than promote that practice.

@adriancole @dsyer @mstine @mheath any opinions?

Activity

changed the title [-]Deprecate Spring MVC annotations on Feign[/-] [+]Deprecate Spring MVC annotations on Feign?[/+] on Nov 21, 2015
codefromthecrypt

codefromthecrypt commented on Nov 21, 2015

@codefromthecrypt
spencergibb

spencergibb commented on Nov 21, 2015

@spencergibb
MemberAuthor

@joshlong or @kbastani any thoughts?

kbastani

kbastani commented on Nov 21, 2015

@kbastani

I don't really know that this is a case of promoting bad practices or that it is really just a requirement of someone's bad architecture decisions (referring to #646). What we have here is a good pattern for implementation but a side-effect of bad practice leading to tight coupling. I say we keep the annotations and be opinionated in the documentation that it isn't advised to try and use a Feign client by extending a shared interface that the server-side controller has implemented. It is far better for service teams to publish versioned feign clients as artifacts so that client-side teams don't have to write them. The upside to this is that you can write integration tests against your versioned client-side contract and break the build, preventing breaking changes to consumers of the service. Alternatively, you can throw an exception in a controller that implements an interface with a feign client annotation.

joshlong

joshlong commented on Nov 21, 2015

@joshlong

Additionally the Feign support is still akin to a Spring Data repository like experience for building REST clients. It's powerful and convenient.

spencergibb

spencergibb commented on Nov 21, 2015

@spencergibb
MemberAuthor

I'm not saying drop feign support. Just support for using the mvc annotations. So instead of @RequestMapping you use the feign annotations like:

interface GitHub {
  @RequestLine("GET /repos/{owner}/{repo}/contributors")
  List<Contributor> contributors(@Param("owner") String owner, @Param("repo") String repo);
}

Maybe using springmvc annotations is opt-in, rather than the default?

kbastani

kbastani commented on Nov 21, 2015

@kbastani

Maybe use @ResponseMapping instead of @RequestMapping

joshlong

joshlong commented on Jan 2, 2016

@joshlong

looking forward, Spring 5's reactive story features - wherever possible (of course) - Spring MVC annotations like @RequestMapping. it'd make sense to keep this support and let those annotations become a lingua franca for all things declarative and HTTP in Spring-land. Plus, it's one less conceptual jump. I think people would sooner use the JAX-RS annotations than anything Feign-specific, and I can't see how that's any better an outcome (not to mention an easier cognitive leap)

cforce

cforce commented on Jan 31, 2016

@cforce

If the server side client artifact offers versioned Interface's (that shall be done always if not downwards compatible) with Mvc Anotations, why to forbid to make use of this contract on client side?
I think it's worth to point out in in which situations you could be drapped into the "yak hair-weave studio" and therfore it's no good practice. I can't see also why its better mixing JAX-RS anotations with Feign instead of MVC.

codefromthecrypt

codefromthecrypt commented on Feb 11, 2016

@codefromthecrypt

I think the general problem with trying to share HTTP server interfaces w/ clients fall in at least three categories:

  • state that's totally irrelevant on the client (such as a parameter for a pending http response), yet passed as args... and visa versa
  • needing to keep track of who you are (client or server) to understand method-scoped annotations. Ex. content-type, accept etc.
  • server interfaces tend to be fancier than clients, which leads to more deps and/or understanding required to understand what clients should not interpret.

All of the above doesn't mean it is a fools errand, some things can work well enough (provided one actually answers what enough is). Regardless, the result of attempts to share interfaces not designed to be shared usually means maintenance++ (and not very fun maintenance to review or maintain later)

PedroAlvarado

PedroAlvarado commented on Mar 1, 2016

@PedroAlvarado
Contributor

We find feign spring-mvc annotations support useful for end-to-end testing spring boot apps using @WebIntegrationTest. We simply generate an interface off the controller, make it a feign client, and start using it in our tests. We've used a similar approach with CXF's(JAX-RS) JAXRSClientFactory and its been working great as well.

I'd like to suggest we keep this support primarily to ease end-to-end testing above all other reasons.

gauravrmazra

gauravrmazra commented on May 10, 2016

@gauravrmazra

Hi,
This is related to OpenFeign/feign#391

On server side, we consumes and produces specific type of content
Now, I was using feign client. What It does in case of GET when there is no requestBody it neither adds Content-Type to headers nor it creates a blank RequestBody and adds mediaType by converting Content-Type header. Whereas if the request is POST/PUT it create empty RequestBody if it is passed null and also passed the mediaType.

Shouldn't it be added Content-Type to headers in any of the above case because server consumes only specific type of Content. If we don't send Content-Type then server throws Unsupported media type exception.

My application is Spring based.

16 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @joshlong@codefromthecrypt@dsyer@cforce@spencergibb

        Issue actions

          Don't use Spring MVC annotations on Feign by default · Issue #659 · spring-cloud/spring-cloud-netflix