Empty query params shouldn't be filtered out in RequestTemplate

[budaimartin]

Background

In https://github.com/OpenFeign/feign/pull/778, there were some modifications in RequestTemplate, affecting query parameter resolution. As a result, empty query parameters are being filtered out. On the other hand, according to RFC 6570, empty query parameters are allowed, but with this change, Feign cannot handle this anymore.

Example

Let's suppose we have this client:

public interface DemoClient {
    @RequestMapping(path = "/test", method = RequestMethod.GET)
    String test(@RequestParam("param") String param);
}

Then the following assertion (using WireMock) is true:

@Test
public void testNonEmptyParam() {
    demoClient.test("asd");
    wireMockServer.verify(getRequestedFor(urlPathEqualTo("/test")).withQueryParam("param", equalTo("asd")));
}

While this is not:

@Test
public void testEmptyParam() {
    demoClient.test("");
    wireMockServer.verify(getRequestedFor(urlPathEqualTo("/test")).withQueryParam("param", equalTo("")));
}

See my respository for full project.

Expected behavior

Empty strings are sent as empty query parameters, such as:

http://localhost:8080/test?param=

[rage-shadowman]

I'm pretty sure feign has been removing empty query params for a long time. That commit just cleaned the code up and maybe added that same logic to other areas (like header params which would previously just leave the placeholder -- "HeaderName: {headerValue}" -- in as well when the value wasn't supplied).

[budaimartin]

This bug was found during upgrading OpenFeign from 9.5.1 to 10.1.0 on our project, so that version let empty query parameters through for sure. I checked the commits affecting RequestTemplate and I found such change related to the PR I linked above; sorry if I'm wrong and this was caused somewhere else. Anyway, I still think that the expected behavior should be that empty query parameters are passed over.

[kdavisk6]

@bmartin0121

What you are experiencing is the exact reason why we changed RequestTemplate in #778. As @rage-shadowman is alluding to, handling of unresolved template expressions, particularly when dealing with expressions as part of query strings, was inconsistent. Before 10.x, query parameters defined with the @Param annotation, or in your case using the Spring Cloud contract, @RequestParam, that were unresolved were included in the query string as you've noted:

http://localhost:8080/test?param=

However, when expanding templates defined using the @QueryMap annotation, for both Map and Bean specifications, unresolved templates were explicitly removed:

public interface DemoClient {
    @RequestLine("GET /test")
    String test(@QueryMap Map<String, Object> params);
}

public void test() {
   Map<String, Object> parameters = new LinkedHashMap<>();
   parameters.put("param", "");
   this.demoClient.test(parameters);
}

Result:

http://localhost:8080/test

This inconsistency bled into @Header and @Body templates as well. After reviewing all of the issues, the consensus was that unresolved parameters should be removed and the changes in #778 were merged.

With regards to RFC 6750, it makes no mention of query parameters or anything itself on the URI, simply that unresolved expressions should result in an empty string.

https://tools.ietf.org/html/rfc6570#section-2.3

An expression MAY reference variables that are unknown to the template processor or whose value is set to a special "undefined" value, such as undef or null. Such undefined variables are given special treatment by the expansion process (Section 3.2.1).

A variable value that is a string of length zero is not considered undefined; it has the defined value of an empty string.

A variable defined as a list value is considered undefined if the list contains zero members. A variable defined as an associative array of (name, value) pairs is considered undefined if the array contains zero members or if all member names in the array are associated with undefined values.

By this definition, our decision to treat empty strings as undefined appears to be in violation of the specification, but I think we need to very specific about this case, to prevent any additional confusion.

My suggestion is that we update the QueryTemplate to consider parameters defined but have an empty string value as empty and include them in the expansion and parameters whose value is explicitly null or missing entirely, to be considered undefined and removed. This should result in the following:

Empty String

public void test() {
   Map<String, Object> parameters = new LinkedHashMap<>();
   parameters.put("param", "");
   this.demoClient.test(parameters);
}

Result

http://localhost:8080/test?param=

Missing

public void test() {
   Map<String, Object> parameters = new LinkedHashMap<>();
   this.demoClient.test(parameters);
}

Result

http://localhost:8080/test

Undefined

public void test() {
   Map<String, Object> parameters = new LinkedHashMap<>();
   parameters.put("param", null);
   this.demoClient.test(parameters);
}

Result

http://localhost:8080/test

Does this approach meet your needs?

[budaimartin]

@kdavisk6 Thank you for your fast and detailed response. Yes, I think this approach would be the best.