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

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

Closed
spencergibb opened this issue Nov 21, 2015 · 21 comments
Closed

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

spencergibb opened this issue Nov 21, 2015 · 21 comments

Comments

@spencergibb
Copy link
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?

@spencergibb spencergibb changed the title Deprecate Spring MVC annotations on Feign Deprecate Spring MVC annotations on Feign? Nov 21, 2015
@codefromthecrypt
Copy link

codefromthecrypt commented Nov 21, 2015 via email

@spencergibb
Copy link
Member Author

@joshlong or @kbastani any thoughts?

@kbastani
Copy link

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
Copy link

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

@spencergibb
Copy link
Member Author

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
Copy link

Maybe use @ResponseMapping instead of @RequestMapping

@joshlong
Copy link

joshlong commented Jan 2, 2016

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
Copy link

cforce commented Jan 31, 2016

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
Copy link

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
Copy link
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
Copy link

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.

@dsyer
Copy link
Contributor

dsyer commented May 10, 2016

@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
Copy link

@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
Copy link

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
Copy link
Member Author

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

@spencergibb spencergibb changed the title Deprecate Spring MVC annotations on Feign? Deprecate Spring MVC annotations on Feign Oct 5, 2016
@codefromthecrypt
Copy link

codefromthecrypt commented Oct 6, 2016 via email

@cforce
Copy link

cforce commented Oct 6, 2016

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

@spencergibb spencergibb changed the title Deprecate Spring MVC annotations on Feign Don't use Spring MVC annotations on Feign by default Oct 6, 2016
@spencergibb
Copy link
Member Author

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

@spencergibb
Copy link
Member Author

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
Copy link

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

@spencergibb
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants