Class name handling for JDK unmodifiable Collection types changed in 2.9.3, 2.8.11

[rwinch]

It appears that the default typing in jackson-databind-2.8.11 and jackson-databind-2.9.3 is inconsistent with prior versions. For example, the following test works in 2.9.2 and fails in 2.9.3 and 2.9.4-SNAPSHOT. The test also works in 2.8.10 but fails in 2.8.11

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

	Set<String> theSet = Collections.unmodifiableSet(Collections.singleton("a"));

	String actualJson = mapper.writeValueAsString(theSet);

	assertEquals("[\"java.util.Collections$UnmodifiableSet\",[\"a\"]]", actualJson);
}

The failure is:

org.junit.ComparisonFailure: 
Expected :["java.util.Collections$UnmodifiableSet",["a"]]
Actual   :["java.util.HashSet",["a"]]

This issue appears to be related to the build failures in spring-projects/spring-security#4918

[cowtowncoder]

I presume this is due to fix #1823 that is required to solve a different problem.
No tests exist to try to determine that unmodifiable set (list, map) would be retained as such, but if this used to work (surprising in a way, wouldn't have assumed it did, being internal JDK class), it'd make sense to try to make it work again.

Fortunately fix referenced only has to handle singleton case specifically, but not sure what good heuristic would be here. Chances are something else could yet fail with changes.

Going forward I would strongly recommend against counting on ability to retain specific aspects like this, given that JDK may choose to represent unmodifiable collections using some type that can not be instantiated from outside (for example, by not exposing zero-arg constructor method).

[rwinch]

Thanks for the fast reply.

Going forward I would strongly recommend against counting on ability to retain specific aspects like this, given that JDK may choose to represent unmodifiable collections using some type that can not be instantiated from outside (for example, by not exposing zero-arg constructor method).

As I'm sure you know, passivity is important to ensure that serialized/deserialized values continue to work consistently. Overall the goal is to ensure if the user serializes Collections.unmodifiableSet that deserializing it produces an unmodifiable Set. The original test I produced for this issue is my attempt to simplify the problem.

More concretely, my goal is to ensure that this test will pass.

@Test
public void passesWith2dot9dot2AndFailsWith2dot9dot3() throws Exception {
	ObjectMapper mapper = new ObjectMapper();
	mapper.enableDefaultTyping(ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);
	SimpleModule module = new SimpleModule();
	Class type = Collections.unmodifiableSet(Collections.emptySet()).getClass();
	module.addDeserializer(type, new UnmodifiableSetDeserializer());
	mapper.registerModule(module);

	Set<String> theSet = Collections.unmodifiableSet(Collections.singleton("a"));

	String actualJson = mapper.writeValueAsString(theSet);

	Set<String> result = mapper.readValue(actualJson, Set.class);

	try {
		result.add("should fail");
		fail("Expected Exception");
	} catch (UnsupportedOperationException  success) {}
}

static class UnmodifiableSetDeserializer extends JsonDeserializer<Set> {

	@Override
	public Set deserialize(JsonParser jp, DeserializationContext ctxt) throws IOException,
			JsonProcessingException {
		ObjectMapper mapper = (ObjectMapper) jp.getCodec();
		JsonNode node = mapper.readTree(jp);
		Set<Object> resultSet = new HashSet<Object>();
		if (node != null) {
			if (node instanceof ArrayNode) {
				ArrayNode arrayNode = (ArrayNode) node;
				Iterator<JsonNode> nodeIterator = arrayNode.iterator();
				while (nodeIterator.hasNext()) {
					JsonNode elementNode = nodeIterator.next();
					resultSet.add(mapper.readValue(elementNode.traverse(mapper), Object.class));
				}
			} else {
				resultSet.add(mapper.readValue(node.traverse(mapper), Object.class));
			}
		}
		return Collections.unmodifiableSet(resultSet);
	}
}

Do you have suggestions on how to do this better?

[cowtowncoder]

Just to make it clear, I don't argue against reasonable desire to preserve immutability as a property. It is clear user would want that. It is more that this is difficult for Jackson to support (more on this below).
if I was to suggest a way to achieve the goal of retaining immutability itself I would have setter take Collection (or List or Set etc) and create immutable instance, and not rely on Jackson being able to retain exact specific implementation dependent collection (etc) type:

public void setValues(Set<ValueType> v) {
    this.values = Collections. unmodifiableSet(v);
}

But be that as it may -- I think it is possible to make behavior work in 2.9.4 like it did in 2.9.2.

What follows is longer explanation of my thinking; feel free to read or ignore. I wrote it to clarify my thinking for myself. :)

So why is it difficult for Jackson to retain unmodifiability wrt unmodifiable sets created this way?
Unfortunately the way JDK chose to do this is to essentially hide all traces at runtime -- it is only implied by users calling a method to create an instance, and invisible after that.
There is nothing on declaration of the type, properties or attributes that indicates desired property of immutability -- while users intent is clear at source code, given an arbitrary collection there is no good way of figuring this out. It is basically an implementation detail, and such may or may not be preserved.

For example of something that is properly exposed, consider TreeSet and TreeMap -- in these cases property is indicated by type.
This can be (and is) supported as expected, even without polymorphic handling.

For polymorphic handling it is possible to retain exact impl class name, and if JDK maintainers keep no-args constructor (... which they have no real requirement to do; it's not instantiated directly by users), it can work. It's just that it seems fragile.

[rwinch]

Just to make it clear, I don't argue against reasonable desire to preserve immutability as a property. It is clear user would want that.

Thanks. If I did imply otherwise, it was not my intention (sorry if it came across wrong). Thank you again for your responsiveness.

and not rely on Jackson being able to retain exact specific implementation dependent collection (etc) type:

The problem is that I don't know if it is always immutable or not. I need it to work for immutable and not immutable collections. As a framework, Spring Security does not know what the user is serializing and what they want.

So why is it difficult for Jackson to retain unmodifiability wrt unmodifiable sets created this way?

I full agree with this difficulty. The only way I could figure out how to do this was to rely on the implementation specific class. Obviously this is not ideal, but it seems the only way to do this since the JDK doesn't support

[cowtowncoder]

Hmmh. After trying this out, I am not 100% sure that I should change the behavior, after all.

The problem here is this: although it is possible to force use of the original class, as-is, this will then mean that by default deserialization will fail, as there is no zero-arg constructor available.
So the change would mean that without custom deserializers there will be a new type of failure.
This could be overcome, perhaps, by yet another tweak to default handlers.

On the other hand I guess above problem is not new in the sense that up until 2.9.2 such deserialization would fail regardless. It just seems wrong to cause another kind of failure, just to support use case where a custom deserializer was used -- something that was unlikely to have been widely used.

So I will have to think about what to do here. I can honestly say that feature (ability to be able to deserialize unmodifiable collections as deemed by JDK) was not something that was planned.
That it happened to work is almost unfortunate since I feel torn about what is the best behavior.

[rwinch]

Thanks for the feedback.

I'll stay tuned for your additional thoughts. If you decide to go against restoring the previous behavior, perhaps you can help guide me to a solution that preserves backward compatibility across jackson versions?

[rwinch]

To be explicit, I also wanted to point out that the issue also appears to impact 2.8.11 (I also updated the original comment to include that information).

[cowtowncoder]

@rwinch thanks; it does make sense 2.8.11 affected since the same fix to allow working of singleton sets/lists was backported.

At this point, I think I will make the change, considering that your use case was affected (even if it could be seen as essentially unintentional support :) ), and since I do not anyone could have relied on new behavior of 2.9.3 wrt immutable sets/lists. So I think it is reasonable to do this for 2.9.

There is the bigger question here, I think, of the fact that sometimes exact specific class is not what is desired, but rather preservation of logical type that is more granular than declared (static type that property has), but not the same as actual runtime implementation type. I will think about what could be done for 3.0.

And last point I think I'll mention is that this might be something where use of Guava (or one of other container libs) might help: they tend to have explicit immutable types. Those would be retained.

Anyway: I hope to get the fix in 2.9.4, having thought it through. Thank you for your help here and apologies for breakage.