Spring-cloud-netflix: Don't use Spring MVC annotations on Feign by default

[spencergibb]

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?

[codefromthecrypt]

yeah it is too tempting. Ex. in feign itself, there are a fraction of devs
tempted to share JAX-RS interfaces, and it works until it doesn't. When it
doesn't, it is a mutually awful experience because someone is "so..
close.." to having what they need work on both client and server side, but
making that happen begins a yak hair-weave studio.

iotw, yeah I'd recommend against feigning server interfaces as client ones.

[spencergibb]

@joshlong or @kbastani any thoughts?

[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]

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

[spencergibb]

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]

Maybe use @ResponseMapping instead of @RequestMapping

[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]

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]

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]

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]

Hi,
This is related to https://github.com/Netflix/feign/issues/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.

[dsyer]

@gauravrmazra is that relevant to the discussion here? If you are asking a new question or want to request a new feature, can you open a new issue?

[gauravrmazra]

@dsyer I was redirected to this issue when I originally asked my question in feign okhttp project. Any how this is not related to spring-cloud-netflix.
Sorry for any inconvenience. :(

[codefromthecrypt]

ps I referred to this issue, because it was an example for where using the same interface on the client and server became troublesome, in this case confusion on how to handle Content-Type (ex there's desire to send Content-Type when requesting content as opposed to Accept). If we didn't support using the same interface on the client and server side, folks wouldn't run into confusing topics like this.

[spencergibb]

I'd like to make the Spring MVC annotations opt-in in Edgware.

[codefromthecrypt]

Sgtm want to summarise rationale and/or link to issue that led to this?

[cforce]

Please don't deprecate, we sucessfully use this Api contract first provided
from the service app

[spencergibb]

@cforce I guess deprecation might be too strong a word. It would not be enabled by default, you would have to opt in.

[spencergibb]

On balance we believe that people will use MVC annotations even if they are not the default. So we can’t take them away, and therefore they might as well be the default.

[maliqiang]

feign must use @RequestMapping ,why can't use @PostMapping ?

[spencergibb]

@maliqiang, please don't comment on unrelated issues. As far as I know you can use post mapping