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

[abrudin]

After #5254 was fixed and merged, when using Jackson, arrays with uniqueItems=true gets deserialized to HashSet rather than LinkedHashSet, thus not preserving order as expected.

Even though the field (myProp) is initialized to an empty LinkedHashSet, it is promptly overwritten with a regular HashSet given that there is no deserialization-hint (@JsonDeserialize(as = LinkedHashSet.class)) on the setMyProp-method. The method invoked looks like this:

  @JsonProperty(JSON_PROPERTY_MY_PROP)
  @JsonInclude(value = JsonInclude.Include.ALWAYS)
  public void setMyProp(Set<SomeEnum> myProp) {
    this.myProp = myProp;
  }

and thus there is no way for Jackson to understand that order needs to be preserved.

This issue is present in version 5.2.1

[MelleD]

With the new version 5.3.1 the JsonNullable handling ist broken

The PR (#10241) broke the JsonNullable deserialization. If you use a JsonNullable with uniqueItems. This code will generated:

e.g.

@JsonDeserialize(as = LinkedHashSet.class)
public void setFoos(JsonNullable<Set<String>> foos) {
  this.foos = foos;
}

Exception

[.c.j.MappingJackson2HttpMessageConverter] : Failed to evaluate Jackson deserialization for type [[simple type, class FooDto]]: com.fasterxml.jackson.databind.JsonMappingException: Failed to narrow type [simple type, class org.openapitools.jackson.nullable.JsonNullable<java.util.Set<java.lang.String>>] with annotation (value java.util.LinkedHashSet), from 'setFoos': Class `java.util.LinkedHashSet` not subtype of `org.openapitools.jackson.nullable.JsonNullable<java.util.Set<java.lang.String>>`

foos:
  type: array
  uniqueItems: true
  items:
    type: string
  maxItems: 10
  nullable: true

[wing328]

@borsch can you please take a look at this issue when you've time? Thanks.

[MelleD]

@borsch it is at all possible to specify a concrete implementation in a wrapper. You can't do that with optionals either, right?

[borsch]

@MelleD I believe it doesn't. Most likely JsonNullable+Set still would have default impl - HashSet

[MelleD]

I'm not a Jackson expert either, maybe it's possible with an own serializer. I'll ask at stackoverflow. If I find a solution, I'll do a PR .

Are you doing a PR to make it work again with JsonNullable?

[borsch]

@MelleD not yet

[borsch]

@MelleD I spent some time googling possible solution how to keep both JsonNullable & LinkedHashSet, but haven't found anything yet. Here is a solution that I've mentioned before - skip @JsonDeserializer for JsonNullable fields

#11495

[MelleD]

Thanks for the quick PR.

Did you enable the Optional feature? I have never used the option, but not that in the constellation is also broken.

[borsch]

@MelleD I haven't heard about such feature. Can you give an example how to enable it? I'll test it later

[MelleD]

@borsch Ah i think is just for parameter in the API classes
https://openapi-generator.tech/docs/generators/spring

[borsch]

@MelleD yep

[welshm]

FWIW - I am trying to add Optional support to POJOs as well - I will keep this in mind if I can get agreement on adding Optional there - #11384

[MelleD]

@welshm The idea has come up a few times. What don't you like about the JsonNullable?

[welshm]

@welshm The idea has come up a few times. What don't you like about the JsonNullable?

They can both be used - in my view they solve different problems. I see three use cases.

If a value is not required to be present but null is not a valid type for that value - it would be Optional (since it cannot be set to null).

If a value has null as a valid type, then it would be JsonNullable (regardless of required or not).

If a value is required and null is not a valid type - then the value must be present so it can be the data type directly (such as String)

[MelleD]

Yes that's correct JsonNullable is just for the Tristate and in the moste cases for the json patch.
But how you would like to difference if you would like to use JsonNullable or Optional. Because currently you can just set nullable: true, correct?

[welshm]

I see the first case as when I would want Optional over JsonNullable - a value is not required to be present in the JSON schema, but if it is present it must not be null. Optional enforces that (the value is not present so the optional is empty) while JsonNullable does not (because it would allow null to be set)

schemas:
    OptionalsExample:
      required:
        - fieldTwo
      type: object
      properties:
        fieldOne:
          type: string
          description: 'A non-required string that cannot be null'
          nullable: false
        fieldTwo:
          type: string
          description: 'A required string that cannot be null'
          nullable: false
    NullablesExample:
      required:
        - fieldFour
      type: object
      properties:
        fieldThree:
          type: string
          description: 'A non-required string that can be null'
          nullable: true
        fieldFoud:
          type: string
          description: 'A required string that can be null'
          nullable: true

Assuming a patch request...

optionalsExample {
    "fieldOne":  "a_new_value",
    "fieldTwo": null      <---- Converts to Optional.empty() and is ignored in the patch because null is not a valid value, and a value is not required.
}

nullablesExample {
  "fieldThree": "another_new_value",
  "fieldFour":  null     <---- Converts to JsonNullable.of(null) and is used by the patch to set the value to null.
}

[MelleD]

A required/mandatory field should never be Optional or JsonNullable. This contradicts the principle of mandatory.

With required it can no longer be missing in the payload. But this is an important status for an ignore.
I don't see any spec-compliant distinction for a TriState right now that you have an different between Optional or JsonNullable
Missing in payload --> ignore (JsonNullable.empty())
null in the payload --> reset (JsonNullable.of(null))
value in payload --> update (JsonNullable.of("new value")

Missing in payload (required or not). Here ist the difference use direct value String vs JsonNullable /Optional
null in the payload (nullable true or false) is just how to handle the null value
String value = null;
JsonNullable = JsonNullable.of(null)
Optional = Optional.empty();

Currently you have no chance how you can decide use Optional or JsonNullable

[welshm]

A required/mandatory field should never be Optional or JsonNullable. This contradicts the principle of mandatory.

With required it can no longer be missing in the payload. But this is an important status for an ignore. I don't see any spec-compliant distinction for a TriState right now that you have an different between Optional or JsonNullable Missing in payload --> ignore (JsonNullable.empty()) null in the payload --> reset (JsonNullable.of(null)) value in payload --> update (JsonNullable.of("new value")

Missing in payload (required or not). Here ist the difference use direct value String vs JsonNullable /Optional null in the payload (nullable true or false) is just how to handle the null value String value = null; JsonNullable = JsonNullable.of(null) Optional = Optional.empty();

Currently you have no chance how you can decide use Optional or JsonNullable

Sorry you're right, I hadn't reasoned about it properly for required - but my point still stands about an optional value that cannot be null.

I think that JsonNullable can handle all cases Optional does. As long as, if nullable: false, it would be JsonNullable.undefined() as opposed to JsonNullable.of(null) - because null is not a valid value. But that can also be checked/ignored by the API implementer, it just adds another potential programmer error.

If a field is optional and not nullable - I think Java Optional is preferred as it removes the error of JsonNullable.of(null)

[MelleD]

I know what you are trying to achieve and that makes sense in many use cases. The question is whether the value null has a semantic meaning like reset.
That's why Optional makes a lot of sense with a two state and the null value have no semantic meaning.

The problem is currently the specification and yaml. You currently have no chance to use both in one generator, i.e. you have to decide either Optional or JsonNullable. As soon as you patch an endpoint with e.g. Json merge you have to (unfortunately) pass through the JsonNullable for all optional fields although null may have no meaning or you have to just get normal fields with null value.

I don't know if the specification allows additional fields in the yaml. With an additional field you could map what you have in mind.

[welshm]

Let's move the conversation to #11384 as it's tangential to this bug at this point?