Skip to content

Commit

Permalink
Fix FasterXML#4688 2.18.0-rc1 Regression deserializing with no-arg de…
Browse files Browse the repository at this point in the history
…legating JsonCreator

The refactor to bean property introspection in FasterXML#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:
https://github.com/FasterXML/jackson-databind/blob/091d968751ef00150d22a788fe7f45b7cdcb337a/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java#L658-L662

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.
  • Loading branch information
carterkozak committed Sep 4, 2024
1 parent 88f4671 commit 793e2c9
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,10 @@ private boolean _addExplicitDelegatingCreator(DeserializationContext ctxt,
int ix = -1;
final int argCount = candidate.paramCount();
SettableBeanProperty[] properties = new SettableBeanProperty[argCount];
if (argCount == 0) {
creators.addPropertyCreator(candidate.creator(), true, properties);
return true;
}
for (int i = 0; i < argCount; ++i) {
AnnotatedParameter param = candidate.parameter(i);
JacksonInject.Value injectId = candidate.injection(i);
Expand All @@ -484,6 +488,7 @@ private boolean _addExplicitDelegatingCreator(DeserializationContext ctxt,
"More than one argument (#%d and #%d) left as delegating for Creator %s: only one allowed",
ix, i, candidate);
}

// Also, let's require that one Delegating argument does exist
if (ix < 0) {
ctxt.reportBadTypeDefinition(beanDesc,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package com.fasterxml.jackson.databind.deser.creators;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Test;

import static com.fasterxml.jackson.databind.testutil.DatabindTestUtil.newJsonMapper;
import static org.junit.jupiter.api.Assertions.assertSame;

// [databind#4688]
public class SingletonDelegatingCreatorTest
{

static final class NoFieldSingletonWithDelegatingCreator {
private static final NoFieldSingletonWithDelegatingCreator INSTANCE = new NoFieldSingletonWithDelegatingCreator();

private NoFieldSingletonWithDelegatingCreator() {}

@JsonCreator(mode = JsonCreator.Mode.DELEGATING)
static NoFieldSingletonWithDelegatingCreator of() {
return INSTANCE;
}
}

static final class NoFieldSingletonWithPropertiesCreator {
private static final NoFieldSingletonWithPropertiesCreator INSTANCE = new NoFieldSingletonWithPropertiesCreator();

private NoFieldSingletonWithPropertiesCreator() {}

@JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
static NoFieldSingletonWithPropertiesCreator of() {
return INSTANCE;
}
}

static final class NoFieldSingletonWithDefaultCreator {
private static final NoFieldSingletonWithDefaultCreator INSTANCE = new NoFieldSingletonWithDefaultCreator();

private NoFieldSingletonWithDefaultCreator() {}

@JsonCreator
static NoFieldSingletonWithDefaultCreator of() {
return INSTANCE;
}
}

/*
/**********************************************************************
/* Test methods
/**********************************************************************
*/

private final ObjectMapper MAPPER = newJsonMapper();

@Test
public void testNoFieldSingletonWithDelegatingCreator() throws Exception
{
NoFieldSingletonWithDelegatingCreator deserialized = MAPPER.readValue("{}",
NoFieldSingletonWithDelegatingCreator.class);
assertSame(NoFieldSingletonWithDelegatingCreator.INSTANCE, deserialized);
}

@Test
public void testNoFieldSingletonWithPropertiesCreator() throws Exception
{
NoFieldSingletonWithPropertiesCreator deserialized = MAPPER.readValue("{}",
NoFieldSingletonWithPropertiesCreator.class);
assertSame(NoFieldSingletonWithPropertiesCreator.INSTANCE, deserialized);
}

@Test
public void testNoFieldSingletonWithDefaultCreator() throws Exception
{
NoFieldSingletonWithDefaultCreator deserialized = MAPPER.readValue("{}",
NoFieldSingletonWithDefaultCreator.class);
assertSame(NoFieldSingletonWithDefaultCreator.INSTANCE, deserialized);
}
}

0 comments on commit 793e2c9

Please sign in to comment.