Invalid instance injected for generic type in case of partial type variable [SPR-16179]

[spring-projects-issues]

Oliver Drotbohm opened SPR-16179 and commented

The following test fails:

package example;

import static org.hamcrest.CoreMatchers.*;
import static org.junit.Assert.*;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration
public class SpringInjectionTest {

	@Configuration
	static class Config {

		@Bean
		PageAssembler<?> assembler() {
			return new PageAssembler<>();
		}
	}

	@Autowired(required = false) Assembler<SomeOtherType> assembler;

	@Test
	public void testname() {
		assertThat(assembler, is(nullValue()));
	}

	interface Assembler<T> {}

	static class PageAssembler<T> implements Assembler<Page<T>> {}

	interface Page<T> {}

	interface SomeOtherType {}
}

Spring injects the configured bean instance here despite the fact that the generic declarations clearly don't match as Page<T> is not compatible with the requested SomeOtherType.

---

Affects: 4.3.12, 5.0.1

Referenced from: commits 59d654b, e2bb06e

[spring-projects-issues]

Juergen Hoeller commented

This turns out be rather involved since we do mean to accept raw matches as a fallback in case of unresolvable generics there... and in order to differentiate such cases, we'd have to perform a partial generic match where just the unresolvable variable is left open. Let's see what we can do for 5.0.3 here, with a potential backport to 4.3.14.

[spring-projects-issues]

Sergei Ustimenko commented

Hi Juergen Hoeller

I'm quite interested in this ticket being resolved and want to contribute the fix if no one minds.

I've already experimented with this bug and was curious about the last assert statement in the Spr16179 test. Please consider slightly modified version of the test:

@Test
public void repro() {
...
   //the same assert as in original test
   assertSame(bf.getBean("pageAssembler"), bf.getBean(AssemblerInjection.class).assembler6);
}

@Configuration
static class AssemblerConfig {
@Bean
PageAssemblerImpl<?> pageAssembler() {
   //anonymous Integer assembler
   return new PageAssemblerImpl<Integer>() {};
}
...
}
public static class AssemblerInjection {
...
@Autowired(required = false)
   PageAssembler<String> assembler6;
}

I think above should always fail as

PageAssemblerImpl<?> integerAssembler = new PageAssemblerImpl<Integer>() {};
PageAssembler<String> assembler6 = integerAssembler;

wouldn't and shouldn't compile.

Maybe last assert should look like following:

assertNull(bf.getBean(AssemblerInjection.class).assembler6);

instead?

[spring-projects-issues]

Sergei Ustimenko commented

Adding a little bit more on the example above. If test would be like following:

interface PageAssembler<T> extends Assembler<Page<T>> { 
   T getField(); 
}
static class PageAssemblerImpl<T> implements PageAssembler<T> {
   private final T field;
   PageAssemblerImpl(T field) { this.field = field; }
   public T getField() { return field; }
}

and then if one would use

String field = bf.getBean(AssemblerInjection.class).assembler6.getField();

that will successfully cause CCE.

[spring-projects-issues]

Sergei Ustimenko commented

Juergen Hoeller Oliver Drotbohm Stéphane Nicoll rstoyanchev

I understand that spring team has things to do, so once anyone will have time, could that one please put more clarification on the questions above? I'm looking forward to know your thoughts and fix the bug.

[jhoeller]

This requires a dedicated matching step within descriptor.fallbackMatchAllowed() in GenericTypeAwareAutowireCandidateResolver:

	if (targetType.hasUnresolvableGenerics()) {
		return dependencyType.isAssignableWithinResolvedPart(targetType);
	}
	else if (targetType.resolve() == Properties.class) {
		return true;
	}

Within that special isAssignable check, matching the nested generics until an unresolvable part (other.isUnresolvableTypeVariable() or other typeBounds without ourBounds) is encountered on the other side and returning true then - but only after the nested generics matched up until that point. As far as the prototype goes, the test suite passes with such an arrangement. This might be too risky for 6.1.x but easy enough to roll into 6.2.