openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

[Java][Spring] Implement JsonNullable in openapiNullable correctly

Open daberni opened this issue 2 years ago • 7 comments

fixes #14765

JsonNullable was implemented incorrectly, checking for nullable property instead of required property as originally defined by OpenAPI Spec

This is a major breaking change and this PR should primarily be used for discussion. Implementation is only done for JavaSpring first, but other Java CodeGenerators would be affected too.

I first skipped the generation of other samples for the moment, but can be done when further discussion did happen.

PR checklist

  • [x] Read the contribution guidelines.
  • [x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • [ ] Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • [-] In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date.sh
    
    Commit all changed files.
  • [x] File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • [x] If your PR is targeting a particular programming language: @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09)

daberni avatar Feb 20 '23 15:02 daberni

JsonNullable was implemented incorrectly, checking for nullable property instead of required property as originally defined by OpenAPI Spec

Hi @daberni Could you please point to part of spec which you are referring to?

borsch avatar Feb 20 '23 18:02 borsch

JsonNullable was implemented incorrectly, checking for nullable property instead of required property as originally defined by OpenAPI Spec

Hi @daberni Could you please point to part of spec which you are referring to?

@borsch - I added my feedback to the issue https://github.com/OpenAPITools/openapi-generator/issues/14765 - might be worth discussing there?

welshm avatar Feb 20 '23 18:02 welshm

JsonNullable was implemented incorrectly, checking for nullable property instead of required property as originally defined by OpenAPI Spec

Hi @daberni Could you please point to part of spec which you are referring to?

I see it in the second paragram of the README.md file.

jspetrak avatar May 24 '23 11:05 jspetrak

JsonNullable was implemented incorrectly, checking for nullable property instead of required property as originally defined by OpenAPI Spec

Hi @daberni Could you please point to part of spec which you are referring to?

I highlighted it in the related issue, it starts with the second sentence of the jackson-databind-nullable Library:

The JsonNullable wrapper shall be used to wrap Java bean fields for which it is important to distinguish between an explicit "null" and the field not being present. A typical usage is when implementing Json Merge Patch where an explicit "null"has the meaning "set this field to null / remove this field" whereas a non-present field has the meaning "don't change the value of this field".

daberni avatar May 25 '23 09:05 daberni

JsonNullable is needed for Java API server to be able to distinguish between an explicit "null" and the field not being present in the request from the client.

If the field is defined as nullable: false, then Java server has no problem, null in that field should be treated as "not present, do not update the field".

Therefore, using x-is-jackson-optional-nullable only makes sence for fields defined as nullable: true and the current implementation is ok

vershnik avatar Jun 20 '23 06:06 vershnik

Hello @daberni does this patch also fixes inheritence in combination with nullability? We are hitting this issue #15640 so I would be glad if your fix covered it as well.

jspetrak avatar Jul 22 '23 09:07 jspetrak

https://github.com/OpenAPITools/openapi-generator/issues/17538

Kavan72 avatar May 05 '24 13:05 Kavan72