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

@JsonTypeInfo property always serialized before other properties #250

Closed
drekbour opened this issue Jun 25, 2013 · 10 comments
Closed

@JsonTypeInfo property always serialized before other properties #250

drekbour opened this issue Jun 25, 2013 · 10 comments

Comments

@drekbour
Copy link
Contributor

If using an existing property as a polymorphic discriminator, I don't want to change the ordering of the generated output but it always appears first.

The below (Jackson 2.2.1) serializer test fails with:

Expected :{"colour":"red","name":"apple"}
Actual   :{"name":"apple","colour":"red"}

Given there are minor performance implications of having it "somewhere" in the message no one wants this as default behaviour. I think what it comes down to is that lack of something like
JsonTypeInfo.As.EXISTING_PROPERTY instead of just using @JsonIgnore on the real property and letting the type system place it at the beginning.

As.EXISTING_PROPERTY would explicitly ignore setting of the type field during serialisation and allow the normal serialisation code to populate it as per the rest of the mapper's configuration. It might want to assert that it is set (to the correct value) but I have not thought this aspect through.

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeName;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.Before;
import org.junit.Test;

import java.io.IOException;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class ExistingPropertyTest {

    private ObjectMapper mapper;

    @Before
    public void setup() {
        mapper = new ObjectMapper().enable(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS);
        mapper.registerSubtypes(Apple.class);
    }

    @Test
    public void testSerializer() throws JsonProcessingException {
        // Given:
        Apple fruit = new Apple();
        fruit.colour = "red";
        // When:
        String json = mapper.writer().writeValueAsString(fruit);
        // Then:
        assertEquals("{\"colour\":\"red\",\"name\":\"apple\"}", json);
    }

    @Test
    public void testDeserializer() throws IOException {
        // Given:
        String json = "{\"colour\":\"red\",\"name\":\"apple\"}";
        // When:
        Fruit fruit = mapper.reader(Fruit.class).readValue(json);
        // Then:
        assertTrue(fruit instanceof Apple);
        assertEquals("red", ((Apple) fruit).colour);
    }

    @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "name", visible = false)
    public static abstract class Fruit {
        @JsonIgnore
        public abstract String getName();
    }

    @JsonTypeName(Apple.NAME)
    public static class Apple extends Fruit {
        public static final String NAME = "apple";
        public String colour;

        @Override
        public String getName() {
            return NAME;
        }

    }

}

FYI, I hacked the above solution by defining the following @JsonTypeResolver:

public class ExistingPropertyTypeResolver extends ObjectMapper.DefaultTypeResolverBuilder {
    public ExistingPropertyTypeResolver() {
        super(ObjectMapper.DefaultTyping.OBJECT_AND_NON_CONCRETE);
    }

    /**
     * Don't serialise type information (we know it is already in the business fields)
     */
    public TypeSerializer buildTypeSerializer(SerializationConfig config,
                                              JavaType baseType, Collection<NamedType> subtypes) {
        return null;
    }
}
@cowtowncoder
Copy link
Member

First things first: I think that solution to your problem of whether type id is metadata (default: no property -- much more logical to me, from Object perspective; type is not data) or regular data (exposed as property; explicit type discriminator); I think you can use

 @JsonTypeInfo(visible=true)

to do this. Property was added to exposing type ids more as regular properties. Let me know if that works.

As to decision to force outputting of type id (and actually, object id, if any, before it), as the first property by default is a fundamental low-level decision and I am unlikely to change that. Note that performance implications are not trivial for cases of deeply nested structures, because of additional buffering needed, even if they may be for small objects. Unfortunately there is no (efficient) way to know effects a priori.

However: for specific case of ordering when type id property is included in explicit ordering, yes, I think this is a reasonable request, and I can see if that can be implemented. The main challenge is that since handling of type id is separate from handling of regular properties, it may be difficult to actually change this (that is: type serializer is called before serializer for properties).

@cowtowncoder
Copy link
Member

Actually, come to think of it: it may be impossible to change ordering for type identifier. This because it is serialized first, separate from properties; so code that sorts properties won't have access to it.

@drekbour
Copy link
Contributor Author

I take the point regarding type id being metadata. You are completely correct, it's redundant metadata once you have concrete class to do an instanceof against. I'm bound by someone else's models though so cannot easily change either the JSON or pojo. I merely need to write a mapper that accepts a list of differing models.

visible=true doesn't answer this but I have already demonstrated how it is achieved with my workaround. You disable the TypeSerializer with prior understanding that the relevant field will end up in the serialised message naturally.

This ticket is about formalising that prior understanding (and documenting the performance implications) into JsonTypeInfo.As.EXISTING_PROPERTY. The only hitch is that, to do the job properly, I imagine Jackson should still execute the TypeSerializer but stash it's value to be checked against the field as it serialised naturally.

@drekbour
Copy link
Contributor Author

In relation to #263, I think that EXISTING_PROPERTY overrides visible=false. It would also change the handling of visible=true as we would not want to ignore the field during property serialisation since we are already ignoring it during type serialisation.

@cowtowncoder
Copy link
Member

Ah ok. So, EXISTING_PROPERTY could basically instruct TypeSerializer to do nothing as property is expected to be produced via regular property handler. But conversely on deserialization, it should be used as-is, considering 'visible' setting as expected (i.e. exposed to be set as property or not).

I think I finally got it now that I had time to re-read description.

@drekbour
Copy link
Contributor Author

drekbour commented Jun 9, 2014

You released the annotation without the implementation :)

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "myPropertyName", visible = true)
// java.lang.IllegalStateException: Do not know how to construct standard type serializer for inclusion type: EXISTING_PROPERTY

@cowtowncoder
Copy link
Member

Right, it was added for 2.3, and at this point is waiting for code to add support for it. Hence this issue is open, but annotation one closed. Not optimal situation I know. :)

@aaronkorver
Copy link

So....I just found this annotation, and got that error. Was/Is there an implementation planned?

@cowtowncoder
Copy link
Member

@aaronkorver Yes, we still intend to implement support for this value. But when this happens is an open question, so there is no firm version/date.

@cowtowncoder
Copy link
Member

Considering this a dup for #528, which will cover adding support for EXISTING_PROPERTY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants