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

If JacksonInject is specified for field and deserialized by the Creator, the inject process will be executed twice. #4218

Open
1 task done
k163377 opened this issue Nov 25, 2023 · 6 comments
Labels
2.17 Issues planned at earliest for 2.17 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue

Comments

@k163377
Copy link
Contributor

k163377 commented Nov 25, 2023

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

SSIA

Version Information

2.16.0

Reproduction

The following is a sample using InjectableValues that is made to count up when findInjectableValue is called.
Since it is deserialized only once, the first ID is expected, but it is actually the second ID.

import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.*;
import org.junit.jupiter.api.Test;

import java.util.Arrays;
import java.util.List;
import java.util.UUID;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class JacksonInjectForFieldTest {
    static class Dto {
        @JacksonInject("uuid")
        private final UUID uuid;

        @JsonCreator
        Dto(
                // @JacksonInject("uuid")
                @JsonProperty("uuid")
                UUID uuid
        ) {
            this.uuid = uuid;
        }
    }

    List<UUID> ids = Arrays.asList(UUID.randomUUID(), UUID.randomUUID());
    class MyInjectableValues extends InjectableValues.Std {
        int count = 0; // count up if injected

        @Override
        public Object findInjectableValue(
                Object valueId,
                DeserializationContext ctxt,
                BeanProperty forProperty,
                Object beanInstance
        ) throws JsonMappingException {
            if (valueId.equals("uuid")) {
                return ids.get(count++);
            } else {
                return super.findInjectableValue(valueId, ctxt, forProperty, beanInstance);
            }
        }
    }

    @Test
    void test() throws JsonProcessingException {
        ObjectReader reader = new ObjectMapper()
                .readerFor(Dto.class)
                .with(new MyInjectableValues());

        Dto dto = reader.readValue("{}");
        UUID actual = dto.uuid;

        System.out.println(ids);
        System.out.println(ids.get(0) == actual);
        System.out.println(ids.get(1) == actual);

        assertEquals(ids.get(0), actual);
    }
}

Expected behavior

The inject process is executed only once.
In fact, if a specification is made for a parameter, the result is as expected.

Additional context

After processing in the creator, another injection is performed on the field, so it is called twice in total.

I commented but forgot to mention the above initially.
#4218 (comment)

@k163377 k163377 added the to-evaluate Issue that has been received but not yet evaluated label Nov 25, 2023
@mukham12
Copy link
Contributor

mukham12 commented Nov 26, 2023

Investigated this, and it appears the hiccup occurs when we apply @JacksonInject to both the constructor parameter and the class field. Things work smoothly if we annotate either-or. This issue is quite similar to #2678.

@k163377,
Any particular reason for double-annotating 🤔? Curious to know more and thanks for flagging this. Let's figure it out together 🚀.

@cowtowncoder,
Identified the issue in the snippet: at line 526, the bean returns with an initialized class and an injected value. We subsequently inject again at line 533, replacing the old injected value.

Object bean;
try {
bean = creator.build(ctxt, buffer);
} catch (Exception e) {
wrapInstantiationProblem(e, ctxt);
bean = null; // never gets here
}
// 13-Apr-2020, tatu: [databind#2678] need to handle injection here
if (_injectables != null) {
injectValues(ctxt, bean);
}

@k163377
Copy link
Contributor Author

k163377 commented Nov 26, 2023

Sorry, I forgot to mention an important fact.
After processing in the creator, another injection is performed on the field, so it is called twice in total.
Perhaps that is the code you are analyzing.

Any particular reason for double-annotating 🤔? Curious to know more and thanks for flagging this. Let's figure it out together 🚀.

Is this a question about the separate annotations for creator and field in the sample code?
The reason is that this code is a Java reproduction of a behavior discovered during the development of jackson-module-kotlin, which was originally Kotlin code.

In the special case of Kotlin (constructors with value class as an argument), annotations added to constructor parameters cannot be parsed, so I added them explicitly to the field and found this behavior.
https://github.com/ProjectMapK/jackson-module-kogera/blob/develop/src/test/kotlin/io/github/projectmapk/jackson/module/kogera/zIntegration/deser/valueClass/JacksonInjectTest.kt

@k163377 k163377 changed the title If JacksonInject is specified for field and deserialized by the Creator, the inject process will be executed multiple twice. If JacksonInject is specified for field and deserialized by the Creator, the inject process will be executed twice. Nov 26, 2023
@mukham12
Copy link
Contributor

Yes, that is what I was wondering about. You have a commented @JacksonInject annotation in the constructor.

But if that is the case then the test ran successfully for me in Java.

@cowtowncoder
Copy link
Member

This does sound like a bug, and I agree: injection should only happen once per property.

@cowtowncoder cowtowncoder added has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue 2.16 Issues planned for 2.16 and removed to-evaluate Issue that has been received but not yet evaluated labels Nov 29, 2023
cowtowncoder added a commit that referenced this issue Nov 29, 2023
@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 29, 2023

Added failing test. Quick note on double processing: commenting out injection does not work as that fails an existing test for #471 (and as per comment next to injection, also #2678 but that is still failing for other reasons).

So fix isn't quite as easy.

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 29, 2023

Ok, did some more digging. It does look like POJOPropertiesCollector handles introspection and merging of injectables; and does collect _creatorProperties too. But no attempt seems to be made to try to avoid duplication between creator properties (that is, associating Injectable Id with Creator parameter) and regular property accessors (Fields, Setters).
I think conceptually it should be possible to just iterate over creator parameters and see if they have inject values and remove matching id from Field/Setter set... but that does not seem as straight-forward as it should.

Part of the problem is that instead of associating Injectables with POJOPropertyBuilder (instance of which represents information collected for one logical property), they are collected separately by POJOPropertiesCollector (which keeps track of set of all POJO Properties, that is, POJOPropertyBuilders.

@cowtowncoder cowtowncoder added 2.17 Issues planned at earliest for 2.17 and removed 2.16 Issues planned for 2.16 labels Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 Issues planned at earliest for 2.17 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

3 participants