Unify annotation discovery for entry-based extensions (#460 / #485)

This PR removes the duplicated annotation discovery in
`EnvironmentVariableExtension` and `SystemPropertyExtension` and
moves it into `AbstractEntryBasedExtension`.

Closes: #460
PR: #485

README.md

@@ -111,7 +111,7 @@ The least we can do is to thank them and list some of their accomplishments here
 #### 2021
 
 * [Cory Thomas](https://github.com/dump247) contributed the `minSuccess` attribute in [retrying tests](https://junit-pioneer.org/docs/retrying-test/) (#408 / #430)
-* [Daniel Kraus](https://github.com/beatngu13) fixed bugs in the environment variable and system property extensions (#432 / #433, #448 / #449, and more) and improved the build process (#482 / #483)
+* [Daniel Kraus](https://github.com/beatngu13) fixed bugs in the environment variable and system property extensions (#432 / #433, #448 / #449, and more) before revamping its annotation handling (#460 / #485) and improved the build process (#482 / #483)
 * [Slawomir Jaranowski](https://github.com/slawekjaranowski) Migrate to new Shipkit plugins (#410 / #419)
 * [Stefano Cordio](https://github.com/scordio) contributed [the Cartesian Enum source](https://junit-pioneer.org/docs/cartesian-product/#cartesianenumsource) (#379 / #409 and #414 / #453)
 

src/main/java/org/junitpioneer/jupiter/AbstractEntryBasedExtension.java

@@ -10,11 +10,16 @@
 
 package org.junitpioneer.jupiter;
 
+import static java.util.stream.Collectors.toMap;
+
+import java.lang.annotation.Annotation;
+import java.lang.reflect.ParameterizedType;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Function;
 import java.util.stream.Stream;
 
 import org.junit.jupiter.api.extension.AfterAllCallback;
@@ -25,51 +30,21 @@
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
 import org.junit.jupiter.api.extension.ExtensionContext.Store;
+import org.junit.platform.commons.support.AnnotationSupport;
+import org.junitpioneer.internal.PioneerUtils;
 
 /**
  * An abstract base class for entry-based extensions, where entries (key-value
  * pairs) can be cleared or set.
  *
- * @param <K> The entry's key type.
- * @param <V> The entry's value type.
+ * @param <K> The entry key type.
+ * @param <V> The entry value type.
+ * @param <C> The clear annotation type.
+ * @param <S> The set annotation type.
  */
-abstract class AbstractEntryBasedExtension<K, V>
+abstract class AbstractEntryBasedExtension<K, V, C extends Annotation, S extends Annotation>
 		implements BeforeAllCallback, BeforeEachCallback, AfterAllCallback, AfterEachCallback {
 
-	/**
-	 * @param context The current extension context.
-	 * @return The entry keys to be cleared.
-	 */
-	protected abstract Set<K> entriesToClear(ExtensionContext context);
-
-	/**
-	 * @param context The current extension context.
-	 * @return The entry keys and values to be set.
-	 */
-	protected abstract Map<K, V> entriesToSet(ExtensionContext context);
-
-	/**
-	 * Removes the entry indicated by the specified key.
-	 */
-	protected abstract void clearEntry(K key);
-
-	/**
-	 * Gets the entry indicated by the specified key.
-	 */
-	protected abstract V getEntry(K key);
-
-	/**
-	 * Sets the entry indicated by the specified key.
-	 */
-	protected abstract void setEntry(K key, V value);
-
-	/**
-	 * Reports a warning about potentially unsafe practices.
-	 */
-	protected void reportWarning(ExtensionContext context) {
-		// nothing reported by default
-	}
-
 	@Override
 	public void beforeAll(ExtensionContext context) {
 		clearAndSetEntries(context);
@@ -85,8 +60,8 @@ private void clearAndSetEntries(ExtensionContext context) {
 		Map<K, V> entriesToSet;
 
 		try {
-			entriesToClear = entriesToClear(context);
-			entriesToSet = entriesToSet(context);
+			entriesToClear = findEntriesToClear(context);
+			entriesToSet = findEntriesToSet(context);
 			preventClearAndSetSameEntries(entriesToClear, entriesToSet.keySet());
 		}
 		catch (IllegalStateException ex) {
@@ -102,6 +77,41 @@ private void clearAndSetEntries(ExtensionContext context) {
 		setEntries(entriesToSet);
 	}
 
+	private Set<K> findEntriesToClear(ExtensionContext context) {
+		return findAnnotations(context, getClearAnnotationType())
+				.map(clearKeyMapper())
+				.collect(PioneerUtils.distinctToSet());
+	}
+
+	private Map<K, V> findEntriesToSet(ExtensionContext context) {
+		return findAnnotations(context, getSetAnnotationType()).collect(toMap(setKeyMapper(), setValueMapper()));
+	}
+
+	private <A extends Annotation> Stream<A> findAnnotations(ExtensionContext context, Class<A> clazz) {
+		/*
+		 * Implementation notes:
+		 *
+		 * This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class (i.e. a
+		 * class that the test class is @Nested within) uses this extension, this method will be called on those
+		 * extension points and discover the variables to set/clear. That means we don't need to search for
+		 * enclosing annotations here.
+		 */
+		return AnnotationSupport.findRepeatableAnnotations(context.getElement(), clazz).stream();
+	}
+
+	private Class<C> getClearAnnotationType() {
+		return getActualTypeArgumentAt(2);
+	}
+
+	private Class<S> getSetAnnotationType() {
+		return getActualTypeArgumentAt(3);
+	}
+
+	private <T> Class<T> getActualTypeArgumentAt(int index) {
+		ParameterizedType abstractEntryBasedExtensionType = (ParameterizedType) getClass().getGenericSuperclass();
+		return (Class<T>) abstractEntryBasedExtensionType.getActualTypeArguments()[index];
+	}
+
 	private void preventClearAndSetSameEntries(Collection<K> entriesToClear, Collection<K> entriesToSet) {
 		entriesToClear
 				.stream()
@@ -175,4 +185,41 @@ public void restoreBackup() {
 
 	}
 
+	/**
+	 * @return Mapper function to get the key from a clear annotation.
+	 */
+	protected abstract Function<C, K> clearKeyMapper();
+
+	/**
+	 * @return Mapper function to get the key from a set annotation.
+	 */
+	protected abstract Function<S, K> setKeyMapper();
+
+	/**
+	 * @return Mapper function to get the value from a set annotation.
+	 */
+	protected abstract Function<S, V> setValueMapper();
+
+	/**
+	 * Removes the entry indicated by the specified key.
+	 */
+	protected abstract void clearEntry(K key);
+
+	/**
+	 * Gets the entry indicated by the specified key.
+	 */
+	protected abstract V getEntry(K key);
+
+	/**
+	 * Sets the entry indicated by the specified key.
+	 */
+	protected abstract void setEntry(K key, V value);
+
+	/**
+	 * Reports a warning about potentially unsafe practices.
+	 */
+	protected void reportWarning(ExtensionContext context) {
+		// nothing reported by default
+	}
+
 }

src/main/java/org/junitpioneer/jupiter/EnvironmentVariableExtension.java

@@ -10,46 +10,32 @@
 
 package org.junitpioneer.jupiter;
 
-import static java.util.stream.Collectors.toMap;
-
-import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
 
 import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.platform.commons.support.AnnotationSupport;
-import org.junitpioneer.internal.PioneerUtils;
 
-class EnvironmentVariableExtension extends AbstractEntryBasedExtension<String, String> {
+class EnvironmentVariableExtension
+		extends AbstractEntryBasedExtension<String, String, ClearEnvironmentVariable, SetEnvironmentVariable> {
 
 	// package visible to make accessible for tests
 	static final AtomicBoolean REPORTED_WARNING = new AtomicBoolean(false);
 	static final String WARNING_KEY = EnvironmentVariableExtension.class.getSimpleName();
 	static final String WARNING_VALUE = "This extension uses reflection to mutate JDK-internal state, which is fragile. Check the Javadoc or documentation for more details.";
 
 	@Override
-	protected Set<String> entriesToClear(ExtensionContext context) {
-		return AnnotationSupport
-				// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
-				// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
-				// called on those extension points and discover the variables to set/clear. That means we don't need
-				// to search for enclosing annotations here.
-				.findRepeatableAnnotations(context.getElement(), ClearEnvironmentVariable.class)
-				.stream()
-				.map(ClearEnvironmentVariable::key)
-				.collect(PioneerUtils.distinctToSet());
+	protected Function<ClearEnvironmentVariable, String> clearKeyMapper() {
+		return ClearEnvironmentVariable::key;
+	}
+
+	@Override
+	protected Function<SetEnvironmentVariable, String> setKeyMapper() {
+		return SetEnvironmentVariable::key;
 	}
 
 	@Override
-	protected Map<String, String> entriesToSet(ExtensionContext context) {
-		return AnnotationSupport
-				// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
-				// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
-				// called on those extension points and discover the variables to set/clear. That means we don't need
-				// to search for enclosing annotations here.
-				.findRepeatableAnnotations(context.getElement(), SetEnvironmentVariable.class)
-				.stream()
-				.collect(toMap(SetEnvironmentVariable::key, SetEnvironmentVariable::value));
+	protected Function<SetEnvironmentVariable, String> setValueMapper() {
+		return SetEnvironmentVariable::value;
 	}
 
 	@Override

src/main/java/org/junitpioneer/jupiter/SystemPropertyExtension.java

@@ -10,40 +10,24 @@
 
 package org.junitpioneer.jupiter;
 
-import static java.util.stream.Collectors.toMap;
+import java.util.function.Function;
 
-import java.util.Map;
-import java.util.Set;
+class SystemPropertyExtension
+		extends AbstractEntryBasedExtension<String, String, ClearSystemProperty, SetSystemProperty> {
 
-import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.platform.commons.support.AnnotationSupport;
-import org.junitpioneer.internal.PioneerUtils;
-
-class SystemPropertyExtension extends AbstractEntryBasedExtension<String, String> {
+	@Override
+	protected Function<ClearSystemProperty, String> clearKeyMapper() {
+		return ClearSystemProperty::key;
+	}
 
 	@Override
-	protected Set<String> entriesToClear(ExtensionContext context) {
-		return AnnotationSupport
-				// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
-				// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
-				// called on those extension points and discover the properties to set/clear. That means we don't need
-				// to search for enclosing annotations here.
-				.findRepeatableAnnotations(context.getElement(), ClearSystemProperty.class)
-				.stream()
-				.map(ClearSystemProperty::key)
-				.collect(PioneerUtils.distinctToSet());
+	protected Function<SetSystemProperty, String> setKeyMapper() {
+		return SetSystemProperty::key;
 	}
 
 	@Override
-	protected Map<String, String> entriesToSet(ExtensionContext context) {
-		return AnnotationSupport
-				// This extension implements `BeforeAllCallback` and `BeforeEachCallback` and so if an outer class
-				// (i.e. a class that the test class is @Nested within) uses this extension, this method will be
-				// called on those extension points and discover the properties to set/clear. That means we don't need
-				// to search for enclosing annotations here.
-				.findRepeatableAnnotations(context.getElement(), SetSystemProperty.class)
-				.stream()
-				.collect(toMap(SetSystemProperty::key, SetSystemProperty::value));
+	protected Function<SetSystemProperty, String> setValueMapper() {
+		return SetSystemProperty::value;
 	}
 
 	@Override