Should allow deserializing with no-arg @JsonCreator(mode = DELEGATING) (regression)

[carterkozak]

Search before asking

• I searched in the issues and found nothing similar.

Describe the bug

2.18.0-rc1 Regression deserializing with no-arg @JsonCreator(mode = DELEGATING)

The refactor to bean property introspection in #4515 caused
existing no-arg delegatring constructors to fail deserialization.

Example from this branch where we test RC releases:
palantir/conjure-java#2349
specifically this test:
https://github.com/palantir/conjure-java/blob/19d7f54eb0001c49b5d8a4bb897b9ad4cb5a28de/conjure-java-core/src/test/java/com/palantir/conjure/java/types/NoFieldBeanTests.java#L36-L39
Using this type:
https://github.com/palantir/conjure-java/blob/19d7f54eb0001c49b5d8a4bb897b9ad4cb5a28de/conjure-java-core/src/integrationInput/java/com/palantir/product/EmptyObjectExample.java

InvalidDefinitionException: Invalid type definition for type `com.palantir.product.EmptyObjectExample`: No argument left as delegating for Creator [method com.palantir.product.EmptyObjectExample#of()]: exactly one required
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 1]
        at app//com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:62)
        at app//com.fasterxml.jackson.databind.DeserializationContext.reportBadTypeDefinition(DeserializationContext.java:1866)
        at app//com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._addExplicitDelegatingCreator(BasicDeserializerFactory.java:489)
        at app//com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._addExplicitDelegatingCreators(BasicDeserializerFactory.java:365)
        at app//com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._constructDefaultValueInstantiator(BasicDeserializerFactory.java:270)
        at app//com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findValueInstantiator(BasicDeserializerFactory.java:219)
        at app//com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.buildBeanDeserializer(BeanDeserializerFactory.java:262)
        at app//com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.createBeanDeserializer(BeanDeserializerFactory.java:151)
        at app//com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer2(DeserializerCache.java:471)
        at app//com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:415)
        at app//com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:317)
        at app//com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:284)
        at app//com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:174)
        at app//com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:669)
        at app//com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:5048)
        at app//com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4918)
        at app//com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3860)
        at app//com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3828)
        at app//com.palantir.conjure.java.types.NoFieldBeanTests.testDeserializeUsesSingleton(NoFieldBeanTests.java:38)

It's not entirely clear that we are correctly using the DELEGATING type here,
perhaps the PROPERTIES type would be a better fit, however neither necessarily
document support for a no-arg constructor. In general we prefer DELEGATING
when possible to avoid ambiguity with potential property sources -- we want
the ability to fail deserialization in this case when any properties are included
in the incoming object.

In 2.17.2, this branch allowed the code to functiona as we desired:

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

Lines 658 to 662 in 091d968

	// zero-arg method factory methods fine, as long as explicit
	if (argCount == 0) {
	creators.setDefaultCreator(factory);
	continue;
	}

Is there any chance we could allow the previous DELEGATING behavior to continue
to handle the no-arg case? If we'd prefer to move away from DELEGATING for that
case, is there any chance we could emit a deprecation warning for some time?

Version Information

2.18.0-rc1

Reproduction

palantir/conjure-java#2349
specifically this test:
https://github.com/palantir/conjure-java/blob/19d7f54eb0001c49b5d8a4bb897b9ad4cb5a28de/conjure-java-core/src/test/java/com/palantir/conjure/java/types/NoFieldBeanTests.java#L36-L39
Using this type:
https://github.com/palantir/conjure-java/blob/19d7f54eb0001c49b5d8a4bb897b9ad4cb5a28de/conjure-java-core/src/integrationInput/java/com/palantir/product/EmptyObjectExample.java

I will push a PR to jackson-databind which includes a reproducer unit test once I have an issue number from this report :-)
edit: I've created a PR here: #4689

Expected behavior

Ideally no-arg delegating jsoncreators would still be called without throwing an exception.

Additional context

I appreciate that it's possible I'm misusing @JsonCreator(mode = DELEGATING) here, however it's not clear that PROPERTIES is a better option in this case, thought it does appear to work for the time being. Unfortunately the code which uses this pattern is generated using another library I maintain, so it can take months for old api bindings to be replaced, potentially preventing me from upgrading to and beyond 2.18 for much longer than I'm comfortable with!

[cowtowncoder]

Thank you for reporting this -- will try to read with thought later tonight.

[cowtowncoder]

Ok, one quick note: no-arguments case should result in PROPERTIES, as Delegating case has to have 1-and-only-1 parameter being mapped from incoming delegate value. PROPERTIES can have any number of parameters, including 0, wrt definition.
Determination here is not based on incoming value but just definition.

[cowtowncoder]

Ah ok, this is about explicitly annotated mode; I was thinking of auto-detection.

Now, although I do think 0-parameter Creator wouldn't quite be a match to Delegating, I guess I can see why someone might want to use it: to basically Skip whatever value in input matches Java value to product. And whereas for PROPERTIES value would have to be (JSON) Object, for Delegating it could be any value type.

So I am ok in explicit mode = JsonCreator.Mode.DELEGATING being used for no parameters case.

[carterkozak]

I can add some context around why we chose to annotate this way: Our goal was to explicitly match an empty json object {} in a way that allows us to make backward compatible API modifications in the future (we did not intend to match non-object types though). We didn't want to use the default JsonCreator mode out of fear that it may not be as precise, and the decision between DELEGATING and PROPERTIES mode was not terribly clear, but given that there were no declared properties, the the properties option didn't seem like the obvious correct choice.

I agree with your assessment that PROPERTIES would have been a better fit in retrospect. I can begin the migration to to use a no-args properties creator, but it will take a few months for most of my consumers to migrate and become compatible with new jackson releases. I'd be thrilled to retain the previous behavior for some time (even perhaps with a deprecation warning and planned removal in a future release if that's preferable to keep the implementation clean).