[JAVA][Client] New object instead of null for empty POST request by bmordue · Pull Request #7551 · swagger-api/swagger-codegen

bmordue · 2018-02-01T16:52:18Z

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: 3.0.0 branch for changes related to OpenAPI spec 3.0. 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.

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

JFCote

I've commented on a couple of things but I find very odd that there are sooooooo much files that changed and that most of the changes are not linked to your changes. @wing328 , is this something normal?

Also, I'm not sure that replacing null by new Object is a good practice. @bmordue , do you have any documentation backing the fact that it's safer or a good practice?

JFCote · 2018-02-01T17:52:24Z

samples/client/petstore/java/jersey2-java6/build.gradle


 ext {
-    swagger_annotations_version = "1.5.17"
+    swagger_annotations_version = "1.5.15"


why reverting to this version?

JFCote · 2018-02-01T17:52:35Z

samples/client/petstore/java/jersey2-java6/build.sbt

    resolvers += Resolver.mavenLocal,
    libraryDependencies ++= Seq(
-      "io.swagger" % "swagger-annotations" % "1.5.17",
+      "io.swagger" % "swagger-annotations" % "1.5.15",


why reverting to this version?

JFCote · 2018-02-01T17:52:44Z

samples/client/petstore/java/jersey2-java6/pom.xml

    <properties>
        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
-        <swagger-core-version>1.5.18</swagger-core-version>
+        <swagger-core-version>1.5.15</swagger-core-version>


why reverting to this version?

JFCote · 2018-02-01T17:53:08Z

samples/client/petstore/java/jersey2-java8/build.gradle


 ext {
-    swagger_annotations_version = "1.5.17"
+    swagger_annotations_version = "1.5.15"


same question everywhere... i will not mark every places

JFCote · 2018-02-01T18:02:49Z

@bmordue The more I think about it, I'm pretty sure that a body is mandatory in a POST (and PUT) call. So I'm not sure we want to start doing "defensive programming". But I may be wrong. Maybe we should ask the community what they think of this. @wing328 , any thoughs on that?

wing328 · 2018-02-02T05:13:30Z

@JFCote there was a use case before in which the POST operation allows empty body (in other words, the body parameter is optional), and the API server will create an object automatically and return it back to the client.

bmordue · 2018-02-02T11:53:10Z

@JFCote Thanks for your feedback. I had a read through the HTTP spec and couldn't see anything that states that POST (or PUT) requests must have a request body. I also found this thread on the IETF mailing list about a similar question: http://lists.w3.org/Archives/Public/ietf-http-wg/2010JulSep/0272.html

In my specific case, the entity that is being POSTed is described by header and query parameters, which is why there's no body in the request.

From my point of view, part of the problem is that swagger-codegen applies a "Content-Type: application/json" header if none is specified. This means that my service is attempting to parse an empty body as JSON, and fails. The two approaches to resolve this with a generated client seem to be either don't apply the content-type header, or make sure that the request body is parseable as JSON even if I don't have data to send in the body. The second approach seemed to require fewer changes in swagger-codegen with potentially wide effect.

JFCote · 2018-02-02T13:03:51Z

@wing328 @bmordue I understand better now! Thanks for the clarification.

You changes looked good but I wasn't sure about all the generated samples with what looked like regression. Maybe it's because samples didn't get updated in a while but I found strange that the version are going back.

Maybe we should wait for another reviewer to validate.

wing328 · 2018-02-08T08:03:07Z

@bmordue CircleCI reports the following issues with updated samples:

Tests in error: 
  testUpdatePetWithForm(io.swagger.client.api.PetApiTest): Cannot have body and form params
  testUploadFile(io.swagger.client.api.PetApiTest): Cannot have body and form params
  testDeletePet(io.swagger.client.api.PetApiTest): com.fasterxml.jackson.databind.JsonMappingException: No serializer found for class java.lang.Object and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) )
  testDeleteOrder(io.swagger.client.api.StoreApiTest): com.fasterxml.jackson.databind.JsonMappingException: No serializer found for class java.lang.Object and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) )

Tests run: 44, Failures: 0, Errors: 4, Skipped: 3

Please take a look when you've time.

JFCote suggested changes Feb 1, 2018

View reviewed changes

wing328 added Client: Java Enhancement: General labels Feb 8, 2018

wing328 added this to the v2.4.0 milestone Feb 8, 2018

bmordue added 2 commits May 15, 2018 20:05

new Object() for empty request body

694c9a9

Ran bin/java-petstore-all.sh

b15efea

bmordue mentioned this pull request May 18, 2018

[JAVA][Client] New object instead of null for empty POST request OpenAPITools/openapi-generator#98

Merged

4 tasks

frantuma added the Language: java label Jun 27, 2018

Merge branch 'master' into new-object-empty-request

b4c6e51

bmordue closed this by deleting the head repository Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[JAVA][Client] New object instead of null for empty POST request#7551
bmordue wants to merge 3 commits intoswagger-api:masterfrom
bmordue:new-object-empty-request

bmordue commented Feb 1, 2018

Uh oh!

JFCote left a comment •

edited

Loading

Uh oh!

JFCote Feb 1, 2018

Uh oh!

JFCote Feb 1, 2018

Uh oh!

JFCote Feb 1, 2018

Uh oh!

JFCote Feb 1, 2018

Uh oh!

JFCote commented Feb 1, 2018

Uh oh!

wing328 commented Feb 2, 2018

Uh oh!

bmordue commented Feb 2, 2018

Uh oh!

JFCote commented Feb 2, 2018 •

edited

Loading

Uh oh!

wing328 commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

bmordue commented Feb 1, 2018

PR checklist

Description of the PR

Uh oh!

JFCote left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JFCote Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

JFCote Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

JFCote Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

JFCote Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

JFCote commented Feb 1, 2018

Uh oh!

wing328 commented Feb 2, 2018

Uh oh!

bmordue commented Feb 2, 2018

Uh oh!

JFCote commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JFCote left a comment •

edited

Loading

JFCote commented Feb 2, 2018 •

edited

Loading