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

GenericConversionService finds wrong converter for partially unresolvable generic types #34298

Closed
tommyk-gears opened this issue Jan 21, 2025 · 3 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@tommyk-gears
Copy link

If there are multiple converters for similar types, but with different generic type arguments that are partially unresolvable - then the generic type arguments are effectively ignored and as a result one might get a non-matching converter.

The issue can be demonstrated with this test:

class GenericConversionServiceTests {
	
	private final GenericConversionService conversionService = new GenericConversionService();

	@Test
	void stringListToListOfSubClassesOfUnboundGenericClass() throws Exception {
		conversionService.addConverter(new StringListToAListConverter());
		conversionService.addConverter(new StringListToBListConverter());

		List<ARaw> aList = (List<ARaw>) conversionService.convert(List.of("foo"),
				TypeDescriptor.collection(List.class, TypeDescriptor.valueOf(String.class)),
				TypeDescriptor.collection(List.class, TypeDescriptor.valueOf(ARaw.class)));
		assertThat(aList).allMatch(e -> e instanceof ARaw);

		List<BRaw> bList = (List<BRaw>) conversionService.convert(List.of("foo"),
				TypeDescriptor.collection(List.class, TypeDescriptor.valueOf(String.class)),
				TypeDescriptor.collection(List.class, TypeDescriptor.valueOf(BRaw.class)));
		assertThat(bList).allMatch(e -> e instanceof BRaw);
	}

	public class ARaw extends GenericBaseClass {
	}

	public class BRaw extends GenericBaseClass {
	}

	public class GenericBaseClass<T> {
	}

	public class StringListToAListConverter implements Converter<List<String>, List<ARaw>> {

		@Override
		public List<ARaw> convert(List<String> source) {
			return List.of(new ARaw());
		}
	}

	public class StringListToBListConverter implements Converter<List<String>, List<BRaw>> {

		@Override
		public List<BRaw> convert(List<String> source) {
			return List.of(new BRaw());
		}
	}
}

As far as I understand this could be fixed by a small change in GenericConversionService.ConverterAdapter#matches to take the resolvable part into account even if the type is not completely unresolvable. I.e. if the resolvable part does not match, then this converter does not match.

@Override
public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
	// Check raw type first...
	if (this.typeInfo.getTargetType() != targetType.getObjectType()) {
		return false;
	}
	// Full check for complex generic type match required?
	ResolvableType rt = targetType.getResolvableType();
	if (!(rt.getType() instanceof Class) && !rt.isAssignableFrom(this.targetType) &&
-					!this.targetType.hasUnresolvableGenerics()) {
+					(!this.targetType.hasUnresolvableGenerics() || !rt.isAssignableFromResolvedPart(this.targetType))) {
		return false;
	}
	return !(this.converter instanceof ConditionalConverter conditionalConverter) ||
			conditionalConverter.matches(sourceType, targetType);
}

The concrete use case I have is converting a List<A> to List<B> where B is a generated protobuf class which has partially unresolvable generics due to this. Also, I cannot use a plain Converter<A,B> because the converter needs access to the full source list to produce the target list - so I the natural solution is to have a Converter<List<A>, List<B>>, but it does not work since I have multiple such converters with same source type and different target types.
One possible workaround would be to wrap the Lists in distinct wrapper classes during conversion, but I think it would be better to just make GenericConversionService consider partially resolvable generics.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 21, 2025
@jhoeller jhoeller self-assigned this Jan 21, 2025
@jhoeller jhoeller added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 21, 2025
@jhoeller jhoeller added this to the 6.2.3 milestone Jan 21, 2025
@jhoeller
Copy link
Contributor

Good point! It turns out that this can be optimized even further to a common isAssignableFromResolvedPart check without any hasUnresolvableGenerics guarding at all. I'm revising this for 6.2.3.

@jhoeller
Copy link
Contributor

@tommyk-gears any chance you could give the latest 6.2.3 snapshot a try? In addition to this fix, a couple of other generic type matching refinements have been applied in the meantime.

@tommyk-gears
Copy link
Author

@tommyk-gears any chance you could give the latest 6.2.3 snapshot a try? In addition to this fix, a couple of other generic type matching refinements have been applied in the meantime.

For sure. I tried this with version 6.2.3-20250131.143040-22, and it works like a charm.
Thanks for quick response and fix @jhoeller!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants