java: Content-Type: application/json on GET requests

lorenzleutgeb commented 6 years ago

Description

GET requests usually have no body, and therefore most of the time no Content-Type header is transferred. The OpenAPI makes it especially hard (probably impossible) to define one, as I have noted in OAI/OpenAPI-Specification#1628

I only ran into this issue because I observed that the Java client generated by this generator used Content-Type: application/json and I could not explain why. After some searching I think I found the cause for this rather strange behaviour.

In api.mustache (line highlighted) you call ApiClient#selectHeaderContentType (line highlighted) for all types of requests. Since (by validity of the input spec) there are no content types for GET requests, you will always default to application/json. That's bad, and for example makes it impossible to describe JSON API and get a Java client in a straightforward way.

The workaround that I used is an interceptor like this:

if ("GET".equals(request.method()) && request.header("Content-Type") != null) {
  System.err.println("We attempt to send a content type on a GET request! Removing it forcibly!");

  request = request.newBuilder()
    .removeHeader("Content-Type")
    .build();
}

openapi-generator version

020883f (master from 2018-07-04)

OpenAPI declaration file content or url

Use ping.yaml.

Command line used for generation

java -jar openapi-generator-cli.jar generate -g java -i api.yml -o jclient

Steps to reproduce

Generate client.

Related issues/PRs

See Description.

Suggest a fix/enhancement

Do not attempt to even infer a content type for a GET request.

jmini commented 6 years ago

Thank you for this issue.

I am not that familiar with HTTP Headers, but if I understand you correctly when in a OpenAPI Spec with an operation that do not have a produces (it does not send any payload - like the pingGet operation in the ping.yaml spec), then it should not send, this header:

Content-Type: application/json

I have compared the header sent between the java clients for the pingGet operation:

okhttp-gson library (default):

User-Agent: OpenAPI-Generator/1.0.0/java
Connection: keep-alive
Host: localhost:8090
Accept-Encoding: gzip
Content-Type: application/json

rest-assured library:

Connection: keep-alive
User-Agent: Apache-HttpClient/4.5.3 (Java/1.8.0_161)
Host: localhost:8090
Accept-Encoding: gzip,deflate
Accept: application/json

So I guess it is OK to not send Content-Type: as you have written.

lorenzleutgeb commented 6 years ago

Sorry for missing that, I always talk about OAS3. There is no mention of produces in the OAS 3 spec, and I am not familiar with older versions, so I am not sure whether I understand. The only similar concept from OAS 3 that I know is the Request Body Object which is meant to specify the body of POST/PATCH requests.

Your reiteration of the issue is correct. For the example of ping.yaml there should not be a Content-Type header. In my opinion it is not only "OK" to not send the header, but it is actually better! As I said, the way this is implemented currently makes the generated code unusable with a JSON API, and is in general a bit strange with respect to HTTP semantics, which only mention the Content-Type header in connection with payload. And there is no payload in GET requests.

Also, note that in your comment not only the Content-Type header but also the Accept header is inconsistent between the two libraries... Assuming that you are calling the same operation that's pretty messed up.

jmini commented 6 years ago

Yes I have also mixed OAS2 and OAS3 in my message. produces is a OAS2 concept, but OpenAPI-Generator works with OAS3 concepts (when we get an OAS2 input, Swagger-Parser convert it to an OAS3 for us).

in a OpenAPI Spec with an operation that do not have a produces

Is to understand as: that does not have a requestBody with a mimeType.

Thank you for the quick feedback. I think changing the code as you have proposed is a good idea.

In ApiClient (to be changed in ApiClient.mustache)

    /**
     * Select the Content-Type header's value from the given array:
     *   if JSON exists in the given array, use it;
     *   otherwise use the first one of the array.
     *
     * @param contentTypes The Content-Type array to select from
     * @return The Content-Type header to use. If the given array is empty, null will be returned.
     *   If the given array matches "any", JSON will be used.
     */
    public String selectHeaderContentType(String[] contentTypes) {
        if (contentTypes.length == 0) {
             return null;
        }
        for (String contentType : contentTypes) {
            if (isJsonMime(contentType)) {
                return contentType;
            }
        }
        if(contentTypes[0].equals("*/*")) {
            return "application/json";
        }
        return contentTypes[0];
    }

In api.mustache:

        final String localVarContentType = apiClient.selectHeaderContentType(localVarContentTypes);
        if(localVarContentType != null) {
            localVarHeaderParams.put("Content-Type", localVarContentType);
        }

Does it makes sense to you? Do you want to open a pull request for that?

lorenzleutgeb commented 6 years ago

These two changes LGTM. I could open a PR, but you did the work and are maintainer, so I guess the change would get through quicker if you do it.

But wait, one question though: What if the OAS 3 spec defines some MIME type that returns false for isJsonMime. Let's say someone needs to describe a "custom" MIME type for their API that does not match. Then these people will end up with Content-Type: application/json, which might be unexpected. Not sure if the GitHub API only uses custom MIME types for responses or also requests, but their MIME types are even listed as examples in the spec.

jmini commented 6 years ago

I have started to work on this on a branch issue476_contentType in my fork. I would like to perform some tests before I open a PR.

You can already review it there.