OkHttp and Feign produce "method GET must not have a request body."

[scrat98]

Imagine that we have searchable api which accepts "request" and "pageable" like
/api/1.0/search?sort=id,desc&size=20000&name=feign&version=10.3.0
In that case I can't use "@QueryMap", because it should present only on one parameter.

This is our REST API interface:

@Headers(value = ["Accept: application/json", "Content-Type: application/json"])
interface ControllerApi {

  @RequestLine("GET /api/1.0/search?{request}&{pageable}")
  fun someEndpoint(
      @Param(
          value = "request",
          expander = GenericDataClassExpander::class
      ) request: SearchRequestDto,
      @Param(
          value = "pageable",
          expander = PageableExpander::class) pageable: Pageable
  )

This is our custom DTO definition:

data class SearchRequestDto(
    @field:ApiModelProperty("search by name contains")
    @field:JsonProperty("name")
    var name: String? = null,

    @field:ApiModelProperty("search by version contains")
    @field:JsonProperty("version")
    var version: String? = null
)

Pageable is Spring commonly used interface.

These are the expanders that we are using to generate the final request:

class PageableExpander : Param.Expander {
  override fun expand(value: Any): String {
    val pageable = value as? Pageable
        ?: throw IllegalStateException("Error while expanding Pageable")
    val page = pageable.pageNumber
    val size = pageable.pageSize
    val sort = pageable.sort
    var res = ""
    res += "page=$page&"
    res += "size=$size&"
    if (sort != null) {
      res += "sort=$sort"
    }

    return res
  }
}

class GenericDataClassExpander : Param.Expander {
  override fun expand(value: Any): String {
    val res = value::class.declaredMemberProperties.fold("") { acc, field ->
      field as KProperty1<Any, *>
      val fieldValue = field.get(value)
      if (fieldValue != null) {
        acc + "${field.name}=$fieldValue" + "&"
      } else {
        acc
      }
    }
    return res.removeSuffix("&")
  }
}

The problem that we faced is that while using this API, Feign is unable to construct a proper QueryTemplate, since the CollectionFormat is supposed to be something like ?[key]=[value], e.g. key1={value1}&key2={value2}. However, we would like to omit having keys and just include already expanded values like ?{expanded-value}.

Therefore, if we do not follow the standart CollectionFormat ?[key]=[value], the QueryTemplate gets constructed with empty templateChunks, so the getVariables method returns empty array.
Then, in feign.Contract we could see this:

if (!data.template().hasRequestVariable(name)) {
     data.formParams().add(name);
}

In our case when getVariables is empty, it will add our query parameters to form parameters and therefore to the body of RequestTemplate, which is not what we want.
When trying to run this request via the OkHttpClient, we will receive an exception that "method GET must not have a request body."

This behavior seems strange to me, since it basically violates the standards. And this is obviously not the behavior what I want when I generate the custom request line myself with expanders.

Maybe you could consider adding a simple condition to prevent adding any form params for the GET request like this?

HttpMethod method = HttpMethod.valueOf(data.template().getMethod());
if (method != HttpMethod.GET || !data.template().hasRequestVariable(name)) {
     data.formParams().add(name);
}

I would love to hear any response of this, but please be aware that I cannot change the API that I am using and I have to stick with it.

[kdavisk6]

@scrat98

First, this can be achieved using a @QueryMap annotation, in fact, this is primary use case for why it was created. I'm confused as to why @QueryMap did not work and you resorted to this approach. Have you checked our documentation on Dynamic Query Parameters?

Using your example: /api/1.0/search?sort=id,desc&size=20000&name=feign&version=10.3.0, you can construct a Map and use QueryMap as follows:

Map<String, String> parameters = new LinkedHashMap<>();
parameters.add("sort", "id,desc");
parameters.add("size", "200000");
parameters.add("name", "feign");
parameters.add("version", "10.3.0");

@RequestLine("GET /api/1.0/search?{parameters}")
Results search(@QueryMap parameters);

Before we look into any other areas you may be having trouble with, let's start there. Have you tried this? If so, can you please explain what behavior you saw?

[scrat98]

@kdavisk6

Hi Kevin,

Yes, I checked the @QueryMap as I wrote in the begging. And I also wrote, "that I cannot change the API" because "feign api" it's an interface first of all and I would like to use it for Inheritance and for consistency between web-controller and api to this controller.

So basically I have

class Controller : ConrollerApi {
  override fun someEndpoint(request: SearchRequestDto, pageable: Pageable) {
  }
}

And I would not prefer either use "inconsistency way" where is my controller and api to that controller not inherited like this

class Controller {
  override fun someEndpoint(request: SearchRequestDto, pageable: Pageable) {
  }
}

@Headers(value = ["Accept: application/json", "Content-Type: application/json"])
interface ControllerApi {
  @RequestLine("GET /api/1.0/search?{parameters}")
  fun someEndpoint(@QueryMap parameters: Map<String, String>)
}

or "bad arguments for controller way" like this:

class Controller : ConrollerApi {
  override fun someEndpoint(parameters: Map<String, String>) {
  }
}

But I still can't understand why request parameters with expanders should be in forms parameters(in body of request when it's request parameters)

[kdavisk6]

@scrat98

That variable is named poorly. It deals with tracking request parameters and request body values and attempts to ensure that we don't create invalid requests. In your situation, this is what we call a red herring.

Thank you for providing that additional context. I was getting tripped up in the code example. I'm not familiar with Kotlin.

Let me see if I can recap this discussion and your issue. You have an Interface that you want to share between a Controller and the Feign Client. This interface has a method that takes two parameters, request and pageable. On the controller side, you are relying on Spring to read, parse, and validate these parameters. On the Feign side, you are using custom parameter Expanders to convert the SearchRequestDTO and Pageable into query string parameters. However, the result from Feign is not what you expect.

I suspect that you are seeing something like this:

/api/1.0/search?request=....&pageable=....

and you want something like this:

/api/1.0/search?name=...&version=...&page=...&size=...

If so, then I think I understand what you need. Let me look it over some more.

[scrat98]

@kdavisk6

You are exactly right about the issue.

Let me see if I can recap this discussion and your issue. You have an Interface that you want to share between a Controller and the Feign Client. This interface has a method that takes two parameters, request and pageable. On the controller side, you are relying on Spring to read, parse, and validate these parameters. On the Feign side, you are using custom parameter Expanders to convert the SearchRequestDTO and Pageable into query string parameters. However, the result from Feign is not what you expect.

But there is one important thing about this issue. Yes, I expected something like this and get the same from RequsetTemplate
/api/1.0/search?name=...&version=...&page=...&size=...(I don't seeing /api/1.0/search?request=....&pageable=...., i.e. request line is absolutely correct), but besides the correct request line, it's also important to have correct body parameters. But when I use expanders in my case all parameters put in the body of the request, but in my example, I use GET method and this is unacceptable to have body in GET method and that is why when I use OkHttp client I got the error "method GET must not have a request body.". To summarize - when you are using expanders all parameters put in the body of the request despite the request method. See the screenshot for full understanding.

So, I think now you caught the point and have a complete understanding of the problem.

And also sorry for Kotlin:) If you wish I could make a demo project with this problem on Java+Spring+Feign if you still have questions.

Very thanks!

[scrat98]

@kdavisk6

Please feel free to reference on demo project of this issue. Just run FeignApiTest

https://github.com/scrat98/feign-issue-1038

[scrat98]

@kdavisk6 any updates on this bug?

[cezar-tech]

@scrat98 I've been reading your problem man, and even though I'm no expert on the field I have tried to carefully debug the Test class you've provided in the comment above.

Long story short, looks like Feign uses this class in the middle of its processing, part of this method calls an encoder:

From ReflectiveFeign.java

In the end this encoder turns out to be the encoder you have used to construct your WebControllerApi in your test:

However this encoder writes stuff on the body of the request, which is what you do not want:

So I have passed a different encoder than yours, basically the same except it doesn't write stuff on the body:

Then your test passes.

How I have discovered this:

1. The test you provided helped a lot, I have set a debug breakpoint on each one of the stacktraces method calls from the error in the test.
2. Then at each breakpoint I have looked for something odd and I saw this:
   Content-Length header was suddenly changing from 0 to 100 somehow. Then I've fine tuned my debug to get to this relevant method and class. According to this stack overflow, content length is related to body content: https://stackoverflow.com/questions/2773396/whats-the-content-length-field-in-http-header, so that's what I thought that could be wrong.

If it doesn't help with your problem I think it can at least help people for further debugging,

[scrat98]

@cezar-tech

Please, look more carefully. I wrote about workaround and you did the same.

https://github.com/scrat98/feign-issue-1038/blob/0e132bdbec54ad9deac662bb927a3913a9e3feb4/src/test/java/github/scrat98/FeignApiTest.java#L50

The problem is not in encoder, the problem is while constructing RequestTemplate - it shouldn't have body param at all. See my explanation above

[cezar-tech]

@scrat98
Totally my bad.

As I see here all RequestTemplate.Factory#resolve implementations set a request body regardless of the method indeed as you said.
And so does RequestTemplate's resolve method.

I have pointed your project to a local feign-core with this modification:

And it worked too. Maybe this is the solution you need ?
Could anyone validate this ?

Btw:

• Changing RequestTemplate's resolve method alone didn't help;
• Changing BuildEncodedTemplateFromArgs (just below the EncodedForms class)'s resolve method also didn't help.

[scrat98]

@cezar-tech

Of course, there are a lot of places where you could "fix" this. For example, I suggest
feign.Contract modification above.

But the actual right solution will be extend CollectionFormat to support those types of formats.

And also this issue has another problem. I actually don't know which "http standards" feign supports, but imagine that someone uses feign with his own http protocol where GET method could accept body. Should feign handle this usecases? I don't know. That this why I can't make a pull request and need support from @kdavisk6 for example.

[kdavisk6]

Reviewing this thread, it appears that during contract parsing, the variables present are being incorrectly flagged as formParams. This will cause them to be encoded in the request body, resulting in the situation you have identified. This behavior appears to be a hold over from when we had support for application/x-www-form-urlencoded built in, which has since been moved out to feign-form.

I am not against the proposed changes to Contract as an intermediate fix. We really should refactor that formParams out completely. I'm open to either approach.

[kdavisk6]

#1144 should correct the original issue of Feign incorrectly identifying certain parameters are form parameters.

However, the second issue, where query parameters are always named, remains. I'm not quite sure how to proceed there. I'm open to suggestions.

[kdavisk6]

I've considered a few other options but I think the best answer for this issue has been merged and released in #1144. With regards to the naming issue, I think that #1019 will provide the right balance of support and flexibility. I'm going to close this issue and ask that any additional comments be placed in #1019