-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Don't use Spring MVC annotations on Feign by default #659
Comments
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.
|
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. |
Additionally the Feign support is still akin to a Spring Data repository like experience for building REST clients. It's powerful and convenient. |
I'm not saying drop feign support. Just support for using the mvc annotations. So instead of
Maybe using springmvc annotations is opt-in, rather than the default? |
Maybe use |
looking forward, Spring 5's reactive story features - wherever possible (of course) - Spring MVC annotations like |
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 the general problem with trying to share HTTP server interfaces w/ clients fall in at least three categories:
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) |
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. |
Hi, On server side, we consumes and produces specific type of content 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. |
@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? |
@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. |
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. |
I'd like to make the Spring MVC annotations opt-in in Edgware. |
Sgtm want to summarise rationale and/or link to issue that led to this?
|
Please don't deprecate, we sucessfully use this Api contract first provided |
@cforce I guess deprecation might be too strong a word. It would not be enabled by default, you would have to opt in. |
|
feign must use @RequestMapping ,why can't use @PostMapping ? |
@maliqiang, please don't comment on unrelated issues. As far as I know you can use post mapping |
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?
The text was updated successfully, but these errors were encountered: