Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConstructorDetector.USE_PROPERTIES_BASED does not work with multiple constructors since 2.18 #4860

Closed
1 task done
Saljack opened this issue Dec 20, 2024 · 9 comments
Closed
1 task done
Labels
Milestone

Comments

@Saljack
Copy link

Saljack commented Dec 20, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

I have this class:

public class Foo {
    private String id;
    private String name;

    public Foo() {
    }

    public Foo(String id) {
      this.id = id;
    }

    // getter setter omitted
} 

I use ConstructorDetector.USE_PROPERTIES_BASED configuration.

JsonMapper objectMapper = JsonMapper.builder().constructorDetector(ConstructorDetector.USE_PROPERTIES_BASED).build();

Since Jackson 2.18.0 I am no able to deserialize any JSON to this object. It worked in previous versions.
The error is:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: 
Invalid type definition for type `com.example.Foo`: Argument #0 of Creator [constructor for `com.example.Foo` (1 arg), annotations: [null] has no property name (and is not Injectable): can not use as property-based Creator
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 1]
	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:62)
	at com.fasterxml.jackson.databind.DeserializationContext.reportBadTypeDefinition(DeserializationContext.java:1866)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._addSelectedPropertiesBasedCreator(BasicDeserializerFactory.java:549)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._constructDefaultValueInstantiator(BasicDeserializerFactory.java:269)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findValueInstantiator(BasicDeserializerFactory.java:219)
	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:471)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:415)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:317)
	at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:284)
	at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:174)
	at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:669)
	at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:5048)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4918)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3860)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3828)

It does not deserialize these JSONs

{}
{"id": "something"}
{"id": "something", "name": "name"}
"something"

I would expect that it will work at least for {} (no args constructor) and {"id": "something"} (1 arg constructor with the same parameter name).

Version Information

2.18.0, 2.18.1 and 2.18.2 is affected
2.17.2 works

Reproduction

<-- Any of the following

  1. Brief code sample/snippet: include here in preformatted/code section
  2. Longer example stored somewhere else (diff repo, snippet), add a link
  3. Textual explanation: include here
    -->
public class Foo {
    private String id;
    private String name;

    public Foo() {
    }

    public Foo(String id) {
        this.id = id;
    }

    // getter setter omitted
   
   public static void main(String[] args) {
       // Test
       JsonMapper objectMapper = JsonMapper.builder()
          .constructorDetector(ConstructorDetector.USE_PROPERTIES_BASED).build();

        Foo foo = objectMapper.readValue("{}", Foo.class);
   }

}

Expected behavior

I would expect that it will deserialize JSON instead of throwing an exception.

Additional context

The object for deserialization is an external class and I cannot change it. I can use mixin to override it but I am afraid that this issue can happen with other external objects only if someone adds two constructors.s

@Saljack Saljack added the to-evaluate Issue that has been received but not yet evaluated label Dec 20, 2024
JooHyukKim added a commit to JooHyukKim/jackson-databind that referenced this issue Dec 20, 2024
@JooHyukKim
Copy link
Member

Filed #4861, hopefully enough to fix the issue.

I excluded case with "something", it does not seem to work in 2.17 as well.
@Saljack Lemme know if you think we should include "something" case.

@cowtowncoder
Copy link
Member

@Saljack quick note: please do not omit getters and setters for brevity -- their existence is important wrt property introspection. Reproduction should be faithful to actual use case.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 20, 2024

Actually, I am not sure this should work quite the way that submitter expects.
Automatic (no-annotation) detection is meant to only work if there is EXACTLY one constructor (including no 0-args constructor). But given there is 0-args constructor, that should instead be used.

Note, too, that Jackson does not select multiple properties-based constructors; they must resolve into to one. So there is no dynamic choice from { } into 0-args and others into 1-arg.

In this case it does not seem to matter as the end result is about same.

So I guess I do not quite understand why auto-detection works.

@cowtowncoder
Copy link
Member

@JooHyukKim plain "something" case cannot work (and should not work) without explicit delegating Creator so no it should not be tested for.

But support can be added with something like

@JsonCreator
public static Foo valueOf(String str) {
   return new Foo(str);
}

or @JsonCreator(mode = JsonCreator.Mode.DELEGATING) annotated Constructor.

But would not be auto-detected.

@Saljack
Copy link
Author

Saljack commented Dec 20, 2024

Sorry for confusion. Let me try to explain. I use ConstructorDetector.USE_PROPERTIES_BASED be cause I do not want "something" to be deserialize into this object (it is with other ConstructorDetector options because one arg string constructor is used).
I added "something" just to show that anything works if there are multiple constructors and I do not expect (or I do not want) to deserialize it.

My expectation is that Jackson uses no args constructor if there are multiple constructors and there is no @JsonCreator annotation. The whole problem is that you cannot deserialize into this class if you use ConstructorDetector.USE_PROPERTIES_BASED.

@cowtowncoder
Copy link
Member

@Saljack Ok, good thank you for adding more info -- that's how I think things should actually work. I am not 100% sure why auto-detection kicks in, need to dig deeper.

@cowtowncoder
Copy link
Member

Hmmh. Ok, no, for 2.18 this was changed slightly so that an implicit properties-based Creator MAY be detected not just if there's no 0-args constructor but also if ConstructorDetector.USE_PROPERTIES_BASED is enabled.

In POJOPropertiesCollector 718:

        // One more thing: if neither explicit (constructor or factory) nor
        // canonical (constructor?), consider implicit Constructor with
        // all named.
        final ConstructorDetector ctorDetector = _config.getConstructorDetector();
        if (!creators.hasPropertiesBasedOrDelegating()
                && !ctorDetector.requireCtorAnnotation()) {
            // But only if no Default (0-args) constructor available OR if we are configured
            // to prefer properties-based Creators
            if ((_classDef.getDefaultConstructor() == null)
                    || ctorDetector.singleArgCreatorDefaultsToProperties()) {
                _addImplicitConstructor(creators, constructors, props);
            }
        }

@cowtowncoder
Copy link
Member

Ok so what I think is specifically needed, to address the failure is this: we can only auto-detect Properties-based Constructor if (and only if) it:

  • Has either Injectable definition OR implicit name for EVERY parameter

This is being checked for multi-parameter case, but not for 1-parameter.
I will add that check.

There will still be auto-detection, however; I will not change that -- but in this case without parameter-names module it won't kick in.

@cowtowncoder cowtowncoder changed the title ConstructorDetector.USE_PROPERTIES_BASED does not work with multiple constructors since 2.18 ConstructorDetector.USE_PROPERTIES_BASED does not work with multiple constructors since 2.18 Dec 21, 2024
@cowtowncoder cowtowncoder added 2.18 and removed to-evaluate Issue that has been received but not yet evaluated labels Dec 21, 2024
@cowtowncoder cowtowncoder added this to the 2.18.3 milestone Dec 21, 2024
@JooHyukKim
Copy link
Member

Thank you for reporting @Saljack !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants