Class name handling for Collections.unmodifiableList changed in 2.9.4-SNAPSHOT

[rwinch]

Similar to #1868 the handling of Collections.unmodifiableList has changed in a non-passive manner. The following test passes prior to jackson-databind 2.9.4-SNAPSHOT

@Test
public void serializeUnmodifiableList() throws Exception {
	ObjectMapper mapper = new ObjectMapper();
	mapper.enableDefaultTyping(ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);

	String actualJson = mapper.writeValueAsString(Collections.unmodifiableList(Collections.emptyList()));
	assertEquals("[\"java.util.ArrayList\",[]]", actualJson);
}

However, it fails in jackson-databind 2.9.4-SNAPSHOT with the following error:

org.junit.ComparisonFailure: 
Expected :["java.util.ArrayList",[]]
Actual   :["java.util.Collections$UnmodifiableRandomAccessList",[]]

It could possibly be argued this is a bug that has been fixed. However, given the length of time this has been working this way I'd consider it a bug. This is compounded by the fact that in jackson-databind 2.9.4-SNAPSHOT the following fails:

@Test
public void derializeUnmodifiableList() throws Exception {
	ObjectMapper mapper = new ObjectMapper();
	mapper.enableDefaultTyping(ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);

	String actualJson = mapper.writeValueAsString(Collections.unmodifiableList(Collections.emptyList()));

	List<?> value = mapper.readValue(actualJson, List.class);
	assertTrue(value instanceof List);
}

with the following exception:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `java.util.Collections$UnmodifiableRandomAccessList` (no Creators, like default construct, exist): no default no-arguments constructor found
 at [Source: (String)"["java.util.Collections$UnmodifiableRandomAccessList",[]]"; line: 1, column: 55]

	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:67)
	at com.fasterxml.jackson.databind.DeserializationContext.reportBadDefinition(DeserializationContext.java:1451)
	at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1027)
	at com.fasterxml.jackson.databind.deser.ValueInstantiator.createUsingDefault(ValueInstantiator.java:189)
	at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createUsingDefault(StdValueInstantiator.java:267)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.createDefaultInstance(CollectionDeserializer.java:255)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:245)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:27)
	at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer._deserialize(AsArrayTypeDeserializer.java:116)
	at com.fasterxml.jackson.databind.jsontype.impl.AsArrayTypeDeserializer.deserializeTypedFromArray(AsArrayTypeDeserializer.java:53)
	at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserializeWithType(CollectionDeserializer.java:314)
	at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:68)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4001)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2992)

I think it would be ideal to revert jackson-databind 2.9.4-SNAPSHOT to serializing Collections.unmodifiableList in the same way it was prior to 2.9.4-SNAPSHOT

[cowtowncoder]

I am not sure I follow -- isn't this opposite of #1868? Where request specifically was not to change class name?

[cowtowncoder]

Also: if I understand report correctly, I think I disagree here. Unmodifiable list and set should, I think, have identical behavior: either class name is changed to allow deserialization (but lose "unmodifiability"), or class name should remain exactly as is, but fail on deserialization unless custom deserializer is used.
Latter would be more in line with keeping before that existed before 2.9.3, trying to go for minimal changes. Change in 2.9.3 was not requested (or designed) for unmodifiable types for singleton variants; so while potentially useful, it was not intentional fix.

[rwinch]

Thanks for the fast response.

In both cases I'm asking that the behavior remain consistent through the patch releases (do not introduce breaking changes). For this specific issue, I think users are going to be caught off guard that the following test use to work, but no longer works:

@Test
public void derializeUnmodifiableList() throws Exception {
	ObjectMapper mapper = new ObjectMapper();
	mapper.enableDefaultTyping(ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);

	String actualJson = mapper.writeValueAsString(Collections.unmodifiableList(Collections.emptyList()));

	// fails with exception
	List<?> value = mapper.readValue(actualJson, List.class);
	assertTrue(value instanceof List);
}

[cowtowncoder]

@rwinch I still do not understand this. As far as I understand, code would have failed up until 2.9.2, but worked in 2.9.3 as a side-effect of #1823 which did not directly address it. It would have broken any custom deserializers similar to #1868, however.

As such there is no way to make this work not change behavior of either 2.9.2 or 2.9.3. And if #1868 is to go through, I would think that behavior as it is now should stay: or, if not, then #1868 should also be rolled back.

Benefit of going back to 2.9.2 way, with respect to unmodifiable types, is that user can at least register custom deserializer, or, if preferable, abstract type mapping. Or if nothing else helps, ClassNameIdResolver. So the name replacement should only apply to singleton types (if even them -- but it is an intentional committed fix, whereas doing the same for empty list/set was unintentional).

[rwinch]

Thank you for your response.

As far as I understand, code would have failed up until 2.9.2, but worked in 2.9.3 as a side-effect of #1823

My testing does not reflect this. From what I have experienced, the test does not break until 2.9.4-SNAPSHOT (it works in prior versions).

NOTE: This test is for an unmodifiableList while #1868 is an unmodifiableSet

Benefit of going back to 2.9.2 way, with respect to unmodifiable types, is that user can at least register custom deserializer, or, if preferable, abstract type mapping. Or if nothing else helps, ClassNameIdResolver. So the name replacement should only apply to singleton types (if even them -- but it is an intentional committed fix, whereas doing the same for empty list/set was unintentional).

I can see this line of reasoning and is why I deliberated even reporting the issues so long. However, I think these changes would be better off outside of a patch release. In my opinion, telling users to create their own deserializer, mapping, or ClassNameIdResolver is a big ask for taking a patch release. Instead I would request for passivity within the patch releases and wait for something that involves user interaction for 3.x

[rwinch]

I should add that this is also happening in 2.8.11.1-SNAPSHOT

[cowtowncoder]

Yes, behavior should be the same for both snapshots.
And yes, I realize this is for List vs Set.

Having thought about this bit more, I realized that maybe I should go for proper full solutions, which would:

1. Remove mapping of class name on serialization for all types involved. But:
2. Add specific deserialization side handling to ensure they can be deserialized back into exact types

For (2), I think we could:

1. Use ValueInstantiator for "empty" types (since they ought not to have content, hence no problem that put would fail)
2. Use "converting deserializer" for singleton and unmodifiable types, so that default Map/Collection deserializer is first used to collect entries, do the heavy lifting, and then Converter will construct actual container type (ensuring that singleton only has one entry etc)

This is quite a bit more work, but I think it really is the best way to go, now that some special handling is done. It will still allow custom deserializers (since class name is not modified) AND retain intended implementation type.

This would also cover #1881. The only open thing would once again be 2.8.11: this change is still non-trivial wrt regression risk. So I will most likely do smallest change there (map name for "singleton", as per earlier fix).

[cowtowncoder]

As to "making users use ClassNameIdResolver", yes; but you may be missing one aspect: up until 2.9.2 (and 2.8.10), deserialization was NOT working for these types. 2.9.3/2.8.11 did unintentionally allow this, but had problems for the other type. I do not believe users would be counting on this new behavior; and if they had work-arounds in place prior, would still have them.
And I really really do not like behavior to differ between List and Set variants of same physical type. That just makes no sense at all. It is unfortunate to have distinction between unmodifiable/singleton, but I can live with that given sequence of bug reports for both (in different direction).

This is mostly wrt what 2.8.11.1 will have: I think 2.9.4 can deal with it better if I can implement what I detailed in previous comment.

[cowtowncoder]

Fixed (commit references #1880) so that:

1. No changes are made for class name for any of Collection.xxxx() (or Arrays.asList()).
2. Custom deserializer (delegating) and/or ValueInstantiator used for properly constructing instances on deserialization.

This seems like a better fix to me, all around, although theoretically it is always possible someone somewhere might prefer conversion to standard types. Specifically, singleton/empty instances are implemented as read-only by JDK (I think), and if code expects these to be mutable this is not the case.
However, it is now possible to override handling (which would be more difficult all around with ClassNameIdResolver).

Given bit bigger scope of the fix I will leave 2.8.11(.1) at something that approximates behavior of 2.8.10, and not merging this bundle of changes.

Let me know what you think; I hope we can soon proceed with 2.9.4 at least regarding this class of issues.

[aaronlangford31]

Hi, I'm looking into managing an upgrade from 2.8 to 2.10. I'm seeing some issues in backwards compatibility as a result of this issue (for example: 2.10 writes java.util.Arrays$ArrayList, 2.8 doesn't know how to deserialize this). Any suggestions on how to make 2.10 write compatible types that 2.8 can read? Looking for something that can be done at the ObjectMapper level, rather than standardizing the way my code instantiates lists so that I get java.util.ArrayList.

[antenko]

I have the same issue. We lost forward compatibility after jackson 2.8.8 -> >= 2.9.4 upgrade.
@cowtowncoder could you suggest the best way to reproduce previous (v2.8.8) serialization behaviour with Jackson >= 2.9.4?
As I understand here you explains options for backward compatible deserialization:

can at least register custom deserializer, or, if preferable, abstract type mapping. Or if nothing else helps, ClassNameIdResolver.

But we need change serialization of this private collection types and support deserialization possibility by previous service version.