Exception when deserialization uses a record with a constructor property with access=READ_ONLY

[Mochis]

Search before asking

• [X] I searched in the issues and found nothing similar.

Describe the bug

When deserialization uses a record with a constructor property with access=READ_ONLY, the following exception is thrown:

Caused by: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid type definition for type `com.company.common.vo.RuleVO`: Argument #1 of constructor [constructor for `com.company.common.vo.RuleVO` (4 args), annotations: [null] has no property name annotation; must have name when multiple-parameter constructor annotated as Creator
 at [Source: (BufferedReader); line: 1, column: 1]
	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:62)
	at com.fasterxml.jackson.databind.DeserializationContext.reportBadTypeDefinition(DeserializationContext.java:1893)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._addImplicitConstructorCreators(BasicDeserializerFactory.java:602)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._constructDefaultValueInstantiator(BasicDeserializerFactory.java:301)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findValueInstantiator(BasicDeserializerFactory.java:222)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.buildBeanDeserializer(BeanDeserializerFactory.java:262)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.createBeanDeserializer(BeanDeserializerFactory.java:151)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer2(DeserializerCache.java:415)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:350)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:264)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
	at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
	at com.fasterxml.jackson.databind.DeserializationContext.findContextualValueDeserializer(DeserializationContext.java:621)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.createContextual(CollectionDeserializer.java:188)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.createContextual(CollectionDeserializer.java:28)
	at com.fasterxml.jackson.databind.DeserializationContext.handleSecondaryContextualization(DeserializationContext.java:867)
	at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:659)
	at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:4956)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4826)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3801)

A sample record is provided bellow.

Version Information

2.15.2

Reproduction

Sample record with the issue:

public record RuleVO(
  @JsonIgnore
  Integer id,

  @JsonProperty(access = JsonProperty.Access.READ_ONLY)
  Integer order,

  @JsonProperty(access = JsonProperty.Access.READ_ONLY)
  String name,

  String description,
) {}

If I remove the JsonProperty.Access.READ_ONLY from order field, a new exception is thrown indicating the argument with the issue is ...: Argument #2 of constructor [constructor for..., that is, the other field with READ_ONLY.

The object mapper is:

  public static ObjectMapper customObjectMapper() {
    return JsonMapper.builder()
      .addModule(new JavaTimeModule())
      .disable(MapperFeature.DEFAULT_VIEW_INCLUSION)
      .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
      .build();
  }

I also tried with a plain object mapper JsonMapper.builder().build() without customizations and the error is still there.

Expected behavior

In 2.14.3, it works perfectly.

Additional context

I have tested the application with 2.15.3-SNAPSHOT and 2.16.0-SNAPSHOT and the error is thrown too.

[yawkat]

you probably need to define the name explicitly if you use @JsonProperty.

[soc]

One of the benefits of record is that you have first-class support for getting the names (Class#getRecordComponents), so losing that feels kinda wrong.

[yawkat]

oh duh, the issue is much more simple. since you define the property as READ_ONLY, it can't be deserialized. the error could be better but there is no functional change needed here imo.

[Mochis]

I tried to put a name explicitly, but didn't work.

since you define the property as READ_ONLY, it can't be deserialized

Before 2.15.x the result of deserialize or ignoring on deserialization a field with access=READ_ONLY was a null value, I don't understand why it should change to an exception.

For me this is a regression because with the previous version 2.14.3 it works, the change to 2.15.x is not a major change according semver and this is not compatible with previous versions if it was intended.

[yawkat]

I would not call it a regression, I would call it finally having the right behavior :)

AIUI, the record implementation changed a lot 2.14->2.15. It now uses most of the existing bean infrastructure instead of its own special handling. That means aspects that were previously not implemented for records, now are.

But once again this is @cowtowncoder's call.

[Mochis]

Thank you @yawkat for the explanation.

I'm curious about your reasoning, how would you use the READ_ONLY property instead, only to throw an exception when the JSON input contains a field marked with it?

My use case is to use the same value object class to serialize and deserialize, but I want to ignore some JSON fields when deserializing, that's the reason why READ_ONLY is useful in my understanding.

[cowtowncoder]

Hmmh. Semantics of READ_ONLY predate introduction of Records, and with POJOs things are bit simpler. But I think that it is reasonable to expect Record to work same as a POJO with constrictor regarding this case. I am not 100% sure what that behavior is.

But I agree that at very least exception thrown is not good. So I think there is a valid issue here. If anyone has time to see how POJO would behave, that'd be useful information.

[yawkat]

normal beans behave the same way. this constructor:

        @JsonCreator
        public Bean(@JsonProperty("foo") String foo, @JsonProperty(value = "bar", access = JsonProperty.Access.READ_ONLY) String bar) {

fails with

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid type definition for type `com.fasterxml.jackson.databind.records.RecordImplicitName4119Test$Bean`: Argument #1 of constructor [constructor for `com.fasterxml.jackson.databind.records.RecordImplicitName4119Test$Bean` (2 args), annotations: {interface com.fasterxml.jackson.annotation.JsonCreator=@com.fasterxml.jackson.annotation.JsonCreator(mode=DEFAULT)} has no property name (and is not Injectable): can not use as property-based Creator
 at [Source: (String)"{"foo":"fizz","bar":"buzz"}"; line: 1, column: 1]

[cowtowncoder]

Ok. So at least we know handling is the same. Questions would then be:

1. Should READ_ONLY setting work via constructor -- and if so, would a null be passed?
2. If failure should occur (this use case not allowed), how to improve exception message.

[yawkat]

imo it should still throw an exception, just a better one.

[Mochis]

The access property is documented in this way in JsonProperty class:

/**
 * Optional property that may be used to change the way visibility of
 * accessors (getter, field-as-getter) and mutators (constructor parameter,
 * setter, field-as-setter) is determined, either so that otherwise
 * non-visible accessors (like private getters) may be used; or that
 * otherwise visible accessors are ignored.
 *<p>
 * Default value os {@link Access#AUTO} which means that access is determined
 * solely based on visibility and other annotations.
 *
 * @since 2.6
 */
Access access() default Access.AUTO;

Optional property that may be used to change the way visibility of accessors (...) and mutators (constructor parameter, ...) is determined

So, should READ_ONLY be an exception to this statement?

[yawkat]

in my opinion, the docs read like mutators (including creators) that mutate READ_ONLY properties will not be called. and this leads to the exception you see.

[Mochis]

I have been debugging and the exception happens before the data to be parsed is known.

It is my first time inspecting the internals of jackson code, so I could probably be wrong, but when I call a method which need to deserialize a JSON data into a Java object the first action is try to get from the cache a proper deserializer for that VO or create it.

This exception happens during deserializer creation if a constructor argument contains a property like READ_ONLY no matters what the data input is.

[yawkat]

Yes, it is not possible for jackson to deserialize an object if the creator/constructor would have to set a READ_ONLY property. That is reasonable behavior, it's just the error message that is awful.

[saugion]

After upgrading to 2.15 most of my records are broken, I used READ_ONLY a lot 🥲 Which fix do you suggest?

[cowtowncoder]

@saulgiordani Please no piling on existing issues unless you have the same problem. Sounds like yours is different so please file a new issue with details.

[saugion]

@cowtowncoder Sorry, I receive the same exception using the Lombok builder. Something that didn't happen before upgrading. As the exception Is exclty the same I guessed this was the right thread.

[cowtowncoder]

@saulgiordani But you specifically said

I used write_only a lot

and here issue is about READ_ONLY. Unless you meant "I use READ_ONLY a lot"?

Be that as it may, if you are hitting the same problem then you need to stop using READ_ONLY as it cannot work when using Constructor for deserialization, as per @yawkat. Regardless of behavior in 2.14.x it will not be made to work in 2.15 or later as looking at semantics of READ_ONLY annotation it should not work; it not being reported as an error seems erroneous behavior in itself.

[saugion]

I'm sorry @cowtowncoder it was my mistake. Will edit the message!

[Mochis]

I understand that the error message is not the best one, but taking into account that versions < 2.14.x were working with READ_ONLY with much probability a lot of apps will break with 2.15.x, so what is the alternative to use READ_ONLY or something similar in records to not deserialize some fields?

[cowtowncoder]

Deciding factor is whether it SHOULD HAVE worked in a way it did -- or was that a bug wrt semantics. Just because earlier version behaves in certain way does not automatically mean it was how it should have. Even in cases where users found that behavior useful. It is of course very unfortunate if this is the case -- what seems like useful Feature is seen by maintainers as a bug.

As to "a lot of apps", that is hard to estimate. I do not know, but I am bit surprised if it turns out READ_ONLY option is widely used. There have been some bug reports but I never it was a commonly used feature, fwtw. And we do not really have any usage metrics to check: rate of bug reports is not a strong indicator either.

But as to alternatives, plain @JsonIgnore on argument could work, but probably requires an explicit getter with @JsonProperty annotation.

Another, similarly awkward possibility would be to add a bogus setter that does nothing: it'd get called but value would be ignored. That setter might require @JsonProperty (or @JsonSetter).

Or, come to think of it, an explicit constructor that calls super() with same arguments but not passing read-only argument?

Writing all of this, I am starting to think maybe we should allow READ_ONLY to mean "partial ignore", given how awkward work-arounds are for Record types.

So I would consider PR to support it.

[cowtowncoder]

Ok, looking at test for issue #1890 (com.fasterxml.jackson.databind.deser.filter.ReadOnlyDeser1890Test), it appears we do accept such case, passing null instead of value. I wonder if this is accidental wrt annotations from Field not being merged with those for Creator parameters. But regardless, I think the behavior to support would indeed be not to fail but to pass null (and with possibly value defaulting wrt null setting (SKIP and IGNORE not doing anything, but EMPTY providing empty value).

Also: as per earlier comments, not specific to Records: works the same way for regular POJOs (as of 2.16.1)

[cowtowncoder]

Two places that seem relevant.

First, in POJOPropertyBuilder (where details of a single logical property are combined for POJOPropertiesCollector), method removeNonVisible (line 942) has:

      switch (acc) {
        case READ_ONLY:
            ...
            // Remove setters, creators for sure, but fields too if deserializing
            _setters = null;
            _ctorParameters = null;

which is where information will be dropped. Commenting this out will fail 2 tests for #1890 (where we retain property). It would also pass actual value through to Constructor instead of null.

Second potential place would be in BeanDeserializer method _deserializeUsingPropertyBased (line 414 or so) where actual deserialization occurs -- if we do not creator property we get here and could potentially ignore value. Maybe.

[cowtowncoder]

Looks like this was working with 3.0 (master), fwtw.

[cowtowncoder]

Fixed via #4515, will be in 2.18(.0).