[BUG][JAVA][Client] Regression: Unconditionally sends empty object request body

[kevinoid]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• What's the version of OpenAPI Generator used?
• Have you search for related issues/PRs?
• What's the actual output vs expected output?
• [Optional] Bounty to sponsor the fix (example)

Description

The java okhttp-gson client (and likely other Java clients) always send an empty object in the request body for DELETE (and likely POST and PUT) operations which do not define requestBody. Since DELETE request payload has no defined semantics this is likely to cause compatibility issues (and causes 4XX responses from the API I am working with).

openapi-generator version

The issue first appeared in 0fb1ffa (#98) and still occurs on master (0cb9212).

OpenAPI declaration file content or url

openapi: '3.0.2'
info:
  title: delete example
  version: '1.0.0'
servers:
- url: http://example.com/api
components:
  schemas:
    Error:
      type: object
      properties:
        message:
          type: string
paths:
  /:
    delete:
      responses:
        '204':
          description: Deleted
        default:
          description: Error
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Error'

(Note: At least one schema must be defined for the generated code to compile, so I added Error for this purpose.)

Command line used for generation

openapi-generator-cli.jar generate -i openapi.yaml -g java -o openapi-generator-test-client

Steps to reproduce

1. Generate the code using the YAML spec and command from above, replacing example.com/api with a test server.
2. Create src/main/java/TestApp.java with content

import org.openapitools.client.*;
import org.openapitools.client.api.*;

public class TestApp {
    public static void main(String[] args) throws ApiException {
        ApiClient apiClient = new ApiClient();
        DefaultApi defaultApi = new DefaultApi(apiClient);
        defaultApi.rootDelete();
    }
}

3. Run gradle -DmainClass=TestApp execute and observe traffic on the test server.

The client generated from code before 0fb1ffa will send:

DELETE /api/ HTTP/1.1
Accept: application/json
Content-Type: application/json
User-Agent: OpenAPI-Generator/1.0.0/java
Host: example.com
Connection: Keep-Alive
Accept-Encoding: gzip

The client generated with 0fb1ffa or later will send:

DELETE /api/ HTTP/1.1
Accept: application/json
User-Agent: OpenAPI-Generator/1.0.0/java
Content-Type: application/json; charset=utf-8
Content-Length: 2
Host: example.com
Connection: Keep-Alive
Accept-Encoding: gzip

{}

Suggest a fix

This was fixed for resttemplate by #605 but not for jersey2, okhttp-gson, or resteasy. Reverting 0fb1ffa for the other clients would fix this, but presumably reintroduce #98 (although I'm not sure why Jackson would be called to serialize null, presumably we could just stop doing that).

Thoughts from the participants on #98 and #605? @bmordue, @jmini, @rubms

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[jmini]

Thank you a lot for this issue!

---

I did a review while working on a test suite for the Java clients (more to come in #689)

With jersey2 (with Object localVarPostBody = new Object();) the current exception is:

javax.ws.rs.ProcessingException: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class java.lang.Object and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS)
	at org.glassfish.jersey.client.internal.HttpUrlConnector.apply(HttpUrlConnector.java:284)
	at org.glassfish.jersey.client.ClientRuntime.invoke(ClientRuntime.java:278)
	at org.glassfish.jersey.client.JerseyInvocation.lambda$invoke$0(JerseyInvocation.java:753)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:316)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:298)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:229)
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:414)
	at org.glassfish.jersey.client.JerseyInvocation.invoke(JerseyInvocation.java:752)
	at org.glassfish.jersey.client.JerseyInvocation$Builder.method(JerseyInvocation.java:445)
	at org.glassfish.jersey.client.JerseyInvocation$Builder.post(JerseyInvocation.java:351)

I agree that localVarPostBody needs to be set to null if no request-body is defined.

---

For resteasy:

javax.ws.rs.ProcessingException: RESTEASY004655: Unable to invoke request
	at org.jboss.resteasy.client.jaxrs.engines.ApacheHttpClient4Engine.invoke(ApacheHttpClient4Engine.java:289)
	at org.jboss.resteasy.client.jaxrs.internal.ClientInvocation.invoke(ClientInvocation.java:454)
	at org.jboss.resteasy.client.jaxrs.internal.ClientInvocationBuilder.post(ClientInvocationBuilder.java:193)

---

As far as I can tell, the other clients are OK

[jmini]

@kevinoid: your issue report is realy great.

I have proposed a fix for resteasy and jersey2: #3703, because it was not working with those libraries.

