Set request parameter templates with [] inside do not pass Expressions matching check

[OlgaMaciaszek]

When adding a Set query param template using the feign.RequestTemplate#query(java.lang.String, java.lang.Iterable<java.lang.String>) method, we pass a query with a name containing [] and a corresponding template value, for example name = "ids[]" and then a List containing {ids[]} value. This causes an issue as a template containing [] does not pass the entry -> entry.getKey().matcher(expression).matches() filter in feign.template.Expressions#create method against the SimpleExpression pattern, created as follows: Pattern.compile("(\\w[-\\w.]*[ ]*)(:(.+))?"), SimpleExpression.class;

As an http call to [requestUrl]?ids[]=1,2,3 would work, it should probably work via Feign as well.

This issue causes the following issue in Spring Cloud OpenFeign: spring-cloud/spring-cloud-openfeign#143 that has been reported to us by the users.

[kdavisk6]

@OlgaMaciaszek

Thanks for bringing this to our attention. Looking over RFC 6570 again, it looks like brackets [ ] are not allowed within a variable name unless they are pct-encoded. Regardless, the regular expression we use to identify a template expression is not compliant. We should update it to accordingly.

This is also similar to #922 and #924, where certain Contract implementations are generating template expressions that are not compliant. My first thoughts were to pct-encode them to make them compliant, but I need more time to vet that out. I'm open to any other suggestions you may have.

[OlgaMaciaszek]

@kdavisk6 Thanks for the quick reply. I understand the principle to comply with the RFC. However, the issue is that our users might currently be consuming APIs containing endpoints where the set parameters should be passed and query params such as ("ids[]") are expected. As we are providing tools for the clients and the issue is on the server side, it's difficult to request our users to fix it according to the RFC, as it might well not be their app that needs the changes. Wdyt?
Do you mean to pct-encode it and pass forward to the server? I think, this would also cause API compatibility issues, but maybe it's worth further investigating.

If there's anything we could do or change in our Contract implementation in order to make fixing it easier, please let me know.

[kdavisk6]

@OlgaMaciaszek

I completely agree that we should do what we can to support our users and I don't want to force these concerns into other areas that are not directly in Feign's space.

With regards to pct-encoding, id[] and id%5B%5D should result in the same result on the server side, but I have also been around long enough what should work may not work. My concern is keeping the parsing of template expressions consistent between those provided directly in the uri and those that are being generated by Contracts.

For this particular issue, I think we may be able to solve it by allowing users to specify which Collection format they want on the parameter level. Right now, it can be set at the Request Line level. This way, users can control each parameters expansion. For this particular issue, we have CollectionFormat.CSV, which looks like this when expanded:

@RequestLine("/path", collectionFormat=CollectionFormat.CSV)
public Response request(@Param("collection") Set<String> collection);

Result:

https://request/path?collection=1,2,3,4

An updated version could look like this:

/* feign core */
@RequestLine("/path")
public Response request(@Param("collection", collectionFormat=CollectionFormat.SET) Set<String> collection);

/* jaxrs / Spring with Collection autodetection in the Contract */
@Path("/path")
public Response request(@QueryParam Set<String> collection);

Result:

https://request/path?collection[]=1,2,3,4

Would something like this work? Assuming we can update RequestTemplate#query to take a Collection Format value.

[OlgaMaciaszek]

@kdavisk6 yes, sounds good to me. If that would produce the following result https://request/path?collection[]=1,2,3,4, it should work for our users.

[kdavisk6]

@OlgaMaciaszek

Would it be ok if the brackets were pct-encoded as %5B %5D respectively? If I do that, then I can stay RFC compliant.

[OlgaMaciaszek]

@kdavisk6 Yes, that'll probably be the best solution.

[kdavisk6]

@OlgaMaciaszek

#939 is ready for your review. Let me know if this meets your needs.

[OlgaMaciaszek]

Thanks a lot, @kdavisk6 Sorry for not getting back to you earlier - I was traveling; will take a look at it soon.

[OlgaMaciaszek]

Hello @kdavisk6 have verified that. Looks good. Thanks :). When do you think a version containing this change could be released?

[kdavisk6]

@OlgaMaciaszek

I'll check with the others, but I think we can release a patched 10.2.1 sometime in the next week.

[robmoore-i]

Hello,

I have the feign client endpoint declaration:

@RequestLine(value = "GET /api/v1/customers?filter[profile-id]={profileId}")
String fetchCustomerProfile(@Param("profileId") String profileId);

I have a test which fails with the below message (I have paraphrased):

Expected: a call to /api/v1/customers?filter[profile-id]=01S00312
Got:          a call to /api/v1/customers?filter%5Bprofile-id%5D=01S00312

What's the least I can do to make my test pass, while still using feign?

Many thanks,

Rob