Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Spring - update nullable and required validation #1308

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

frantuma
Copy link
Member

Replaces and includes #1305

This PR provides a thorough update of spring generator logic and options handling validation scenarios where schema properties are defined as required or nullable or any combinations of these keywords.

This is (also) related to #1295 and #1291

Current status (before this PR, Codegen <=3.0.61 / Generators <=1.0.51)

Before this PR, validation for properties with nullable or required keywords or any combination of these was based on the following:

  • by default all properties defined as required are annotated with @NotNull. This is not correct for cases where a property is both required and nullable.
  • Java - fix @NotNull resolving from required/nullable #1291 introduced option useNullableForNotNull (true by default). When set to true all properties not defined with nullable=true are annotated with @NotNull. This is not correct for cases where a property is nullable but also required. This is also possibly too strict (even if correct according to OpenAPI / Json Schema specification) for properties without a nullable keyword. This rule is the one applied by default in Codegen 3.0.61+ / Generators 1.0.51+

Updated behavior

Both behaviors above are limited and not correct, as there are 4 possible combinations:

  1. nullable + required
  2. nullable + not required
  3. not nullable + required
  4. not nullable + not required

Note that scenario 4. not nullable + not required is the one occurring when no nullable nor required keyword is applied to a property; in this case OpenAPI / JSON Schema mandate that the property CANNOT be null if a type is defined, therefore in a strict mode this should be considered in validation.

Scenario 1. nullable + required and 4. nullable + required are the most "problematic" in context of a deserialization and representation in Java, as they involve distinguishing between "non absent" and null values.

Specifically for Scenario 1. nullable + required, this is only achievable by wrapping the type into a container (JsonNullable) and applying custom validation. JsonNullable has been chosen instead of e.g. Optional for various reasons, detailed and discussed in project README, OpenAPI Generator issue among others)

A custom validator has been added as validation provided by jackson-databind-nullable is not working as expected (see e.g. OpenAPITools/openapi-generator#14766, OpenAPITools/openapi-generator#14765, OpenAPITools/openapi-generator#11969 (comment), OpenAPITools/jackson-databind-nullable#17

Scenario 4. is addressed using Jackson (de) serialization annotations while Scenario 3. is addressed simply using @NotNull

validationMode option

This PR introduces option validationMode (limited to spring generator) which allows to choose the validation strategy. Default mode is strict.

  • strict (default)

    1. nullable + required: JsonNullable + custom validation
    2. nullable + not required: no validation annotations
    3. not nullable + required: @NotNull
    4. not nullable + not required: @JsonInclude(NON_ABSENT) + @JsonSetter(nulls = FAIL)`
  • loose

    1. nullable + required: JsonNullable + custom validation
    2. nullable + not required: no validation annotations
    3. not nullable + required: @NotNull
    4. not nullable + not required: no validation annotations

    loose mode behaves like strict except for scenario 4 not nullable + not required where no validation is defined; this is for cases where the correct behavior when no nullable nor required keyword is applied to a property is considered too strict.

  • legacy

    1. nullable + required: @NotNull
    2. nullable + not required: no validation annotations
    3. not nullable + required: @NotNull
    4. not nullable + not required: no validation annotations

    legacy mode provides the same validation applied by default before Java - fix @NotNull resolving from required/nullable #1291

  • legacyNullable

    1. nullable + required: no validation annotations
    2. nullable + not required: no validation annotations
    3. not nullable + required: @NotNull
    4. not nullable + not required: @NotNull

    legacyNullable mode provides the same validation applied by default after Java - fix @NotNull resolving from required/nullable #1291 and before this PR.

We are asking the community to provide feedback to this change and we welcome and ready to further refinement and/or additional options.

micryc and others added 2 commits August 26, 2024 21:46
@frantuma frantuma merged commit c1db428 into master Aug 27, 2024
6 checks passed
@frantuma frantuma deleted the spring-required-nullable branch August 27, 2024 05:14
@pe-st
Copy link

pe-st commented Sep 3, 2024

Unfortunately (as stated above) this fixes the problem only for Spring, for me (language=java, library=resteasy) the bug #1295 is still present. Or did I miss something?

BTW: I had to search the web to see how to configure for Spring to try it out (the @NotNull annotation vanishes, but my project of course lacks all the spring dependencies, so it doesn't compile):

                            <language>spring</language>
                            <configOptions>
                                <library>spring-boot3</library>
                            </configOptions>
                            <additionalProperties>
                                <additionalProperty>jakarta=true</additionalProperty>
                                <additionalProperty>validationMode=legacy</additionalProperty>
                            </additionalProperties>

@okotsopoulos
Copy link

Hi @frantuma -- we are running into a new issue upgrading from 3.0.61 -> 3.0.62 in our open source Spring Boot project (see linked PR above). Our generated code fails to compile with errors like the following:

/github/workspace/build/generated-src/swagger/src/main/java/bio/terra/model/ResourcePolicyModel.java:10: error: package io.swagger.configuration does not exist
import io.swagger.configuration.NotUndefined;
                               ^
/github/workspace/build/generated-src/swagger/src/main/java/bio/terra/model/ResourcePolicyModel.java:21: error: cannot find symbol
@NotUndefined
 ^

To work around we've elected validationMode=legacy for now, but are we missing something that's leading to this compilation error in default mode?

@frantuma
Copy link
Member Author

frantuma commented Sep 8, 2024

@okotsopoulos

Would you be able to share the spec causing the issue along with configuration?

@frantuma
Copy link
Member Author

frantuma commented Sep 8, 2024

Unfortunately (as stated above) this fixes the problem only for Spring, for me (language=java, library=resteasy) the bug #1295 is still present. Or did I miss something?

BTW: I had to search the web to see how to configure for Spring to try it out (the @NotNull annotation vanishes, but my project of course lacks all the spring dependencies, so it doesn't compile):

                            <language>spring</language>
                            <configOptions>
                                <library>spring-boot3</library>
                            </configOptions>
                            <additionalProperties>
                                <additionalProperty>jakarta=true</additionalProperty>
                                <additionalProperty>validationMode=legacy</additionalProperty>
                            </additionalProperties>

@pe-st the fix is indeed only for spring at this moment, a similar change for all java generators needs more effort, it is planned with no fixed ETA atm. Re-opening #1295 to track full java update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants