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

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

Open
1 task done
k163377 opened this issue Nov 2, 2024 · 5 comments
Open
1 task done
Labels
2.18 to-evaluate Issue that has been received but not yet evaluated

Comments

@k163377
Copy link
Contributor

k163377 commented Nov 2, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

This changes the behavior of StdValueInstantiator when deserializing, resulting in the following problem in kotlin-module.
FasterXML/jackson-module-kotlin#841

From @JooHyukKim 's research, this has changed the result of the _vanillaProcessing determination.
FasterXML/jackson-module-kotlin#841 (comment)

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
Copy link
Member

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
Copy link
Contributor Author

k163377 commented Nov 3, 2024

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
Copy link
Member

Ok that makes sense then.

@cowtowncoder
Copy link
Member

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:

  • AnnotationIntrospector has new findDefaultCreator() which is for finding such Primary creator
  • StdValueInstantiator has _defaultCreator which is specifically for 0-args Creator.

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

@cowtowncoder
Copy link
Member

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 added a commit that referenced this issue Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.18 to-evaluate Issue that has been received but not yet evaluated
Projects
None yet
Development

No branches or pull requests

2 participants