From 4e32615b2297149b75da98aa1be2cbfa967d2212 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Tue, 28 Apr 2020 14:55:37 +0200 Subject: [PATCH] Filter out duplicates in SpringFactoriesLoader Prior to this commit, SpringFactoriesLoader discovered all registered factory implementations for a given factory type even if duplicates were registered within a single META-INF/spring.factories file or in multiple such files in the classpath. This commit updates the internals of SpringFactoriesLoader so that duplicate registrations are ignored, thereby aligning with the well-known semantics for java.util.ServiceLoader in this regard. Closes gh-24985 --- .../io/support/SpringFactoriesLoader.java | 36 +++++++++++++------ .../support/SpringFactoriesLoaderTests.java | 8 ++++- .../test/resources/META-INF/spring.factories | 1 + 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java b/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java index 6379cf7c9a..a1f4b9ebb8 100644 --- a/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java +++ b/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -21,9 +21,12 @@ import java.net.URL; import java.util.ArrayList; import java.util.Collections; import java.util.Enumeration; +import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -34,8 +37,6 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ConcurrentReferenceHashMap; -import org.springframework.util.LinkedMultiValueMap; -import org.springframework.util.MultiValueMap; import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; @@ -70,7 +71,7 @@ public final class SpringFactoriesLoader { private static final Log logger = LogFactory.getLog(SpringFactoriesLoader.class); - private static final Map> cache = new ConcurrentReferenceHashMap<>(); + private static final Map>> cache = new ConcurrentReferenceHashMap<>(); private SpringFactoriesLoader() { @@ -83,6 +84,9 @@ public final class SpringFactoriesLoader { *

The returned factories are sorted through {@link AnnotationAwareOrderComparator}. *

If a custom instantiation strategy is required, use {@link #loadFactoryNames} * to obtain all registered factory names. + *

As of Spring Framework 5.3, if duplicate implementation class names are + * discovered for a given factory type, only one instance of the duplicated + * implementation type will be instantiated. * @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 @@ -111,6 +115,9 @@ 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. + *

As of Spring Framework 5.3, if a particular implementation class name + * is discovered more than once for the given factory type, duplicates will + * be ignored. * @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 @@ -119,38 +126,45 @@ public final class SpringFactoriesLoader { */ public static List loadFactoryNames(Class factoryType, @Nullable ClassLoader classLoader) { String factoryTypeName = factoryType.getName(); - return loadSpringFactories(classLoader).getOrDefault(factoryTypeName, Collections.emptyList()); + Set factoryNames = loadSpringFactories(classLoader).get(factoryTypeName); + if (factoryNames == null || factoryNames.isEmpty()) { + return Collections.emptyList(); + } + return Collections.unmodifiableList(new ArrayList<>(factoryNames)); } - private static Map> loadSpringFactories(@Nullable ClassLoader classLoader) { - MultiValueMap result = cache.get(classLoader); + private static Map> loadSpringFactories(@Nullable ClassLoader classLoader) { + Map> result = cache.get(classLoader); if (result != null) { return result; } + result = new HashMap<>(); try { Enumeration urls = (classLoader != null ? classLoader.getResources(FACTORIES_RESOURCE_LOCATION) : ClassLoader.getSystemResources(FACTORIES_RESOURCE_LOCATION)); - result = new LinkedMultiValueMap<>(); while (urls.hasMoreElements()) { URL url = urls.nextElement(); UrlResource resource = new UrlResource(url); Properties properties = PropertiesLoaderUtils.loadProperties(resource); for (Map.Entry entry : properties.entrySet()) { String factoryTypeName = ((String) entry.getKey()).trim(); - for (String factoryImplementationName : StringUtils.commaDelimitedListToStringArray((String) entry.getValue())) { - result.add(factoryTypeName, factoryImplementationName.trim()); + String[] factoryImplementationNames = + StringUtils.commaDelimitedListToStringArray((String) entry.getValue()); + for (String factoryImplementationName : factoryImplementationNames) { + result.computeIfAbsent(factoryTypeName, key -> new LinkedHashSet()) + .add(factoryImplementationName.trim()); } } } cache.put(classLoader, result); - return result; } catch (IOException ex) { throw new IllegalArgumentException("Unable to load factories from location [" + FACTORIES_RESOURCE_LOCATION + "]", ex); } + return result; } @SuppressWarnings("unchecked") diff --git a/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java b/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java index 25b1f46267..aff7c7fea1 100644 --- a/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java +++ b/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java @@ -34,7 +34,13 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException class SpringFactoriesLoaderTests { @Test - void loadFactoriesInCorrectOrder() { + void loadFactoriesWithNoRegisteredImplementations() { + List factories = SpringFactoriesLoader.loadFactories(Integer.class, null); + assertThat(factories).isEmpty(); + } + + @Test + void loadFactoriesInCorrectOrderWithDuplicateRegistrationsPresent() { List factories = SpringFactoriesLoader.loadFactories(DummyFactory.class, null); assertThat(factories).hasSize(2); assertThat(factories.get(0)).isInstanceOf(MyDummyFactory1.class); diff --git a/spring-core/src/test/resources/META-INF/spring.factories b/spring-core/src/test/resources/META-INF/spring.factories index 3c2c4e9d95..8710b50e47 100644 --- a/spring-core/src/test/resources/META-INF/spring.factories +++ b/spring-core/src/test/resources/META-INF/spring.factories @@ -1,5 +1,6 @@ org.springframework.core.io.support.DummyFactory =\ org.springframework.core.io.support.MyDummyFactory2, \ +org.springframework.core.io.support.MyDummyFactory1, \ org.springframework.core.io.support.MyDummyFactory1 java.lang.String=\