ObjectMapper.registerSubtypes(NamedType...) doesn't allow to register the same POJO for two different type ids

[mbaechler]

registerSubtypes takes an array of NamedTypes:

    @Override    
    public void registerSubtypes(NamedType... types) {
        if (_registeredSubtypes == null) {
            _registeredSubtypes = new LinkedHashSet<NamedType>();
        }
        for (NamedType type : types) {
            _registeredSubtypes.add(type);
        }
    }

But unfortunately, if I want to register the same POJO for several types (let's say the JSONs only differ on their type field), it will only register the first one because they are added to a LinkedHashSet and NamedType doesn't include name field in equals/hashcode.

I don't even create a subtype of NamedType to override equals/hashcode because the class it final.

Would it make sense to care about NamedType.name field ?

[cowtowncoder]

Missing name does seem like an oversight. My main concern is that this behavior might be relied upon somewhere, so I wouldn't want to make a change in 2.10 patch (i.e. should go in 2.11); even if none of current tests fail.

Other than that that sounds reasonable as I thought behavior of multiple names mapping to same class was already allowed and working.

Also tagging as "good first issue" as fix seems simple enough assuming no complications arise.
If so, fix should be made either against 2.11 branch or masters (3.0.0-snapshot -- if so, I can backport into 2.11`).

[jkosh44]

If @mbaechler isn't working on this, then I'd like to give it a shot if that's alright.

[cowtowncoder]

@jkosh44 sounds good to me.

[jkosh44]

I've just been thinking about this issue and have a question.

Lets say we have the following two classes (borrowed from some of the tests)

@JsonTypeInfo(use=JsonTypeInfo.Id.NAME)
static abstract class SuperType {}

static class Sub extends SuperType {
    public int a;
}

Then from the Deserialization side of things I understand that we should expect the following to create two similar objects of type Sub with field a having a value of 1.

ObjectMapper mapper = new ObjectMapper();
mapper.registerSubtypes(new NamedType(Sub.class, "sub1"));
mapper.registerSubtypes(new NamedType(Sub.class, "sub2"));
SuperType bean1 = mapper.readValue("{\"@type\":\"sub1\", \"a\":1}", SuperType.class);
SuperType bean2 = mapper.readValue("{\"@type\":\"sub2\", \"a\":1}", SuperType.class);

However from the Serialization side of things what would be the expected behavior of the following:

ObjectMapper mapper = new ObjectMapper();
mapper.registerSubtypes(new NamedType(Sub.class, "sub1"));
mapper.registerSubtypes(new NamedType(Sub.class, "sub2"));
Sub bean = new Sub();
bean.a = 1;
String s = mapper.writeValueAsString(bean);

What should the value of s be? "{\"@type\":\"sub1\",\"a\":1}" or "{\"@type\":\"sub2\",\"a\":1}"?

[mbaechler]

Hi @jkosh44 ,
In this case I don't think there's any good solution.
However, my case is different: I use

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", visible = true)

That means the type is an actual field of the POJO, so there's no ambiguity.
That also means that we should restrict the registering of named subtypes to the case where to property is explicit in the POJO.
What do you think?

[cowtowncoder]

I think that in case of one class with two ids, on serialization side, one obvious outcome would be Exception: this is unsupported case. It would be asymmetric and as you point out would only work for deserialization. This with existing system as-is.
Another possibility would be to assume that the first id defined is the primary/principal id and just use that. It might be preferable to users.

If system was to be improved, I suppose one approach could be to allow "aliases" as secondary accetable ids on deserialization. This would specify primary id explicitly. However, this would require much more significant changes.

[jkosh44]

So the current behavior for my example above is that s would be "{\"@type\":\"sub1\",\"a\":1}", due to caching. However there is actually a test asserting our ability to override registered subtypes (TestSubtypes.testSerialization). An example using the classes above is the following

ObjectMapper mapper = new ObjectMapper();
mapper.registerSubtypes(new NamedType(Sub.class, "sub1"));
Sub bean = new Sub();
bean.a = 1;
String s = mapper.writeValueAsString(bean);
assertEquals("{\"@type\":\"sub1\",\"a\":1}",s)

//new ObjectMapper is needed because Typed Serializers are cached
mapper = new ObjectMapper();
mapper.registerSubtypes(new NamedType(Sub.class, "sub2"));
s = mapper.writeValueAsString(bean);
assertEquals("{\"@type\":\"sub2\",\"a\":1}",s)

I'm not sure how common of a use case this is but there are tests for it, and all the suggestions above do break this use case.

One possible solution is that instead of assuming that the first id defined is the primary/principal id, we assume that the most recent id defined is the primary/principal id.

The fact that there is caching and you need a new ObjectMapper for this use case leads me to think it's not valid and is just erroneously being tested for. I thought it was worth bringing up though.

[jkosh44]

In terms of the use case @mbaechler brought up for this issue, hashing on the class and name seems to take care of deserialization case. However when we serialize a field we rely on TypeNameIdResolver::idFromValue to resolve the field id. This method relies only on the Object itself and not the field name. Also TypeNameIdResolver stores it's id information in a Map<String, String> which maps class names to ids. I'll have to look into it more tomorrow to see how we can include the field name to resolve the id, because currently that class can only store one id per class.

[cowtowncoder]

@jkosh44 To claierfy: registering of subtypes absolutely must be done before any use, and it is not something that can be done after use -- this is explicitly declared as "does not work" wrt "config-then-use" pattern. Behavior will not be changed; in 3.0, however, it will not be possible to even try to change things when mappers are built using builder pattern. This hopefully removes confusion as I can see how current behavior can be unexpected.

So question goes back to what to do with duplicates for specific use case: I think it should be one of:

1. Use the first relevant mapping registered
2. Use the last (most recent) relevant mapping registered
3. Fail on ambiguous case (throw exception) -- however, probably should allow re-registering for same type (i.e. trying to add same mapping would be silently ignored)

[jkosh44]

Gotcha, I think I misunderstood the test I was referencing. It wasn't testing overriding a previous call to registerSubtypes, it was testing overriding a @JsonTypeName annotation.

In terms of what to do about duplicates, I think option 2 feels more intuitive, however I think right now option 1 is the better option. I think this because that's the current behavior today and because of how caching works. When SerializerProvider::findTypedValueSerializer is called it will cache the created Serializer by class. Next time SerializerProvider::findTypedValueSerializer is called it will just pull from this cache instead of creating a new Serializer. Therefore once you serialize a class once, all subsequent calls to registerSubtypes with that class will be ignored with respect to Serialization. Changing this behavior is a much more involved change IMO.

[cowtowncoder]

I may not have explained (2) properly: I do not mean that you would be able to change definitions, but rather that in case of multiple calls during initialization phase, this would be checked.
Given that type mappings may be registered via modules and so on, it is not necessarily rare for multiple registrations to be called.

At any rate, both options are possible and about as easy to implement.

[mbaechler]

Thank you very much for this fix