FasterXML / jackson-databind

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

Record class support for ObjectMapper#updateValue #3079

Open dodie opened 3 years ago

dodie commented 3 years ago

updateValue is a convenient way to update the fields of a POJO and it even supports changing hierarchical values.

Currently when the object to be updated is an instance of a Record class it throws an exception, rightfully because the fields of the record are immutable:

record Person(String name, Integer age);
Person original = new Person("John", 15);
new ObjectMapper().updateValue(originalRecord, Map.of("name", "Jane"));
// ↑ JsonMappingException: Can not set final java.lang.String field a.b.c.Person.name to java.lang.String 

Would it be feasible for the updateValue to support Record classes in such a way that it would not update the object in place, but instead it would return a new object with the updated values?

For example:

record Person(String name, Integer age);
Person original = new Person("John", 25);
Person updated = new ObjectMapper().updateValue(originalRecord, Map.of("name", "Jane"));

// the values of `original` would remain unchanged
// Person[name="John", age=25]

// the data of the `updated` record would be based on the `original` record and the updates
// Person[name="Jane", age=25] 

Based on its signature this behavior is already in place for arrays.

cowtowncoder commented 3 years ago

Sounds reasonable, and agreed, copy-if-cannot-update is conceptually expected. I hope to look into this in near future.

One thing that would help a bit would be to create a small unit test to show expected behavior, just to verify the fix. It's not a ton of work to create by whoever works on this, but could help a little still; and would also work as verification of the fix.

cowtowncoder commented 3 years ago

Sigh. Testing Record feature from Java 8 baseline is a royal PITA, partly due to having to pass "enable-review" flag on some JDKs (14 and 15) and ABSOLUTELY MUSTING NOT TO DO SO for others (16). And "--" being illegal in XML comments so commenting out in pom.xml no-go as well.

But I am able to reproduce this. Need to figure out how to indicate that "POJO" deserializer for Record shall not be applied in case of underlying Record type.

cowtowncoder commented 3 years ago

Note to self: while in theory we could try:

    public Boolean supportsUpdate(DeserializationConfig config) {
        if (ClassUtil.isRecordType(_beanType.getRawClass())) {
            return Boolean.FALSE;
        }
        return Boolean.TRUE;
    }

that would not work for root values which simply try calling deserialize() method that takes existing instance. So that path will need to be blocked too.

Which at first seems fine except that now it would also need to access existing values, somehow, to merge.

And in fact same is true for both paths since it is not possible to just create an empty instance anyway: values of existing properties are needed.

cowtowncoder commented 3 years ago

Starting to think that we actually should create separate RecordDeserializer and NOT reuse BeanDeserializer.

Or, possibly, make RecordDeserializer extend BeanDeserializerBase. That might make more sense. Also not sure if and how to support Array shape (serialize/deseriaize as array).

yihtserns commented 1 year ago

An alternative:

record Person(String name, Integer age) { }
Person original = new Person("John", 15);

ObjectMapper mapper = new ObjectMapper();
ObjectNode jsonObject = mapper.convertValue(original, ObjectNode.class);
jsonObject.put("name", "Jane");

Person updated = mapper.convertValue(jsonObject, Person.class); // Person[name=Jane, age=15]

Caveat:

k163377 commented 7 months ago

A similar problem was reported for KotlinModule. When reading with updateValue, the creator seems to be ignored. https://github.com/FasterXML/jackson-module-kotlin/issues/766

cowtowncoder commented 6 months ago

@yihtserns while users can use convertValue(), I don't think it (or underlying machinery) can be used for updateValue() unfortunately: this because serialization aspects needed are not available from deserialization side.

Another thought: while we could attempt to access field values via canonical accessors, there might be problems with potential annotation-based renaming (or NamingStrategy). At least if trying to use convertValue() like process.

So on short term I don't see this as being plausible to implement. Then again, I am often proven wrong when I declare something impractical -- so consider this a challenge of a kind :)

cowtowncoder commented 5 months ago

Just realized something important: although full convertValue() cannot be used (as it requires deserializer to invoke serializer, something it has no access to; unlike ObjectMapper that can access both), there IS access to underlying getter-method/field that is part of logical property. So it just might be possible to -- in theory at least -- to have "updating deserializer" that would read existing values of Record (or even POJO) properties of an instance, and then use those if input does not contain new value.

Not sure how practical this would be to implement, but at least it would be feasible.

JooHyukKim commented 5 months ago

If I understand correctly, ObjectReader._bind() method is the underlying implementation? If so, knowing that Kotlin module is also affected, this problem may be effectively handled only after structural change cross modules? (Kotlin/Scala)

cowtowncoder commented 5 months ago

@JooHyukKim Not necessarily, no. This is cross-cutting concern and if databind handled using of getter-method/field to access information, it should work with Kotlin and Scala modules too (unless they override some aspects etc), without requiring additional work.