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

[daberni]

Bug Report Checklist

• [x] Have you provided a full/minimal spec to reproduce the issue?
• [x] Have you validated the input using an OpenAPI validator (example)?
• [x] Have you tested with the latest master to confirm the issue still exists?
• [x] Have you searched for related issues/PRs?
• [x] What's the actual output vs expected output?
• [ ] [Optional] Sponsorship to speed up the bug fix or feature request (example)

Description

In Java CodeGeneration openapiNullable is used to determine if the special type JsonNullable should be used or not. The description of this type from the original documentation is as follows:

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

So, in a request body (especially in PATCH requests) it is utilized to signalize, if a specifc field was specified in the body or not. This can be used to just ignore any change to the property and keep the old value or to apply a new value to it.

BUT. The current implementation is using the nullable property of a field instead of the required property. So nullable should actually tell about if a field can be null or not IF it is specified. nullable tells nothing about if a field must be present or can be omitted. This is what required is for. required declares, if a field as to be present or not, independent of if it may be nullable or not.

The fix would be to swap nullable and required for the usage of JsonNullable.

openapi-generator version

6.4.0, but goes back until 5.6.0 minimum (probably since openapiNullable was implemented first).

OpenAPI declaration file content or url

Existing /3_0/petstore-with-nullable-required.yaml can be used as an example

Taking the following Pet model from the referency specification:

    Pet:
      title: a Pet
      description: A pet for sale in the pet store
      type: object
      required:
        - name
        - photoUrls
      properties:
        id:
          type: integer
          format: int64
        category:
          $ref: '#/components/schemas/Category'
        name:
          type: string
          example: doggie
          nullable: true
        photoUrls:
          type: array
          xml:
            name: photoUrl
            wrapped: true
          items:
            type: string

The Pet class would be generated like:

public class Pet {

  @JsonProperty("id")
  private Long id;

  @JsonProperty("category")
  private Category category;

  @JsonProperty("name")
  private String name;

  @JsonProperty("photoUrls")
  @Valid
  private List<String> photoUrls = new ArrayList<>();
}

Although, because only name and photoUrls are declared as required, all other properties should be wrapped in JsonNullable and name should be initialized with null as it is declared as nullable additionally.

public class Pet {

  @JsonProperty("id")
  private JsonNullable<Long> id = JsonNullable.undefined();

  @JsonProperty("category")
  private JsonNullable<Category> category = JsonNullable.undefined();

  @JsonProperty("name")
  private String name = null;

  @JsonProperty("photoUrls")
  @Valid
  private List<String> photoUrls = new ArrayList<>();
}

Generation Details

./bin/generate-samples.sh bin/configs/spring-openapi-nullable-required.yaml

Steps to reproduce

I already created a pull request: #14766

Related issues/PRs

Didn't find any existing issue to this (which is quite suprising).

Suggest a fix

I already created a pull request: #14766

I am totally aware that this is a huge breaking change so I would like to discuss this issue openly. Maybe we need some kind of flag for the new behaviour. Any critics to this behaviour are welcome.

[daberni]

Just stumbled around an additional mustache template >beanValidation where required and isNullable are also mixed up.

[welshm]

I disagree with the assessment that JsonNullable should be set based on required. For the purpose of this discussion, null is considered a value.

nullable indicates that the field may accept the null value.

required indicates that a field must be set and have a value.

If a field is both nullable and required - that value may be null or any other valid value for that field.

JsonNullable is used to determine whether a null value was explicitly set or not. If a field is not nullable - then there is no need to use JsonNullable; a null value is not permitted and can be ignored. Whether it is required or not is irrelevant.

If a field is required and nullable it does not need JsonNullable because there must be a value set. There cannot be an absence of a value.

So I think JsonNullable should be used only when a field is both nullable and not required (which is what the generator is currently doing). In your example, id and category are not declared nullable and so do not need JsonNullable.

[borsch]

@daberni in general there are 4 possible scenarion for nullable + required

1. nullable=true & required=true - means field itself is required to be present in payload, but can be set to null. No need to add JsonNullable
2. nullable=true & required=false - means that payload for this field should match to one of the below case a) can have valid value b) can be set to null with the meaning "let's set value (ex. in DB) explicitly to null" c) can be absent which mean "do nothing with this field"
3. nullable=false & required=true - same as point 1, but null does not allowed. No need to use JsonNullable
4. nullable=false & required=false - same as point 2, but b) does not applicable here since can not be set to null. No need to use JsonNullable

So based on all these cases only for point 2 we can use JsonNullabl, because that's the only case where valid value, null & undefined(not present) can happen

[daberni]

@welshm Interesting approach, I didn't see it this way so far but I understand the idea.

Probably this would be a somehow working approach, the problem is, that it is not implemented this way right now.

Currently, usage of JsonNullable is only determined by the definition of nullable. You can find this in the nullableDataType.mustache template. As far as I can see, there is no further differentiation.

If this would be combined with required this might be one way.

But additionally, which is quite confusing, the beanValidation.mustache depends on required instead of nullable.

So that this would work as you described, one would have to combine this together with openapiNullable too.

One problem which currently also arises, probably because of the combination of the above incomplete implementations, that Spring Model Binding Validation doesn't work properly either. @NotNull annotations are missing on nullable=false annotations, etc.

Also after generating the code it is no longer clear which combination of flags resulted in the current code. So as a developer I would have to write different validation and mapping logic depending on required/nullable attributes, instead of coupling my mapping logic to the present types directly, which would be way more straight forward.

I see that optimizing away the one case required=false & nullable=false where theoretically JsonNullable could be determined by if the value is null or not might be an idea. But this kinda doesn't work with real life scenarios, where only because the openapi-spec tells it this way, clients of the API wouldn't sent null although it is defined nullable=false. So as a developer one must still be able to determine, if a client did send null for a nullable=false & required=false property to throw proper errors. This is currently not possible and the source of why we started to discuss this topic.

For better understanding, It would be more verbose to couple JsonNullable to the required attribute, and @NotNull to the nullable attribute.

[welshm]

openApiNullable is respected - it's used in both nullableDataType as well as pojo - and likely should be used in beanValidation as well.

I think if the @NotNull annotations are missing or incorrect, that is likely a separate issue that should also be resolved. I would also argue that beanValidation should be updated to account for nullability, since you cannot mark a field as @NotNull if is required and nullable (since it may receive a value of null).

Also after generating the code it is no longer clear which combination of flags resulted in the current code. So as a developer I would have to write different validation and mapping logic depending on required/nullable attributes, instead of coupling my mapping logic to the present types directly, which would be way more straight forward.

I think that is likely a matter of perspective - but I agree it could definitely be more clear.

So as a developer one must still be able to determine, if a client did send null for a nullable=false & required=false property to throw proper errors

That's more of a discussion on whether the generated code should be done to allow determining violations of the API contract (explicitly sending a null when it's not nullable and not required) or whether the generated code should enforce API conformance (if you do send null, the value is ignored because it's not a valid value in the contract). Are you able to determine the error if nullable=false & required=true currently? Since that would also be a violation to send an explicit null.

In my opinion, JsonNullable and @NotNull are both connected to the relationship between required and nullable and not connected to just one of the properties, but always both.

One way to resolve this would be to introduce a flag in the generator that could produce JsonNullable in the pattern you desire - and then the user of the generator can decide what pattern they prefer. This would have the added benefit of also being backwards compatible for users of the generator. generateJsonNullableForAllNonRequiredFields - what do you think?

[daberni]

Usage of openApiNullable is absolutely fine, and is set to true by default anyway.

The issue is, that @NotNul is bound to required, and JsonNullable is bound to nullable only. This is not implemented as highlighted previously ("not required and nullable"). So these two topics would need to be addressed anyway. These changes would also be backwards incompatible, as maybe many developers now already rely on the unintuitive implementation or adopted their specifications in a way to meet the desired result (as we did so far).

Regarding validation, because it is implemented incomplete right now it is not possible to validate correctly at the moment.

Having a property which is nullable = false andrequired = false (e.g. a PATCH request for non-nullable property), a client can send null for this without any issues. In the backend, one would have to assume that the property might not be passed at all, because there is no differentiation possible because of missing JsonNullable, and also javax bean validation doesn't work because of missing @NotNull annotation.

I don't see why JsonNullable and @NotNull would require do be connected to both required and nullable at the same time, just for the sake of optimizing one case where JsonNullable may be not absolutely necessary. But I get the idea behind this.

The PR was just to highlight a radical change on how I would see the correct implementation of required and nullable. It's nothing which would ever could be merged that way because it would break every existing code generation. Some configuration for a backwards compatible change is definitely necessary.

The question is, how do we want to specify the behaviour.

1. How should beanValidation work. Imho this is independent from JsonNullable and just stumbled onto it when debugging it. The current beanValidation.mustache doesn't work right now for some cases. I would prefer to bind this only to the nullable property, because I don't see why the simple validation of nullability should be bound to the JsonNullable type. Even in the desired behaviour described above, this should not be necessary.

This would be independent from the openapiNullable config but would also need some backwards compatible migration I guess (extend useBeanValidation option)

2. Should the existing JsonNullable usage be fixed in a way to represent the desired behaviour above? Because currently JsonNullable is only bound to nullable. Then a combination would be necessary with fixes to nullableDataType.mustache and pojo.mustache

3. Should there be the option to configure the generator to use a more explicit and verbose version, where nullable and required do not influence each other and have their well defined associations to @NotNull and JsonNullable.

I would definitely vote for 3) from our side (obviously :D) because 1) and 2) wouldn't matter in that case. But I would also see a clear definition and implementation for 1) and 2).

Maybe utilizing the existing openapiNullable option would be better, because all the rest is depending on it anyway. Extending it with custom values instead of true/false would imho work best.

So keeping openapiNullable with true/false for backwards compatibility but adding new values like:

• true (deprecated)
• false (deprecated)
• none (= false)
• combined (= true, default) = required & nullable will be respected
• requiredOnly

[Djerem-]

@borsch Am I correct that the behavior you describe, if validation is also considered, should be:

• Case 1: required=true & nullable=true We can accept omission as a null value and consider it to be always provided. -> No use of JsonNullable and no validation
• Case 2: required=true & nullable=false The absence of a value can be considered equivalent to a provided null value. -> No use of JsonNullable but use of @NotNull for validation.
• Case 3: required=false & nullable=true We need to know if a value is provided (e.g. for patch) but we can accept a null value. -> Use JsonNullable but no validation
• Case 4: required=false & nullable=false We need to know if a value is provided (e.g. for patch) and it cannot be a null value. -> Use JsonNullable and use of a @NotNullIfPresent** annotation

** I do not know the validation behavior of JsonNullable but, in this case, we want it to be valid on: isPresent() == false || get() != null

[ddnyer]

@Djerem- To perform correctly all validations you need to use JsonNullable for required fields as well. Else you can not detect when a required field is missing. Consider that a missing required field is equivalent to this field being null is not correct. But for those required fields, the getter/setter can use the raw type and hide the JsonNullable to the user, it's only needed for bean validation and does not need to be exposed.

• Case 1: required=true & nullable=true We need to validate that the field is present but the developers should directly use the raw type -> Use JsonNullable to store the property, use the raw type on getter/setter and use a @Present annotation on the property (not on the getter) for validation
• Case 2: required=true & nullable=false We need to validate that the field is present but the developers should directly use the raw type -> Use JsonNullable to store the property, use the raw type on getter/setter, use a @Present annotation on the property (not on the getter) and a @NotNull on the getter for validation

Obviously the @Present annotation checks that the JsonNullable has a value (null or not).

That said for retrocompatibility this strict validation for required fields should probably be optional with a parameter to enable it.

[MelleD]

From my point of view, you/we should basically think again about the JsonNullable. In my use cases it would make sense to return Optional everywhere except for the PATCH endpoints and I think that's exactly what the JsonNullable is for. IMHO the TriState only makes sense for PATCH otherwise it's not needed.

See https://github.com/OpenAPITools/jackson-databind-nullable. "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"."

Currently i habe no other good RESTful usecase for JsonNullable

[welshm]

I think if it's valid for PATCH it's valid for POST as well. Some projects just use POST instead of PATCH - even it it's not "correct"

[daberni]

I absolutely agree that it is primarily relevant in a PATCH request where one can define, it is important to know if the value was passed or not.

But you can't decide on this based on the HTTP method but you have to define something different for that, and for me this is the required attribute of the properties (or the required-list).

If it is "not required" this implies that it is "optional". And in an optional manner I must be able to check if it was present or not.

If it is "not optional", it would be okay to me that the Spring implementation then uses the default value of the property if it is not present. And then an additional validation like "not null"/"nullable" would kick in. But this way you could clearly differentiate between these two configurations

[daberni]

As a sidenote: we have already adopted the mustache templates in this manner and are very happy with this approach. It would be interesting if someone else would have different requirements to this. It would be nice if we wouldn't have to maintain these templates again all the time (reintegrate with upstream changes) and in my opinion others may would benefit from this approach too.

I think the main aspect would be, when one doesn't define the "required-list" completely, every property would be JsonNullable and break a lot of stuff immediatelly. It is a bit cumbersome that everything has to be specified again in the required list, but imho that's the more correct way anyway. I don't know if there would be any simpler solution to this.

[MelleD]

I think if it's valid for PATCH it's valid for POST as well. Some projects just use POST instead of PATCH - even it it's not "correct"

Yeah you are right, that why I said in good RESTful Api(s) ;)

@daberni I think you need the nullable + required field, if you would like to have a general solution for every operation. If you say it's only for PATCH (maybe POST) then you have other solutions.

See discussion here @welshm have a good conclusion

How I said in our use cases, I would like to have everywhere Optional and for PATCH JsonNullable. There you could decide if you like to use required or nullable.

[daberni]

We are already using the suggested approach for every operation, not only PATCH.

What is the benefit of using Optional by default everywhere and JsonNullable for PATCH only? Functional wise they reflect the same thing, saying if the property is present or not. I don't see how it would improve the behaviour when we have 2 options for the same thing? Maybe I don't understand it yet and you could elaborate this?

Or would you couple Optional to nullable, and JsonNullable to required? Or what would your approach be?

[MelleD]

2 options for the same thing?

Thats not correct. @daberni Optional and JsonNullable are complete different things. One is two state the other is a tri state object. Optional present/not present JsonNullable present/not present / present with null value

Also with Collections Optional make no sense, because you can use a empty List for that. But a JsonNullable with Collection can make sense if you use it for patch, because it has a different semantic.

Look into the JSON Merge Patch specification. So how the name is calling its for PATCH :)... Again link to docs: See https://github.com/OpenAPITools/jackson-databind-nullable. "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"."

Easy and quick solution: use it just for PATCH (maybe POST) operation. More general solution see @welshm closed PR. You need nullable + required to create all cases

e.g. required: true --> no Optional and no JsonNullable nullable: true required: false: --> JsonNullable nullable: false required: false: --> Optional

@welshm correct?

But how I said for a GET/DELETE/PUT JsonNullable make not so much sense :)

[MelleD]

@daberni more clear?

[jspetrak]

Persisting in 7.0.0 beta

[gnuemacscoin]

I would like to offer another perspective. To me it makes sense for the outermost null to be tied to the object's required list, the way it is now, since required precedes nullable in the schema graph. The way it is, it integrates well with MapStruct's NullValuePropertyMappingStrategy.IGNORE precisely for this PATCH use-case. All that's missing is an Optional-equivalent wrapper for nullable schemas. The current JsonNullable awkwardly ties up required with nullable instead of aiming for something more orthogonal.

[MelleD]

I don't understand how NullValuePropertyMappingStrategy.IGNORE can help you with a tristate for a PATCH use case.

[gnuemacscoin]

When merging one object into another (using void mapper methods and a @MappingTarget parameter), you tell MapStruct to skip all properties that are null in the source object. So when those null properties correspond to missing JSON keys - you get RFC 7386 semantics. The actual JSON null value needs to be represented somehow, which is what this Optional-like wrapper would be for.

[MelleD]

I don’t get what you would like to say. Please make an example with code. I don’t see how you can achieve a tristate semantic.

But also a little bit strange that we should solve a issue with extra big dependencies after a mapping step. IMHO the semantic is totally clear how to handle JsonNullable and Optional.

[jspetrak]

@gnuemacscoin When the property is null in the source object, it means that it should be set to null. When it is not present, than it should be not modified. That is why JsonNullable is used. To provide isPresent() function and distinguish the third state.

