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

[stolsvik]

Describe the bug
This code used to work with 2.14.2, but not with 2.15.0

ObjectMapper mapper = new ObjectMapper();
mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
RecordTest recordTest_deserialized = mapper.readValue("{}", RecordTest.class);

Version information
2.15.0

To Reproduce

/**
 * This works fine with Jackson 2.14.2, but not with 2.15.0.
 */
public class Jackson_2_15_0_Regression {

    record RecordTest(String string, int integer) {
    }

    @Test
    public void emptyJsonToRecord() throws JsonProcessingException {
        ObjectMapper mapper = new ObjectMapper();

        mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
        // mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY);

        RecordTest recordTest_deserialized = mapper.readValue("{}", RecordTest.class);


        System.out.println("RecordTest deserialized: " + recordTest_deserialized);
        Assert.assertEquals(new RecordTest(null, 0), recordTest_deserialized);
    }
}

Comment out the mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE) and it works.

Note that the commented-out mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY) is included in my actual code to get intended behaviour (i.e. only fields are serialized and deserialized, no methods involved), but this did not make any to or from for the problem.

Exception is:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `io.mats3.examples.jbang.Jackson_2_15_0_Regression$RecordTest` (no Creators, like default constructor, exist): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: (String)"{}"; line: 1, column: 2]

	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:67)
	at com.fasterxml.jackson.databind.DeserializationContext.reportBadDefinition(DeserializationContext.java:1915)
	at com.fasterxml.jackson.databind.DatabindContext.reportBadDefinition(DatabindContext.java:414)
	at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1360)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1424)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:352)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4825)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3772)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3740)
	at io.mats3.examples.jbang.Jackson_2_15_0_Regression.emptyJsonToRecord(Jackson_2_15_0_Regression.java:25)

[pjfanning]

Seems to work if you don't change the mapper visibility setting - see #3907. Would it be possible for you to not set this value?

This issue will still be investigated. I'm just making the suggestion above as a workaround. I'm no expert on the benefits of setting the visibility to non-default values.

[yihtserns]

Fix/workaround

Allow visibility for PropertyAccessor.CREATOR:

mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
mapper.setVisibility(PropertyAccessor.CREATOR, Visibility.ANY);

Explanation

Previously, Records deserialization is specially handled, resulting in some behavioural differences (mostly missing behaviours) compared to POJO deserialization.

#3724 was done to address those differences, to close the gap between Records vs POJO de/serialization:

1. This means now, Jackson looks for deserialization "creators" for Records the same way it looks for POJO's.
2. setVisibility(PropertyAccessor.ALL, Visibility.NONE) "hides" those "creators", because PropertyAccessor.ALL includes PropertyAccessor.CREATOR.

[yihtserns]

(Interesting to see the different config used to utilise Jackson as only a deserializer not serializer: here & #3897 🤔.)

[stolsvik]

Thanks.

This is used both for serialization and deserialization. It was the deserialization that failed - here the "assert that this will actually work with an empty {} object" boot-up verification - and it only failed with records.

I need this config to work both for 2.14.2 and 2.15. Will that new suggested configuration do that just the same? That is: I want ONLY fields evaluated, with any visibility, for both classes and records. No methods whatsoever. These are DTOs, if people slap methods on it, I don't care: I want the data transferred. I guess I found that config somewhere sometime, and it have done exactly what I wanted for at least 6 years (used to be just classes, and then it also worked just as expected with records. Nice.)

(PS: I cannot really "test if it works" with an alternate config, as this is a "foundation lib", there are at least 50 different random services that depend on this just working as it always has. I just wanted to upgrade to keep with the times! :-) That bit pretty hard, both this, and the 5M char limit for String: FasterXML/jackson-core#863 (comment) )

(PPS: The bullet 2 where "setVisibility(PropertyAccessor.ALL, Visibility.NONE) "hides" those "creators", because PropertyAccessor.ALL includes PropertyAccessor.CREATOR" on the face of it sounds like a clear bug to me. For a record, there is no other proper way to get this thing made, so they should probably not be included? At least not the canonical constructor?)

[cowtowncoder]

One thing worth noting: Records are technically very different from POJOs/Beans, so there are challenges from Jackson implementation side. Especially regarding access to Record Fields; something that will start failing on newer JDKs. It is not certain handling can be fully unified.

I agree that it is important to be able to have value types, handling that works for both 2.14 and 2.15. But we really did not realize that there was Record usage that relied on explicit Visibility settings -- there's no testing so one can view at as unsupported use case, technically.
Assumption rather was that usage wrt Records would use default visibility settings.

[stolsvik]

@cowtowncoder Thanks!

Records are technically very different from POJOs/Beans, so there are challenges from Jackson implementation side.

yes, I agree, which makes me wonder a bit about this plan of jamming records and POJOs into the same regime, as seems suggested by @yihtserns?

Assumption rather was that usage wrt Records would use default visibility settings.

This is really not the point: The point is that I want to use the same ObjectMapper no matter what my users are choosing: POJOs (as they have for 8 years), or records (the new kid). There is thus a jumble of some POJOs here, and some records there, people now also migrating due to the extreme terse-ness and nice-ness of records. I feel it makes little sense to have to handle this differently? How should I even do that? A POJO could have a record inside it, and other way around.

[cowtowncoder]

Records are technically very different from POJOs/Beans, so there are challenges from Jackson implementation side.

yes, I agree, which makes me wonder a bit about this plan of jamming records and POJOs into the same regime, as seems suggested by @yihtserns?

Right... and which is why code originally was separate.

I had plans to rewrite Introspection of POJOs, Records, but didn't have time so others had a chance to try out alternatives here.

Assumption rather was that usage wrt Records would use default visibility settings.

This is really not the point: The point is that I want to use the same ObjectMapper no matter what my users are choosing: POJOs (as they have for 8 years), or records (the new kid). There is thus a jumble of some POJOs here, and some records there, people now also migrating due to the extreme terse-ness and nice-ness of records. I feel it makes little sense to have to handle this differently? How should I even do that? A POJO could have a record inside it, and other way around.

Sorry, I did not mean that as a justification for breaking things: but rather explanation of why this use case was not covered by tests. I understand it was used as a way to unify handling with 2.14 when there was no other way.
I agree in that the same ObjectMapper should be usable.

I'd have to dig up where it was done, but the visibility checker defaults were forked for 2.15 (or was it 2.14 already?) and I think there's a way to change them separately as well. I just do not remember details.

EDIT: I think I got confused with above: visibility checker defaults had to do with different defaults for JDK types, where we do NOT want to detect fields. Records (outside of JDK packages) not affected by that.

[yihtserns]

This is used both for serialization and deserialization. It was the deserialization that failed - here the "assert that this will actually work with an empty {} object" boot-up verification - and it only failed with records...
...I want ONLY fields evaluated, with any visibility, for both classes and records. No methods whatsoever. These are DTOs, if people slap methods on it, I don't care: I want the data transferred.

Test result for setVisibility(PropertyAccessor.ALL, Visibility.NONE) with 2.14.2:

Mechanism (without annotations)	Serialization	Deserialization with non-empty JSON object	Deserialization with empty JSON object
No-arg constructor with getters & setters	❌ Failed: "no properties discovered to create BeanSerializer"	❌ Failed: "0 known properties"	✔ Successful: created empty instance
Constructor creator	❌ Failed: "no properties discovered to create BeanSerializer"	❌ Failed: "no Creators exist"	❌ Failed: "no Creators exist"
Record class	❌ Empty JSON Object	✔ Successful: created instance using the JSON fields	✔ Successful: created instance using default values

The only way the failing tests above can work is when @JsonCreator/@ConstructorProperties & @JsonProperty annotations are used on the constructor & fields, respectively. @stolsvik is that what was done in your codebase?

[yihtserns]

I'd have to dig up where it was done, but the visibility checker defaults were forked for 2.15 (or was it 2.14 already?)...

3.x

I think there's a way to change them separately as well...

The only way I know is this:

objectMapper.registerModule(new SimpleModule() {
    @Override
    public void setupModule(SetupContext context) {
        super.setupModule(context);
        context.insertAnnotationIntrospector(new NopAnnotationIntrospector() {
            @Override
            public VisibilityChecker<?> findAutoDetectVisibility(AnnotatedClass ac, VisibilityChecker<?> checker) {
                return ac.getType().isRecordType()
                        ? checker.withCreatorVisibility(JsonAutoDetect.Visibility.NON_PRIVATE)
                        : checker;
            }
        });
    }
});

Alternatives

1. Annotate the override directly on the Records:

@JsonAutoDetect(creatorVisibility = Visibility.NON_PRIVATE)
record RecordTest(String string, int integer) {
}

2. Annotate @JsonCreator on the canonical compact constructor:

record RecordTest(String string, int integer) {
    @JsonCreator
    RecordTest {
    }
}

I want to use the same ObjectMapper no matter what my users are choosing: POJOs (as they have for 8 years), or records (the new kid). There is thus a jumble of some POJOs here, and some records there, people now also migrating due to the extreme terse-ness and nice-ness of records. I feel it makes little sense to have to handle this differently? How should I even do that? A POJO could have a record inside it, and other way around.

...which was the whole point of #3724's "jamming records and POJOs into the same regime", so that using Records is no (or not much) different from using POJO - the issue you're facing now is because Records & POJOs are handled similarly...

[yihtserns]

I'd have to dig up where it was done, but the visibility checker defaults were forked for 2.15 (or was it 2.14 already?)...

3.x

@cowtowncoder Do you think we should copy that to 2.15? E.g.:

public abstract class MapperConfigBase ... {
    ...
    public final VisibilityChecker<?> getDefaultVisibilityChecker(Class<?> baseType, AnnotatedClass actualClass) {
        ...
        if (ClassUtil.isJDKClass(baseType)) {
            vc = VisibilityChecker.Std.allPublicInstance();
        } else if (baseType.isRecord()) {
            // For Records, ignore any config that hides constructor & factory method creators e.g. via
            // - ObjectMapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE)
            // - ObjectMapper.setVisibility(PropertyAccessor.CREATOR, Visibility.NONE)
            // - ObjectMapper.disable(MapperFeature.AUTO_DETECT_CREATORS)

            vc = getDefaultVisibilityChecker().withCreatorVisibility(Visibility.NON_PRIVATE);
        } else {
            vc = getDefaultVisibilityChecker();
        }
        ...
    }
}

[stolsvik]

@yihtserns:

is that what was done in your codebase?

No, as I said at top, I have both of:

ObjectMapper mapper = new ObjectMapper();
mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY);

The full init is:

ObjectMapper mapper = new ObjectMapper();

// Read and write any access modifier fields (e.g. private)
mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY);

// Drop nulls
mapper.setSerializationInclusion(Include.NON_NULL);

// If props are in JSON that aren't in Java DTO, do not fail.
mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);

// Write e.g. Dates as "1975-03-11" instead of timestamp, and instead of array-of-ints [1975, 3, 11].
// Uses ISO8601 with milliseconds and timezone (if present).
mapper.registerModule(new JavaTimeModule());
mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);

// Handle Optional, OptionalLong, OptionalDouble
mapper.registerModule(new Jdk8Module());

It is here: https://github.com/centiservice/mats3/blob/main/mats-serial-json/src/main/java/io/mats3/serial/json/MatsSerializerJson.java#L120-L152 - now also with the attempt at handling the 5M chars-in-String limit.

Note: I had accepted that I always need a no-args constructor (as opposed to GSON). (Note: If it is possible to support missing no-args constructor for a class, even with Java 17+, that would be excellent!)

the issue you're facing now is because Records & POJOs are handled similarly...

Exactly. Which is why I said: "yes, I agree, which makes me wonder a bit about this plan of jamming records and POJOs into the same regime, as seems suggested by @yihtserns?"

My point is that the construction of a Record and of a Class, and their population of fields, are rather different, so it sounds .. ambitious .. to do it with the same codebase/flow? Note, I do not try to dictate anything here, it is just an observation. But it would be nice if this - as seen from my side - regression - was not present!

[yihtserns]

No, as I said at top, I had both of:

ObjectMapper mapper = new ObjectMapper();
mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY);

OK, was confused because it was commented out in the sample code.

That means you might actually have 2 problems:

1. This issue for Records deserialization.
2. 2.15.0 breaking behaviour change for records and Getter Visibility #3895 for Records serialization.

[stolsvik]

Yes, but I tried to mention:

Note that the commented-out mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY) is included in my actual code to get intended behaviour (i.e. only fields are serialized and deserialized, no methods involved), but this did not make any to or from for the problem.

It was an attempt to reduce the problem to the bare minimum.

[yihtserns]

To summarize, there are 2 things broken when using this config:

ObjectMapper mapper = new ObjectMapper();
mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY);

1. For Records deserialization

Caused by #3724 to make Records de/serialization behaves just like POJO de/serialization - it sees:

record RecordTest(String string, int integer) {
}

...similarly to:

public class RecordTest {
    private String string;
    private int integer;

    @ConstructorProperties({"string", "integer"})
    public RecordTest(String string, int integer) {
        this.string = string;
        this.integer = integer
    }

    public String getString() {
        return this.string;
    }

    public String getInteger() {
        return this.integer;
    }
}

Fixes/workarounds

1. Make all classes' constructor/factory method Creators visible:

ObjectMapper mapper = new ObjectMapper();
mapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE);
mapper.setVisibility(PropertyAccessor.FIELD, Visibility.ANY);
mapper.setVisibility(PropertyAccessor.CREATOR, Visibility.NON_PRIVATE);

2. Make only Records' constructor/factory method Creators visible:

objectMapper.registerModule(new SimpleModule() {
    @Override
    public void setupModule(SetupContext context) {
        super.setupModule(context);
        context.insertAnnotationIntrospector(new NopAnnotationIntrospector() {
            @Override
            public VisibilityChecker<?> findAutoDetectVisibility(AnnotatedClass ac, VisibilityChecker<?> checker) {
                return ac.getType().isRecordType()
                        ? checker.withCreatorVisibility(JsonAutoDetect.Visibility.NON_PRIVATE)
                        : checker;
            }
        });
    }
});

3. Override visibility directly on the Record class:

@JsonAutoDetect(creatorVisibility = Visibility.NON_PRIVATE)
record RecordTest(String string, int integer) {
}

4. Override visibility of constructor Creator directly in the Record class:

record RecordTest(String string, int integer) {
    @JsonCreator
    RecordTest {
    }
}

2. For Records serialization

Broken by #3737 - Records serialization will always result in empty JSON hash (i.e. {}).

#3894 to fix that is pending review.

[yihtserns]

As for changing Jackson to make Records' creators always visibility regardless of config, I can/will create a PR if the core maintainers think that's the way to go.

[stolsvik]

=1. Do you mean that this won't be solved "natively"? I must change the code?

