Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JAVA][Client] New object instead of null for empty POST request #98

Merged
merged 8 commits into from
Jun 7, 2018
Merged

[JAVA][Client] New object instead of null for empty POST request #98

merged 8 commits into from
Jun 7, 2018

Conversation

bmordue
Copy link
Contributor

@bmordue bmordue commented May 18, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

It's safer to initialize the request body to an empty Object instead of null if there are no body parameters. In particular, this allows Jackson to successfully serialize a POST request with no body parameters when the Content-Type header is set to "application/json, which is set by default.

See swagger-api/swagger-codegen#7551 for more context -- bringing this over to openapi-generator

cc @bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger

@jmini
Copy link
Member

jmini commented May 25, 2018

Because the "ci/circleci" if failing for some download failed errors:

sbt.ResolveException: download failed: joda-time#joda-time;2.2!joda-time.jar
download failed: org.scala-lang.modules#scala-xml_2.11;1.0.5!scala-xml_2.11.jar(bundle)

I have tried to run the java sample locally. This is not working for me:

mvn clean verify -f samples/client/petstore/java/rest-assured/pom.xml

I get these compile errors:

[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/PetApi.java:[606,37] variable _FORM is already defined in class org.openapitools.client.api.PetApi.UpdatePetWithFormOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[637,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[648,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[659,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[670,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[681,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[692,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[703,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[714,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[725,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[736,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[747,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[758,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEndpointParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[856,36] variable _HEADER is already defined in class org.openapitools.client.api.FakeApi.TestEnumParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[878,36] variable _QUERY is already defined in class org.openapitools.client.api.FakeApi.TestEnumParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[889,36] variable _QUERY is already defined in class org.openapitools.client.api.FakeApi.TestEnumParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[900,36] variable _QUERY is already defined in class org.openapitools.client.api.FakeApi.TestEnumParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[922,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestEnumParametersOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:[1070,37] variable _FORM is already defined in class org.openapitools.client.api.FakeApi.TestJsonFormDataOper
[ERROR] samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/UserApi.java:[497,36] variable _QUERY is already defined in class org.openapitools.client.api.UserApi.LoginUser

The reset-assured project on master is compiling before your change.

Because of the review delay (sorry about that), can you also merge master into your branch (or rebase your commits on top of master and force push on your branch)?

Copy link
Member

@jmini jmini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the compile errors that are created in the samples/ folder

@@ -623,146 +623,146 @@ public TestEndpointParametersOper(RequestSpecBuilder reqSpec) {
return handler.apply(RestAssured.given().spec(reqSpec.build()).expect().spec(respSpec.build()).when().request(POST, REQ_URI));
}

public static final String INTEGER_FORM = "integer";
public static final String _FORM = "integer";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produces compile errors

@jmini
Copy link
Member

jmini commented May 25, 2018

By the way to test locally all java clients, I am using a pom file that looks like this:

<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <groupId>tmp</groupId>
    <artifactId>tmp-build-root</artifactId>
    <packaging>pom</packaging>
    <version>1.0.0-SNAPSHOT</version>
    <modules>

<module>samples/client/petstore/java/feign/</module>
<module>samples/client/petstore/java/google-api-client/</module>
<module>samples/client/petstore/java/jersey1/</module>
<module>samples/client/petstore/java/jersey2-java6/</module>
<module>samples/client/petstore/java/jersey2-java8/</module>
<module>samples/client/petstore/java/jersey2/</module>
<module>samples/client/petstore/java/okhttp-gson-parcelableModel/</module>
<module>samples/client/petstore/java/okhttp-gson/</module>
<module>samples/client/petstore/java/rest-assured/</module>
<module>samples/client/petstore/java/resteasy/</module>
<module>samples/client/petstore/java/resttemplate-withXml/</module>
<module>samples/client/petstore/java/resttemplate/</module>
<module>samples/client/petstore/java/retrofit/</module>
<module>samples/client/petstore/java/retrofit2-play24/</module>
<module>samples/client/petstore/java/retrofit2/</module>
<module>samples/client/petstore/java/retrofit2rx/</module>
<module>samples/client/petstore/java/retrofit2rx2/</module>

    </modules>
</project>

@bmordue
Copy link
Contributor Author

bmordue commented May 25, 2018

Thanks @jmini
I've back out the change to modules/openapi-generator/src/main/resources/Java/api.mustache. I don't understand why this was causing compile errors in the rest-assured sample, so I've just backed out that change to resolve the errors. I was able to build all the samples locally.

@jmini
Copy link
Member

jmini commented May 25, 2018

Shippable — Run 389 status is FAILED.

I see this in the logs:

UNCOMMITTED CHANGES ERROR
There are uncommitted changes in working tree after execution of 'bin/ensure-up-to-date'
HEAD detached from FETCH_HEAD
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   samples/client/petstore/java/jersey2-java6/src/main/java/org/openapitools/client/api/FakeApi.java
	modified:   samples/client/petstore/java/jersey2-java6/src/main/java/org/openapitools/client/api/PetApi.java
	modified:   samples/client/petstore/java/jersey2-java6/src/main/java/org/openapitools/client/api/StoreApi.java
	modified:   samples/client/petstore/java/jersey2-java6/src/main/java/org/openapitools/client/api/UserApi.java
	modified:   samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/api/FakeApi.java
	modified:   samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/api/PetApi.java
	modified:   samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/api/StoreApi.java
	modified:   samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/api/UserApi.java
	modified:   samples/client/petstore/java/jersey2/src/main/java/org/openapitools/client/api/FakeApi.java
	modified:   samples/client/petstore/java/jersey2/src/main/java/org/openapitools/client/api/PetApi.java
	modified:   samples/client/petstore/java/jersey2/src/main/java/org/openapitools/client/api/StoreApi.java
	modified:   samples/client/petstore/java/jersey2/src/main/java/org/openapitools/client/api/UserApi.java
	modified:   samples/client/petstore/java/resteasy/src/main/java/org/openapitools/client/api/FakeApi.java
	modified:   samples/client/petstore/java/resteasy/src/main/java/org/openapitools/client/api/PetApi.java
	modified:   samples/client/petstore/java/resteasy/src/main/java/org/openapitools/client/api/StoreApi.java
	modified:   samples/client/petstore/java/resteasy/src/main/java/org/openapitools/client/api/UserApi.java
	modified:   samples/client/petstore/java/resttemplate-withXml/src/main/java/org/openapitools/client/api/FakeApi.java
	modified:   samples/client/petstore/java/resttemplate-withXml/src/main/java/org/openapitools/client/api/PetApi.java
	modified:   samples/client/petstore/java/resttemplate-withXml/src/main/java/org/openapitools/client/api/StoreApi.java
	modified:   samples/client/petstore/java/resttemplate-withXml/src/main/java/org/openapitools/client/api/UserApi.java
	modified:   samples/client/petstore/java/resttemplate/src/main/java/org/openapitools/client/api/FakeApi.java
	modified:   samples/client/petstore/java/resttemplate/src/main/java/org/openapitools/client/api/PetApi.java
	modified:   samples/client/petstore/java/resttemplate/src/main/java/org/openapitools/client/api/StoreApi.java
	modified:   samples/client/petstore/java/resttemplate/src/main/java/org/openapitools/client/api/UserApi.java

no changes added to commit (use "git add" and/or "git commit -a")
Please run 'bin/ensure-up-to-date' locally and commit changes

@bmordue: what is your workflow when you create the commit "Ran bin/java-petstore-all.sh again". It is not documented, but it should be:

  • Run mvn verify on the root of the repo
  • Run .bin/java-petstore-all.sh
  • Commit the changes

Then the check in Shippable will be OK.

@jmini
Copy link
Member

jmini commented May 28, 2018

Thank you for the fix, the build is still red:

Shippable — Run 437 status is FAILED.

But this is because of 71b5de3 introduced by #104. I hope this will be fixed ASAP in order to be able to verify that your PR is clean.

@jmini
Copy link
Member

jmini commented May 28, 2018

Commit c30afdf fixes the Shippable build. Can you please merge master into your branch and push the merge commit or rebase on top of master and force push?

@jmini
Copy link
Member

jmini commented Jun 6, 2018

I have merged master into this PR again. I hope this will fix the CI.

@wing328 wing328 added this to the 3.0.1 milestone Jun 7, 2018
@jmini jmini merged commit 0fb1ffa into OpenAPITools:master Jun 7, 2018
@bmordue bmordue deleted the new-object-empty-request branch June 7, 2018 08:14
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jun 8, 2018
* master:
  Add 'unblu inc.' to company list (OpenAPITools#246)
  put company list in alphabetical order (OpenAPITools#244)
  [jaxrs-spec] generate spec file (yaml) correctly (OpenAPITools#243)
  [C++] Adjust the names (script, sample folder, generator) to lang option (OpenAPITools#220)
  Add GMO Pepabo to company list (OpenAPITools#242)
  [Spring] Add apiFirst option (OpenAPITools#184)
  [cli] Write to stdout/stderr, allow redirection (OpenAPITools#207)
  [JAVA][Client] New object instead of null for empty POST request (OpenAPITools#98)
  Make yaml serialization deterministic (OpenAPITools#233)
  Add syntax highlighting to migration guide (OpenAPITools#237)
  Fix shippable badge (OpenAPITools#232)
  update company list (OpenAPITools#227)
  Fix OpenAPITools#210: [Ada] Update the code generator for required and optional parameters (OpenAPITools#211)
  Delete unused methods in DefaultCodegen (OpenAPITools#209)
  add note about maven plugins (OpenAPITools#216)
  add raiffeisen to company list (OpenAPITools#223)
  add a remark about homebrew installatio (OpenAPITools#217)
@rubms
Copy link
Contributor

rubms commented Jul 19, 2018

This pull request is causing an error, that originated issue #513: now an empty Object is being set as request body on every request (not only POST requests) and by default message converters don't know how to treat empty Objects, throwing the exception org.springframework.web.client.RestClientException: Could not write request: no suitable HttpMessageConverter found for request type [java.lang.Object] and content type [application/json].

Before this pull request, when post objects were null by default, this error did not happen.

@MarvGilb
Copy link
Contributor

@rubms I was about to write the same problem.
I' am generating a Java resttemplate client and simple GET requests aren't working anymore.

By setting 'postBody' to 'new Object' the RestTemplate.doWithRequest() does not jump into the first if block in line 829. It now jumps into the block to where it tries to serialize the body (Within a GET request)

@rubms
Copy link
Contributor

rubms commented Jul 19, 2018

I just created the #605 pull request in order to get this fixed.

@MarvGilb
Copy link
Contributor

I'm concerned. Why aren't there tests to cover this? Simple GET/PUT/POST for every library should be mandatory!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants