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

Deserializing to single-value Record from scalar values #2980

Closed
bfncs opened this issue Dec 11, 2020 · 18 comments
Closed

Deserializing to single-value Record from scalar values #2980

bfncs opened this issue Dec 11, 2020 · 18 comments

Comments

@bfncs
Copy link

bfncs commented Dec 11, 2020

Describe the bug
I experience a potential regression since 2.12.0 when trying to deserialize a scalar value to a single-value Record. Consider this test case:

import static org.junit.jupiter.api.Assertions.assertEquals;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Test;

class RecordStringDeserializationTest {

  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();

  @Test
  void deserializeMyValueRecordFromPrimitiveValue() throws Exception {
    assertEquals(new MyValueRecord("foo"), OBJECT_MAPPER.readValue("\"foo\"", MyValueRecord.class));
  }

  final record MyValueRecord(@JsonProperty String value) {}
}

This works as expected in 2.11.3, since 2.12.0 a MismatchedInputException is thrown:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `us.byteb.jackson.RecordStringDeserializationTest$MyValueRecord` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('foo')
 at [Source: (String)""foo""; line: 1, column: 1]

This still works as expected if an explicit factory method annotated with @JsonCreator is provided for the Record or for an equivalent Class without explicit @JsonCreator (see additional test cases). I would expect the behavior for Records to be the same as for Classes, it also seems contrary to the brevity of Records to have to define an additional factory method.

Version information
2.12.0

To Reproduce
See test case above.

@bfncs bfncs added the to-evaluate Issue that has been received but not yet evaluated label Dec 11, 2020
@cowtowncoder
Copy link
Member

Ok so this is a common stumbling block: you are assuming that constructors would be taken as "delegating" creator, for 1-argument case. Unfortunately:

  1. This is fundamentally ambiguous case and one that has never worked well for POJOs without annotations (but see Allow handling of single-arg constructor as property based by default #1498 added in 2.12 to help a bit) -- but heuristics can certainly result in "delegating" being the answer (if constructor parameter name is not available or if there is nothing indicating property with that name)
  2. Addition of @JsonProperty for parameter explicitly indicates that this should be properties-based case (even with POJOs)
  3. For Records, explicit support was only added in 2.12, and it defines that the default mode is "properties" unless explicitly overridden by annotation.3.

So this is working as intended from my perspective.

To use delegating style you will need to add @JsonCreator(mode = JsonCreator.Mode.DELEGATING) annotation on constructor.

@cowtowncoder cowtowncoder added will-not-fix Closed as either non-issue or something not planned to be worked on and removed to-evaluate Issue that has been received but not yet evaluated labels Dec 14, 2020
@cowtowncoder
Copy link
Member

However... it does not look like there is a proper test for verifying ability to use delegation so I will add one to verify it actually works. :)

@cowtowncoder cowtowncoder removed the will-not-fix Closed as either non-issue or something not planned to be worked on label Dec 14, 2020
cowtowncoder added a commit that referenced this issue Dec 14, 2020
@cowtowncoder
Copy link
Member

And what do I know: the test case fails. So somehow constructor annotation is not being found; possibly same as or related to #2974.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 15, 2020

... except that my test was faulty; when fixed, works as expected. Should always double-check tests when they fail.

@cowtowncoder
Copy link
Member

Ok, so: I think this is working as expected: by default, Records will use "properties"-style construction.
It is unfortunate that behavior was different in 2.11, as auto-detection and heuristics determined that "delegating" style would be most appropriate (as Record handling was using default POJO logics), but this is the intended behavior with explicit Record support.

To use delegating-style with Records on 2.12 and later, necessary annotation will be:

    record MyValueRecord(String value) {
        @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
        public MyValueRecord(String value) {
           this.value = value;
       }
    }

since there is no shorter way to annotate primary constructor (as far as I know).

@bfncs
Copy link
Author

bfncs commented Dec 17, 2020

Thanks very much for answering this in great depth 💐
This makes sense and is still easy and consistent to use. I was only confused by the changed heuristic behavior between versions.

@cowtowncoder
Copy link
Member

@bfncs Yeah, this wasn't something I had thought of until you reported it; I did not realize Records were already used, although it was mentioned as something that sort of works (when discussing initially addition of explicit support for 2.12).

I will add a note on 2.12 release page: thank you for bringing this up.

@bfncs
Copy link
Author

bfncs commented Dec 22, 2020

One more thing just now came to my mind: this also works with a record's compact constructor and is a bit less boilerplate:

    record MyValueRecord(String value) {
        @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
        public MyValueRecord {}
    }

@cowtowncoder
Copy link
Member

Interesting, did not know such shortcut was available. Thank you for sharing!

@soc
Copy link

soc commented Jan 26, 2022

I wanted to report that I came across this issue because a class → record migration lead to this (mysterious) failure. In my case it required backing out of the changes, because the type in question existed in a module that did not depend on Jackson, so adding the annotation was not an option.

I think the current behavior is really unfortunate, and I hope the behavior can be fixed in Jackson 3.0. :-/

Thanks!

@yawkat
Copy link
Member

yawkat commented Jan 27, 2022

@soc It is possible to add annotations to classes in other modules through the mixin feature. I'm not sure how it interacts with records, though.

@soc
Copy link

soc commented Jan 27, 2022

@yawkat

It is possible to add annotations to classes in other modules through the mixin feature.

Thanks, I didn't know about this feature! (Keeping the class and the mixin in sync may create more work than it is worth in my case though.)

I'm not sure how it interacts with records, though.

With records not even a custom deserializer worked. I reverted things back to a class and things are converting again.

@cowtowncoder
Copy link
Member

Quick note: use of custom deserializer for Record types should absolutely work. Although I suspect it depends on how you are registering custom deserializer (there are some general issues wrt annotation unification across Constructor parameters vs fields/getters/setters).
So worth filing a separate issue if that does not work.

Mix-in handling is general so it should work, but could have similar issues with annotation merging between Constructor parameters, other accessors.

As to changing behavior: unfortunately I think that the way Records work is the "Right" way -- default without annotation should probably be Properties-based. Explicit annotation would be needed for delegating case.
But I also agree that configurability, including that for Records, would be good.

@soc
Copy link

soc commented Jan 28, 2022

@cowtowncoder

So worth filing a separate issue if that does not work.

I will investigate further. I have cases where it works and cases where it doesn't.

As to changing behavior: unfortunately I think that the way Records work is the "Right" way -- default without annotation should probably be Properties-based.

Agree in that. I hope it can be changed for classes in Jackson 3 to work the same for classes as it currently does for records.


Is there any documentation on property-based vs. delegation-based serialization? I searched the documentation, but couldn't find more information on it.

Same for "creators" – I know that there are some "special" method names like of and from, but only because I read the implementation.

Please let me know, if there isn't any, I'd be happy to write it, because I keep forgetting the details.

@soc
Copy link

soc commented Jan 28, 2022

Further debugging:

  • not related to modules
  • not related to having both a of and a new
  • does not work with @JsonCreator
  • does not work with an explicit Deserializer
  • works with @JsonCreator(mode = JsonCreator.Mode.DELEGATING)

Surprisingly, it fails with a complaint on a type that doesn't even have the offending record:

com.fasterxml.jackson.databind.exc.MismatchedInputException:
  Cannot construct instance of `FooRecord` (although at least one Creator
  exists): no String-argument constructor/factory method to deserialize
  from String value ('850886fc-c23d-4567-8714-a6e4b1a445dd')
    at [Source: UNKNOWN; byte offset: #UNKNOWN]
    (through reference chain: SomeUnrelatedClass["fooRecord"])

SomeUnrelatedClass has neither a member called "fooRecord" nor any member of type FooRecord.

@cowtowncoder
Copy link
Member

@soc Alas, no documentation exists that I know of. Would be happy to add under jackson-docs (probably under wiki, although separate xxxx.md would be fine too. Or even jackson/ -- there's FAQ which has not been updated in years.

Every now and then we have discussions on mailing lists, ideas, but it all really needs (at least) one person to kind of define structure, skeleton, over which content can be added. To me at least it is much easier to incrementally add bits and pieces when scaffolding is there.

@cowtowncoder
Copy link
Member

@soc That unrelated reference case is pretty odd, agreed. I would still be particularly interested in explicit deserializer case, if you can show how you are adding it. That might be an easy fix.

I really, really hope I can find time to tackle Record handling: my first goal was actually to tackle #3352 for future compatibility. But beyond that there is the big need to resolve introspection which affects POJOs as well -- it just happens to be that for Records there are fewer non-affected parts.
But what I lack right now is contiguous time to dig deep, remember pieces I had to work on for Jackson 2.12 when introducing explicit Record support. Work then was minimalistic, for better or worse. More intrusive changes would have made future improvements easier, in hindsight. But at the time there were so many things to work on... (lol -- that hasn't changed).

I really appreciate your help here & wish I could devote more time here. But let's keep things going at whatever rate we can.

@soc
Copy link

soc commented Jan 31, 2022

Hey @cowtowncoder, no worries! Nobody has unlimited time, power or focus.

Thanks for all the work you do, and if people think more should be done ... they need to step up themselves!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants