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

`@JacksonInject` added to property overrides value from the JSON even if `useInput` is `OptBoolean.TRUE` #2678

Open ptanov opened 4 years ago

ptanov commented 4 years ago

Hello, I have problem when using @JacksonInject on final field that is initialized using constructor (see the whole example at the end of the message).

The problem is that the correct value from JSON is used when the constructor is called, see the stacktrace:

AnnotatedConstructor.call(Object[]) line: 124   
StdValueInstantiator.createFromObjectWith(DeserializationContext, Object[]) line: 283   
StdValueInstantiator(ValueInstantiator).createFromObjectWith(DeserializationContext, SettableBeanProperty[], PropertyValueBuffer) line: 229 
PropertyBasedCreator.build(DeserializationContext, PropertyValueBuffer) line: 195   
BeanDeserializer._deserializeUsingPropertyBased(JsonParser, DeserializationContext) line: 422   
BeanDeserializer(BeanDeserializerBase).deserializeFromObjectUsingNonDefault(JsonParser, DeserializationContext) line: 1287  
BeanDeserializer.deserializeFromObject(JsonParser, DeserializationContext) line: 326    
BeanDeserializer.deserialize(JsonParser, DeserializationContext) line: 159  
ObjectMapper._readMapAndClose(JsonParser, JavaType) line: 4013  
ObjectMapper.readValue(String, Class<T>) line: 3004 
JacksonInjectTest.testReadValueInjectables()

But later this value is overrided by the default value of the injectable (but useInput is OptBoolean.TRUE) because _injectables != null at BeanDeserializer.deserialize(JsonParser, DeserializationContext, Object) line: 216, full stacktrace:

AnnotatedField.setValue(Object, Object) line: 105   
ValueInjector.inject(DeserializationContext, Object) line: 51   
BeanDeserializer(BeanDeserializerBase).injectValues(DeserializationContext, Object) line: 1520  
BeanDeserializer.deserialize(JsonParser, DeserializationContext, Object) line: 217  
BeanDeserializer._deserializeUsingPropertyBased(JsonParser, DeserializationContext) line: 441   
BeanDeserializer(BeanDeserializerBase).deserializeFromObjectUsingNonDefault(JsonParser, DeserializationContext) line: 1287  
BeanDeserializer.deserializeFromObject(JsonParser, DeserializationContext) line: 326    
BeanDeserializer.deserialize(JsonParser, DeserializationContext) line: 159  
ObjectMapper._readMapAndClose(JsonParser, JavaType) line: 4013  
ObjectMapper.readValue(String, Class<T>) line: 3004 
JacksonInjectTest.testReadValueInjectables()    

This happens even if I set mapper.configure(MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS, false); which is weired.

Here is the whole example:

package com.drooms.semantic.flink.jobs;

import static java.util.Objects.requireNonNull;
import static org.testng.Assert.assertEquals;

import java.io.IOException;

import org.testng.annotations.Test;

import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.OptBoolean;
import com.fasterxml.jackson.core.JsonParseException;
import com.fasterxml.jackson.databind.InjectableValues;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;

public class JacksonInjectTest {
    protected static class Some {
        private final String field1;

        @JacksonInject(value = "defaultValueForField2", useInput = OptBoolean.TRUE)
        private final String field2;

        public Some(@JsonProperty("field1") final String field1,
                @JsonProperty("field2") @JacksonInject(value = "defaultValueForField2",
                        useInput = OptBoolean.TRUE) final String field2) {
            this.field1 = requireNonNull(field1);
            this.field2 = requireNonNull(field2);
        }

        public String getField1() {
            return field1;
        }

        public String getField2() {
            return field2;
        }
    }

