jackson-dataformat-csv icon indicating copy to clipboard operation
jackson-dataformat-csv copied to clipboard

Mixin fails to override in an expected manner in some cases.

Open MattFriedmanAfilias opened this issue 8 years ago • 3 comments

When a mixin json ignore annotation (@JsonIgnore) is used, the expectation for the field is for it to be ignored. In the simple case this is what happens.

However, if @JsonProperty exists on the original model field the ignore annotation on the mixin is itself ignored and an exception can be thrown, if the model field is itself an object, or in some cases if it contains other nested objects.

I have written the following test to show the issue as I see it.

The hoped for fix would be to always have the mixin annotations take precedence over the model annotations. Otherwise, I don't think I understand the purpose of the mixin correctly.

package info.afilias.intdir.core.util;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonGenerationException;
import com.fasterxml.jackson.dataformat.csv.CsvMapper;
import com.fasterxml.jackson.dataformat.csv.CsvSchema;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;

/**
 *
 */
public class JacksonCsvTest {

    @Rule
    public ExpectedException expectedException = ExpectedException.none();

    /**
     * This is the mixin happy path. The object property is ignored.
     */
    @Test
    public void testMixinOverride_ExpectNoException() throws Exception {

        CsvNestedNestedModel2 nestedNestedModel = new CsvNestedNestedModel2();
        nestedNestedModel.prop1A = "prop1A";

        CsvNestedModel2 nestedModel = new CsvNestedModel2();
        nestedModel.nestedNestedModel = nestedNestedModel;

        CsvModel2 csvModel = new CsvModel2();
        csvModel.foo = "f1";
        csvModel.nestedModel = nestedModel;

        CsvMapper csvMapper = new CsvMapper();

        CsvSchema schema = csvMapper.schemaFor(CsvModel.class);

        csvMapper.addMixIn(CsvModel2.class, CsvModelMixin2.class);

        String csv = csvMapper.writer(schema).writeValueAsString(csvModel);

        assertThat(csv, containsString("f1"));
    }

    /**
     * In this case, the mixin doesn't do its job and override the annotation. 
     * IMO this is a bug and this test should pass.
     */
    @Test
    public void testMixinOverride_MixinFailsToOverride() throws Exception {

        CsvNestedNestedModel3 nestedNestedModel = new CsvNestedNestedModel3();
        nestedNestedModel.prop1A = "prop1A";

        CsvNestedModel3 nestedModel = new CsvNestedModel3();
        nestedModel.nestedNestedModel = nestedNestedModel;

        CsvModel3 csvModel = new CsvModel3();
        csvModel.foo = "f1";
        csvModel.nestedModel = nestedModel;

        CsvMapper csvMapper = new CsvMapper();

        CsvSchema schema = csvMapper.schemaFor(CsvModel.class);

        csvMapper.addMixIn(CsvModel3.class, CsvModelMixin3.class);

        String csv = csvMapper.writer(schema).writeValueAsString(csvModel);

        assertThat(csv, containsString("f1"));
    }

    /**
     * This test passes because the exception is expected when the property has an object value but that property
     * has not been ignored.
     * <p/>
     * It would be useful to be able to set an option for a default behaviour that would cancel the throwing of
     * the exception. For instance, just print "Object" or "n/a" when the value encountered is an object.
     * <p/>
     * This would make the resulting system less brittle and prone to errors when the model changes, but the
     * csv schema or mixing is not also updated.
     */
    @Test
    public void testMixinOverride_ExpectJsonGenerationException() throws Exception {

        CsvNestedNestedModel nestedNestedModel = new CsvNestedNestedModel();
        nestedNestedModel.prop1A = "prop1A";

        CsvNestedModel nestedModel = new CsvNestedModel();
        nestedModel.nestedNestedModel = nestedNestedModel;

        CsvModel csvModel = new CsvModel();
        csvModel.foo = "f1";
        csvModel.nestedModel = nestedModel;

        CsvMapper csvMapper = new CsvMapper();

        CsvSchema schema = csvMapper.schemaFor(CsvModel.class);

        csvMapper.addMixIn(CsvModel.class, CsvModelMixin.class);

        expectedException.expect(JsonGenerationException.class);
        expectedException.expectMessage("CSV generator does not support Object values for properties");
        csvMapper.writer(schema).writeValueAsString(csvModel);
    }
}

class CsvModel {

    String foo;

    CsvNestedModel nestedModel;

    public String getFoo() {
        return foo;
    }

    public void setFoo(String foo) {
        this.foo = foo;
    }

    public CsvNestedModel getNestedModel() {
        return nestedModel;
    }

    public void setNestedModel(CsvNestedModel nestedModel) {
        this.nestedModel = nestedModel;
    }
}

class CsvNestedModel {
    String prop1;

    CsvNestedNestedModel nestedNestedModel;

    public String getProp1() {
        return prop1;
    }

    public void setProp1(String prop1) {
        this.prop1 = prop1;
    }

    public CsvNestedNestedModel getNestedNestedModel() {
        return nestedNestedModel;
    }

    public void setNestedNestedModel(CsvNestedNestedModel nestedNestedModel) {
        this.nestedNestedModel = nestedNestedModel;
    }
}

class CsvNestedNestedModel {
    String prop1A;

    public String getProp1A() {
        return prop1A;
    }

    public void setProp1A(String prop1A) {
        this.prop1A = prop1A;
    }
}

class CsvModelMixin {
}

////////////////////////////////////////////////////////////////

class CsvModel2 {

    String foo;

    CsvNestedModel2 nestedModel;

    public String getFoo() {
        return foo;
    }

    public void setFoo(String foo) {
        this.foo = foo;
    }

    public CsvNestedModel2 getNestedModel() {
        return nestedModel;
    }

    public void setNestedModel(CsvNestedModel2 nestedModel) {
        this.nestedModel = nestedModel;
    }
}

class CsvNestedModel2 {
    String prop1;

    CsvNestedNestedModel2 nestedNestedModel;

    public String getProp1() {
        return prop1;
    }

    public void setProp1(String prop1) {
        this.prop1 = prop1;
    }

    public CsvNestedNestedModel2 getNestedNestedModel() {
        return nestedNestedModel;
    }

    public void setNestedNestedModel(CsvNestedNestedModel2 nestedNestedModel) {
        this.nestedNestedModel = nestedNestedModel;
    }
}

class CsvNestedNestedModel2 {
    String prop1A;

    public String getProp1A() {
        return prop1A;
    }

    public void setProp1A(String prop1A) {
        this.prop1A = prop1A;
    }
}

class CsvModelMixin2 {
    @JsonIgnore
    CsvNestedModel2 nestedModel;
}

////////////////////////////////////////////////////////////////


class CsvModel3 {

    String foo;
    // this annotation causes the mixin ignore annotation to be ignored
    @JsonProperty("alternateName")
    CsvNestedModel3 nestedModel;

    public String getFoo() {
        return foo;
    }

    public void setFoo(String foo) {
        this.foo = foo;
    }

    public CsvNestedModel3 getNestedModel() {
        return nestedModel;
    }

    public void setNestedModel(CsvNestedModel3 nestedModel) {
        this.nestedModel = nestedModel;
    }
}

class CsvNestedModel3 {
    String prop1;

    CsvNestedNestedModel3 nestedNestedModel;

    public String getProp1() {
        return prop1;
    }

    public void setProp1(String prop1) {
        this.prop1 = prop1;
    }

    public CsvNestedNestedModel3 getNestedNestedModel() {
        return nestedNestedModel;
    }

    public void setNestedNestedModel(CsvNestedNestedModel3 nestedNestedModel) {
        this.nestedNestedModel = nestedNestedModel;
    }
}

class CsvNestedNestedModel3 {
    String prop1A;

    public String getProp1A() {
        return prop1A;
    }

    public void setProp1A(String prop1A) {
        this.prop1A = prop1A;
    }
}

class CsvModelMixin3 {
    @JsonIgnore
    CsvNestedModel3 nestedModel;
}

MattFriedmanAfilias avatar Jul 20 '15 18:07 MattFriedmanAfilias

First of all, thank you for reporting this problem.

I will have to read the model with thought to make sure I understand what is happening here, but I have a guess as to how things may go wrong.

First: mix-ins are primarily meant to augment annotation set (that is, to add configuration in cases where no annotations exist). But overriding of specific annotations should also work, such that annotations from mix-in should work as if they replaced specific annotation in target class.

Second: when merging mix-ins, all annotations are handled separately, without knowing expected semantics or dependencies between types. For example, there is no special handling to try to merge @JsonIgnore and @JsonProperty information since mix-in merging code has no knowledge that they somehow relate to each other.

When applying annotation information in determining how things should work, there is the question of what should happen if both @JsonIgnore and @JsonProperty exist for a method or field: which one should have precedence? It may be possible to determine that one is more specific, like when being applied to a sub-class; although there may also be cases where there is no clear precedence (annotations come from interfaces, for example).

If I remember things correctly, @JsonIgnore in general should have precedence. So it would seem that your usage should work, regardless of inheritance hierarchy.

I also assume this is not CSV-specific, although you have encountered it with CSV module. This is mostly important for me to know what is the best place to reproduce the problem: if it applies to other datatypes, it should be in core databind; but if only CSV, could be due to something within this module.

cowtowncoder avatar Jul 20 '15 18:07 cowtowncoder

Hi Tatu, thanks very much for this response. I gather that you believe this is a defect and will be looking at it further in the future. Thanks for these very useful libraries.

MattFriedmanAfilias avatar Jul 23 '15 17:07 MattFriedmanAfilias

@MattFriedmanAfilias yes.

cowtowncoder avatar Jul 25 '15 05:07 cowtowncoder