From 841c556dec91631ad986d427a04583cfee4fcb8e Mon Sep 17 00:00:00 2001 From: Cedric Champeau Date: Fri, 19 Jul 2024 11:45:03 +0200 Subject: [PATCH] Fix test resources properties factory This commit fixes the `TestResourcesPropertiesFactory`, which incorrectly passed the `testResourcesConfig` map. Instead of removing the `test-resources.` prefix, like the client expects, it was passing the full property name which led to resolvers being unable to resolve properties. While this commit fixes the problem at the source, in the properties factory, a sanity cleanup has been introduced in the controller, in order to cleanup such entries in case they show up. This could be useful if a program uses the client directly but forgets to do the same (in the "normal" use case, the properties are passed "naturally" without the prefix because that's how the config API works). Fixes #663 --- .../TestResourcesPropertiesFactory.java | 2 +- .../FakeTestResourcesClient.java | 7 +++ .../server/TestResourcesController.java | 47 ++++++++++++++++--- 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/test-resources-extensions/test-resources-extensions-core/src/main/java/io/micronaut/test/extensions/testresources/TestResourcesPropertiesFactory.java b/test-resources-extensions/test-resources-extensions-core/src/main/java/io/micronaut/test/extensions/testresources/TestResourcesPropertiesFactory.java index 910288cad..71f6ac183 100644 --- a/test-resources-extensions/test-resources-extensions-core/src/main/java/io/micronaut/test/extensions/testresources/TestResourcesPropertiesFactory.java +++ b/test-resources-extensions/test-resources-extensions-core/src/main/java/io/micronaut/test/extensions/testresources/TestResourcesPropertiesFactory.java @@ -78,7 +78,7 @@ private Map extractAnnotationProperties(TestResourcesProperties var testResourcesConfig = properties.entrySet() .stream() .filter(e -> e.getKey().startsWith(TEST_RESOURCES_PROPERTY_PREFIX)) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + .collect(Collectors.toMap(e -> e.getKey().substring(TEST_RESOURCES_PROPERTY_PREFIX.length()), Map.Entry::getValue)); Map resolvedProperties = Stream.of(requestedProperties) .map(v -> new Object() { private final String key = v; diff --git a/test-resources-extensions/test-resources-extensions-core/src/testFixtures/java/io/micronaut/test/extensions/testresources/FakeTestResourcesClient.java b/test-resources-extensions/test-resources-extensions-core/src/testFixtures/java/io/micronaut/test/extensions/testresources/FakeTestResourcesClient.java index 0e17388b1..0a4c7a930 100644 --- a/test-resources-extensions/test-resources-extensions-core/src/testFixtures/java/io/micronaut/test/extensions/testresources/FakeTestResourcesClient.java +++ b/test-resources-extensions/test-resources-extensions-core/src/testFixtures/java/io/micronaut/test/extensions/testresources/FakeTestResourcesClient.java @@ -45,6 +45,13 @@ public Optional resolve(String name, Map properties, Map value += ": " + properties.get("required-property"); } } + testResourcesConfig. + keySet() + .forEach(k -> { + if (k.startsWith("test-resources.")) { + throw new AssertionError("Test resources properties must be passed without the prefix \"test-resources.\""); + } + }); return Optional.ofNullable(value); } diff --git a/test-resources-server/src/main/java/io/micronaut/testresources/server/TestResourcesController.java b/test-resources-server/src/main/java/io/micronaut/testresources/server/TestResourcesController.java index dc8dba178..f849cf451 100644 --- a/test-resources-server/src/main/java/io/micronaut/testresources/server/TestResourcesController.java +++ b/test-resources-server/src/main/java/io/micronaut/testresources/server/TestResourcesController.java @@ -38,6 +38,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; /** * The main test resources controller, which will answer requests performed by the @@ -49,6 +50,7 @@ public class TestResourcesController implements TestResourcesResolver { private static final Logger LOGGER = LoggerFactory.getLogger(TestResourcesController.class); private static final int MAX_STOP_TIMEOUT = 5000; + private static final String TEST_RESOURCES_PREFIX = "test-resources."; private final ResolverLoader loader; @@ -87,15 +89,16 @@ public List getResolvableProperties() { @Post("/list") public List getResolvableProperties(Map> propertyEntries, Map testResourcesConfig) { + var sanitizedTestResourcesConfig = sanitizeTestResourcesConfig(testResourcesConfig); return loader.getResolvers() .stream() - .filter(testResourcesResolver -> isEnabled(testResourcesResolver, testResourcesConfig)) - .map(r -> r.getResolvableProperties(propertyEntries, testResourcesConfig)) + .filter(testResourcesResolver -> isEnabled(testResourcesResolver, sanitizedTestResourcesConfig)) + .map(r -> r.getResolvableProperties(propertyEntries, sanitizedTestResourcesConfig)) .flatMap(Collection::stream) .distinct() .peek(p -> LOGGER.debug( "For configuration {} and property entries {} , resolvable property: {}", - testResourcesConfig, propertyEntries, p)) + sanitizedTestResourcesConfig, propertyEntries, p)) .toList(); } @@ -144,17 +147,18 @@ public List getRequiredPropertyEntries() { public Optional resolve(String name, Map properties, Map testResourcesConfig) { + var sanitizedTestResourcesConfig = sanitizeTestResourcesConfig(testResourcesConfig); Optional result = Optional.empty(); for (TestResourcesResolver resolver : loader.getResolvers()) { if (resolver instanceof ToggableTestResourcesResolver toggable && - !toggable.isEnabled(testResourcesConfig)) { + !toggable.isEnabled(sanitizedTestResourcesConfig)) { continue; } try { - result = resolver.resolve(name, properties, testResourcesConfig); + result = resolver.resolve(name, properties, sanitizedTestResourcesConfig); LOGGER.debug( "Attempt to resolve {} with resolver {}, properties {} and test resources configuration {} : {}", - name, resolver.getClass(), properties, testResourcesConfig, + name, resolver.getClass(), properties, sanitizedTestResourcesConfig, result.orElse("\uD83D\uDEAB")); } catch (Exception ex) { for (PropertyResolutionListener listener : propertyResolutionListeners) { @@ -165,7 +169,7 @@ public Optional resolve(String name, if (result.isPresent()) { for (PropertyResolutionListener listener : propertyResolutionListeners) { listener.resolved(name, result.get(), resolver, properties, - testResourcesConfig); + sanitizedTestResourcesConfig); } return result; } @@ -274,4 +278,33 @@ private static boolean isEnabled(TestResourcesResolver resolver, } return true; } + + /** + * This method is responsible of making sure that the test resources configuration + * map doesn't contain any entry which starts with `test-resources.` since the prefix + * is supposed to be removed. However, depending on how the client is used, or even + * if the service is called directly, it may be possible to pass in such configuration. + * This is therefore a "best effort" to cleanup the configuration in case this happens. + * + * @param testResourcesConfig the configuration map + * @return a cleanup map + */ + private static Map sanitizeTestResourcesConfig(Map testResourcesConfig) { + if (testResourcesConfig.keySet().stream().noneMatch(k -> k.startsWith(TEST_RESOURCES_PREFIX))) { + return testResourcesConfig; + } + return testResourcesConfig.entrySet() + .stream() + .collect(Collectors.toMap( + e -> { + var key = e.getKey(); + if (key.startsWith(TEST_RESOURCES_PREFIX)) { + LOGGER.warn("Test resources configuration key '{}' should be passed without the '{}' prefix", key, TEST_RESOURCES_PREFIX); + return key.substring(TEST_RESOURCES_PREFIX.length()); + } + return key; + }, + Map.Entry::getValue + )); + } }