Report offending class in SpringFactoriesLoader in exception message

Prior to this commit, the exception message generated by
instantiateFactory() in SpringFactoriesLoader did not report the
offending class in the top-level exception message. The offending class
was in fact included in the message of the nested exception, but the
top-level exception message on its own was a bit misleading.

This commit improves the diagnostics for such failures by including the
offending class and the target factory type in the top-level exception
message.

Closes gh-22453

spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -83,25 +83,25 @@ public final class SpringFactoriesLoader {
 	 * <p>The returned factories are sorted through {@link AnnotationAwareOrderComparator}.
 	 * <p>If a custom instantiation strategy is required, use {@link #loadFactoryNames}
 	 * to obtain all registered factory names.
-	 * @param factoryClass the interface or abstract class representing the factory
+	 * @param factoryType the interface or abstract class representing the factory
 	 * @param classLoader the ClassLoader to use for loading (can be {@code null} to use the default)
 	 * @throws IllegalArgumentException if any factory implementation class cannot
 	 * be loaded or if an error occurs while instantiating any factory
 	 * @see #loadFactoryNames
 	 */
-	public static <T> List<T> loadFactories(Class<T> factoryClass, @Nullable ClassLoader classLoader) {
-		Assert.notNull(factoryClass, "'factoryClass' must not be null");
+	public static <T> List<T> loadFactories(Class<T> factoryType, @Nullable ClassLoader classLoader) {
+		Assert.notNull(factoryType, "'factoryType' must not be null");
 		ClassLoader classLoaderToUse = classLoader;
 		if (classLoaderToUse == null) {
 			classLoaderToUse = SpringFactoriesLoader.class.getClassLoader();
 		}
-		List<String> factoryNames = loadFactoryNames(factoryClass, classLoaderToUse);
+		List<String> factoryImplementationNames = loadFactoryNames(factoryType, classLoaderToUse);
 		if (logger.isTraceEnabled()) {
-			logger.trace("Loaded [" + factoryClass.getName() + "] names: " + factoryNames);
+			logger.trace("Loaded [" + factoryType.getName() + "] names: " + factoryImplementationNames);
 		}
-		List<T> result = new ArrayList<>(factoryNames.size());
-		for (String factoryName : factoryNames) {
-			result.add(instantiateFactory(factoryName, factoryClass, classLoaderToUse));
+		List<T> result = new ArrayList<>(factoryImplementationNames.size());
+		for (String factoryImplementationName : factoryImplementationNames) {
+			result.add(instantiateFactory(factoryImplementationName, factoryType, classLoaderToUse));
 		}
 		AnnotationAwareOrderComparator.sort(result);
 		return result;
@@ -111,15 +111,15 @@ public final class SpringFactoriesLoader {
 	 * Load the fully qualified class names of factory implementations of the
 	 * given type from {@value #FACTORIES_RESOURCE_LOCATION}, using the given
 	 * class loader.
-	 * @param factoryClass the interface or abstract class representing the factory
+	 * @param factoryType the interface or abstract class representing the factory
 	 * @param classLoader the ClassLoader to use for loading resources; can be
 	 * {@code null} to use the default
 	 * @throws IllegalArgumentException if an error occurs while loading factory names
 	 * @see #loadFactories
 	 */
-	public static List<String> loadFactoryNames(Class<?> factoryClass, @Nullable ClassLoader classLoader) {
-		String factoryClassName = factoryClass.getName();
-		return loadSpringFactories(classLoader).getOrDefault(factoryClassName, Collections.emptyList());
+	public static List<String> loadFactoryNames(Class<?> factoryType, @Nullable ClassLoader classLoader) {
+		String factoryTypeName = factoryType.getName();
+		return loadSpringFactories(classLoader).getOrDefault(factoryTypeName, Collections.emptyList());
 	}
 
 	private static Map<String, List<String>> loadSpringFactories(@Nullable ClassLoader classLoader) {
@@ -138,9 +138,9 @@ public final class SpringFactoriesLoader {
 				UrlResource resource = new UrlResource(url);
 				Properties properties = PropertiesLoaderUtils.loadProperties(resource);
 				for (Map.Entry<?, ?> entry : properties.entrySet()) {
-					String factoryClassName = ((String) entry.getKey()).trim();
-					for (String factoryName : StringUtils.commaDelimitedListToStringArray((String) entry.getValue())) {
-						result.add(factoryClassName, factoryName.trim());
+					String factoryTypeName = ((String) entry.getKey()).trim();
+					for (String factoryImplementationName : StringUtils.commaDelimitedListToStringArray((String) entry.getValue())) {
+						result.add(factoryTypeName, factoryImplementationName.trim());
 					}
 				}
 			}
@@ -154,17 +154,19 @@ public final class SpringFactoriesLoader {
 	}
 
 	@SuppressWarnings("unchecked")
-	private static <T> T instantiateFactory(String instanceClassName, Class<T> factoryClass, ClassLoader classLoader) {
+	private static <T> T instantiateFactory(String factoryImplementationName, Class<T> factoryType, ClassLoader classLoader) {
 		try {
-			Class<?> instanceClass = ClassUtils.forName(instanceClassName, classLoader);
-			if (!factoryClass.isAssignableFrom(instanceClass)) {
+			Class<?> factoryImplementationClass = ClassUtils.forName(factoryImplementationName, classLoader);
+			if (!factoryType.isAssignableFrom(factoryImplementationClass)) {
 				throw new IllegalArgumentException(
-						"Class [" + instanceClassName + "] is not assignable to [" + factoryClass.getName() + "]");
+						"Class [" + factoryImplementationName + "] is not assignable to factory type [" + factoryType.getName() + "]");
 			}
-			return (T) ReflectionUtils.accessibleConstructor(instanceClass).newInstance();
+			return (T) ReflectionUtils.accessibleConstructor(factoryImplementationClass).newInstance();
 		}
 		catch (Throwable ex) {
-			throw new IllegalArgumentException("Unable to instantiate factory class: " + factoryClass.getName(), ex);
+			throw new IllegalArgumentException(
+				"Unable to instantiate factory class [" + factoryImplementationName + "] for factory type [" + factoryType.getName() + "]",
+				ex);
 		}
 	}
 

spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,7 +19,9 @@ package org.springframework.core.io.support;
 import java.lang.reflect.Modifier;
 import java.util.List;
 
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import static org.junit.Assert.*;
 
@@ -28,9 +30,13 @@ import static org.junit.Assert.*;
  *
  * @author Arjen Poutsma
  * @author Phillip Webb
+ * @author Sam Brannen
  */
 public class SpringFactoriesLoaderTests {
 
+	@Rule
+	public final ExpectedException exception = ExpectedException.none();
+
 	@Test
 	public void loadFactoriesInCorrectOrder() {
 		List<DummyFactory> factories = SpringFactoriesLoader.loadFactories(DummyFactory.class, null);
@@ -39,17 +45,20 @@ public class SpringFactoriesLoaderTests {
 		assertTrue(factories.get(1) instanceof MyDummyFactory2);
 	}
 
-	@Test(expected = IllegalArgumentException.class)
-	public void loadInvalid() {
-		SpringFactoriesLoader.loadFactories(String.class, null);
-	}
-
 	@Test
 	public void loadPackagePrivateFactory() {
 		List<DummyPackagePrivateFactory> factories =
 				SpringFactoriesLoader.loadFactories(DummyPackagePrivateFactory.class, null);
 		assertEquals(1, factories.size());
-		assertTrue((factories.get(0).getClass().getModifiers() & Modifier.PUBLIC) == 0);
+		assertFalse(Modifier.isPublic(factories.get(0).getClass().getModifiers()));
+	}
+
+	@Test
+	public void attemptToLoadFactoryOfIncompatibleType() {
+		exception.expect(IllegalArgumentException.class);
+		exception.expectMessage("Unable to instantiate factory class [org.springframework.core.io.support.MyDummyFactory1] for factory type [java.lang.String]");
+
+		SpringFactoriesLoader.loadFactories(String.class, null);
 	}
 
 }