[kevinoid]

Your PR looks great @jmini, thanks for taking the time to work through it and implement a fix!

Note that it doesn't fix the empty object request body for okhttp-gson which was my original motivation for opening the issue. It does appear to fix the exceptions for jersey2 and resteasy, which are more severe issues and look more urgent to me. I'm all in favor of merging it as-is to fix those issues, but I'd appreciate if this issue were left open until the okhttp-gson issue is also fixed.

Thanks again!

[jmini]

I'd appreciate if this issue were left open until the okhttp-gson issue is also fixed.

Of course.
There is also one task pending for jersey2 and PUT, see: #3703 (comment)

---

Note that it doesn't fix the empty object request body for okhttp-gson

I can have a look at this too.

I am trying to understand the original motivation in #98, but I do not get it (I must admit that I did not dig into the history as you did).

• okhttp-gson: no jackson is involved.
• and for the clients using jackson, it seems that having the new Object() instance is creating the stacktraces (at least in with JaxRS clients [BUG][JAVA][Client] Regression: Unconditionally sends empty object request body #3276 (comment); spring is already modified back to null with [Java][Client][RestTemplate] Do not create new Object for empty body  #605)

I must admit that now that I have a test-suite (see details in #689 (comment)), I am much more confident with these kind of changes. I have the proof that there is an improvement (and no regression).

[jmini]

I'm not sure why Jackson would be called to serialize null, presumably we could just stop doing that).

I agree with that. And this might be the problematic point back then (I think Jackson was updated in-between).
When the entity is null (especially for Jersey2 client), I think that the solution is to send an empty string as body.

---

I guess I have found how to formulate the expected behavior when no request-body is defined in the OpenAPI spec:

• The header "Content-Length" must have value "0" (see https://stackoverflow.com/a/1233569)
• About the "Content-Type" header value (mentioned in [JAVA][Client] New object instead of null for empty POST request #98) the header can be sent or not (see https://stackoverflow.com/q/29784398). We can keep the current behavior that adds "application/json"

=> This "Content-Length" constraint justify why the change new Object() in okhttp-json must be changed.
=>

[kevinoid]

Thanks for the additional investigation @jmini! I agree about the test suite. Great addition!

I would expect most request libraries have a way to make a request without a body (e.g. okhttp3.Request.Builder.delete() without arguments for OkHttp) so you wouldn't have to set the headers explicitly. However, I agree that under the hood they would likely use Content-Length: 0, so if it's easier to set that explicitly, I'd say go for it.

Sending Content-Type without a body seems a little odd to me, but I agree that implementations should ignore it if the payload body length is 0, so I have no objection.

Thanks again!

[jmini]

However, I agree that under the hood they would likely use Content-Length: 0, so if it's easier to set that explicitly, I'd say go for it.

I am speaking from the expectation at test suite level, the library should take care of this stuff.

---

The code generated by OpenAPI-Generator is just about using the HTTP-Library in an correct & opinionated way (this is more or less the role of ApiClient) and creating convenient methods to call each of the endpoints described in the spec.

[jmini]

I would expect most request libraries have a way to make a request without a body (e.g. okhttp3.Request.Builder.delete() without arguments for OkHttp)

IMO and according to my tests, the ApiClient of okhttp-gson is already doing what it should in case the body is null:

openapi-generator/samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/ApiClient.java

Lines 1084 to 1091 in 10a310e

	} else if (body == null) {
	if ("DELETE".equals(method)) {
	// allow calling DELETE without sending a request body
	reqBody = null;
	} else {
	// use an empty request body (for POST, PUT and PATCH)
	reqBody = RequestBody.create(MediaType.parse(contentType), "");
	}

So it is safe to set Object localVarPostBody = null instead of Object localVarPostBody = new Object();.

[jmini]

I found an other issue with google-api-client:

When the request body is not defined in the OpenAPI Spec, the generated code is:

openapi-generator/samples/client/petstore/java/google-api-client/src/main/java/org/openapitools/client/api/FakeApi.java

Line 99 in 10a310e

apiClient.new JacksonJsonHttpContent(null) :

This is wrong, the request send to the server is:

• Content-Length : 4
• Body is null (as text)

The solution is the instantiate acom.google.api.client.http.EmptyContent object.

[jmini]

@kevinoid I have now updated all the points discussed here with PR #3703.

[kevinoid]

Looks great @jmini. Thanks so much for all of your effort on this!