[MelleD]

Yes and the third state (reset state) make just sense for PATCH ( maybe sometimes for POST not sure).

[gnuemacscoin]

public class Opt<T> {
    public @Nullable T value;
}

public class Target {
    public @Nullable String prop;
    public @Nonnull String nonnullProp = ".*";
}

public class Source {
    public @Nullable Opt<String> prop;
    public @Nullable String nonnullProp;
}

@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE)
public interface OptMapper {
    default <T> @Nullable T from(@Nonnull Opt<T> opt) {
        return opt.value;
    }
    void patch(@MappingTarget @Nonnull Target target, Source source);
}

produces the following

public class OptMapperImpl implements OptMapper {

    @Override
    public void patch(Target target, Source source) {
        if ( source == null ) {
            return;
        }

        if ( source.nonnullProp != null ) {
            target.nonnullProp = source.nonnullProp;
        }
        if ( source.prop != null ) {
            target.prop = from( source.prop );
        }
    }

}

[MelleD]

@gnuemacscoin now I just see two states. Where is the reset state?

And again I don’t like to have a mapping step from dto to dto.

[jspetrak]

@MelleD As I see it, the if(source.prop != null) is the same logic as JsonNullable.isPresent but expressed with null value instead of method call. The from() method extracts value from Opt only if the Opt object is present and can be null. In fact, the Opt class is actually JsonNullable equivalent here.

[gnuemacscoin]

In fact, the Opt class is actually JsonNullable equivalent here.

That's what I at first thought the role of JsonNullable was, but JsonNullable wraps both null and undefined (missing key), making the outermost java null unused, and making @Nullable JsonNullable<T> illegal.

[gnuemacscoin]

And again I don’t like to have a mapping step from dto to dto.

In my example Source would be a patch DTO, and Target would be an @Entity class.

[MelleD]

I need a closer look, but I don't recognize the reset case when it is set to null.

But in the end, you create another wrapper class like jsonnullable. Instead of jackson, mapstruct does the translation. The semantic is a little different and reminds me more of a bad practice from optional wrapper. Even with an optional you can get a tristate with null, as I said, bad practice. The sense of JsonNullable and optional is that you no longer want to have null states but rather work with objects. See good article from Parlog https://medium.com/97-things/using-optional-nowhere-somewhere-or-everywhere-b1eb5645eab5

making the outermost java null unused, and making

That’s perfectly the sense of JsonNullable and Optionals not to handle nulls. If you like to handle nulls you and work with nullable you can disable JsonNullable and also Optionals. Then you don’t need it.

For this reason I see no advantage in the suggestion here with mapstruct. We can do the same things with Optional/JsonNullable when we handle required and nullable flags.

PS: I have a jsonnullable mapper with mapstruct to map the status into the domain objects. It's perfect for that. And that’s a perfect mapping use case.

What is the issue with the proposal?

[gnuemacscoin]

Anyway here's a MapStruct configuration grounded in reality of the current JsonNullable.

@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE)
public interface OptMapper {
    @Condition
    default <T> boolean isPresent(@Nonnull JsonNullable<T> nullable) {
        return nullable.isPresent();
    }
    default <T> @Nullable T from(@Nonnull JsonNullable<T> nullable) {
        return nullable.get();
    }
    default <T> @Nonnull JsonNullable<T> toJsonNullableFrom(@Nullable T value) {
        return JsonNullable.of(value);
    }
    void patch(@MappingTarget SomeEntity entity, SomeDto dto);
    SomeDto from(SomeEntity entity);
}

public class OptMapperImpl implements OptMapper {
    @Override
    public void patch(SomeEntity entity, SomeDto dto) {
        if ( dto == null ) {
            return;
        }
        if ( dto.notNullable != null ) {
            entity.notNullable = dto.notNullable;
        }
        if ( isPresent( dto.nullable ) ) {
            entity.nullable = from( dto.nullable );
        }
    }
    @Override
    public SomeDto from(SomeEntity entity) {
        if ( entity == null ) {
            return null;
        }
        SomeDto someDto = new SomeDto();
        someDto.notNullable = entity.notNullable;
        someDto.nullable = toJsonNullableFrom( entity.nullable );
        return someDto;
    }
}