Skip to content
New issue

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

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

Already on GitHub? # to your account

[java][client] Don't include nullable attributes twice in serialized JSON #3923

Merged
merged 3 commits into from
Oct 22, 2019

Conversation

bkabrda
Copy link
Contributor

@bkabrda bkabrda commented Sep 20, 2019

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

CC @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04)

Description of the PR

The problem that this PR solves is that for JsonNullable fields of POJOs, the field would appear twice, once in snake case and once in camel case (e.g. both some_field and someField would appear in the JSON). The cause is:

  • JsonNullable fields have two sets of getters and setters - "simple ones" (e.g. getSomeField) and JsonNullable ones (e.g. getSomeField_JsonNullable).
  • For the JsonNullable fields, the getSomeField_JsonNullable getter is marked as the attribute and explicitly marked to be serialized as some_field.
  • By default, the Jackson serialization library takes all getters of the object and serializes them. This means that Jackson would also see the non-JsonNullable getter and add it to the serialized object.
  • This would only demonstrate on fields that have at least two words in their name. For example, getEnd would work fine, as there would be getEnd and getEnd_JsonNullable. The getEnd_JsonNullable getter would be marked to be serialized as end, which would make Jackson override getEnd. But for getSomeField and getSomeField_JsonNullable, Jackson would serialize getSomeField as someField and getSomeField_JsonNullable as some_field.
  • Note that this fix also uncovered the need to add a private setter if the nullable field is read-only (Jackson can use that to properly set the value of the readOnly nullable attribute).

This patch fixes the above by explicitly marking the simple getters as ignored during serialization.

There's a slight problem now that this adds an extraneous newline in between the getters' annotations, so I'm trying to get rid of that.

@auto-labeler
Copy link

auto-labeler bot commented Sep 20, 2019

👍 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.

@wing328
Copy link
Member

wing328 commented Sep 27, 2019

Looks like the samples (Java client) are not up-to-date: https://circleci.com/gh/OpenAPITools/openapi-generator/8989#tests/containers/2

@bkabrda
Copy link
Contributor Author

bkabrda commented Sep 27, 2019

@wing328 yeah, there's a problem with an extra added newline that really shouldn't be there - this change shouldn't affect (most of) the generated samples, but it does add an extra newline when [1] does not get rendered (the condition evaluates to false). I couldn't find any way to remove this extra newline. Do you know how I could get rid of it?

[1] https://github.com/OpenAPITools/openapi-generator/pull/3923/files#diff-48702a3c157b1d9b0d527e28c5f99c13R183

@bkabrda bkabrda force-pushed the java-client-explicit-nulls-fix branch from 88bb11b to 1537867 Compare October 2, 2019 11:18
@bkabrda
Copy link
Contributor Author

bkabrda commented Oct 2, 2019

Allright, so I managed to get rid of the extra added whitespace by this change and I even got rid of some whitespace that shouldn't have been there in the first place. Hopefully this is now ready for review.

@wing328 wing328 added this to the 4.2.0 milestone Oct 22, 2019
Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

LGTM and I assume there's no further feedback on this PR as it has been pending review for a while

@wing328 wing328 merged commit cf58a42 into OpenAPITools:master Oct 22, 2019
@wing328
Copy link
Member

wing328 commented Oct 31, 2019

@bkabrda thanks for the PR, which has been included in v4.2.0 release: https://twitter.com/oas_generator/status/1189824932345069569

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

Successfully merging this pull request may close these issues.

2 participants