spring-cloud / spring-cloud-openfeign

Support for using OpenFeign in Spring Cloud apps
Apache License 2.0
1.21k stars 785 forks source link

Unable to use request parameter with special characters #198

Closed mpapirkovskyy closed 5 years ago

mpapirkovskyy commented 5 years ago

After upgrading from 'spring-cloud-starter-openfeign' from 1.4.5.RELEASE to 2.1.2.RELEASE, following client stopped working:

@RequestMapping(method = RequestMethod.GET, value = "/groups/{groupId}/datasets/{datasetId}/refreshes", consumes = "application/json")
RefreshHistory getRefreshHistory(@PathVariable("groupId") String groupId, @PathVariable("datasetId") String datasetId,
    @RequestParam("$top") int top);

class RefreshHistory {

  @JsonProperty("value")
  private List<Refresh> refreshes;

  public static class Refresh {

    private String status;
    private Date startTime;
    private Date endTime;

  }

}

This tries to use real PowerBI api: https://docs.microsoft.com/en-us/rest/api/power-bi/datasets/getrefreshhistory

After some debugging it looks like root cause of issue is following change to Feign: https://github.com/OpenFeign/feign/commit/2d761cb6a92efc6b4a95e7092c945f47431b76cd

Which is not really problem for Feign itself, as you usually provide param name and variable for expansion separately:

GET /?$top={number}

But spring-cloud-openfeign seems to produce something like this:

GET /?$top={$top}

As result variable expansion is skipped and client sends request with unprocessed variable:

/groups/1/datasets/2/refreshes?$top=%7B$top%7D

while in previous version it produced this:

/groups/1/datasets/2/refreshes?%24top=1

Please, advice how I can work this around. May be I'm missing something.

ryanjbaxter commented 5 years ago

Since the breaking change occurred in OpenFeign you should report the bug there. I dont think there is much we can do to fix this in the spring-cloud-openfeign project. If they decide not to address it please comment back on this issue and we can see what we can do.

mpapirkovskyy commented 5 years ago

@ryanjbaxter I don't think theres issue with Feign itself, as you can provide RFC-compliant variable name there manually (see example above). Feign don't fully support RFC6570 yet, but even with full support, names like "$top" and similar are not valid variable names.

With SpringMvcContract I don't see the way to specify parameter and variable names separately. It seems to always reuse @RequestParam value as both parameter and variable name. This means that spring-cloud-openfeign effectively doesn't support parameter names which violate variable name restrictions of RFC6570.

mpapirkovskyy commented 5 years ago

Actually I've found workaround for this, leaving it here

@RequestMapping(method = RequestMethod.GET, value = "/groups/{groupId}/datasets/{datasetId}/refreshes?$top={top}", consumes = "application/json")
RefreshHistory getRefreshHistory(@PathVariable("groupId") String groupId, @PathVariable("datasetId") String datasetId,
  @PathVariable("top") int top);

This seems to work exactly like expected from pure Feign. But some special handling of complex parameter names in SpringMvcContract would be really useful