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

cachescrubber · 2022-02-19T17:16:53Z

POC - generic wrapper data type

Enhance CodegenProperty to support Wrapper Types

    public String wrapperType;
    public String wrapperFunc;
    public boolean isWrapped;

Wrapper Types are for Example java.util.Optional or org.openapitools.jackson.nullable.JsonNullable.

By adding generic support for such types we avoid clumpsy and difficult to read templates - Mustache templates are advertised as Logic-less templates.. for a reason ...

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.
[x ] 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.
[ x] File the PR against the correct branch: master (5.3.0), 6.0.x
[ 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.

…e the noise)

cachescrubber · 2022-02-19T17:32:23Z

@welshm @MelleD FYI

welshm · 2022-02-19T18:00:45Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java

+            }
+        }
+        if (useOptional) {
+            if (Boolean.FALSE.equals(property.required) && Boolean.FALSE.equals(property.isNullable)


I think this misses a case:

useOptional: true openApiNullable: false private Optional<String> message; private Optional<String> nullableMessage private String requiredMessage; private String requiredNullableMessage;

I think this would skip nullableMessage because it's marked nullable (but the overall nullability feature is disabled)

So I think this might still be a problem but I'm happy to iterate on this

You mean it should be:

a) Boolean.FALSE.equals(property.required) && Boolean.FALSE.equals(property.isNullable)
or
b) Boolean.FALSE.equals(property.required) && Boolean.FALSE.equals(openApiNullable)

Yes - but happy to follow up with that or merge into my PR dealing with Optionals in POJOs

welshm · 2022-02-19T18:04:11Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java

+                }
+            }
+        }
+        if (useOptional) {


I think if optionals are enabled, it would be nice for non-nullable non-required maps/arrays to default to empty map/array instead of null since an empty list/map has the same meanining as a null one in that case.

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

welshm · 2022-02-19T18:10:49Z

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

-  private {{>nullableDataType}} {{name}}{{#isNullable}} = null{{/isNullable}}{{^isNullable}}{{#defaultValue}} = {{{.}}}{{/defaultValue}}{{/isNullable}};
-  {{/openApiNullable}}
-  {{/isContainer}}
+  private {{>dataType}} {{name}}{{#defaultValue}} = {{{defaultValue}}}{{/defaultValue}};


Worth noting: This changes the default way that containers were working before (defaulted to null if not required)

cachescrubber · 2022-02-19T18:59:44Z

@welshm Thx for your review. I think all your findings could be addressed using this approach. Let's see how we can proceed with this. I would like to see you finishing the useOptional story. My main interest here is to avoid to further template clutter with more and more complicated logic - which adds more and more to the maintenance burden.

welshm · 2022-02-19T19:13:56Z

I agree - this will make it much simpler to handle wrapped types and greatly simplify the mustache files

Use Beanvalidation annotation in Optional gette retrun values

cachescrubber · 2022-02-19T20:41:31Z

@welshm The last commit add the beanvalidation and setter adjustments.

For the setter:

public void setName(Optional<String> name)

I personally would vote for

public void setName(String name) { Optional.ofNullable(name); }

I there a technical requirement to use the Optional as a method Parameter?

welshm · 2022-02-19T21:06:03Z

@welshm The last commit add the beanvalidation and setter adjustments.

For the setter:

public void setName(Optional<String> name)

I personally would vote for

public void setName(String name) { Optional.ofNullable(name); }

I there a technical requirement to use the Optional as a method Parameter?

For the setter - it was a discussion between @MelleD and I, with a decision to be consistent with JsonNullable and to opt for the Optional everywhere approach.

I don't think there is a technical requirement otherwise - but I did not test that setup with Jackson (I think it typically expected getters and setters to match for serialization - and you may need to use @JsonIgnore)

cachescrubber · 2022-02-20T07:34:00Z

I don't think there is a technical requirement otherwise - but I did not test that setup with Jackson (I think it typically expected getters and setters to match for serialization - and you may need to use @JsonIgnore)

See latest commit. We could even introduce a flag - which I would like to avoid, but still possible.

MelleD · 2022-02-20T08:05:16Z

I don't think there is a technical requirement otherwise - but I did not test that setup with Jackson (I think it typically expected getters and setters to match for serialization - and you may need to use @JsonIgnore)

See latest commit. We could even introduce a flag - which I would like to avoid, but still possible.

For me it make more sense getter and setter use optional (see chapter Optional everywhere) and I would not use null! setFoo(null) vs setFoo(Optional.empty()). If you like to use Optionals then you would like to avoid nulls.
But currently the default value is Optional.empty so you can just use the builder method to set a new value without optional. And there null should be not possible

Additional there is a builder Methode where you can set a value directly without a Optional!

cachescrubber · 2022-02-20T09:36:00Z

For me it make more sense getter and setter use optional (see chapter Optional everywhere) and I would not use null! setFoo(null) vs setFoo(Optional.empty()). If you like to use Optionals then you would like to avoid nulls. But currently the

I don't get it - you even introduce a new possibility to cause a NPE ... (the Optional parameter is actually null).

There even is a Sonar Rule against using Optinal as method Parameter: https://rules.sonarsource.com/java/RSPEC-3553

The Topic seems to be highly opinionated, so introducing an option might by the right thing even at the cost of an additional flag to support. it is already prepared in pojo.mustache (x-expose-wrapper-in-setter).

That said, I'll google Optional everywhere ...

MelleD · 2022-02-20T12:23:57Z

There is also a Sonar rule don’t use Optional for fields.

There are a lot of different opinions around Optional. IMHO use Optional everywhere and you can get rid of null. And this is a good aim don‘t use null in your code!!!

See here blog of Nicolai Parlog
https://nipafx.dev/nicolai-parlog/

Another one:
https://medium.com/97-things/using-optional-nowhere-somewhere-or-everywhere-b1eb5645eab5

And here:
https://slides.nipafx.dev/expert-java-8/2017-09-18-objektforum-stuttgart/index.html#/

Next advantage it looks and works like the JsonNullable Builder.

Can’t understand that developer would like to use a type like Optional + would use null to create a empty optional !!!

EDIT: Your sonar rule is not for setter! This is if you use the parameter like a Boolean and create a if else with a flag. Then it’s better to create two methods instead of a flag parameter

welshm · 2022-02-20T12:50:47Z

IMO Optionals are not used to avoid null, but rather to ensure that null scenarios are properly accounted for.

When using an Optional, it's easy to see how the null case is evaluated. Without an Optional, it may not be clear if null is a valid (or expected) value - and not handling the null case may be "correct".

So, having said that, we opt to not use Optionals as parameters - and as return types only when null is a valid value.

I think a flag for this makes sense. If we don't want to support a flag, I think it is easiest to have Optional be the input argument (easiest to be more strict and ease up than the other way around).

cachescrubber · 2022-02-20T13:55:14Z

Next try. Changes:

Prepare everything for an cli option wether to expose the Optional wrapper type in setter methods (defaults to true).
JsonNullable requires to be explicitly passed in the setter, because it can actually have three states (undefined, null, with value) which could not be expressed through a simple setter.
Guard Optional or JsonNullable assignment with Objects.requireNonNull.

MelleD · 2022-02-20T14:30:32Z

IMO Optionals are not used to avoid null, but rather to ensure that null scenarios are properly accounted for.

Therefore you need no Optionals just use Annotations https://www.baeldung.com/spring-null-safety-annotations

So then another:

The main advantage of this new construct is that No more too many null checks and NullPointerException

Java 8 Optional In Depth - Mkyong.comhttps://mkyong.com › java8 › java-8-optional-in-depth

So normally you can completely avoid 'null' values in your code and avoid the billion dollar mistake ;).
With JsonNullable the 'null' have a semantic meaning (reset) and is currently related to Jackson

MelleD · 2022-02-20T14:39:35Z

...springboot-useoptional/src/main/java/org/openapitools/model/AdditionalPropertiesAnyType.java


  public AdditionalPropertiesAnyType name(String name) {
-    this.name = name;
+    this.name = Optional.ofNullable(name);


Should be just Optional.of().

I like the spring boot way all parameter are non null by default and if you would like to use a null parameter you mark it with @nullable.

So i would use Optional.of and if you would like to reset the Optional you have to use the setter!

welshm · 2022-02-23T06:10:59Z

@welshm JavaCxfClient Codegen rules out required optionals: // see org.openapitools.codegen.java.JavaCXFClientCodegenTest.testPostProcessNullableModelPropertyWithOpenApiNullableEnabledForRequiredProperties

What should we do? I don't like diverging behaviour in different Java Based Codegens for such basic stuff.

Let's be consistent with JavaCxfClient then.

I don't think it's a big deal. The value is required so it should be set - if it's not set I am guessing Jackson will set it to null - which is weird to provide a default but I think usable. Would be better if it errored.

This will likely fix the outstanding spring issue where nullable required fields did not need to be set.

docs/generators/spring.md

cachescrubber · 2022-02-23T19:13:06Z

TODO: Unit-Test org.openapitools.codegen.languages.AbstractJavaCodegen#postProcessModelProperty

cachescrubber · 2022-02-23T19:13:50Z

This will likely fix the outstanding spring issue where nullable required fields did not need to be set.

Could you Link the issue here?

@welshm

welshm · 2022-02-23T20:37:25Z

This will likely fix the outstanding spring issue where nullable required fields did not need to be set.

Could you Link the issue here?

@welshm

https://openapi-generator.slack.com/archives/CLSB0U0R5/p1643615281300959

I'll see if I can find an actual issue and not just the Slack discussion...

MelleD · 2022-02-23T21:17:56Z

Looks good, I really like it.

@cachescrubber Out of interest, why do you think it's good to reset with 'null'?
Do you have a good article or similar where this setup like this?

cachescrubber · 2022-02-26T11:01:12Z

I still see an issue with Jackson Mappings

The default Jackson ObjectMapper configured by spring boot will use null both for explicit JsonNullable nulls and empty Optionals.

    // JsonNullable: explicit: null
    demoModel.setWorkEmail(JsonNullable.of(null));
    // empty optional
    demoModel.setNumberOfUsers(Optional.empty());

Will be serialized to

{
  "workEmail" : null,
  "numberOfUsers" : null
}

Should we annotate the Optional fields with @JsonInclude(Include.NON_ABSENT)? Empty Optionals would be not present in the json output.

  @JsonProperty("numberOfUsers")
  @JsonInclude(Include.NON_ABSENT)
  private Optional<@Min(2) Integer> numberOfUsers = Optional.empty();

@MelleD Some people might abuse (Optional)null to achieve basically what JsonNullable does using @JsonInclude(Include.NON_NULL) or a configuration like spring.jackson.default-property-inclusion=non_null. We could prevent such abuse of optional by checking the parameter for null.

WDYT?

BTW, I created a demo project to test such issues in isolation.

cachescrubber · 2022-02-26T11:02:38Z

Do you have a good article or similar where this setup like this?

No, it's just following the principal of least surprise. Consider for example tools like Mapstruct, which are often used to map entities to model.

welshm · 2022-02-26T16:39:57Z

I still see an issue with Jackson Mappings

The default Jackson ObjectMapper configured by spring boot will use null both for explicit JsonNullable nulls and empty Optionals.
    // JsonNullable: explicit: null
    demoModel.setWorkEmail(JsonNullable.of(null));
    // empty optional
    demoModel.setNumberOfUsers(Optional.empty());
Will be serialized to
{
  "workEmail" : null,
  "numberOfUsers" : null
}
Should we annotate the Optional fields with @JsonInclude(Include.NON_ABSENT)? Empty Optionals would be not present in the json output.
  @JsonProperty("numberOfUsers")
  @JsonInclude(Include.NON_ABSENT)
  private Optional<@Min(2) Integer> numberOfUsers = Optional.empty();
@MelleD Some people might abuse (Optional)null to achieve basically what JsonNullable does using @JsonInclude(Include.NON_NULL) or a configuration like spring.jackson.default-property-inclusion=non_null. We could prevent such abuse of optional by checking the parameter for null.

WDYT?

BTW, I created a demo project to test such issues in isolation.

IMO it's fine if it writes it out with an explicit null.

If nullable is not set to true for the field, then there is no way to know if null indicates that the field is unpopulated or intentionally null, and trying to distinguish that for an Optional seems like a case we don't need to support. The field being missing or null resulting in an Optoinal.empty() seems correct to me (since an Optional should never be null - and we should disallow that with the generated code). So I think the annotation should be JsonInclude(Include.ALWAYS) for Optionals.

If nullable is set to true, and openApiNullable support is enabled, then it will be a JsonNullable field which will allow the user to determine if it's unpopulated or intentionally null.

People trying to abuse the optional are using the system in a way it was not intended (or supported) for

MelleD · 2022-02-26T20:22:44Z

No you have nothing todo. Everybody can setup the ObjectMapper how its needed.
We set the non absent directly on the object mapper as well as the non empty value.

How I said for me it’s a big surprise to use a null as parameter value and it’s look like a bad design. I also use Mapstruct without nullable values

MelleD · 2022-02-28T09:55:43Z

Example this is our ObjectMapper

      objectMapper.configure( DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false );
      objectMapper.setSerializationInclusion( JsonInclude.Include.NON_ABSENT );
      objectMapper.registerModule( new JsonNullableModule() );

MelleD · 2022-02-28T09:59:55Z

And if this is merged we can also close this
#8690

wing328 · 2022-03-04T15:17:32Z

cc @OpenAPITools/generator-core-team as well

welshm · 2022-03-04T15:18:04Z

@cachescrubber just wanted to follow up here - anything else you think should go into this PR? Happy to contribute

CLI option for using the wrapper in the setter ?

cachescrubber · 2022-03-04T15:51:49Z

@cachescrubber just wanted to follow up here - anything else you think should go into this PR? Happy to contribute

@welshm The unit tests for the Implementation in AbstractJavaCodegen are still TODO.

I think most important is having a go! from the @generator-core-team regarding the extension in the CodegenProperty.

…:cachescrubber/openapi-generator into feature/wrapped_tyoes_and_default_values

welshm · 2022-03-23T14:42:23Z

Bringing this PR back up to master and updating - @wing328 what's the best way to seek approval to add to the CodegenProperty?

I'll aim to have the tests done this week for AbstractJavaCodegen

wing328 · 2022-03-30T10:40:40Z

This PR contains commits by the others (likely due to incorrect rebase). I would suggest you filing a new one by cherry-picking the commits authored by you.

cachescrubber · 2022-03-30T11:24:48Z

There are commits by me and @welshm - that would be alright I guess. Or are there other problems?

welshm · 2022-03-30T11:57:46Z

There are commits by me and @welshm - that would be alright I guess. Or are there other problems?

I didn't squash the commits when I rebased. I can do that, shouldn't take too much time

cachescrubber · 2022-04-25T07:56:44Z

Sorry, this PR need some love.... In the meantime, java-micronaut has added support for Optional. Curious how they implemented it. The PR is #12144.

welshm · 2022-04-25T15:13:58Z

Sorry, this PR need some love.... In the meantime, java-micronaut has added support for Optional. Curious how they implemented it. The PR is #12144.

Looks like they did what I did originally, which was just to wrap the types directly with Optional and not have a WrappedTypes feature.

Their pojo.mustache also seems much simpler since the change itself was quite small

welshm · 2022-04-25T15:15:12Z

I also need to find time to actually do some work on this in the next week or two...

welshm · 2022-06-01T22:18:57Z

Springboot and spring cloud 3 are having issues with HttpStatus (wrong version) - which I think is likely a misconfiguration of the POM but I am not familiar enough with using the milestone repo - I would guess it's a collisions with other dependencies

cachescrubber added 3 commits February 19, 2022 18:00

POC - generic wrapper data type

d35c175

Generate Samples

c18252c

POC - generic wrapper data type (skip arrys and maps for now to reduc…

939e9cb

…e the noise)

cachescrubber mentioned this pull request Feb 19, 2022

Add in Optional handling for SpringBoot POJOs #11384

Closed

5 tasks

cachescrubber added 2 commits February 19, 2022 18:51

Honor useOptional.

3493e69

Generate Samples

b173d8a

welshm reviewed Feb 19, 2022

View reviewed changes

cachescrubber added 3 commits February 19, 2022 21:22

use Optional as setter method argument

49367c9

Use Beanvalidation annotation in Optional gette retrun values

Generate Samples

cea8c75

Generate Samples

17f636a

cachescrubber added 2 commits February 20, 2022 08:28

Add Javadoc to new Properties, add equals, hasCode, and toString.

a574d46

Generate Samples

60bf823

cachescrubber added 3 commits February 20, 2022 08:35

Add JsonIgnore to imports...

fafec77

Generate Samples

7e7380f

Fix Javadoc

43c41c8

cachescrubber added 2 commits February 20, 2022 14:38

vendorExtensions.x-expose-wrapper-in-setter

1a696b9

Generate Samples

328a906

MelleD reviewed Feb 20, 2022

View reviewed changes

welshm reviewed Feb 23, 2022

View reviewed changes

docs/generators/spring.md Show resolved Hide resolved

welshm added 2 commits March 23, 2022 10:36

Merge branch 'feature/wrapped_tyoes_and_default_values' of github.com…

87f6900

…:cachescrubber/openapi-generator into feature/wrapped_tyoes_and_default_values

Update documentation and generated examples

166a60c

welshm added 4 commits June 1, 2022 15:54

Merge branch 'master' into feature/wrapped_tyoes_and_default_values

eee53f5

Update generated output

976dad2

Convert to from http to https for maven repo for SpringBoot 3

90d8fec

Update to https for maven repo

e20c617

Get springboot-3 to pass with Java 17

7fe6942

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

Are you sure you want to change the base?

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

Conversation

cachescrubber commented Feb 19, 2022 • edited Loading

PR checklist

cachescrubber commented Feb 19, 2022

welshm Feb 19, 2022

Choose a reason for hiding this comment

welshm Feb 20, 2022

Choose a reason for hiding this comment

MelleD Feb 20, 2022

Choose a reason for hiding this comment

welshm Feb 21, 2022

Choose a reason for hiding this comment

welshm Feb 19, 2022

Choose a reason for hiding this comment

welshm Feb 19, 2022

Choose a reason for hiding this comment

cachescrubber commented Feb 19, 2022

welshm commented Feb 19, 2022

cachescrubber commented Feb 19, 2022

welshm commented Feb 19, 2022

cachescrubber commented Feb 20, 2022

MelleD commented Feb 20, 2022 • edited Loading

cachescrubber commented Feb 20, 2022 • edited Loading

MelleD commented Feb 20, 2022 • edited Loading

welshm commented Feb 20, 2022

cachescrubber commented Feb 20, 2022

MelleD commented Feb 20, 2022

MelleD Feb 20, 2022

Choose a reason for hiding this comment

welshm commented Feb 23, 2022

cachescrubber commented Feb 23, 2022

cachescrubber commented Feb 23, 2022

welshm commented Feb 23, 2022

MelleD commented Feb 23, 2022

cachescrubber commented Feb 26, 2022 • edited Loading

cachescrubber commented Feb 26, 2022

welshm commented Feb 26, 2022

MelleD commented Feb 26, 2022

MelleD commented Feb 28, 2022

MelleD commented Feb 28, 2022

wing328 commented Mar 4, 2022

welshm commented Mar 4, 2022

cachescrubber commented Mar 4, 2022

welshm commented Mar 23, 2022

wing328 commented Mar 30, 2022

cachescrubber commented Mar 30, 2022

welshm commented Mar 30, 2022

cachescrubber commented Apr 25, 2022

welshm commented Apr 25, 2022

welshm commented Apr 25, 2022

welshm commented Jun 1, 2022

cachescrubber commented Feb 19, 2022 •

edited

Loading

MelleD commented Feb 20, 2022 •

edited

Loading

cachescrubber commented Feb 20, 2022 •

edited

Loading

MelleD commented Feb 20, 2022 •

edited

Loading

cachescrubber commented Feb 26, 2022 •

edited

Loading