Fix #4688 2.18.0-rc1 Regression deserializing with no-arg delegating JsonCreator

[carterkozak]

(fixes #4688)

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?

I've included a potential patch which recovers the previous behavior.

[carterkozak]

It's not entirely clear if we should use a property creator with no properties, or creators.setDefaultCreator(candidate.creator());. The former seems more explicit, but the latter more closely matches the previous implementation.

[cowtowncoder]

I agree with your choice; let's go with that (possible to change, too, if need arises).

[carterkozak]

I'm not attached to the proposed fix, happy to take another path if you prefer, the tests are the piece I wanted to convey :-)

[cowtowncoder]

Ok so would the idea here be to basically drop any JSON value that would match this type?
(I guess it must).

This was not something intended to work, although if it did use to work there's something to be said for not changing behavior. Will add a note on issue itself.

[carterkozak]

Yes, our intent was to explicitly match an empty json object on the server-side (where we fail-on-unknown-fields) and allow clients which do not set that property to be api-compatible with future additions to the type.

[cowtowncoder]

@carterkozak Very good, I like this!

[cowtowncoder]

Thank you @carterkozak for reporting & fixing this issue so quickly!!!

[carterkozak]

Thank you so much for the review :-)