FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.53k stars 1.38k forks source link

`StdValueInstantiator.withArgsCreator` is now set for creators with no arguments #4777

Closed k163377 closed 1 week ago

k163377 commented 3 weeks ago

Search before asking

Describe the bug

This changes the behavior of StdValueInstantiator when deserializing, resulting in the following problem in kotlin-module. https://github.com/FasterXML/jackson-module-kotlin/issues/841

From @JooHyukKim 's research, this has changed the result of the _vanillaProcessing determination. https://github.com/FasterXML/jackson-module-kotlin/issues/841#issuecomment-2444178093

I have confirmed that this value is populated by StdValueInstantiator.configurationFromObjectSettings, but have not been able to do a deeper analysis.

Version Information

2.18

Reproduction

The following code succeeds in 2.17 but fails in 2.18.

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.deser.ValueInstantiator;
import com.fasterxml.jackson.databind.deser.ValueInstantiators;
import com.fasterxml.jackson.databind.deser.std.StdValueInstantiator;
import com.fasterxml.jackson.databind.module.SimpleModule;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class Kotlin841Sample {
    static class Foo {
        @JsonCreator
        static Foo create() { return new Foo(); }
    }

    static class Instantiator extends StdValueInstantiator {
        public Instantiator(StdValueInstantiator src) {
            super(src);
        }
    }

    static class Instantiators implements ValueInstantiators {
        Instantiator last = null;

        @Override
        public ValueInstantiator findValueInstantiator(
                DeserializationConfig config,
                BeanDescription beanDesc,
                ValueInstantiator defaultInstantiator
        ) {
            if (defaultInstantiator instanceof StdValueInstantiator) {
                Instantiator instantiator = new Instantiator((StdValueInstantiator) defaultInstantiator);
                last = instantiator;
                return instantiator;
            } else {
                return defaultInstantiator;
            }
        }
    }

    @Test
    public void test() throws Exception {
        Instantiators i = new Instantiators();

        SimpleModule sm = new SimpleModule() {
            @Override
            public void setupModule(SetupContext context) {
                super.setupModule(context);
                context.addValueInstantiators(i);
            }
        };
        ObjectMapper mapper = JsonMapper.builder().addModule(sm).build();

        mapper.readValue("{}", Foo.class);

        Assertions.assertNull(i.last.getWithArgsCreator());
    }
}

Expected behavior

It must be the same as in 2.17.

Additional context

No response

cowtowncoder commented 3 weeks ago

I don't really like this test, as it tries to verify internal state, instead of showing externally observable problem: something users would hit. But perhaps this is because issue is otherwise hard to reproduce outside Kotlin module?

But even without that, test logic is pretty obscure: I don't think it really tests something documented to behave certain way.

But I'll play with the test, see what I can find.

k163377 commented 3 weeks ago

But perhaps this is because issue is otherwise hard to reproduce outside Kotlin module?

That is correct. I would have liked to correct the problem, but the cause of such a result seems to be very deep, and it was difficult to analyze it without understanding the structure.

cowtowncoder commented 3 weeks ago

Ok that makes sense then.

cowtowncoder commented 3 weeks ago

Uhhh. Realized one mistake I made with naming: "Default Creator" already had a meaning in Jackson, pre-2.18 -- 0-argument Constructor/Factory method. So calling Record type's Canonical Contructor Default Creator adds unfortunate overloading. It really should have been called maybe Primary Creator. Not sure how I missed this conflict when thinking about naming because now:

Since I cannot really change API, some of this remains. But I may try to change code comments to better reflect new addition.

cowtowncoder commented 3 weeks ago

As to the change itself: I think only Constructors are detected as "default creators" on 2.18: explicitly annotated factory methods will all be considered as such, no special handling for 0-arg one. Databind works fine with that.

To change this, two approaches, both in POJOPropertiesCollector (mostly?), could be tried:

  1. Detect 0-args factory case, keep track of factory Default creator (not just Constructor one)
  2. Collect Creators like now, but before constructing ValueInstantiator, see if there's explicitly annotated 0-args factory method collected (i.e. fix at a later point)
cowtowncoder commented 1 week ago

@k163377 Ok, this is now fixed in 2.18 branch for 2.18.2. Could you see if this helps on Kotlin module side, with 2.18.2-SNAPSHOT of jackson-databind?

k163377 commented 1 week ago

I have confirmed that https://github.com/FasterXML/jackson-module-kotlin/issues/841 has been fixed. Thank you very much!

cowtowncoder commented 1 week ago

@k163377 Great! Thank you for confirming.