Change Records deserialization to use the same mechanism as POJO deserialization. #3724

yihtserns · 2023-01-09T12:02:15Z

New

Supports @JsonCreator.mode=DISABLED on canonical constructor.
Supports deserialization using "implicit" (i.e. without @JsonCreator annotation) non-canonical constructor when 1/more of its parameter is/are annotated with @JsonProperty.
Supports "implicit" (i.e. without @JsonCreator annotation) 1-arg delegating constructor.
Supports "implicit" (i.e. without @JsonCreator annotation) 1-arg delegating factory method.
Supports disabling "implicit" (i.e. without @JsonCreator annotation) constructor/factory method detection via MapperFeature.AUTO_DETECT_CREATORS.

This is a non-exhaustive list - I don't have full knowledge of existing JavaBeans deserialization capabilities.

Fixes

Fixes Null coercion with @JsonSetter does not work with java.lang.Record #2974
Fixes @JsonNaming does not work with Records #2992
Resolves Support @JsonCreator annotation on record classes #3180 (via "implicit" 1-arg delegating constructor/factory method)
Fixes @JsonDeserialize(converter = ...) does not work with Records #3297
Fixes JsonTypeInfo.As.EXTERNAL_PROPERTY does not work with record wrappers #3342

src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java

pjfanning · 2023-01-09T17:58:11Z

src/main/java/com/fasterxml/jackson/databind/jdk14/JDK14Util.java

-    public static AnnotatedConstructor findRecordConstructor(DeserializationContext ctxt,
-            BeanDescription beanDesc, List<String> names) {
-        return new CreatorLocator(ctxt, beanDesc)
+    public static AnnotatedConstructor findRecordConstructor(AnnotatedClass recordClass, AnnotationIntrospector intr, MapperConfig<?> config, List<String> names) {


API change? Possibly not a good thing - any chance that the old version can be kept and deprecated?

Yeah no, please leave the signature as it was: DeserializationContext and BeanDescription can be used to access AnnotationIntrospector and MapperConfig.

API change? Possibly not a good thing - any chance that the old version can be kept and deprecated?

Done - added a new method while keeping (but not deprecating) the old method.

Yeah no, please leave the signature as it was: DeserializationContext and BeanDescription can be used to access AnnotationIntrospector and MapperConfig.

The problem is in the place I'm using it now (POJOPropertiesCollector), I don't have DeserializationContext nor BeanDescription...

Hmmm, ok that makes sense. But that's bit of a problem. Let me think about it.

src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java

src/main/java/com/fasterxml/jackson/databind/jdk14/JDK14Util.java

cowtowncoder · 2023-01-10T05:11:30Z

First of all, thank you for contributing this @yihtserns ! It sounds like solid improvement, potentially.

I added some smaller notes on keeping existing API in some places, but there is one bigger request I have: this PR needs to be against 2.15 branch; I don't feel comfortable merging it in a patch to 2.14 -- patches are typically for smaller, less risky changes, and while I don't have specific issues here I feel there is potential for some breakage.

yihtserns · 2023-01-10T08:33:49Z

...this PR needs to be against 2.15 branch; I don't feel comfortable merging it in a patch to 2.14...

Do I put "since 2.15" in the deprecation notice?

And yeah I wasn't sure whether I should go with 2.14 or 2.15 since https://github.com/FasterXML/jackson/blob/master/CONTRIBUTING.md didn't mention the latter... That doc needs to be updated?

cowtowncoder · 2023-01-12T03:13:27Z

...this PR needs to be against 2.15 branch; I don't feel comfortable merging it in a patch to 2.14...

Do I put "since 2.15" in the deprecation notice?

And yeah I wasn't sure whether I should go with 2.14 or 2.15 since https://github.com/FasterXML/jackson/blob/master/CONTRIBUTING.md didn't mention the latter... That doc needs to be updated?

...this PR needs to be against 2.15 branch; I don't feel comfortable merging it in a patch to 2.14...

Do I put "since 2.15" in the deprecation notice?

Yes.

And yeah I wasn't sure whether I should go with 2.14 or 2.15 since https://github.com/FasterXML/jackson/blob/master/CONTRIBUTING.md didn't mention the latter... That doc needs to be updated?

Thanks, I'll need to update that. There are some things that should go into earlier branches too -- generally earliest maintained, for safe enough fixes. So it is not easy to correctly guess what I think is the right branch :)
(which is also why I don't mind pointing out what I think should be used).

cowtowncoder · 2023-01-12T03:17:06Z

src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializer.java

@@ -467,6 +467,10 @@ protected Object _deserializeUsingPropertyBased(final JsonParser p, final Deseri
            // regular property? needs buffering
            SettableBeanProperty prop = _beanProperties.find(propName);
            if (prop != null) {
+                if (prop.isIgnorable() && _beanType.isRecordType()) {


Was there a problem here, to require additional processing? And/or would this work even for POJO types?

src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java

cowtowncoder · 2023-01-12T03:22:01Z

Ok, looks pretty good. I do need to go over it again with more thought, but I like how small changes are.

…rialization.

cowtowncoder · 2023-01-14T04:58:25Z

@yihtserns One more thing: I hope to be able to merge this soon. One last thing before that is that we need a Contributor License Agreement (CLA) before merging the first contribution. This only needs to be done once and then it is good for all future PRs for Jackson project.

Document is here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the usual way is to print, fill & sign, scan/photo, email to info at fasterxml dot com.
Once I get the document I will be able to merge the PR (I will do one more code review but I assume we are very close).

Thank you again for providing this patch -- looking forward to merging it!

yihtserns · 2023-01-14T10:01:48Z

I'm currently re-reviewing if @JsonIgnore can be supported in a different way (else the explanation of the current approach will be a headache).

UPDATE: Didn't manage to find a low-change way, so decided to stick with this approach (with some difference) + smothered it with docs.

yihtserns · 2023-01-14T12:52:30Z

One last thing before that is that we need a Contributor License Agreement (CLA) before merging the first contribution

CLA sent.

yihtserns · 2023-04-24T09:00:17Z

@leonardehrenfried that symptom sounds similar to the one describe in #3628 (comment) - let me test that.

yihtserns · 2023-04-24T10:19:24Z

@leonardehrenfried so I tested OpenTripPlanner:

Changed jackson.version in pom.xml to 2.15.0.
Run mvn test to observe the test failures (there's 1 unrelated to Records, so I'm ignoring it).
Create a local fork of jackson-databind:2.15.0, reverting 0a4cfc4.
Change OpenTripPlanner/pom.xml's jackson-databind to refer to that local fork.
Run mvn test again - no more test failures related to Records.

I've confirmed that the issue you're facing is related to the broken workaround mentioned in #3628, unrelated to this change.

leonardehrenfried · 2023-04-24T14:58:52Z

I since checked and we seem to be using a custom object mapper in the test:

      objectMapper = new ObjectMapper();
      objectMapper.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);
      objectMapper.enable(SerializationFeature.WRITE_DATES_WITH_ZONE_ID);
      objectMapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);
      objectMapper.disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);
      objectMapper.configure(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY, true);
      objectMapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
      objectMapper.registerModule(new JavaTimeModule());
      objectMapper.registerModule(new Jdk8Module());

      objectMapper.setVisibility(
        objectMapper
          .getSerializationConfig()
          .getDefaultVisibilityChecker()
          .withFieldVisibility(JsonAutoDetect.Visibility.ANY)
          .withGetterVisibility(JsonAutoDetect.Visibility.NONE)
          .withSetterVisibility(JsonAutoDetect.Visibility.NONE)
          .withCreatorVisibility(JsonAutoDetect.Visibility.NONE)
      );

https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/test/java/org/opentripplanner/routing/algorithm/mapping/SnapshotTestBase.java#L366-L384

So it appears that your analysis is totally correct.

I'm still not clear if this is was an intentional change in Jackson or not - can you clarify this for me?

Asked the other way around: is the old behaviour going to be restored and I should just wait for a new release or do I need to change the config of my object mapper?

yihtserns · 2023-04-24T15:12:38Z

...do I need to change the config of my object mapper?

I think that config was used because you had the same problem as #3628, and I can't think of an alternative solution for that problem.

...is the old behaviour going to be restored and I should just wait for a new release...

I've helped create #3894 to support that type of config.

cowtowncoder · 2023-04-24T16:36:52Z

Renovate auto-updated our application to Jackson 2.15 tonight. Some tests which check that records can be serialized fail as records are always serialized to empty objects.

These are plain records with no annotations or methods. Should that just work or do I need to configure/annotate something? In Jackson 2.14 it just worked.

This is why we publish release candidates -- 2.15.0-rc1, rc2, rc3 -- to try to get users to verify if they find any regressions beyond what our unit test suite catches. Nothing was reported for this.

Now, I don't know what tests you have so I cannot say if behavior is expected or not: sometimes usage is based on undefined behavior, in which case change may be intended (typically due to a fix to some known problem, and change to unsupported case incidental).

Thank you for helping create a new issue. We'll see where that leads.

leonardehrenfried · 2023-04-24T17:40:40Z

I'm sorry and as an open source maintainer myself I feel you.

I'm the last guy showing up on an issue making demands.

I would love to help diagnose and test and don't expect you to make a potentially broken config work for my benefit.

cowtowncoder · 2023-04-24T18:25:34Z

@leonardehrenfried Oh and just to make sure: I did not mean to blame you or users at all. I appreciate your reaching out and reporting the issue. I think we can figure this out; PR submitted might be the way, although I have some reservations. But it's a tricky problem.

yihtserns · 2023-04-24T18:33:59Z

@leonardehrenfried I've spent a bit of time to study your codebase to understand why that config was used - seems like it was to prevent ApiLeg.getDuration() from being serialized.

I assume that is to make assertion easier, since that method returns non-constant number?

If that's the case, here's my suggestion (tested with 2.15.0):

public abstract class SnapshotTestBase {
    ...

    private static class SnapshotItinerarySerializer implements SnapshotSerializer {

        private SnapshotItinerarySerializer() {
            ...

            // Remove this config:
            //objectMapper.setVisibility(
            //    objectMapper
            //      .getSerializationConfig()
            //      .getDefaultVisibilityChecker()
            //      .withFieldVisibility(JsonAutoDetect.Visibility.ANY)
            //      .withGetterVisibility(JsonAutoDetect.Visibility.NONE)
            //      .withSetterVisibility(JsonAutoDetect.Visibility.NONE)
            //      .withCreatorVisibility(JsonAutoDetect.Visibility.NONE)
            //);

            // Add this:
            objectMapper.addMixIn(ApiLeg.class, ApiLegMixin.class);
            ...
        }
    }

    
    /**
     * To exclude {@link ApiLeg#getDuration()} from being deserialized because the returned number
     * is non-constant making it impossible to assert.
     */
    private abstract static class ApiLegMixin {

      @JsonIgnore
      abstract double getDuration();
    }
}

leonardehrenfried · 2023-04-25T20:26:08Z

@yihtserns thanks so much for this. It absolutely fixes the problems we are having. Fantastic work!

agavrilov76 · 2023-04-27T15:56:23Z

Related to #3900

cowtowncoder · 2023-05-18T23:30:11Z

Another breakage: #3938.

yihtserns · 2023-05-19T02:27:44Z

I used to think "nobody would ever put a setter in a Record since nothing is mutable, right?"

Now I know how ignorant I was...

cowtowncoder · 2023-05-19T04:17:00Z

To be honest, that use case is a bit "Creative" and just used to prevent deserialization of something without full ignoral.
But it is something I'd have used with regular POJOs and works very well for what is needed.

If someone else has shown that use case to me, I might have argued against it tho, with Records. Given that they are immutable.

Anyway, was happy to figure out how to make it work again via fix for #3928.

yihtserns · 2023-06-07T11:01:19Z

Caused #3968.

yihtserns changed the base branch from 2.15 to 2.14 January 9, 2023 12:03

pjfanning reviewed Jan 9, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java Show resolved Hide resolved

pjfanning reviewed Jan 9, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Jan 10, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/jdk14/JDK14Util.java Show resolved Hide resolved

yihtserns force-pushed the 2992-records-jsonnaming branch from 698e248 to bb2ba45 Compare January 10, 2023 10:10

yihtserns changed the base branch from 2.14 to 2.15 January 10, 2023 10:11

yihtserns force-pushed the 2992-records-jsonnaming branch 2 times, most recently from 9a69ce8 to 1ca9b17 Compare January 10, 2023 10:24

yihtserns mentioned this pull request Jan 11, 2023

Nulls.AS_EMPTY doesn't work for collections containing values without default constructor (like Records) #3084

Open

cowtowncoder added the 2.15 label Jan 12, 2023

cowtowncoder reviewed Jan 12, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Jan 12, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java Show resolved Hide resolved

yihtserns force-pushed the 2992-records-jsonnaming branch from 1ca9b17 to 2c15147 Compare January 12, 2023 13:50

yihtserns changed the title ~~Change Records deserialization to use the same mechanism as JavaBeans deserialization.~~ Change Records deserialization to use the same mechanism as POJO deserialization. Jan 12, 2023

Change Records deserialization to use the same mechanism as POJO dese…

bd89187

…rialization.

cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jan 14, 2023

yihtserns force-pushed the 2992-records-jsonnaming branch from 2c15147 to 3d6c6cf Compare January 14, 2023 12:38

Special handling to support JsonIgnore for Records deserialization.

eaf013a

yihtserns force-pushed the 2992-records-jsonnaming branch from 3d6c6cf to eaf013a Compare January 14, 2023 16:25

cowtowncoder approved these changes Jan 14, 2023

View reviewed changes

cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jan 14, 2023

yihtserns mentioned this pull request Apr 24, 2023

Serializing a Java 16 Record includes transient data from getters from implemented interfaces #3628

Open

yihtserns mentioned this pull request Apr 24, 2023

Only avoid Records fields detection for deserialization #3894

Merged

This was referenced Apr 26, 2023

2.15.0 breaks deserialization when POJO/Record only has a single field and is marked Access.WRITE_ONLY #3897

Closed

2.15.0 breaking behaviour change for records and Getter Visibility #3895

Closed

This was referenced Apr 29, 2023

MapperFeature.SORT_PROPERTIES_ALPHABETICALLY is not respected for records #3900

Closed

Regression: 2.15.0 breaks deserialization for records when mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE); #3906

Closed

This was referenced May 7, 2023

Support @JsonCreator annotation on record classes #3180

Closed

constructorDetector seems to invalidate defaultSetterInfo for nullability #3241

Closed

cowtowncoder mentioned this pull request May 18, 2023

Record setter not included from interface (2.15 regression) #3938

Closed

yihtserns mentioned this pull request May 21, 2023

Fix #3906 by forcing Record constructors to be visible regardless of visibility overrides #3944

Closed

This was referenced Jun 7, 2023

Records with additional constructors failed to deserialize #3968

Closed

Primitive to record mapping fails to find constructor #3381

Open

yihtserns mentioned this pull request Jun 16, 2023

Exception thrown for incorrectly annotated record changed from InvalidDefinitionException to MismatchedInputException #3988

Closed

yihtserns mentioned this pull request Jun 27, 2023

Record component is not ignored when overriding accessor method #4005

Closed

yihtserns mentioned this pull request Sep 5, 2023

Record deserialization bug/breaking change in 2.15 #4105

Closed

1 task

yihtserns mentioned this pull request Oct 13, 2023

MapperFeature to ignore setter/getter method implicit detection for Record types #4157

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Records deserialization to use the same mechanism as POJO deserialization. #3724

Change Records deserialization to use the same mechanism as POJO deserialization. #3724

yihtserns commented Jan 9, 2023 •

edited

Loading

pjfanning Jan 9, 2023

cowtowncoder Jan 10, 2023

yihtserns Jan 10, 2023 •

edited

Loading

cowtowncoder Jan 12, 2023

cowtowncoder commented Jan 10, 2023

yihtserns commented Jan 10, 2023

cowtowncoder commented Jan 12, 2023

cowtowncoder Jan 12, 2023

cowtowncoder commented Jan 12, 2023

cowtowncoder commented Jan 14, 2023

yihtserns commented Jan 14, 2023 •

edited

Loading

yihtserns commented Jan 14, 2023 •

edited

Loading

yihtserns commented Apr 24, 2023

yihtserns commented Apr 24, 2023

leonardehrenfried commented Apr 24, 2023

yihtserns commented Apr 24, 2023

cowtowncoder commented Apr 24, 2023

leonardehrenfried commented Apr 24, 2023

cowtowncoder commented Apr 24, 2023

yihtserns commented Apr 24, 2023 •

edited

Loading

leonardehrenfried commented Apr 25, 2023

agavrilov76 commented Apr 27, 2023

cowtowncoder commented May 18, 2023

yihtserns commented May 19, 2023

cowtowncoder commented May 19, 2023

yihtserns commented Jun 7, 2023

Change Records deserialization to use the same mechanism as POJO deserialization. #3724

Change Records deserialization to use the same mechanism as POJO deserialization. #3724

Conversation

yihtserns commented Jan 9, 2023 • edited Loading

New

Fixes

pjfanning Jan 9, 2023

Choose a reason for hiding this comment

cowtowncoder Jan 10, 2023

Choose a reason for hiding this comment

yihtserns Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

cowtowncoder Jan 12, 2023

Choose a reason for hiding this comment

cowtowncoder commented Jan 10, 2023

yihtserns commented Jan 10, 2023

cowtowncoder commented Jan 12, 2023

cowtowncoder Jan 12, 2023

Choose a reason for hiding this comment

cowtowncoder commented Jan 12, 2023

cowtowncoder commented Jan 14, 2023

yihtserns commented Jan 14, 2023 • edited Loading

yihtserns commented Jan 14, 2023 • edited Loading

yihtserns commented Apr 24, 2023

yihtserns commented Apr 24, 2023

leonardehrenfried commented Apr 24, 2023

yihtserns commented Apr 24, 2023

cowtowncoder commented Apr 24, 2023

leonardehrenfried commented Apr 24, 2023

cowtowncoder commented Apr 24, 2023

yihtserns commented Apr 24, 2023 • edited Loading

leonardehrenfried commented Apr 25, 2023

agavrilov76 commented Apr 27, 2023

cowtowncoder commented May 18, 2023

yihtserns commented May 19, 2023

cowtowncoder commented May 19, 2023

yihtserns commented Jun 7, 2023

yihtserns commented Jan 9, 2023 •

edited

Loading

yihtserns Jan 10, 2023 •

edited

Loading

yihtserns commented Jan 14, 2023 •

edited

Loading

yihtserns commented Jan 14, 2023 •

edited

Loading

yihtserns commented Apr 24, 2023 •

edited

Loading