Add in `Optional` handling for SpringBoot POJOs #11384

welshm · 2022-01-23T03:04:26Z

Modified pojo.mustache to have Optional be used on POJOs

Added spring-boot-oas3-nullable.yaml to generate nullable examples to validate against
Added spring-boot-oas3-nullable-disabled.yaml to generate optional behaviour when nullable is disabled
[BUG] [JAVA] [SPRING] Optional values aren't optional in POJOs, only in path params #11383

NOTE: This is a breaking change for anyone with useOptional enabled.

PR checklist

Read the contribution guidelines.
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*.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Spec that shows the use case

ApiDefault:
  type: object
  required:
    - requiredMessage
    - requiredNullableMessage
  properties:
    message:
      type: string
      description: A message
    nullableMessage:
      type: string
      description: A nullable message
      nullable: true
    requiredMessage:
      type: string
      description: A required message
    requiredNullableMessage:
      type: string
      description: A required nullable message
      nullable: true

Output

useOptional: false
openApiNullable: false

private String message;
private String nullableMessage
private String requiredMessage;
private String requiredNullableMessage;

useOptional: true
openApiNullable: false

private Optional<String> message;
private Optional<String> nullableMessage
private String requiredMessage;
private String requiredNullableMessage;

useOptional: false
openApiNullable: true

private String message;
private JsonNullable<String> nullableMessage
private String requiredMessage;
private JsonNullable<String> requiredNullableMessage;

useOptional: true
openApiNullable: true

private Optional<String> message;
private JsonNullable<String> nullableMessage
private String requiredMessage;
private JsonNullable<String> requiredNullableMessage;

Bring `welshm` fork up to master

Bring `welshm` fork up to `OpenAPITools` master

Bring `welshm` fork up to latest `OpenAPITools` master

Bring welshm fork up to date with OpenAPI master

Merge lastest to welshm fork for development

- Introduces too much risk for path/header/query params - enums are expected to be primitive types there

welshm · 2022-01-23T04:13:46Z

bin/configs/spring-boot-oas3-nullable-disabled.yaml

@@ -0,0 +1,12 @@
+generatorName: spring
+outputDir: samples/openapi3/server/petstore/springboot-nullable-disabled
+inputSpec: modules/openapi-generator/src/test/resources/3_0/petstore-with-nullable-required.yaml


Note: This validates against 3_0

welshm · 2022-01-23T04:16:21Z

@wing328 @cachescrubber

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

…api-generator

wing328 · 2022-02-12T07:33:54Z

NOTE: This is a breaking change for anyone with useOptional enabled.

Thanks for the PR. Is it correct to say that the uesOptional option will work as expected after this PR gets merged? If other words, this PR aims to fix issues when the useOptional option is set to true, right?

welshm · 2022-02-12T15:43:42Z

NOTE: This is a breaking change for anyone with useOptional enabled.

Thanks for the PR. Is it correct to say that the uesOptional option will work as expected after this PR gets merged? If other words, this PR aims to fix issues when the useOptional option is set to true, right?

That is my opinion on how useOptional should function, yes. I will post the issue to the Slack to see if anyone has any differing opinions

welshm · 2022-02-17T21:26:21Z

modules/openapi-generator/src/test/resources/3_0/spring/issue_11383.yaml

+        - FOO
+        - BAR
+      default: BAR
+    Params:


@MelleD I tried to cover all relevant cases I could think of here

welshm · 2022-02-17T22:12:00Z

A quick test shows this to be working with both enabled:

    @Test
    void jdk8ModuleObjectMapper() throws JsonProcessingException {
        ObjectMapper objectMapper = new ObjectMapper()
                .registerModule(new Jdk8Module().configureAbsentsAsNulls(true))
                .registerModule(new JsonNullableModule());

        final Params params = new Params()
                .optionalString("OPTIONAL STRING")
                .requiredString("REQUIRED STRING")
                .requiredNullableString(null)
                .requiredEnum(RequiredEnumEnum.ONE)
                .requiredNumber(123L);

        final String jsonResult = objectMapper.writeValueAsString(params);

        final Params readBack = objectMapper.readValue(jsonResult, Params.class);


        System.console().printf(readBack.toString());
    }

However, I don't see nullability being enforced - if I don't set a required field (such as requiredNumber) this will still parse to/from JSON no problem. This is unrelated to Optional work here.

wing328 · 2022-02-18T17:58:33Z

modules/openapi-generator/src/main/resources/JavaSpring/libraries/spring-boot/pom.mustache

+           <artifactId>jackson-datatype-jdk8</artifactId>
+           <version>2.9.6</version>
+        </dependency>
+        {{/useOptional}}


About the maven plug-in test failure, I think you will also need to update modules/openapi-generator-maven-plugin/examples/spring.xml

Test: https://github.com/OpenAPITools/openapi-generator/blob/master/.github/workflows/openapi-generator.yaml#L188

cachescrubber · 2022-02-18T18:53:23Z

@welshm You do not need to provide the jackson-datatype-jdk8 dependency. It is provided by spring-boot-starter-json which in turn is a transitive dependency of spring-boot-starter-web . The reason it failed on the maven-plugin test is the outdated version of spring-boot used there (2.2.1.RELEASE). Just update the spring-boot version instead.

The Jackson Jdk8Module is configured by default in an recent version of spring-boot. No need to provide it as an @bean.

MelleD · 2022-02-18T23:02:20Z

...ver/petstore/springboot-nullable-disabled/src/main/java/org/openapitools/model/Category.java

+public class Category   {
+
+  @JsonProperty("id")
+  private Optional<Long> id;


You should initialize the member with Optional.empty

A null optional field should never exists.

Good catch - fixed in next patch

MelleD · 2022-02-18T23:02:39Z

...ver/petstore/springboot-nullable-disabled/src/main/java/org/openapitools/model/Category.java

+  private Optional<Long> id;
+
+  @JsonProperty("name")
+  private Optional<@Pattern(regexp = "^[a-zA-Z0-9]+[a-zA-Z0-9\\.\\-_]*[a-zA-Z0-9]+$") String> name;


Bean validation is duplicated also in the getter. It should add to the getter

Hmm... that becomes tricky because the mustache generates both as {{>optionalDataTypeWithEnum}} which internally uses {{>optionalDataType}} which has {{>beanValidationCore}} as part of the type.

While it is redundant, I am fine with the duplication for how it makes the use of the mustache simpler.

Alernatively, we could have optionalDataTypeWithEnum and annotatedOptionalDataTypeWithEnum - but would require making the same distinction for the inner usage of optionalDataType.

I see.
So far, optionals have only been used as parameters. I think for member variables we need a new mustache without bean validation. Because the beanvalidation is in <> always makes it awkward.

Created optionalDataTypeWithoutAnnotations - I did leave it on the setter but I'm not that familiar with how the validation is applied. I can convert it to getter only if that's more correct.

Bean validation for DTOs/Pojos have to be set on getter or fields. Currently the template set it on getter

Will remove from setter then

MelleD · 2022-02-18T23:06:36Z

...server/petstore/springboot-nullable-disabled/src/main/java/org/openapitools/model/Order.java

+
+  @JsonProperty("shipDate")
+  @DateTimeFormat(iso = DateTimeFormat.ISO.DATE_TIME)
+  private Optional<OffsetDateTime> shipDate;


Is this really working with Optional?

It seems to be from my test:

@Test void jdk8ModuleObjectMapper() throws JsonProcessingException { ObjectMapper objectMapper = new ObjectMapper() .registerModule(new Jdk8Module().configureAbsentsAsNulls(true)) .registerModule(new JavaTimeModule()) .registerModule(new JsonNullableModule()) ; final Params params = new Params() .optionalString("OPTIONAL STRING") .requiredString("REQUIRED STRING") .requiredNullableString(null) .requiredEnum(RequiredEnumEnum.ONE) .optionalDate(LocalDate.of(2020, 10, 2)) .optionalDateTime(OffsetDateTime.of(LocalDateTime.of(2020, 10, 2, 12, 30), ZoneOffset.UTC)) .requiredNumber(123L); final String jsonResult = objectMapper.writeValueAsString(params); final Params readBack = objectMapper.readValue(jsonResult, Params.class); System.console().printf(readBack.toString()); }

I can see the date time and read it back - it matches. JSON output looks fine too.

cachescrubber · 2022-01-27T05:14:57Z

modules/openapi-generator/src/main/resources/JavaSpring/pojo.mustache

  {{#swagger2AnnotationLibrary}}
  @Schema(name = "{{{baseName}}}", {{#isReadOnly}}accessMode = Schema.AccessMode.READ_ONLY, {{/isReadOnly}}{{#example}}example = "{{{.}}}", {{/example}}{{#description}}description = "{{{.}}}", {{/description}}required = {{{required}}})
  {{/swagger2AnnotationLibrary}}
  {{#swagger1AnnotationLibrary}}
  @ApiModelProperty({{#example}}example = "{{{.}}}", {{/example}}{{#required}}required = {{required}}, {{/required}}{{#isReadOnly}}readOnly = {{{isReadOnly}}}, {{/isReadOnly}}value = "{{{description}}}")
  {{/swagger1AnnotationLibrary}}
-  public {{>nullableDataType}} {{getter}}() {
-    return {{name}};
+  public {{#isNullable}}{{>nullableDataType}}{{/isNullable}}{{^isNullable}}{{#required}}{{>nullableDataType}}{{/required}}{{^required}}{{#isEnum}}{{#useOptional}}Optional<{{/useOptional}}{{{datatypeWithEnum}}}{{#useOptional}}>{{/useOptional}}{{/isEnum}}{{^isEnum}}{{>optionalDataType}}{{/isEnum}}{{/required}}{{/isNullable}} {{getter}}() {


I would prefer to use an include here. The code is difficult to read and hard to understand. Isn't it possible to use optionalDataType.mustache?

This comment seems old - but I need to fix the bean annotations anyway so this might change to an include

Old, but still valid IMO

Moved it's own include - I had trouble coming up with a good name so it's dataTypeDeclaration - which is kind of misleading because these are all data type declarations.

Open to suggestions of better names - and I'm taking a look at your wrapper type change now

cachescrubber · 2022-02-19T08:10:14Z

modules/openapi-generator/src/main/resources/JavaSpring/pojo.mustache

@@ -106,7 +106,7 @@ public class {{classname}} {{#parent}}extends {{{.}}}{{/parent}}{{^parent}}{{#ha
  private {{>nullableDataType}} {{name}}{{#isNullable}} = null{{/isNullable}}{{^isNullable}}{{#defaultValue}} = {{{.}}}{{/defaultValue}}{{/isNullable}};
  {{/required}}
  {{^required}}
-  private {{>optionalDataTypeWithEnum}} {{name}}{{#isNullable}} = Optional.empty(){{/isNullable}}{{^isNullable}}{{#defaultValue}} = Optional.ofNullable({{{.}}}){{/defaultValue}}{{/isNullable}};
+  private {{>optionalDataTypeWithEnum}} {{name}} = {{#defaultValue}}Optional.ofNullable({{{.}}}){{/defaultValue}}{{^defaultValue}}Optional.empty(){{/defaultValue}};


Have you considered to override AbstractJavaCodegen#toDefaultValue() in SpringCodegen? I would prefer to avoid logic in the templates if possible.

Sure I can look into that. I did it this way because that's how it was already done for JsonNullable but I'll try tk improve it

Not as straight forward as one might think (but doable) I currently doing a proof of concept with JsonNullable.

Might make sense to do it as a separate PR then? Consolidate the defaultValue decision and generation?

cachescrubber · 2022-02-19T17:46:14Z

modules/openapi-generator/src/main/resources/JavaSpring/pojo.mustache

@@ -223,15 +223,15 @@ public class {{classname}} {{#parent}}extends {{{.}}}{{/parent}}{{^parent}}{{#ha
  public {{>nullableDataType}} {{getter}}() {
  {{/isNullable}}
  {{^isNullable}}
-  public {{#isMap}}{{>nullableDataType}}{{/isMap}}{{#isArray}}{{>nullableDataType}}{{/isArray}}{{^isMap}}{{^isArray}}{{#required}}{{>nullableDataType}}{{/required}}{{^required}}{{>optionalDataTypeWithEnum}}{{/required}}{{/isArray}}{{/isMap}} {{getter}}() {
+  public {{>dataTypeDeclaration}} {{getter}}() {


Looks good!

I think this can still be improved, but it'll be a lot easier with https://github.com/OpenAPITools/openapi-generator/pull/11666/files so I'll iterate on it after that

Did you want to get #11666 in first? This change is good to go otherwise, but might be easier to deal with merge conflicts and resolve any unit tests changes if I update this after yours is in first.

welshm · 2022-10-29T20:38:27Z

Closing - since this is way out of date

welshm and others added 11 commits June 15, 2021 14:29

Merge pull request #1 from OpenAPITools/master

2d68708

Bring `welshm` fork up to master

Merge pull request #5 from OpenAPITools/master

a3d03bb

Bring `welshm` fork up to `OpenAPITools` master

Merge branch 'master' of github.com:OpenAPITools/openapi-generator

8c30649

Merge pull request #7 from OpenAPITools/master

6e1982a

Bring `welshm` fork up to latest `OpenAPITools` master

Merge branch 'master' of github.com:welshm/openapi-generator

3455229

Merge pull request #9 from OpenAPITools/master

64028ca

Bring welshm fork up to date with OpenAPI master

Merge pull request #13 from OpenAPITools/master

e87ace6

Merge lastest to welshm fork for development

Add in Optional handling for SpringBoot POJOs

a63b860

Try to add enum type to DefaultCodegen

faeb347

Revert changes to nullableDataType and optionalDataType

38872bb

- Introduces too much risk for path/header/query params - enums are expected to be primitive types there

Simpligy mustache

f9c22de

welshm commented Jan 23, 2022

View reviewed changes

welshm added 6 commits January 23, 2022 00:05

Add JsonIgnore import

69c7202

Fix some maven artifact names

a85cde9

t checkout spri Merge branch 'master' of github.com:OpenAPITools/open…

b499f0f

…api-generator

Merge branch 'master' into spring_oas3_nullability

57601a8

Fix spacing

10cae3b

Spaces not tabs

bdb64d6

welshm mentioned this pull request Feb 6, 2022

[BUG][JAVA] Deserialization of array with uniqueItems=true doesn't preserve order #10167

Open

welshm added 3 commits February 5, 2022 23:03

Merge branch 'master' into spring_oas3_nullability

f9eea2f

Update samples

0b043ef

Merge branch 'master' into spring_oas3_nullability

9103217

wing328 added Breaking change (without fallback) Issue: Bug Server: Java Server: Spring labels Feb 12, 2022

wing328 added this to the 6.0.0 milestone Feb 12, 2022

welshm commented Feb 17, 2022

View reviewed changes

welshm added 2 commits February 17, 2022 16:36

Fix JDK8 module bean

b952636

Fix additional properties

4e92c8e

Merge branch 'master' into spring_oas3_nullability

66d9eb2

wing328 reviewed Feb 18, 2022

View reviewed changes

Fix test xml to include JDK8 module

6bbc3a8

welshm requested a review from jimschubert as a code owner February 18, 2022 18:16

MelleD reviewed Feb 18, 2022

View reviewed changes

welshm added 2 commits February 18, 2022 18:42

Merge branch 'master' into spring_oas3_nullability

e094a08

Fix default optoinal to empty() and add some datetime tests

7cb3767

cachescrubber suggested changes Feb 19, 2022

View reviewed changes

welshm added 2 commits February 19, 2022 12:33

Cleanup and remove annotations from member variables for optionals

ad76d49

Merge branch 'master' into spring_oas3_nullability

ee9b290

cachescrubber reviewed Feb 19, 2022

View reviewed changes

welshm mentioned this pull request Feb 21, 2022

[Java] Feature/wrapped types and default values #11666

Open

wing328 modified the milestones: 6.0.0, 6.0.1 May 26, 2022

wing328 modified the milestones: 6.0.1, 6.1.0 Jul 5, 2022

wing328 modified the milestones: 6.1.0, 6.1.1 Sep 11, 2022

wing328 modified the milestones: 6.1.1, 6.2.1 Sep 24, 2022

welshm closed this Oct 29, 2022

MelleD mentioned this pull request May 31, 2023

[BUG][Java][Spring] openapiNullable - JsonNullable usage is based on nullable property instead of required property #14765

Open

6 tasks

Uh oh!

Add in Optional handling for SpringBoot POJOs #11384

Add in Optional handling for SpringBoot POJOs #11384

Uh oh!

Conversation

welshm commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Spec that shows the use case

Output

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welshm commented Jan 23, 2022

Uh oh!

wing328 commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welshm commented Feb 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welshm commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cachescrubber commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MelleD Feb 18, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welshm commented Oct 29, 2022

Uh oh!

Uh oh!

Add in `Optional` handling for SpringBoot POJOs #11384

Add in `Optional` handling for SpringBoot POJOs #11384

welshm commented Jan 23, 2022 •

edited

Loading

wing328 commented Feb 12, 2022 •

edited

Loading

cachescrubber commented Feb 18, 2022 •

edited

Loading

MelleD Feb 18, 2022 •

edited

Loading