@PathVariable is not getting interpolated if the variable is null

[jaisonpjohn]

@RequestMapping(value = "/items/{id}", method = RequestMethod.PUT)
  ItemResponse updateItem(@PathVariable("id") final Integer id, ItemRequest request);

FeignClient is not throwing any Null pointer exception or Errors when id is null while invoking the above feign client method; it simply send a string {id} to the service, instead of trying to interpolate and fail. This is very misleading IMHO.

My server side was a spring boot app and it had the similar configuration for the PUT endpoint and it went to a http500 with the following exception. java.lang.NumberFormatException: For input string: "{id}"

[ryanjbaxter]

I am not sure it is valid to have an "optional" @PathVariable. I believe the correct thing to do is to have to @RequestMappings.

@RequestMapping(value = "/items", method = RequestMethod.PUT)
 ItemResponse updateItem(ItemRequest request);

@RequestMapping(value = "/items/{id}", method = RequestMethod.PUT)
 ItemResponse updateItem(@PathVariable("id") final Integer id, ItemRequest request);

[jaisonpjohn]

Exactly! it is not optional, it is mandatory. So, in my opinion, what Feignclient should have done differently is , instead of concealing the fact that id came as null, it could just throw say a null pointer exception. If feignclient just keep sending non-interpolated string "{id}", I will not be able to understand where is the problem. I faced this issue, it drove me crazy for 1-2 hour. The issue is, when you see a non-interpolated string literal "{id}", we might (at least I did) naturally think that "there is some issue in my feignclient configuration, am I using the feignclient right way"

[ryanjbaxter]

I suppose that is something we could do.

[jaisonpjohn]

@ryanjbaxter I want to help. But, I am new to spring cloud codebase. Any pointers to where to start will be really helpful?

[ryanjbaxter]

I just tried to reproduce this and in my test it never made it to the server. The client through a URISyntaxException because of the {. You are not seeing that exception being thrown?

[jaisonpjohn]

@ryanjbaxter Thanks for looking into this. Here is a repo where issue is reproducible. This is pretty much vanilla implementation. https://github.com/jaisonpjohn/feignclient-issue

Apologies for not submitting a repo before.

[ryanjbaxter]

Could you provide the "ItemService" as well to make the sample work?

[jaisonpjohn]

@ryanjbaxter Sorry for the late reply. https://github.com/jaisonpjohn/feignclient-issue-api This is the API which my feign-client was trying to hit.

[ryanjbaxter]

I am still not seeing it actually make it to the service. Feign is throwing the following exception in my environment (using your samples)

Caused by: feign.FeignException: status 400 reading ItemRestClient#updateItem(Object,String); content:
{"timestamp":1507150701220,"status":400,"error":"Bad Request","exception":"org.springframework.web.method.annotation.MethodArgumentTypeMismatchException","message":"Failed to convert value of type 'java.lang.String' to required type 'java.lang.Integer'; nested exception is java.lang.NumberFormatException: For input string: \"{id}\"","path":"/items/{id}"}
    at feign.FeignException.errorStatus(FeignException.java:62) ~[feign-core-9.5.0.jar:na]
    at feign.codec.ErrorDecoder$Default.decode(ErrorDecoder.java:91) ~[feign-core-9.5.0.jar:na]
    at feign.SynchronousMethodHandler.executeAndDecode(SynchronousMethodHandler.java:138) ~[feign-core-9.5.0.jar:na]
    at feign.SynchronousMethodHandler.invoke(SynchronousMethodHandler.java:76) ~[feign-core-9.5.0.jar:na]
    at feign.ReflectiveFeign$FeignInvocationHandler.invoke(ReflectiveFeign.java:103) ~[feign-core-9.5.0.jar:na]
    at com.sun.proxy.$Proxy58.updateItem(Unknown Source) ~[na:na]
    at com.jaison.feignclientdemo.Runner.run(Runner.java:15) ~[main/:na]
    at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:732) [spring-boot-1.5.7.RELEASE.jar:1.5.7.RELEASE]
    ... 6 common frames omitted

[jaisonpjohn]

@ryanjbaxter I believe the error message {"timestamp":1507150701220,"status":400,.... is coming from the service. Otherwise why would it end up in this error java.lang.NumberFormatException: For input string: \{id}\"","path":"/items/{id}" (if you scroll all the way to right in the stacktrace you can see this)

[jaisonpjohn]

@ryanjbaxter even if it is not hitting the service (I still don't think that's the case) error For input string: \{id}\"","path":"/items/{id}" appears cryptic to me. I think, if we gave back a nullpointer exception saying path variable came as null, that would be more straight forward.

[ryanjbaxter]

@jaisonpjohn the reason I don't think it is hitting the service is because I don't see the log message in your sample app being printed when I execute the request.

[jaisonpjohn]

@ryanjbaxter That is because spring MVC couldn't map the request to that method (it failed during mapping) I think that log statement is a bit misleading in that respect, sorry about that. Updated the api project, added a webfilter which will log the request URL much before spring-mvc filter chain kicks in. That webfilter is printing Request Received. URL is: /items/{id}

[M4tiz]

This issue is fixed with https://github.com/spring-cloud/spring-cloud-openfeign/commit/15be7111ec545ea5588dea9b82e3ebc423f57231 (2.1.2.RELEASE) with feign upgrade (>=10.2.0)

Now null path variable parameters are replaced with empty string as defined with RFC 6570 More info: https://github.com/OpenFeign/feign/blob/master/README.md#undefined-vs-empty-values

The API in the example will return 400 or 404, which is correct behavior IMO.

[OlgaMaciaszek]

Closing as issue has been fixed in upstream and the dependency has been upgraded here.