Openapi-generator: [Java] [Spring] provide optionals also for model fields

[atomfrede]

Description

Follow up of discussion with @cbornet https://twitter.com/atomfrede/status/1051787697201827840

When setting useOptional=true only query parameter are affected. All model fields are are not affected.

Why is that useful? When using json-patch-merge a field not send is different from a value explicitly set to null. Where in the first case the field should not be updated in the second case it means the field should be removed. Currently this is not possible as both a not provided field and field set to null will result in the field being null. When the model fields are optional a field being null would mean it was not send and an empty optional would mean it was send but set to null.

Example

• {a:b,c:d} -- Patch/Merge {c:null} --> {a:b} (c is removed)
• {a:b,c:d} -- Patch/Merge {c:""} --> {a:b,c:""} (c is set to emtpy string)

openapi-generator version

3.3.0

Related issues/PRs

Couldn't find any.

Suggest a fix/enhancement

Optional fields (or maybe optional fields when using patch) and setting useOptional=true should make a difference between value not set (==null) and value set to null (==empty optional) to support json-patch-merge

[cbornet]

Actually there's a nullable field option in OASv3 and we support x-nullable in swagger v2.
So we could probably generate an Optional when this option is true.

[cbornet]

Also I don't think it should be mixed with useOptional since useOptional is a completely flawed option : you should not put Optional in method arguments.

[cbornet]

What does Optional give when serializing ?

[atomfrede]

Agree more fine grained configuration via x- is a good idea. When using Jackson with the java8 module an empty optional will be serialized as null by default so still not what we need. Maybe we need to handle that for the Patch Part manually

[cbornet]

So here are my thoughts:

• Optional in Jackson has a behavior that allows to distinguish between "null" value and "not present"
• java.util's Optional is not serializable (Guava's is), is not available for jdk<8 and shouldn't be used in POJO fields or parameters.
• Also an Optional should never be null
• Optional in itself doesn't carry the meaning we want : it says the value can be present or absent but not null (ie : it replaces null by "absent"). But what we need is an object that can be present/absent and if present can be something or null (or Optional?). So I propose to use a distinct generic that can be present/absent and null/something.

public class PresenceAware<T> implements Serializable {

    private final static PresenceAware<?> ABSENT = new PresenceAware<>(null, false);

    private T value;

    private boolean isPresent;

    private PresenceAware(T value, boolean isPresent) {
        this.value = value;
        this.isPresent = isPresent;
    }

    public static <T> PresenceAware<T> absent() {
        @SuppressWarnings("unchecked")
        PresenceAware<T> t = (PresenceAware<T>) ABSENT;
        return t;
    }

    public static <T> PresenceAware<T> of(T value) {
        return new PresenceAware<>(value, true);
    }

    public T get() {
        return value;
    }

    public boolean isPresent() {
        return isPresent;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj) {
            return true;
        }

        if (!(obj instanceof PresenceAware)) {
            return false;
        }

        PresenceAware<?> other = (PresenceAware<?>) obj;
        return Objects.equals(value, other.value) &&
                Objects.equals(isPresent, other.isPresent);
    }

    @Override
    public int hashCode() {
        return Objects.hash(value, isPresent);
    }

    @Override
    public String toString() {
        return this.isPresent
                ? String.format("PresenceAware[%s]", value)
                : "PresenceAware.absent";
    }
}

Then in the DTO

  private PresenceAware<String> name = PresenceAware.absent();

  public Pet name(String name) {
    this.name = PresenceAware.of(name);
    return this;
  }

  public PresenceAware<String> getName() {
    return name;
  }

  public void setName(PresenceAware<String> name) {
      this.name = name == null ? PresenceAware.absent() : name;
  }

  @JsonGetter("name")
  private Optional<String> _getName() {
    return name.isPresent() ? Optional.ofNullable(name.get()) : null;
  }

  @JsonSetter("name")
  private void _setName(String name) {
    this.name = PresenceAware.of(name);
  }

Tests that show it works as expected

@RunWith(SpringRunner.class)
@SpringBootTest
public class PetTest {

    @Autowired
    private ObjectMapper mapper;

    @Test
    public void sometest() throws IOException {
        mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
        Pet pet;
        String petStr;

        pet = mapper.readValue("{\"name\": null}", Pet.class);
        assertTrue(pet.getName().isPresent());
        assertNull(pet.getName().get());

        petStr = mapper.writeValueAsString(pet);
        assertTrue(petStr.contains( "\"name\":null"));

        pet = mapper.readValue("{}", Pet.class);
        assertFalse(pet.getName().isPresent());
        assertNull(pet.getName().get());

        petStr = mapper.writeValueAsString(pet);
        assertFalse(petStr.contains( "\"name\""));

        pet = mapper.readValue("{\"name\": \"foo\"}", Pet.class);
        assertTrue(pet.getName().isPresent());
        pet = new Pet();
        assertFalse(pet.getName().isPresent());
        assertNull(pet.getName().get());

        pet.name("foo");
        assertTrue(pet.getName().isPresent());
        assertEquals(pet.getName().get(), "foo");
        assertEquals(pet.getName().get(), "foo");

        petStr = mapper.writeValueAsString(pet);
        assertTrue(petStr.contains( "\"name\":\"foo\""));

        pet = new Pet();
        assertFalse(pet.getName().isPresent());
        assertNull(pet.getName().get());

        pet.name("foo");
        assertTrue(pet.getName().isPresent());
        assertEquals(pet.getName().get(), "foo");
    }
}

Notes:

• Here, name can never be null
• I called it PresenceAware to clearly distinguish it from Optional although they look similar. Nullable could also be a choice since it would echo the nullable: true option. Tell me your preference or other proposals.
• It would probably be cleaner to replace the JsonGetter/JsonSetter by a module. I'll see what can be done.
• The generic and the module could be part of a dedicated lib (ideally a FasterXML one !)

cc @cowtowncoder (twitter follow-up)
cc Java tech comitee : @bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @jeff9finger @wing328

[atomfrede]

Looks good I think. How would you model the PresenceAware in the api? Per x parameter at the model value (as it is quite special)

[cbornet]

It corresponds to the nullable/x-nullable option which was defined for this exact use-case.

[cowtowncoder]

Sounds legit to me, at a high level. One implementation-side thing wrt Jackson support is that this type could probably use much of support that Optional has (namely, JavaType for new class being a Referencetype) to make support via jackson datatype module quite simple (I think).

[cbornet]

Yes. I've begun to look at ReferenceType to write a module but I'm a bit lost...

[cowtowncoder]

@cbornet I think AtomicReference handling might help: type becomes ReferenceType and deserializer/serializer extend default reference type (de)serializer. And it's most up-to-date example; Guava's Optional being probably second best example.

[cbornet]

@cowtowncoder thanks for your guidance. I could write the ReferenceType based wrapper module by mimicking the JDK8 one. Now all there is to do is put it in an independent lib and find the correct naming for the wrapper class (hardest thing to do).
My proposals are:

• Nullable --> this recalls the OAS option name
• JsonNullable --> this is nice to recall that it's linked to JSON having three "states": null, undefined, value (which translates to Json-Schema {"type": [xxx, null], "required": false). The method for the absence of value could even be called undefined() instead of absent or empty.
• PresenceAware --> more generic, but maybe too generic...

Can you vote for your preferred one or propose a better one ?

[jeff9finger]

I would prefer JsonNullable. @Nullable it also a javax annotation and the use of "Nullable" might cause confusion with that annotation

[atomfrede]

I think JsonNullable is perfect. No confusion and not too generic

[cbornet]

Making progress on the module. I have some interrogations about the behavior:

• Should JsonNullable.undefined never be serialized or should it depend on the JsonInclude.Include.NON_NULL/NON_ABSENT serialization config (like Optional) ? I think the former one but I'd appreciate your opinion on that.
• Also how should JsonNullable.undefined be serialized when inside a collection (which should be avoided in the first place but can't be controlled) ? I believe to null ?
• Eventually, to what should a null inside a collection be deserialized (again should be avoided but can't be controlled) ? I believe to JsonNullable[null] (not JsonNullable.undefined) ?

[cbornet]

First version of the module here : https://github.com/OpenAPITools/jackson-databind-nullable
I still have things not working as I would want to:

• When using a constructor (JsonCreator) with JsonNullable args, not present args are deserialized as JsonNullable[null] where I would have expected JsonNullable.undefined. See failing CreatorTest. For me this is the biggest issue.
• @JsonUnwrapped doesn't work. I'm not sure why you would use a JsonNullable there so maybe it can just be ignored. See failing JsonNullableUnwrappedTest
• Non bean-property JsonNullable.undefined serializes to null. I think it's OK because of collections but I'm not sure if there are other use cases where it should instead not serialize. See failing JsonNullableSimpleTest
• Non bean-property "" deserializes to JsonNullable[null] instead of JsonNullable.undefined. See failing JsonNullWithEmptyTest

@cowtowncoder would you help me with that ? (esp. the JsonCreator issue)

[cowtowncoder]

Ok let's see...

1. On creator, I think value to use for null is fetched using JsonDeserializer.getNullValue()
2. @JsonUnwrapped is designed to just work on POJOs, and although I could kind of see how one might expect it to work elsewhere... it won't. So I would not worry about that yet.
3. There is no way to suppress serialization of anything other than property value (root value or element in JSON Array). It has been suggested there should be way but as things are there isn't.
4. I think coercion from "" is handled as JsonDeserializer.getEmptyValue()?

So I think you may need to override methods from default/standard reference type deserializer, for (1) and (4).

[cbornet]

1. On creator, I think value to use for null is fetched using JsonDeserializer.getNullValue()

Indeed. But getNullValue is also called to deserialize "null" in bean properties so it has to return JsonNullable[null].

1. I think coercion from "" is handled as JsonDeserializer.getEmptyValue()?

Unfortunately no, it gets handled as JsonDeserializer.referenceValue(null) (which also gets called when deserializing non bean property "null")

[cbornet]

For 4., _valueDeserializer.deserialize and the value deserializer deserializes both "" and "null" to null. So maybe I can override ReferenceTypeDeserializer::deserialize to detect empty strings before calling the value deserializer ?

[cowtowncoder]

Ok. Yes, (1) was retrofit to have some way to pre-populate missing entries as Creator must be passed something.

As to (4), handling should still be controlled by ReferenceTypeDeserializer, which is bit easier -- nulls are a pain just because historically deserializer/serializer was NOT called since it was (initially) possible to just have general null handling, and keep (de)serializers simpler (as they needed not check for null).
So I think it's coercion of nulls that is more difficult to customize.

[cbornet]

For (1), could PropertyValueBuffer::_findMissing call getEmptyValue instead of getNullValue or is there a reason not to do so ?

For (4), I'm not sure I understand what you recommend. I've tested this:

    @Override
    public JsonNullable<Object> deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
        JsonToken t = p.getCurrentToken();
        if (t == JsonToken.VALUE_STRING && !_fullType.isTypeOrSubTypeOf(String.class)) {
            String str = p.getText().trim();
            if (str.isEmpty()) {
                return JsonNullable.undefined();
            }
        }
        return super.deserialize(p, ctxt);
    }

which seems to work. Is it the correct way to do it ?

[cbornet]

Forget my comment about getEmptyValue, it's returning "" for StringDeserializer so it's not fit for _findMissing. I've tried various things for JsonCreator including playing with ValueInstanciator but couldn't make it. Anyway I think there's no reason to force to provide a JsonNullable in a constructor since it normally represents a non-required value. So it can be described as a limitation of this reference type.