`TypeFactory.constructType()` does not take `TypeBindings` correctly

[TheSpiritXIII]

Using Jackson version 2.11.0, suppose we want to deserialize a type as Optional<Set<Integer>> so I construct a JavaType for it and deserialize.

final ObjectMapper objectMapper = new ObjectMapper();
objectMapper.registerModule(new Jdk8Module());
final TypeFactory typeFactory = objectMapper.getTypeFactory();

• Option 1: constructType:

typeFactory.constructType(Optional.class, TypeBindings.create(Optional.class, new JavaType[] {
    typeFactory.constructType(Set.class, TypeBindings.create(Set.class, new JavaType[] {
        typeFactory.constructType(Integer.class)
    }))
}));

Inside _fromClass, _typeCache.get(key) return a cached entry and exits early. This simple type becomes refined with missing bindings (EMPTY_BINDINGS is passed into _fromClass, even though I passed explicit bindings) because resultType.getBindings() is empty. Our end result is a reference type Optional<Object>.

I deserialize and uh oh! I actually get an Optional<ArrayList> and I get an exception.

• Option 2: constructParametricType:

typeFactory.constructParametricType(Optional.class,
    typeFactory.constructParametricType(Set.class,
        typeFactory.constructType(Integer.class))
);

We go inside _fromClass and we do not early exit because the bindings I give are passed directly in. However, there's no refining happening so we end up with with a simple type Optional<Set<Integer>> which causes deserialization issues because the simple type is being deserialized as if it were a bean.

I deserialize and uh oh!

• Workaround:
  I was able to workaround this issue by extending TypeFactory and overriding constructParametricType and adding the refining as seen in _fromClass, as well as the with builder APIs because otherwise registering a module caused me to revert back to the original TypeFactory class:

public class TypeFactory2 extends TypeFactory {
	private static final long serialVersionUID = 6804290482184294993L;

	public TypeFactory2() {
		super(null);
	}

	public TypeFactory2(
		@Nullable LRUMap<Object, JavaType> typeCache,
		@Nullable TypeParser p,
		@Nullable TypeModifier[] mods,
		@Nullable ClassLoader classLoader
	) {
		super(typeCache, p, mods, classLoader);
	}

	@Override
	public TypeFactory withModifier(TypeModifier mod) {
		LRUMap<Object, JavaType> typeCache = _typeCache;
		TypeModifier[] mods;
		if (mod == null) { // mostly for unit tests
			mods = null;
			// 30-Jun-2016, tatu: for some reason expected semantics are to clear cache
			// in this case; can't recall why, but keeping the same
			typeCache = null;
		} else if (_modifiers == null) {
			mods = new TypeModifier[] { mod };
			// 29-Jul-2019, tatu: Actually I think we better clear cache in this case
			// as well to ensure no leakage occurs (see [databind#2395])
			typeCache = null;
		} else {
			// but may keep existing cache otherwise
			mods = ArrayBuilders.insertInListNoDup(_modifiers, mod);
		}
		return new TypeFactory2(typeCache, _parser, mods, _classLoader);
	}

	@Override
	public TypeFactory withClassLoader(ClassLoader classLoader) {
		return new TypeFactory2(_typeCache, _parser, _modifiers, classLoader);
	}

	@Override
	public TypeFactory withCache(LRUMap<Object, JavaType> cache) {
		return new TypeFactory2(cache, _parser, _modifiers, _classLoader);
	}

	@Override
	public JavaType constructParametricType(Class<?> rawType, JavaType... parameterTypes) {
		JavaType resultType = _fromClass(null, rawType, TypeBindings
			.create(rawType, parameterTypes));
		if (_modifiers != null) {
			TypeBindings b = resultType.getBindings();
			if (b == null) {
				b = EMPTY_BINDINGS;
			}
			for (TypeModifier mod : _modifiers) {
				JavaType t = mod.modifyType(resultType, rawType, b, this);
				if (t == null) {
					throw new IllegalStateException(String
						.format("TypeModifier %s (of type %s) return null for type %s", mod, mod
							.getClass()
							.getName(), resultType));
				}
				resultType = t;
			}
		}
		return resultType;
	}
}

Then,

objectMapper.setTypeFactory(new TypeFactory2());

If my problem was the API I was calling, please let me know kindly. TypeFactory is a bit of a minefield with all those deprecated and seemingly un-deprecated methods. :)

Thanks

[cowtowncoder]

I will have to read this through a few more times, perhaps create a test (complicated by the fact test needs to either copy part of jdk8 module for databind, or located in jdk8 module), but I suspect you are correct in that there is a bug. This would likely be a regression due to rewrite a few minor versions ago, to better handle type parameter substitution.

And yes, it is bit of a minefield. But I think your usage is perfectly fine -- complication likely comes from type refinement needed for Optional, which is mechanism by which modules can make TypeFactory "detect" specific kinds of types (Optional being a ReferenceType; something databind would not know on its own).

[cowtowncoder]

Oh, one quick comment before I have any tests: creating type for Set should ideally use createCollectionType(), since otherwise it might not get properly recognized as CollectionType (although constructType() probably should detect that).

There is also the "obvious" way of TypeReference:

JavaType type = typeFactory.constructType(new TypeReference<Optional<Set<Integer>>>() { });

but maybe there is a reason this won't work (for example, when there is need to pass components separately, fully programmatic construction).

[cowtowncoder]

Ok, first things first: yes, I think there is a bug in constructType(Type, TypeBindings) (or rather, _fromAny() that it calls); will fix that for 2.11.2; and I think that should make usage work.
Will see if I can find an issue with the other approach; sounds like refinement is not being called properly for some reason.

[cowtowncoder]

@TheSpiritXIII Ah. Actually, the method that should work right now is constructReferenceType(), since that is what Optional is: although it should not be required knowledge (i.e. constructType() with bindings should work).

But I think you are right, too, that constructParametricType() should invoke refinement methods as expected. I will add a test for that, fix the issue for 2.11.2 as well.
Other constructXxxType() methods do not call TypeModifier due to assumption being that caller indicates desired type -- not sure if that is optimal but I will probably want to limit amount of changes to do in patch release (and perhaps address it if there is use case).

[TheSpiritXIII]

constructType is what I intended to call originally. My input is actually a tree of Class<?> nodes. In my small and highly specific example, constructReferenceType would probably work correctly, but in reality when I receive a collection of Class<?> objects, I do not have insight on what is supposed to be a reference type vs a simple type vs a collection type. My opinion is that ultimately, this differentiation should be done by the Jackson library because these are concepts defined within Jackson.

I highly appreciate the very timely reply. Thank you for the prompt responses and for promising the turnaround in 2.11.2!