[ServiceExtension] Better sanity checking and wildcard support

org.osgi.test.junit5/src/main/java/org/osgi/test/junit5/inject/InjectingExtension.java

@@ -106,25 +106,21 @@ protected boolean supportsField(Field field, ExtensionContext extensionContext)
 		if (!isAnnotated(field, annotation())) {
 			return false;
 		}
+		if ((field.getModifiers() & disallowedFieldModifiers()) != 0) {
+			throw new ExtensionConfigurationException(String.format("Field %s must not be %s for annotation @%s.",
+				field.getName(), Modifier.toString(field.getModifiers() & disallowedFieldModifiers()),
+				annotation().getSimpleName()));
+		}
+
 		TargetType targetType = TargetType.of(field);
 		try {
-			if (!supportsType(targetType, extensionContext)) {
-				return false;
-			}
+			return supportsType(targetType, extensionContext);
 		} catch (ParameterResolutionException pre) {
 			// Convert to ExtensionConfigurationException for field
 			ExtensionConfigurationException ece = new ExtensionConfigurationException(pre.getMessage(), pre.getCause());
 			ece.setStackTrace(pre.getStackTrace());
 			throw ece;
 		}
-
-		if ((field.getModifiers() & disallowedFieldModifiers()) != 0) {
-			throw new ExtensionConfigurationException(
-				String.format("Field %s must not be %s for annotation @%s.", field.getName(),
-					Modifier.toString(field.getModifiers() & disallowedFieldModifiers()),
-					annotation().getSimpleName()));
-		}
-		return true;
 	}
 
 	/**
@@ -139,15 +135,14 @@ public boolean supportsParameter(ParameterContext parameterContext, ExtensionCon
 			return false;
 		}
 		TargetType targetType = TargetType.of(parameterContext.getParameter());
-		if (!supportsType(targetType, extensionContext)) {
-			return false;
-		}
-		return true;
+		return supportsType(targetType, extensionContext);
 	}
 
 	/**
 	 * Determine if this extender supports resolution for the specified
-	 * {@link TargetType} for the specified {@link ExtensionContext}.
+	 * {@link TargetType} for the specified {@link ExtensionContext}. The
+	 * default implementation checks through all of the types that were passed
+	 * to the constructor.
 	 */
 	protected boolean supportsType(TargetType targetType, ExtensionContext extensionContext)
 		throws ParameterResolutionException {

org.osgi.test.junit5/src/main/java/org/osgi/test/junit5/service/ServiceExtension.java

@@ -62,57 +62,72 @@ public ServiceExtension() {
 	}
 
 	@Override
-	protected boolean supportsType(TargetType targetType, ExtensionContext extensionContext)
+	protected boolean supportsType(TargetType targetType, ExtensionContext extensionContext) {
+		return true;
+	}
+
+	private Type extractServiceType(TargetType targetType, InjectService injectService)
 		throws ParameterResolutionException {
-		Type serviceType;
+		Type valueType;
+		Type upperBoundType = null;
 		if (targetType.matches(List.class) || targetType.matches(ServiceAware.class)) {
 			if (targetType.hasParameterizedTypes()) {
-				serviceType = targetType.getFirstGenericTypes()
+				valueType = targetType.getFirstGenericTypes()
 					.get();
-				if (serviceType instanceof WildcardType) {
-					serviceType = Object.class;
+				if (valueType instanceof WildcardType) {
+					final WildcardType wild = (WildcardType) valueType;
+					upperBoundType = (wild.getUpperBounds().length > 0) ? wild.getUpperBounds()[0] : Object.class;
 				}
 			} else {
-				serviceType = Object.class;
+				valueType = Object.class;
 			}
 		} else {
-			serviceType = targetType.getGenericType();
+			valueType = targetType.getGenericType();
 		}
-
-		if (!(serviceType instanceof Class)) {
+		if (upperBoundType == null) {
+			upperBoundType = valueType;
+		}
+		if (!(upperBoundType instanceof Class)) {
 			throw new ParameterResolutionException(String.format(
 				"Element %s has an unsupported type %s for annotation @%s. Service must have non-generic type.",
-				targetType.getName(), serviceType.getTypeName(), annotation().getSimpleName()));
+				targetType.getName(), upperBoundType.getTypeName(), annotation().getSimpleName()));
 		}
+		final Class<?> upperBoundClass = (Class<?>) upperBoundType;
 
-		return true;
-	}
+		final Class<?> serviceClass = injectService.service();
+		if (serviceClass.equals(annotation())) {
+			return upperBoundClass;
+		}
 
-	@Override
-	protected Object resolveValue(TargetType targetType, InjectService injectService, ExtensionContext extensionContext)
-		throws ParameterResolutionException {
-		Type serviceType = injectService.service();
-		if (serviceType.equals(annotation())) {
-			if (targetType.matches(List.class) || targetType.matches(ServiceAware.class)) {
-				if (targetType.hasParameterizedTypes()) {
-					serviceType = targetType.getFirstGenericTypes()
-						.get();
-					if (serviceType instanceof WildcardType) {
-						serviceType = Object.class;
-					}
-				} else {
-					serviceType = Object.class;
+		if (!upperBoundClass.isAssignableFrom(serviceClass)) {
+			throw new ParameterResolutionException(String.format(
+				"Element %s has service type %s for annotation @%s but field expects %s.", targetType.getName(),
+				serviceClass.getName(), annotation().getSimpleName(), valueType.getTypeName()));
+		}
+		if (valueType instanceof WildcardType) {
+			final WildcardType wild = (WildcardType) valueType;
+			if (wild.getLowerBounds().length > 0) {
+				final Type lowerBoundType = wild.getLowerBounds()[0];
+				if (!(lowerBoundType instanceof Class)) {
+					throw new ParameterResolutionException(String.format(
+						"Element %s has an unsupported lower bound %s for annotation @%s. Service must have non-generic type.",
+						targetType.getName(), lowerBoundType.getTypeName(), annotation().getSimpleName()));
+				}
+				if (!serviceClass.isAssignableFrom((Class<?>) lowerBoundType)) {
+					throw new ParameterResolutionException(String.format(
+						"Element %s has service type %s for annotation @%s but field expects %s.", targetType.getName(),
+						serviceClass.getName(), annotation().getSimpleName(), valueType.getTypeName()));
 				}
-			} else {
-				serviceType = targetType.getType();
 			}
 		}
 
-		if (!(serviceType instanceof Class)) {
-			throw new ParameterResolutionException(String.format(
-				"Element %s has an unsupported type %s for annotation @%s. Service must have non-generic type.",
-				targetType.getName(), serviceType.getTypeName(), annotation().getSimpleName()));
-		}
+		return serviceClass;
+	}
+
+	@Override
+	protected Object resolveValue(TargetType targetType, InjectService injectService, ExtensionContext extensionContext)
+		throws ParameterResolutionException {
+		final Type serviceType = extractServiceType(targetType, injectService);
 
 		ServiceConfiguration<?> configuration = getServiceConfiguration((Class<?>) serviceType, injectService.filter(),
 			injectService.filterArguments(), injectService.cardinality(), injectService.timeout(), extensionContext);

org.osgi.test.junit5/src/test/java/org/osgi/test/junit5/test/service/ServiceExtension_SanityCheckingTest.java

@@ -22,6 +22,7 @@
 
 import java.util.Date;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.jupiter.api.Test;
@@ -44,24 +45,42 @@ void myTest() {}
 	static class NonRawParameterType {
 		@SuppressWarnings("unused")
 		@Test
-		void myParameterTest(@InjectService AtomicReference<?> param) {}
+		void myParameterTest(@InjectService
+		AtomicReference<?> param) {}
 	}
 
 	static class ListOfNonRawParameterType {
 		@SuppressWarnings("unused")
 		@Test
-		void myParameterTest(@InjectService List<AtomicReference<?>> param) {}
+		void myParameterTest(@InjectService
+		List<AtomicReference<?>> param) {}
 	}
 
 	static class ServiceAwareOfNonRawParameterType {
 		@SuppressWarnings("unused")
 		@Test
-		void myParameterTest(@InjectService ServiceAware<AtomicReference<?>> param) {}
+		void myParameterTest(@InjectService
+		ServiceAware<AtomicReference<?>> param) {}
+	}
+
+	static class ServiceAwareOfNonRawUpperBoundType {
+		@SuppressWarnings("unused")
+		@Test
+		void myParameterTest(@InjectService
+		ServiceAware<? extends AtomicReference<?>> param) {}
+	}
+
+	static class ServiceAwareOfNonRawLowerBoundType {
+		@SuppressWarnings("unused")
+		@Test
+		void myParameterTest(@InjectService(service = AtomicReference.class)
+		ServiceAware<? super AtomicReference<?>> param) {}
 	}
 
 	@ParameterizedTest
 	@ValueSource(classes = {
-		NonRawParameterType.class, ListOfNonRawParameterType.class, ServiceAwareOfNonRawParameterType.class
+		NonRawParameterType.class, ListOfNonRawParameterType.class, ServiceAwareOfNonRawParameterType.class,
+		ServiceAwareOfNonRawUpperBoundType.class, ServiceAwareOfNonRawLowerBoundType.class
 	})
 	void annotatedParameter_withNonRawType_throwsException(Class<?> test) {
 		assertThatTest(test).isInstanceOf(ParameterResolutionException.class)
@@ -71,10 +90,6 @@ void annotatedParameter_withNonRawType_throwsException(Class<?> test) {
 	static class FinalField extends TestBase {
 		@InjectService
 		final Date bc = null;
-
-		@Override
-		@Test
-		void myTest() {}
 	}
 
 	@Test
@@ -85,16 +100,96 @@ void annotatedField_thatIsFinal_throwsException() {
 
 	static class PrivateField extends TestBase {
 		@InjectService
-		private Date bc = null;
-
-		@Override
-		@Test
-		void myTest() {}
+		private Date bc;
 	}
 
 	@Test
 	void annotatedField_thatIsPrivate_throwsException() {
 		assertThatTest(PrivateField.class).isInstanceOf(ExtensionConfigurationException.class)
 			.hasMessageContainingAll("bc", "must not be private", "@InjectService");
 	}
+
+	static class MismatchedServiceType extends TestBase {
+		@InjectService(service = AtomicBoolean.class)
+		Date bc;
+	}
+
+	static class MismatchedServiceType_List extends TestBase {
+		@InjectService(service = AtomicBoolean.class)
+		List<Date> bc;
+	}
+
+	static class MismatchedServiceType_ServiceAware extends TestBase {
+		@InjectService(service = AtomicBoolean.class)
+		ServiceAware<Date> bc;
+	}
+
+	static class MismatchedServiceType_ServiceAware_UpperWildcard extends TestBase {
+		// AtomicBoolean is not a subclass of Date
+		@InjectService(service = AtomicBoolean.class)
+		ServiceAware<? extends Date> bc;
+	}
+
+	static class MismatchedServiceType_ServiceAware_LowerWildcard extends TestBase {
+		// AtomicBoolean is not a superclass of Date
+		@InjectService(service = AtomicBoolean.class)
+		ServiceAware<? super Date> bc;
+	}
+
+	@ParameterizedTest
+	@ValueSource(classes = {
+		MismatchedServiceType.class, MismatchedServiceType_List.class, MismatchedServiceType_ServiceAware.class,
+	})
+	void annotatedField_withExplicitServiceType_thatDoesntMatchField_throwsException(Class<?> clazz) {
+		assertThatTest(clazz).isInstanceOf(ExtensionConfigurationException.class)
+			.hasMessageContainingAll("bc", "service type " + AtomicBoolean.class.getName(),
+				"expects " + Date.class.getName(),
+				"@InjectService");
+	}
+
+	@Test
+	void annotatedField_withExplicitServiceType_andUpperBoundThatDoesntMatch_throwsException() {
+		assertThatTest(MismatchedServiceType_ServiceAware_UpperWildcard.class)
+			.isInstanceOf(ExtensionConfigurationException.class)
+			.hasMessageContainingAll("bc", "service type " + AtomicBoolean.class.getName(),
+				"expects ? extends " + Date.class.getName(), "@InjectService");
+	}
+
+	@Test
+	void annotatedField_withExplicitServiceType_andLowerBoundThatDoesntMatch_throwsException() {
+		assertThatTest(MismatchedServiceType_ServiceAware_LowerWildcard.class)
+			.isInstanceOf(ExtensionConfigurationException.class)
+			.hasMessageContainingAll("bc", "service type " + AtomicBoolean.class.getName(),
+				"expects ? super " + Date.class.getName(), "@InjectService");
+	}
+
+	static class MismatchedServiceType_Parameter {
+		@Test
+		void myParameterTest(@InjectService(service = AtomicBoolean.class)
+		Date bc) {}
+	}
+
+	static class MismatchedServiceType_Parameter_List {
+		@Test
+		void myParameterTest(@InjectService(service = AtomicBoolean.class)
+		List<Date> bc) {}
+	}
+
+	static class MismatchedServiceType_Parameter_ServiceAware {
+		@Test
+		void myParameterTest(@InjectService(service = AtomicBoolean.class)
+		ServiceAware<Date> bc) {}
+	}
+
+	@ParameterizedTest
+	@ValueSource(classes = {
+		MismatchedServiceType_Parameter.class, MismatchedServiceType_Parameter_List.class,
+		MismatchedServiceType_Parameter_ServiceAware.class
+	})
+	void annotatedParameter_withExplicitServiceType_thatDoesntMatchParameter_throwsException(Class<?> testClass) {
+		assertThatTest(testClass)
+			.isInstanceOf(ParameterResolutionException.class)
+			.hasMessageContainingAll("service type " + AtomicBoolean.class.getName(),
+				"expects " + Date.class.getName(), "@InjectService");
+	}
 }

org.osgi.test.junit5/src/test/java/org/osgi/test/junit5/test/service/ServiceListTest.java

@@ -37,6 +37,12 @@ public class ServiceListTest {
 	BundleContext										bc;
 	@InjectService
 	List<LogService>									logServices;
+	@InjectService(service = LogService.class)
+	List<?>												unboundedWildcardLogService;
+	@InjectService
+	List<? extends LogService>							upperWildcardLogService;
+	@InjectService(service = LogService.class)
+	List<? super LogService>							lowerWildcardLogService;
 
 	@InjectService(cardinality = 0)
 	List<TestServiceWithMultipleCardinality>			testServices;
@@ -46,8 +52,25 @@ public class ServiceListTest {
 
 	@Test
 	public void testField() throws Exception {
-		assertThat(logServices).size()
-			.isEqualTo(1);
+		assertThat(logServices).hasSize(1);
+	}
+
+	@Test
+	public void testUpperWildcard() {
+		assertThat(upperWildcardLogService).hasSize(1)
+			.isEqualTo(logServices);
+	}
+
+	@Test
+	public void testLowerWildcard() {
+		assertThat(lowerWildcardLogService).hasSize(1)
+			.isEqualTo(logServices);
+	}
+
+	@Test
+	public void testUnboundedWildcard() {
+		assertThat(unboundedWildcardLogService).hasSize(1)
+			.isEqualTo(logServices);
 	}
 
 	@Test