FasterXML / jackson-docs

Documentation for the Jackson JSON processor.
726 stars 111 forks source link

Documentation of mixins is obsolete. Adding mixins has no effect. #2

Closed jnizet closed 10 years ago

jnizet commented 11 years ago

The documentation at http://wiki.fasterxml.com/JacksonMixInAnnotations explains that to use mixin annotations, one should do

objectMapper.getSerializationConfig().addMixInAnnotations(Target.class, MixIn.class);

But there is no such method in objectMapper.getSerializationConfig() (v2.2.3)

Moreover, trying to add mixin directly to the object mapper (using http://fasterxml.github.io/jackson-databind/javadoc/2.2.0/com/fasterxml/jackson/databind/ObjectMapper.html#addMixInAnnotations%28java.lang.Class,%20java.lang.Class%29 not only modifies the ObjectMapper, but has 0 effect. I had to copy the ObjectMapper and use the created copy to have the annotations applied. To apply mixin annotations for a given serialization, using a shared, reusable ObjectMapper, you thus need to

Maybe I missed something, but given the incorrect documentation, that's the only way I found to use mixins, and it's very far from being optimal.

cowtowncoder commented 11 years ago

Right, this is not the 2.x way of adding mix-in annotations (it documents 1.x method); rather you should either:

  1. Use Module interface (usually just create or extend SimpleModule), register mix-ins that way, or
  2. Use ObjectMapper.addMixInAnnotations() or ObjectMapper.setMixInAnnotations()

I am not quite sure what specific problems you had with second option. It definitely should not be necessary to copy ObjectMapper. But one thing that is still true (same as with 1.x) is that all mix-ins MUST be registered before any use of ObjectMapper: these configurations can not be changed after use. So I am wondering if this could be the case here, that you are trying to change or add mix-in annotations after already serializing something?

jnizet commented 11 years ago

My goal is to use a shared ObjectMapper, that shouldn't be modified, but to customize the serialization in one particular use-case using a Mixin (i.e. do the same thing as I could do with a view, but using a Mixin). This is why I need a copy of the shared mapper (because, unfortunately, I haven't found any other way to apply a mixin for a specific use-case: there is no writerWithMixins() method that would be similar to writerWithView()).

Here's a complete test case showing the problem:

package jackson.mixin;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.Before;
import org.junit.Test;

import static org.junit.Assert.*;

public class MixinTest {

    private ObjectMapper sharedObjectMapperThatShouldntBeModified;

    @Before
    public void prepare() {
        sharedObjectMapperThatShouldntBeModified = new ObjectMapper();
    }

    // This test fails
    @Test
    public void mixinAnnotationsShouldApplyOnCopy() throws Exception {
        ObjectMapper copyUsedToAvoidModifyingTheSharedMapper = this.sharedObjectMapperThatShouldntBeModified.copy();
        copyUsedToAvoidModifyingTheSharedMapper.addMixInAnnotations(User.class, UserMixin.class);
        String json = copyUsedToAvoidModifyingTheSharedMapper.writeValueAsString(new User("John", "Doe"));
        assertFalse(json.contains("Doe"));
    }

    // This test passes, and shows that the Mixin class is correct. But it modifies the shared ObjectMapper.
    @Test
    public void mixinAnnotationsShouldApplyOnOriginal() throws Exception {
        sharedObjectMapperThatShouldntBeModified.addMixInAnnotations(User.class, UserMixin.class);
        String json = sharedObjectMapperThatShouldntBeModified.writeValueAsString(new User("John", "Doe"));
        assertFalse(json.contains("Doe"));
    }

    // This test shows how to workaround the problem: it passes
    @Test
    public void mixinAnnotationsShouldApplyOnCopyOfCopy() throws Exception {
        ObjectMapper copyUsedToAvoidModifyingTheSharedMapper = this.sharedObjectMapperThatShouldntBeModified.copy();
        copyUsedToAvoidModifyingTheSharedMapper.addMixInAnnotations(User.class, UserMixin.class);
        ObjectMapper copyUsedToWorkaroundTheProblem = copyUsedToAvoidModifyingTheSharedMapper.copy();
        String json = copyUsedToWorkaroundTheProblem.writeValueAsString(new User("John", "Doe"));
        assertFalse(json.contains("Doe"));
    }

    public static class User {
        private String firstName;
        private String lastName;

        private User() {
        }

        private User(String firstName, String lastName) {
            this.firstName = firstName;
            this.lastName = lastName;
        }

        public String getFirstName() {
            return firstName;
        }

        public void setFirstName(String firstName) {
            this.firstName = firstName;
        }

        public String getLastName() {
            return lastName;
        }

        public void setLastName(String lastName) {
            this.lastName = lastName;
        }
    }

    public static abstract class UserMixin {
        @JsonIgnore
        private String lastName;
    }
}
cowtowncoder commented 11 years ago

Ok. I just wanted to rule out possible reasons. And you are right, a copy unfortunately then has to be made -- writers and readers are fully immutable, and mix-ins are one things that can not work that dynamically (since they affect the way serializers and deserializers are constructed; results are cached).

So it sounds like you are experiencing a bug: extra copy (beyond one) should not be needed.

Since I just pushed out version 2.3.0-rc1, perhaps it would make sense to try it just to make sure this is not something fixed since 2.2. I do remember some changes that could be relevant: copy() method is relatively new addition, and I did fix something particularly related to mixins (I don't remember issue number right now).

jnizet commented 11 years ago

Good news: the test passes with 2.3.0-rc1. Now you have one more fix to mention in the release notes :-)

The documentation issue still stands though. It would be nice to document the way mixins should be used on 2.x versions, and especially in the case where the mixins must be applied for a given serialization, and not globally (which, IMO, could be a quite frequent use-case).

cowtowncoder commented 11 years ago

Agreed with documentation.

The static nature of mix-ins is bit problematic: with enough time and effort, some of limitations could perhaps be resolved, by keeping track of (de)serializers for types that are NOT affected by any of registered mix-ins. But then again, addition of mix-ins would also require re-creation of (de)serializers.

So perhaps mention that if variation is needed, copies of ObjectMapper are also needed. And specifically explain the rationale behind intentional inability to change mix-ins for ObjectReader and ObjectWriter (for thread-safety).

One final thing: we are always open for more documentation; and although FasterXML wiki is the biggest, we hope to move documentation over to github. Why? For bigger participation (pull requests; ease of granting write access; current wiki is real PITA wrt access right management). So if you happened to be interested, jackson-docs project (and per-project wikis) are more than happy to accept contributions too.

jnizet commented 11 years ago

I'm a really new adopter of Jackson, and don't know much about it, so I don't feel confident enough to write documentation for it. But I'll keep it in mind if I happen to become more experienced with it.

I agree with what you suggest though. Just mentioning that copy() is (for the time being at least), the best way to apply mixins for a given use-case, and that it's needed for thread-safety, would be nice. That is what I guessed from the javadoc and source code, but couldn't confirm from the documentation.

And, if I may suggest something regarding the documentation: the two main problems I have with the documentation right now are that

It would be really cool if the Jackson build process also produced the documentation for the version being built, and if every release had its associated documentation published with, preferrably, a single-page version.

And a last question, if you don't mind: Is ObjectMapper.copy() a heavyweight operation that should be avoided, or is it cheap enough to be done for every HTTP request (for example) that needs a mixin-customized version of the mapper? I wonder if I should create a copy, customize it and keep a reference to it, or if I shouldn't care and simply create a copy every time I need one.