    @Test
    public void testReadValueInjectables() throws JsonParseException, JsonMappingException, IOException {
        final ObjectMapper mapper = new ObjectMapper();
        final InjectableValues injectableValues =
                new InjectableValues.Std().addValue("defaultValueForField2", "somedefaultValue");
        mapper.setInjectableValues(injectableValues);

        final Some actualValueMissing = mapper.readValue("{\"field1\": \"field1value\"}", Some.class);
        assertEquals(actualValueMissing.getField1(), "field1value");
        assertEquals(actualValueMissing.getField2(), "somedefaultValue");

        final Some actualValuePresent =
                mapper.readValue("{\"field1\": \"field1value\", \"field2\": \"field2value\"}", Some.class);
        assertEquals(actualValuePresent.getField1(), "field1value");
        assertEquals(actualValuePresent.getField2(), "somedefaultValue");
        // if I comment @JacksonInject that is next to the property the valid assert is the correct one:
        // assertEquals(actualValuePresent.getField2(), "field2value");
    }
}

In my original case I'm using lombok to generate the constructor and the annotation is copied from property to the constructor parameter (you can check it here: https://github.com/rzwitserloot/lombok/issues/1528#issuecomment-607725333 ). You can check https://stackoverflow.com/a/60987798/2054634 for more information on the context of the problem.

P.S. I'm using jackson v. 2.9.6

Thanks in advance, Plamen

cowtowncoder commented 4 years ago

Thank you for reporting the issue: without looking into it, sounds legit, hope to have time to look into this soon.

ptanov commented 4 years ago

Thanks, take your time.

cowtowncoder commented 4 years ago

One quick note: looks like adding @JacksonInject on field causes the problem, so a work-around is to remove it from there. I'll try to see how if there is a way to prevent unwanted injection in described case.

cowtowncoder commented 4 years ago

I can see why this occurs, and what would be needed to prevent it at high level -- injection via properties already passed via Creator should be prevented. But not sure where this could be prevented.

ptanov commented 4 years ago

One quick note: looks like adding @JacksonInject on field causes the problem, so a work-around is to remove it from there. I'll try to see how if there is a way to prevent unwanted injection in described case.

Thank you for looking into this issue. Yes, when I remove it - everything is fine, but the problem is that in my case the constructor is autogenerated by lombok so I don't have mechanism to put the annotation in the constructor without having it on the field as described here: https://github.com/rzwitserloot/lombok/issues/1528#issuecomment-607725333

I can see why this occurs, and what would be needed to prevent it at high level -- injection via properties already passed via Creator should be prevented. But not sure where this could be prevented.

Yes, me too, that's why I didn't provided a fix. Probably we can store all already injected values in a new list here: https://github.com/FasterXML/jackson-databind/blob/jackson-databind-2.9.6/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializer.java#L417 and then we can filter them before injecting them for the second time here: https://github.com/FasterXML/jackson-databind/blob/jackson-databind-2.9.6/src/main/java/com/fasterxml/jackson/databind/deser/BeanDeserializer.java#L217 But I'm not pretty familiar with the jackson's code and don't want to miss something, introduce wrong mechanism or broke something.

cowtowncoder commented 4 years ago

@ptanov Yeah it is tricky as deserializer themselves are stateless, so most likely I'll need to try to remove injector for non-creator properties -- I think injectors applied via creator parameters go through different route. There is unfortunate duality between "regular" properties (fields, methods) and creator properties, stemming from creator properties added later on (in jackson .... 1.5 or something). My hope is to rewrite introspection parts for Jackson 3.0 to resolve some of long-standing issues.

But I think I may be able to resolve this particular problem in 2.x without bigger rewrite.

Thank you very much for the investigation!

ptanov commented 4 years ago

:+1: Thank you very much!

cowtowncoder commented 4 years ago

Actually, digging deeper into this, I am starting to think that this might not be solvable in general. My thinking is based on realizing that @JacksonInject is not applied into logical property, either wrt implementation (since it has to be available for fields/methods that do not acts as setter) or even logically. Rather it attaches to specific mutator (field, 1-arg method, constructor parameter) which may or may not also be mutator of a logical property. Injectable things do not have logical name (underlying member has), so they can not be -- in all cases at least -- bundled up.

Now... I could probably solve this particular case with heuristic, however: given that field is private AND there is annotated constructor, I could filter out specific field injection. I think I could try that, but since it might be slightly risky, will consider for 2.12.

ptanov commented 4 years ago

Thank you very much!