FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.52k stars 1.38k forks source link

Java 8 `Optional` not working with `@JsonUnwrapped` on unwrappable type #2565

Closed yushijinhun closed 4 years ago

yushijinhun commented 4 years ago
public static class Bean {
    public @JsonUnwrapped Optional<String> value;
}

var b = new Bean();
b.value = Optional.of("str");
jackson.writeValueAsString(b);
com.fasterxml.jackson.core.JsonGenerationException: Can not write a string, expecting field name (context: Object)
    at com.fasterxml.jackson.core@2.10.1/com.fasterxml.jackson.core.JsonGenerator._reportError(JsonGenerator.java:2080)
    at com.fasterxml.jackson.core@2.10.1/com.fasterxml.jackson.core.json.JsonGeneratorImpl._reportCantWriteValueExpectName(JsonGeneratorImpl.java:248)
    at com.fasterxml.jackson.core@2.10.1/com.fasterxml.jackson.core.json.JsonGeneratorImpl._verifyPrettyValueWrite(JsonGeneratorImpl.java:238)
    at com.fasterxml.jackson.core@2.10.1/com.fasterxml.jackson.core.json.WriterBasedJsonGenerator._verifyValueWrite(WriterBasedJsonGenerator.java:894)
    at com.fasterxml.jackson.core@2.10.1/com.fasterxml.jackson.core.json.WriterBasedJsonGenerator.writeString(WriterBasedJsonGenerator.java:400)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ser.std.StringSerializer.serialize(StringSerializer.java:41)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ser.std.ReferenceTypeSerializer.serialize(ReferenceTypeSerializer.java:381)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ser.impl.UnwrappingBeanPropertyWriter.serializeAsField(UnwrappingBeanPropertyWriter.java:127)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:722)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:166)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:480)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:319)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ObjectMapper._configAndWriteValue(ObjectMapper.java:4094)
    at com.fasterxml.jackson.databind@2.10.1/com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:3404)

https://github.com/FasterXML/jackson-databind/blob/f315f1b22f47c9cfff2af9629910cb7c38c05463/src/main/java/com/fasterxml/jackson/databind/ser/std/ReferenceTypeSerializer.java#L300

I think change the expression to

_unwrapper != null && _valueSerializer.isUnwrappingSerializer()

may fix it, but _valueSerializer can be null here. Hoping anyone familiar with this can fix it.

cowtowncoder commented 4 years ago

Thank you for reporting this. I'll have to see what might be the correct behavior, given constraints; that is, whether it is possible to reliable support combination of ReferenceType and unwrapped POJOs.

cowtowncoder commented 4 years ago

Ok, so I was able to resolve the issue as reported (used AtomicReference for testing as databind can not depend on JDK8 types, but should work the same for Optional). But since I am bit uneasy on whether this might cause issues elsewhere -- there are no test failures but combination of unwrapped and reference type is not super well tests -- decided to fix only for 2.11 (next minor version) and not for 2.10.x patches. Trying to get bit more cautious on what goes in patch vs minor version.

Was also wondering if this case should instead throw an exception (use of @JsonUnwrapped on type that does not actually support unwrapping), but decided not to change semantics as no such checks have been made before.

yushijinhun commented 4 years ago

Was also wondering if this case should instead throw an exception (use of @JsonUnwrapped on type that does not actually support unwrapping), but decided not to change semantics as no such checks have been made before.

I prefer to keep the current behavior. It is useful when I do not know the exact type of the field to be unwrapped. For example:

class TimestampedValue<T> {
    @JsonProperty("_timestamp") long timestamp;
    @JsonUnwrapped T value;
}

I prefer to unwrap the value field since it's succinct. But this is not always possible, so I need this feature as a fallback.

cowtowncoder commented 4 years ago

Ok. Since it has not been blocked before, it seems reasonable to keep it that way even if this specific style of usage is not something I thought as original use case.