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

Can't send Path Params and Body at same request via PUT or POST #399

Closed
jyoshiriro opened this issue Jun 2, 2016 · 9 comments
Closed

Can't send Path Params and Body at same request via PUT or POST #399

jyoshiriro opened this issue Jun 2, 2016 · 9 comments

Comments

@jyoshiriro
Copy link

jyoshiriro commented Jun 2, 2016

I've seem something at #310, but still occurs.
Feign version: 8.16.2

Interface:

public interface MyRequest {
   @RequestLine("PUT /offers/{id}")  // I've also tried "POST", but same problem
   void update(@Param("id") Integer id, OfferRequest offerRequest);
}

Call:

MyRequest request = Feign.builder().encoder(new GsonEncoder()).decoder(new GsonDecoder()).target(MyRequest.class, "http://xxxxx");

OfferRequest offerRequest = new OfferRequest(x, y, z);
request.update(1, offerRequest);

StackTrace:

Exception in thread "main" java.lang.IllegalStateException: Body parameters cannot be used with form parameters.
    at feign.Util.checkState(Util.java:128)
    at feign.Contract$BaseContract.parseAndValidateMetadata(Contract.java:112)
    at feign.Contract$BaseContract.parseAndValidatateMetadata(Contract.java:64)
    at feign.ReflectiveFeign$ParseHandlersByName.apply(ReflectiveFeign.java:146)
    at feign.ReflectiveFeign.newInstance(ReflectiveFeign.java:53)
    at feign.Feign$Builder.target(Feign.java:198)
    at feign.Feign$Builder.target(Feign.java:194)

I know using path param and request body at same request is not a good approach, but the REST API is not our.

@lolodesbois
Copy link

I'm having the same issue using Feign 8.16.2.

Is there any reasons to this hard coded limitation ?

@jyoshiriro
Copy link
Author

My [not elegant, I know] solution was copy the feign.Contract Interface to my source and comment out lines 104 and 105.

@codefromthecrypt
Copy link
Contributor

I don't think this limitation is intentional. It seems like a bug

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Jul 21, 2016 via email

@csciuto
Copy link

csciuto commented Nov 15, 2016

I just ran into something similar and I wonder if it might be related.

In my case, the Feign annotation Param was confused with the Spring Data annotation with the same name. So, since I inadvertently used the Spring version, I hadn't actually specified a Feign path parameter as Param, so it assumed I wanted it to be a body parameter. Then, the actual unannotated body parameter caused a validation failure.

The error messages Feign generates say the parameters conflict somehow, but doesn't specify details beyond the message signature. Wonder if there would be value in beefing up the error messages to give the state of the validator? If I knew which ones it thought were body parameters and which it thought were form parameters and path parameters, I might've figured it out without debug stepping. Perhaps just return a toString for the MethodMetadata object on validation failures?

@fd8s0
Copy link

fd8s0 commented Mar 29, 2017

It's definitely broken if you use javax.ws.rs.PathParam, but if you switch to feign.Param it works.

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Mar 30, 2017 via email

@fd8s0
Copy link

fd8s0 commented Mar 30, 2017

Sorry it wasn't my code, I just told them to change to the feign annotation and that fixed it, what I can say is they were using jaxrs annotations in other cases and it was working as expected, it was only in this case (mixing requestline put, with a body and jaxrs pathparam) that it wasn't. It seems like a different problem, however it'd be good to recognise if you are seeing cross streams and either support it or not support it but do so explicitly. The error sounded similar but upon reading again it seems like a different thing.
I just thought, because this issue is still open, if everyone was unable to reproduce I'd expect it to be closed.

@codefromthecrypt
Copy link
Contributor

thx I'll close this for now.

Validating jax-rs annotations aren't used from feign-core would be a chicken/egg.

From jax-rs, we could consider checking if any feign-core annotations are in use (as that would be a bug). I'll leave that as an exercise for a contributor with enough interest to do that.

Another way is to use something like error-prone which would make a compile check out of issues like this. It is another form of checking a contract, but it could depend on multiple "contracts" or otherwise be taught what bad is...

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

5 participants