1.: Will this work identical for 2.14 and 2.15? Why the Visibility.NON_PRIVATE? I want the no-args to be private if it needs be there. (Just curious: Why is a constructor called a creator, if this is what we're talking about?)
2.: Hmmm.. This needs to be different between 2.14 and 2.15? How will this affect standard class-based POJO DTOs, which might have a bunch of constructors that I do not want the serialization to touch?
3. and 3. is utterly out of the question. (I .. well .. despise ... implementation-specific annotations for serialization. The serialization used for this Mats3 library should be completely opaque to the user, the use of Jackson is literally an implementation detail which should never be thought about. Provide the Request and Reply DTOs, and I will make them "magically" transported. I would rather put in code that refused to start if there was any sign of such annotations present)

However, I can't shake the feeling that this is a pretty clear regression.

=2:
Good, thanks!

[pjfanning]

@yihtserns thanks for looking at this this. Let's not rush things. We will need to tackle one problem at a time. Maybe getting #3894 progressed is the first priority. @stolsvik will just have to be patient while we resolve the issues. Please stick with Jackson 2.14 in the interim - or try another library, if you prefer.

[stolsvik]

Well, I pretty obviously prefer Jackson, as otherwise I would probably not try to report this.

[yihtserns]

Do you mean that this won't be solved "natively"? I must change the code?

Need to wait for the core maintainers to make the final decision.

1: Will this work identical for 2.14 and 2.15? Why the Visibility.NON_PRIVATE? I want the no-args to be private if it needs be there. (Just curious: Why is a constructor called a creator, if this is what we're talking about?)

Creators are constructors or factory methods (with name valueOf) that have parameters, that will be used for deserialization - they are typically either annotated with @JsonCreator or "magically" chosen.

They are not related to, and nor will that config affects no-arg constructor.

2: Hmmm.. This needs to be different between 2.14 and 2.15? How will this affect standard class-based POJO DTOs, which might have a bunch of constructors that I do not want the serialization to touch?

You can use this with both 2.14 & 2.15, but it basically does nothing for 2.14 (because again, deserialization was implemented differently). This is basically like number 1, except it only targets Record classes:

return ac.getType().isRecordType()                          // if we're deserializing Record class...
    ? checker.withCreatorVisibility(Visibility.NON_PRIVATE) // ...make the Creators visible
    : checker;                                              // ...leave the visibility as NONE for POJO types

[cowtowncoder]

I'd have to dig up where it was done, but the visibility checker defaults were forked for 2.15 (or was it 2.14 already?)...

3.x

@cowtowncoder Do you think we should copy that to 2.15? E.g.:

public abstract class MapperConfigBase ... {
    ...
    public final VisibilityChecker<?> getDefaultVisibilityChecker(Class<?> baseType, AnnotatedClass actualClass) {
        ...
        if (ClassUtil.isJDKClass(baseType)) {
            vc = VisibilityChecker.Std.allPublicInstance();
        } else if (baseType.isRecord()) {
            // For Records, ignore any config that hides constructor & factory method creators e.g. via
            // - ObjectMapper.setVisibility(PropertyAccessor.ALL, Visibility.NONE)
            // - ObjectMapper.setVisibility(PropertyAccessor.CREATOR, Visibility.NONE)
            // - ObjectMapper.disable(MapperFeature.AUTO_DETECT_CREATORS)

            vc = getDefaultVisibilityChecker().withCreatorVisibility(Visibility.NON_PRIVATE);
        } else {
            vc = getDefaultVisibilityChecker();
        }
        ...
    }
}

Hmmmh. I am bit hesitant wrt overriding users' explicit settings but it does make some sense -- esp. since that's in 3.0.
The main question would be whether to do that in 2.15.1 or 2.16(.0).
If (and only if) this would resolve issues we face wrt 2.14->2.15 changes, I'd say let's do it for 2.15(.1).
Also requires making sure we have good test coverage.

[cowtowncoder]

I merged #3894 fwtw; shouldn't (I think) affect this issue but just in case.

[stolsvik]

Just a comment from the side:

I am bit hesitant wrt overriding users' explicit settings but it does make some sense

I agree to the general idea that this seems just wrong, but somehow you need to get to those constructors to actually be able to make records! Another way could be a special-case "if" when about to create a record, that effectively ignored the setting by just finding the canonical constructor in spite of the setting. However, you would at least not just "overwrite" the user-set setting?!

[cowtowncoder]

@stolsvik Yeah this is the challenge in using same logic, handling as with POJOs, when rules are bit different (Records having clear, well-defined rules of what constitutes canonical Constructor, what properties exist).
I'll get #3910 merged and we'll see how far we might get.

@yihtserns I think we should go with the patch you suggest to unblock remaining issues.

[stolsvik]

Note: I had to adjust the suggestion a bit to get the "only adjust visibility for records" to work: The ac.getType() could apparently return null.

public VisibilityChecker<?> findAutoDetectVisibility(AnnotatedClass ac, VisibilityChecker<?> checker) {
    if (ac.getType() == null) {
        return checker;
    }
    if (!ac.getType().isRecordType()) {
        return checker;
    }
    // If this is a Record, then increase the "creator" visibility again
    return checker.withCreatorVisibility(Visibility.ANY);
}

[stolsvik]

Another comment from the side: I still don't get why the patch suggest vc = getDefaultVisibilityChecker().withCreatorVisibility(Visibility.NON_PRIVATE);, i.e. the NON_PRIVATE part. If I understand this correctly, it would then not find a private record with private constructor? (Visibility of a record's canonical constructor cannot be less than visibility of the record itself - but a record can be private?)

[yihtserns]

...a record can be private?

Yes:

public class Jackson_2_15_0_Regression {

    private record RecordTest(String string, int integer) {
    }
}

...will be compiled into:

public class Jackson_2_15_0_Regression {

    private static record RecordTest(String string, int integer) {
        private RecordTest(String string, int integer) {
            this.string = string;
            this.integer = integer;    
        }

        public String string() {
            return this.string;
        }

        public String integer() {
            return this.integer;
        }
    }
}

...I still don't get why the patch suggest vc = getDefaultVisibilityChecker().withCreatorVisibility(Visibility.NON_PRIVATE);, i.e. the NON_PRIVATE part...

I just copied that directly from 3.x codebase. In hindsight, I should've changed that to Visibility.ANY to make it simple (and practical). 😞

[cowtowncoder]

I do not remember details here, but it is possible the idea was to avoid exposing private constructors, thinking that they are private for reason (but that there are other non-private constructors). For regular POJOs only public constructors are auto-detected; although for Records I think that separate rule for canonical constructor (to be found regardless of visibility?) and other constructors (use configured visibility) would make most sense.
But once that runs counter to trying to make Records work just like POJOs.

[stolsvik]

.. although for Records I think that separate rule for canonical constructor (to be found regardless of visibility?)

Totally agree. You need to create an instance. That I have chosen to make this DTO-record an inner private entity of whatever service class it resides in, shouldn't make any difference.

[cowtowncoder]

@yihtserns I think fix you suggested would be doable (just need to change check to use ClassUtil.isRecordType(baseType) since Class.isRecord() is not available on JDK 8).
I think I'll create a PR.

[cowtowncoder]

Note: need to figure out what (if anything) to do with 3.0 (master); 2 failing tests left.
This due to MapperFeature.AUTO_DETECT_CREATORS being removed so specific workaround cannot be used.