Ignore duplicate config metadata for cache key in TestContext framework

This commit ignores duplicate configuration metadata when generating
the ApplicationContext cache key (i.e., MergedContextConfiguration) in
the Spring TestContext Framework. This performed for the following
annotations.

- @ContextConfiguration
- @activeprofiles
- @TestPropertySource

In addition, this commit reinstates validation of the rules for
repeated @TestPropertySource annotations that was removed when support
for @NestedTestConfiguration was introduced.

Closes spring-projectsgh-25800

spring-test/src/main/java/org/springframework/test/context/MergedContextConfiguration.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 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.
@@ -19,8 +19,8 @@
 import java.io.Serializable;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.LinkedHashSet;
 import java.util.Set;
+import java.util.TreeSet;
 
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextInitializer;
@@ -533,8 +533,8 @@ private static String[] processActiveProfiles(@Nullable String[] activeProfiles)
 			return EMPTY_STRING_ARRAY;
 		}
 
-		// Active profiles must be unique
-		Set<String> profilesSet = new LinkedHashSet<>(Arrays.asList(activeProfiles));
+		// Active profiles must be unique and sorted
+		Set<String> profilesSet = new TreeSet<>(Arrays.asList(activeProfiles));
 		return StringUtils.toStringArray(profilesSet);
 	}
 

spring-test/src/main/java/org/springframework/test/context/support/ActiveProfilesUtils.java

@@ -18,9 +18,9 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.TreeSet;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -115,7 +115,7 @@ static String[] resolveActiveProfiles(Class<?> testClass) {
 		// Reverse the list so that we can traverse "down" the hierarchy.
 		Collections.reverse(profileArrays);
 
-		Set<String> activeProfiles = new LinkedHashSet<>();
+		Set<String> activeProfiles = new TreeSet<>();
 		for (String[] profiles : profileArrays) {
 			for (String profile : profiles) {
 				if (StringUtils.hasText(profile)) {

spring-test/src/main/java/org/springframework/test/context/support/ContextLoaderUtils.java

@@ -244,14 +244,35 @@ static List<ContextConfigurationAttributes> resolveContextConfigurationAttribute
 					annotationType.getName(), testClass.getName()));
 
 		List<ContextConfigurationAttributes> attributesList = new ArrayList<>();
+		ContextConfiguration previousAnnotation = null;
+		Class<?> previousDeclaringClass = null;
 		while (descriptor != null) {
-			convertContextConfigToConfigAttributesAndAddToList(descriptor.synthesizeAnnotation(),
-					descriptor.getRootDeclaringClass(), attributesList);
+			ContextConfiguration currentAnnotation = descriptor.synthesizeAnnotation();
+			// Don't ignore duplicate @ContextConfiguration declaration without resources,
+			// because the ContextLoader will likely detect default resources specific to the
+			// annotated class.
+			if (currentAnnotation.equals(previousAnnotation) && hasResources(currentAnnotation)) {
+				if (logger.isDebugEnabled()) {
+					logger.debug(String.format("Ignoring duplicate %s declaration on [%s], "
+							+ "since it is also declared on [%s].", currentAnnotation,
+							previousDeclaringClass.getName(), descriptor.getRootDeclaringClass().getName()));
+				}
+			}
+			else {
+				convertContextConfigToConfigAttributesAndAddToList(currentAnnotation,
+						descriptor.getRootDeclaringClass(), attributesList);
+			}
+			previousAnnotation = currentAnnotation;
+			previousDeclaringClass = descriptor.getRootDeclaringClass();
 			descriptor = descriptor.next();
 		}
 		return attributesList;
 	}
 
+	private static boolean hasResources(ContextConfiguration contextConfiguration) {
+		return (contextConfiguration.locations().length > 0 || contextConfiguration.classes().length > 0);
+	}
+
 	/**
 	 * Convenience method for creating a {@link ContextConfigurationAttributes}
 	 * instance from the supplied {@link ContextConfiguration} annotation and

spring-test/src/main/java/org/springframework/test/context/support/TestPropertySourceAttributes.java

@@ -17,15 +17,17 @@
 package org.springframework.test.context.support;
 
 import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Arrays;
 import java.util.List;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
 import org.springframework.core.annotation.MergedAnnotation;
 import org.springframework.core.io.ClassPathResource;
+import org.springframework.core.log.LogMessage;
 import org.springframework.core.style.ToStringCreator;
+import org.springframework.lang.Nullable;
 import org.springframework.test.context.TestPropertySource;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
@@ -53,6 +55,8 @@ class TestPropertySourceAttributes {
 
 	private final Class<?> declaringClass;
 
+	private final MergedAnnotation<?> rootAnnotation;
+
 	private final List<String> locations = new ArrayList<>();
 
 	private final boolean inheritLocations;
@@ -64,25 +68,83 @@ class TestPropertySourceAttributes {
 
 	TestPropertySourceAttributes(MergedAnnotation<TestPropertySource> annotation) {
 		this.declaringClass = declaringClass(annotation);
+		this.rootAnnotation = annotation.getRoot();
 		this.inheritLocations = annotation.getBoolean("inheritLocations");
 		this.inheritProperties = annotation.getBoolean("inheritProperties");
-		mergePropertiesAndLocations(annotation);
+		addPropertiesAndLocationsFrom(annotation);
+	}
+
+	/**
+	 * Merge this {@code TestPropertySourceAttributes} instance with the
+	 * supplied {@code TestPropertySourceAttributes}, asserting that the two sets
+	 * of test property source attributes have identical values for the
+	 * {@link TestPropertySource#inheritLocations} and
+	 * {@link TestPropertySource#inheritProperties} flags and that the two
+	 * underlying annotations were declared on the same class.
+	 * @since 5.2
+	 */
+	void mergeWith(TestPropertySourceAttributes attributes) {
+		Assert.state(attributes.declaringClass == this.declaringClass,
+				() -> "Detected @TestPropertySource declarations within an aggregate index "
+						+ "with different sources: " + this.declaringClass.getName() + " and "
+						+ attributes.declaringClass.getName());
+		logger.trace(LogMessage.format("Retrieved %s for declaring class [%s].",
+				attributes, this.declaringClass.getName()));
+		assertSameBooleanAttribute(this.inheritLocations, attributes.inheritLocations,
+				"inheritLocations", attributes);
+		assertSameBooleanAttribute(this.inheritProperties, attributes.inheritProperties,
+				"inheritProperties", attributes);
+		mergePropertiesAndLocationsFrom(attributes);
 	}
 
-	private void mergePropertiesAndLocations(MergedAnnotation<TestPropertySource> annotation) {
-		String[] locations = annotation.getStringArray("locations");
-		String[] properties = annotation.getStringArray("properties");
+	private void assertSameBooleanAttribute(boolean expected, boolean actual,
+			String attributeName, TestPropertySourceAttributes that) {
+
+		Assert.isTrue(expected == actual, () -> String.format(
+				"@%s on %s and @%s on %s must declare the same value for '%s' as other " +
+				"directly present or meta-present @TestPropertySource annotations",
+			this.rootAnnotation.getType().getSimpleName(), this.declaringClass.getSimpleName(),
+			that.rootAnnotation.getType().getSimpleName(), that.declaringClass.getSimpleName(),
+			attributeName));
+	}
+
+	private void addPropertiesAndLocationsFrom(MergedAnnotation<TestPropertySource> mergedAnnotation) {
+		String[] locations = mergedAnnotation.getStringArray("locations");
+		String[] properties = mergedAnnotation.getStringArray("properties");
+		addPropertiesAndLocations(locations, properties, declaringClass(mergedAnnotation), false);
+	}
+
+	private void mergePropertiesAndLocationsFrom(TestPropertySourceAttributes attributes) {
+		addPropertiesAndLocations(attributes.getLocations(), attributes.getProperties(),
+				attributes.getDeclaringClass(), true);
+	}
+
+	private void addPropertiesAndLocations(String[] locations, String[] properties,
+			Class<?> declaringClass, boolean prepend) {
+
 		if (ObjectUtils.isEmpty(locations) && ObjectUtils.isEmpty(properties)) {
-			Collections.addAll(this.locations, detectDefaultPropertiesFile(annotation));
+			addAll(prepend, this.locations, detectDefaultPropertiesFile(declaringClass));
 		}
 		else {
-			Collections.addAll(this.locations, locations);
-			Collections.addAll(this.properties, properties);
+			addAll(prepend, this.locations, locations);
+			addAll(prepend, this.properties, properties);
 		}
 	}
 
-	private String detectDefaultPropertiesFile(MergedAnnotation<TestPropertySource> annotation) {
-		Class<?> testClass = declaringClass(annotation);
+	/**
+	 * Add all of the supplied elements to the provided list, honoring the
+	 * {@code prepend} flag.
+	 * <p>If the {@code prepend} flag is {@code false}, the elements will appended
+	 * to the list.
+	 * @param prepend whether the elements should be prepended to the list
+	 * @param list the list to which to add the elements
+	 * @param elements the elements to add to the list
+	 */
+	private void addAll(boolean prepend, List<String> list, String... elements) {
+		list.addAll((prepend ? 0 : list.size()), Arrays.asList(elements));
+	}
+
+	private String detectDefaultPropertiesFile(Class<?> testClass) {
 		String resourcePath = ClassUtils.convertClassNameToResourcePath(testClass.getName()) + ".properties";
 		ClassPathResource classPathResource = new ClassPathResource(resourcePath);
 		if (!classPathResource.exists()) {
@@ -153,6 +215,45 @@ boolean isInheritProperties() {
 		return this.inheritProperties;
 	}
 
+	boolean isEmpty() {
+		return (this.locations.isEmpty() && this.properties.isEmpty());
+	}
+
+	@Override
+	public boolean equals(@Nullable Object other) {
+		if (this == other) {
+			return true;
+		}
+		if (other == null || other.getClass() != getClass()) {
+			return false;
+		}
+
+		TestPropertySourceAttributes that = (TestPropertySourceAttributes) other;
+		if (!this.locations.equals(that.locations)) {
+			return false;
+		}
+		if (!this.properties.equals(that.properties)) {
+			return false;
+		}
+		if (this.inheritLocations != that.inheritLocations) {
+			return false;
+		}
+		if (this.inheritProperties != that.inheritProperties) {
+			return false;
+		}
+
+		return true;
+	}
+
+	@Override
+	public int hashCode() {
+		int result = this.locations.hashCode();
+		result = 31 * result + this.properties.hashCode();
+		result = 31 * result + (this.inheritLocations ? 1231 : 1237);
+		result = 31 * result + (this.inheritProperties ? 1231 : 1237);
+		return result;
+	}
+
 	/**
 	 * Provide a String representation of the {@code @TestPropertySource}
 	 * attributes and declaring class.

spring-test/src/main/java/org/springframework/test/context/support/TestPropertySourceUtils.java

@@ -27,7 +27,6 @@
 import java.util.Map;
 import java.util.Properties;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -44,6 +43,7 @@
 import org.springframework.core.io.Resource;
 import org.springframework.core.io.ResourceLoader;
 import org.springframework.core.io.support.ResourcePropertySource;
+import org.springframework.lang.Nullable;
 import org.springframework.test.context.TestContextAnnotationUtils;
 import org.springframework.test.context.TestPropertySource;
 import org.springframework.test.context.util.TestContextResourceUtils;
@@ -76,18 +76,71 @@ public abstract class TestPropertySourceUtils {
 
 
 	static MergedTestPropertySources buildMergedTestPropertySources(Class<?> testClass) {
-		List<TestPropertySourceAttributes> attributesList =
-				findRepeatableAnnotations(testClass, TestPropertySource.class)
-					.map(TestPropertySourceAttributes::new)
-					.collect(Collectors.toList());
+		List<TestPropertySourceAttributes> attributesList = new ArrayList<>();
+
+		TestPropertySourceAttributes previousAttributes = null;
+		// Iterate over all aggregate levels, where each level is represented by
+		// a list of merged annotations found at that level (e.g., on a test
+		// class in the class hierarchy).
+		for (List<MergedAnnotation<TestPropertySource>> aggregatedAnnotations :
+				findRepeatableAnnotations(testClass, TestPropertySource.class)) {
+
+			// Convert all of the merged annotations for the current aggregate
+			// level to a list of TestPropertySourceAttributes.
+			List<TestPropertySourceAttributes> aggregatedAttributesList =
+					aggregatedAnnotations.stream().map(TestPropertySourceAttributes::new).collect(Collectors.toList());
+			// Merge all TestPropertySourceAttributes instances for the current
+			// aggregate level into a single TestPropertySourceAttributes instance.
+			TestPropertySourceAttributes mergedAttributes = mergeTestPropertySourceAttributes(aggregatedAttributesList);
+			if (mergedAttributes != null) {
+				if (!duplicationDetected(mergedAttributes, previousAttributes)) {
+					attributesList.add(mergedAttributes);
+				}
+				previousAttributes = mergedAttributes;
+			}
+		}
 
 		if (attributesList.isEmpty()) {
 			return MergedTestPropertySources.empty();
 		}
-
 		return new MergedTestPropertySources(mergeLocations(attributesList), mergeProperties(attributesList));
 	}
 
+	@Nullable
+	private static TestPropertySourceAttributes mergeTestPropertySourceAttributes(
+			List<TestPropertySourceAttributes> aggregatedAttributesList) {
+
+		TestPropertySourceAttributes mergedAttributes = null;
+		TestPropertySourceAttributes previousAttributes = null;
+		for (TestPropertySourceAttributes currentAttributes : aggregatedAttributesList) {
+			if (mergedAttributes == null) {
+				mergedAttributes = currentAttributes;
+			}
+			else if (!duplicationDetected(currentAttributes, previousAttributes)) {
+				mergedAttributes.mergeWith(currentAttributes);
+			}
+			previousAttributes = currentAttributes;
+		}
+
+		return mergedAttributes;
+	}
+
+	private static boolean duplicationDetected(TestPropertySourceAttributes currentAttributes,
+			TestPropertySourceAttributes previousAttributes) {
+
+		boolean duplicationDetected =
+				(currentAttributes.equals(previousAttributes) && !currentAttributes.isEmpty());
+
+		if (duplicationDetected && logger.isDebugEnabled()) {
+			logger.debug(String.format("Ignoring duplicate %s declaration on %s, "
+					+ "since it is also declared on %s.", currentAttributes,
+					previousAttributes.getDeclaringClass().getName(),
+					currentAttributes.getDeclaringClass().getName()));
+		}
+
+		return duplicationDetected;
+	}
+
 	private static String[] mergeLocations(List<TestPropertySourceAttributes> attributesList) {
 		List<String> locations = new ArrayList<>();
 		for (TestPropertySourceAttributes attrs : attributesList) {
@@ -273,12 +326,12 @@ public static Map<String, Object> convertInlinedPropertiesToMap(String... inline
 		return map;
 	}
 
-	private static <T extends Annotation> Stream<MergedAnnotation<T>> findRepeatableAnnotations(
+	private static <T extends Annotation> List<List<MergedAnnotation<T>>> findRepeatableAnnotations(
 			Class<?> clazz, Class<T> annotationType) {
 
 		List<List<MergedAnnotation<T>>> listOfLists = new ArrayList<>();
 		findRepeatableAnnotations(clazz, annotationType, listOfLists, new int[] {0});
-		return listOfLists.stream().flatMap(List::stream);
+		return listOfLists;
 	}
 
 	private static <T extends Annotation> void findRepeatableAnnotations(

spring-test/src/test/java/org/springframework/test/context/MergedContextConfigurationTests.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.
@@ -339,13 +339,13 @@ void equalsWithSameProfilesReversed() {
 				EMPTY_STRING_ARRAY, EMPTY_CLASS_ARRAY, activeProfiles1, loader);
 		MergedContextConfiguration mergedConfig2 = new MergedContextConfiguration(getClass(),
 				EMPTY_STRING_ARRAY, EMPTY_CLASS_ARRAY, activeProfiles2, loader);
-		assertThat(mergedConfig2).isNotEqualTo(mergedConfig1);
+		assertThat(mergedConfig2).isEqualTo(mergedConfig1);
 	}
 
 	@Test
 	void equalsWithSameDuplicateProfiles() {
 		String[] activeProfiles1 = new String[] { "catbert", "dogbert" };
-		String[] activeProfiles2 = new String[] { "catbert", "dogbert", "catbert", "dogbert", "catbert" };
+		String[] activeProfiles2 = new String[] { "dogbert", "catbert", "dogbert", "catbert" };
 		MergedContextConfiguration mergedConfig1 = new MergedContextConfiguration(getClass(),
 				EMPTY_STRING_ARRAY, EMPTY_CLASS_ARRAY, activeProfiles1, loader);
 		MergedContextConfiguration mergedConfig2 = new MergedContextConfiguration(getClass(),