4

Considering the following (failing) JUnit test for Jackson 2.9.5:

package de.azamir.test;

import static org.junit.Assert.assertEquals;

import java.io.IOException;

import org.junit.Test;

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

public class JacksonFinalTest {

    private static final class MyClass {

        @JsonProperty("myProperty")
        public final String myProperty = "myPropertyValueInit";
    }

    @Test
    public void doTest() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException, JsonParseException, JsonMappingException, IOException {

        final MyClass deserializedMyClass1 = new ObjectMapper().readValue("{\"myProperty\":\"myPropertyValueDeserialized\"}", MyClass.class);

        // "myPropertyValueInit"
        final String directValue = deserializedMyClass1.myProperty;

        // "myPropertyValueDeserialized"
        final String reflectValue = (String) MyClass.class.getDeclaredField("myProperty").get(deserializedMyClass1);

        assertEquals(directValue, reflectValue);

    }
}

I suspect the Java VM to optimize accesses to final fields such that the value written by Jackson (via reflection) can only be retrieved again via reflection. This just today lead to a very subtle and hard to find bug in our codebase.

Is this a problem that has been addressed in one way or another by Jackson? My gut feeling would be that there is no way for Jackson code do "see" this but who knows.

I know there are multiple ways of improving this code such as

  • removing the @JsonProperty and disabling final fields as mutators
  • not initializing the field during declaration but in a constructor
  • not making the fields final at all

but is there a recommended way of making sure that no developer in our team (who might not know the caveat) does this?

  • Possible relevant: https://stackoverflow.com/a/3301720/6473398 with links to https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.28 and https://stackoverflow.com/questions/17506329/java-final-field-compile-time-constant-expression – sfiss Feb 21 '20 at 15:07

2 Answers2

0

So there is a way: (idea from this answer, also nice to read is this question) Instead of using the constant expression and directly assign the variable at declaration site, init your final variables in your constructor. See the changes when you use MyClass2:

public class JacksonFinalTest {

    private static final class MyClass {

        @JsonProperty("myProperty")
        public final String myProperty = "myPropertyValueInit";
    }

    private static final class MyClass2 {

        @JsonProperty("myProperty")
        public final String myProperty;

        public MyClass2() {
            myProperty = "myPropertyValueInit";
        }
    }

    @Test
    public void doTest() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException, JsonParseException, JsonMappingException, IOException {

        final String json = "{\"myProperty\":\"myPropertyValueDeserialized\"}";
        final MyClass2 deserializedMyClass1 = new ObjectMapper().readValue(json, MyClass2.class);

        // "myPropertyValueInit"
        final String directValue = deserializedMyClass1.myProperty;

        // "myPropertyValueDeserialized"
        final String reflectValue = (String) MyClass2.class.getDeclaredField("myProperty").get(deserializedMyClass1);

        assertEquals(directValue, reflectValue);

    }
}
sfiss
  • 2,119
  • 13
  • 19
  • I know of this solution but would have hoped there was a way of raising some kind of red flag - either during compile or during runtime. I'll look into writing a static code analysis rule for this in PMD – Frederic Beister Feb 21 '20 at 15:18
  • Accepting this as it *mostly* seems to be the way to go. For reference, The PMD rule I wrote was (XPath) ```//ClassOrInterfaceBodyDeclaration[ FieldDeclaration[ @Final=boolean(1) and VariableDeclarator[ VariableInitializer[ Expression ] ] ] and Annotation[ SingleMemberAnnotation[ @AnnotationName="JsonProperty" ] ] ]``` – Frederic Beister Feb 27 '20 at 10:23
0

I just found another solution that causes a RuntimeException. You can check the field during deserialization using a BeanDeserializationModifier (inspired by this answer):

    @Test
    public void doTest() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException, JsonParseException, JsonMappingException, IOException {

        final BeanDeserializerModifier modifier = new BeanDeserializerModifier() {

            @Override
            public List<BeanPropertyDefinition> updateProperties(final DeserializationConfig config, final BeanDescription beanDesc, final List<BeanPropertyDefinition> propDefs) {
                propDefs.stream()
                        .filter(pd -> pd.hasField())
                        .forEach(propDef -> {
                            final AnnotatedField f = propDef.getField();
                            if (Modifier.isFinal(f.getModifiers())) {
                                throw new RuntimeException("Bad final field " + f.getFullName());
                            }
                        });

                return super.updateProperties(config, beanDesc, propDefs);
            }

        };
        final DeserializerFactory dFactory = BeanDeserializerFactory.instance.withDeserializerModifier(modifier);
        final ObjectMapper mapper = new ObjectMapper(null, null, new DefaultDeserializationContext.Impl(dFactory));

        final ObjectMapper objectMapper = mapper;
        final String json = "{\"myProperty\":\"myPropertyValueDeserialized\"}";
        final MyClass deserializedMyClass1 = objectMapper.readValue(json, MyClass.class);

        // "myPropertyValueInit"
        final String directValue = deserializedMyClass1.myProperty;

        // "myPropertyValueDeserialized"
        final String reflectValue = (String) MyClass.class.getDeclaredField("myProperty").get(deserializedMyClass1);

        assertEquals(directValue, reflectValue);

    }
sfiss
  • 2,119
  • 13
  • 19
  • Looks good, thanks. Sadly this also trips on the constructor-initializing solution you mentioned in https://stackoverflow.com/a/60341452/12939222 but that might just be the price to pay as there might be no way for Jackson to see the optimization – Frederic Beister Feb 21 '20 at 15:36
  • 1
    Yes, but I guess modifying a `final` field via deserialization should never be allowed, so the other answer is really not the way to go (just because you can does not mean you should). I think either a runtime exception or your proposal of static analysis to prevent `final` `JsonProperty` makes most sense. Maybe even checking that final fields are `@JsonIgnore`'d during deserialization would be a good idea. – sfiss Feb 21 '20 at 15:40
  • We usually use the variant where we have final fields but no standard constructor so that the final fields are initialized during construction but with the deserialized value (using `@JsonProperty` on the constructor arguments) – Frederic Beister Feb 23 '20 at 07:16