feign icon indicating copy to clipboard operation
feign copied to clipboard

Missing = character for query parameters when value is empty

Open aber4mx opened this issue 2 years ago • 7 comments

Hello, I have been upgrading feign from 10.1.0 to 11.8 and I noticed a change of behavior when empty parameters are passed. Here is an example with a dummy interface :

public interface FeignClient {
    @RequestLine("GET /path?param={param}")
    void call(@Param("param") String value);
}

On the previous version, when calling the client with an empty string as the parameter value, feign would perform a request on : /path With feign 11.10, the actual request is now performed on : /path?param

I understand that it is an expected behavior that empty strings are not ignored anymore. However I'm surprised that the '=' symbol is removed from the query. As I checked the RFC 6570 standard, I understood that the ideal target would be to write the request line as @RequestLine("GET /path{?param}") when this is supported. In the standard the '=' signs would be present for empty values.

I am curious to get your opinion on this. Do you consider the current behavior as a defect or an expected behavior ?

To give more context I detected this because of unit tests that use PACT to align on a REST contract. PACT's mock server fails to parse the query as it looks explicitly for key/value pairs separated with '&' and '='

I attach a zip with a basic maven project to reproduce the case : feign-query-case.zip

aber4mx avatar Oct 25 '22 22:10 aber4mx

I've duplicated this issue and written 2 unit tests that are failing on master in my branch here: https://github.com/JKomoroski/feign/tree/issue/1807/handle_empty_parameters

I will see if I can find what is going wrong.

JKomoroski avatar Oct 28 '22 21:10 JKomoroski

There is a unit test checking that this behavior is expected.

It appears to have been changed in commit 2d761cb6a92efc6b4a95e7092c945f47431b76cd.

  public void queryMapIterableValuesExpanded() throws Exception {
    server.enqueue(new MockResponse());

    TestInterface api = new TestInterfaceBuilder().target("http://localhost:" + server.getPort());

    Map<String, Object> queryMap = new LinkedHashMap<>();
    queryMap.put("name", Arrays.asList("Alice", "Bob"));
    queryMap.put("fooKey", "fooValue");
    queryMap.put("emptyListKey", new ArrayList<String>());
    queryMap.put("emptyStringKey", ""); // empty values are ignored.
    api.queryMap(queryMap);

    assertThat(server.takeRequest())
        .hasPath("/?name=Alice&name=Bob&fooKey=fooValue&emptyStringKey");
  }

I don't think this behavior is what the RFC specifies for this case.. Section 3.2.2 seems to indicate that for simple text expansion, an empty string value should not have this effect.

I believe this unit test was incorrectly updated to match the behavior of the code after the implementation change. The code appears to behave this way to account for a relative uri with a query string:

  public void resolveTemplateWithRelativeUriWithQuery() {
    RequestTemplate template = new RequestTemplate()
        .method(HttpMethod.GET)
        .uri("/wsdl/testcase?wsdl")
        .target("https://api.example.com");

    assertThat(template).hasUrl("https://api.example.com/wsdl/testcase?wsdl");
  }

I'll see if I can put together a fix.

JKomoroski avatar Oct 28 '22 22:10 JKomoroski

This is correct. Check the Readme

~~https://github.com/OpenFeign/feign#undefined-vs-empty-values~~


Adding more context. This is expected behavior and not a bug. When we have a query string that has no value, it's treated as a pure parameter and thus, no = is present. It looks like the documentation is out of date.

kdavisk6 avatar Oct 28 '22 22:10 kdavisk6

Yeah. I see that flag in the Query template. It's very odd the passing a null results in no query*, but ""results in a pure param. It's a bit of a "magic number"

It seems to me that we could probably handle these better if we made clear distinctions in the code for: Query Literals Query Expressions Pure Query Literals Pure Query Expressions

I'd need to think about it and play with implementations to avoid breaking changes.

JKomoroski avatar Oct 28 '22 23:10 JKomoroski

Many thanks for your analysis and your feedbacks !

I have also a slightly different case but on this topic : it is when the parameter is a list :

    @RequestLine("GET /pathForList?params={params}")
    void callWithList(@Param("params") List<String> values);

When the client is called with an empty list, the resulting request line is : GET /path/pathForList?params In the section 2.3 of the RFC, I see that empty lists should be ignored.

As I try to play with it I now see a behavior that looks weird :

  • If the list is filled with a single empty string element, the result is : GET /path/pathForList
  • If the list is filled with a null element, the result is : GET /path/pathForList?params

It seems to me we should rather have :

  • For empty lists, or lists filled with null elements : GET /path/pathForList
  • Empty string items in lists not ignored, and appended as GET /path/pathForList?params=

What do you think ?

aber4mx avatar Oct 29 '22 15:10 aber4mx

I have the same problem, any custom solution?

vinimdev avatar Mar 10 '23 03:03 vinimdev

I face same issue. For my rest-facade base, I cannot pass an empty string as query parameter from the QueryMap like below:

@GetMapping("locations/countries/") String fetchCountries( @SpringQueryMap queryParamList) { ... }

Then the intercepted URI is: {restFacade.baseurl}/locations/countries?siteId=US&query&languageId=EN

Is there any work-around for this please? Basic tests with setting the key as <query=> does not work as "=" or " " gets encoded to %3D or %20 .

ashwani-bhadani avatar Jan 18 '24 06:01 ashwani-bhadani