Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record class support for ObjectMapper#updateValue #3079

Open
dodie opened this issue Mar 14, 2021 · 10 comments
Open

Record class support for ObjectMapper#updateValue #3079

dodie opened this issue Mar 14, 2021 · 10 comments
Labels
Record Issue related to JDK17 java.lang.Record support

Comments

@dodie
Copy link

dodie commented Mar 14, 2021

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.

@dodie dodie added the to-evaluate Issue that has been received but not yet evaluated label Mar 14, 2021
@cowtowncoder
Copy link
Member

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 cowtowncoder added 2.12 and removed to-evaluate Issue that has been received but not yet evaluated labels Mar 16, 2021
@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Apr 11, 2021
@cowtowncoder
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@cowtowncoder cowtowncoder added 2.14 Record Issue related to JDK17 java.lang.Record support and removed 2.13 labels Jul 15, 2021
@yihtserns
Copy link
Contributor

yihtserns commented Jan 20, 2023

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:

  • Compared to mutating a POJO field, this will definitely be slower. But whether the performance hit is in/consequential depends entirely on your specific context.
  • Don't know how well this will work with (hierarchical) nested values.

@cowtowncoder cowtowncoder added 2.15 and removed 2.14 labels Jan 22, 2023
@k163377
Copy link
Contributor

k163377 commented Apr 13, 2024

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

@cowtowncoder
Copy link
Member

@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
Copy link
Member

cowtowncoder commented Jun 15, 2024

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
Copy link
Member

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
Copy link
Member

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

@cowtowncoder cowtowncoder removed the 2.15 label Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Record Issue related to JDK17 java.lang.Record support
Projects
None yet
Development

No branches or pull requests

5 participants