From f49a5908f6cd60416bd4390186e0bf4fde8d37ff Mon Sep 17 00:00:00 2001 From: Roberto Cortez Date: Thu, 19 Dec 2024 16:23:42 +0000 Subject: [PATCH] Validate mappings on creation instead of on lookup (#1264) --- .../smallrye/config/ConfigMappingContext.java | 48 ++++++++++++------- .../config/ConfigValidationException.java | 4 ++ .../io/smallrye/config/SmallRyeConfig.java | 29 +++++------ .../io/smallrye/config/ObjectCreatorTest.java | 6 +-- .../config/validator/ValidateConfigTest.java | 48 +++++++------------ 5 files changed, 70 insertions(+), 65 deletions(-) diff --git a/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java b/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java index cb5bf5402..512fe078c 100644 --- a/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java +++ b/implementation/src/main/java/io/smallrye/config/ConfigMappingContext.java @@ -2,12 +2,12 @@ import static io.smallrye.config.ConfigValidationException.Problem; import static io.smallrye.config.ProfileConfigSourceInterceptor.activeName; -import static io.smallrye.config.PropertyName.name; import static io.smallrye.config.common.utils.StringUtil.unindexed; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.UndeclaredThrowableException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -35,7 +35,7 @@ */ public final class ConfigMappingContext { private final SmallRyeConfig config; - private final Map, Map> roots = new IdentityHashMap<>(); + private final Map, Map> mappings = new IdentityHashMap<>(); private final Map, Converter> converterInstances = new IdentityHashMap<>(); private NamingStrategy namingStrategy = NamingStrategy.KEBAB_CASE; @@ -52,17 +52,33 @@ public ConfigMappingContext( matchPropertiesWithEnv(mappings); for (Map.Entry, Set> mapping : mappings.entrySet()) { - Map mappingObjects = new HashMap<>(); - for (String rootPath : mapping.getValue()) { - applyRootPath(rootPath); - mappingObjects.put(rootPath, (ConfigMappingObject) constructRoot(mapping.getKey())); + Map mappingObjects = new HashMap<>(); + for (String prefix : mapping.getValue()) { + applyPrefix(prefix); + mappingObjects.put(prefix, constructMapping(mapping.getKey(), prefix)); } - this.roots.put(mapping.getKey(), mappingObjects); + this.mappings.put(mapping.getKey(), mappingObjects); } } - T constructRoot(Class interfaceType) { - return constructGroup(interfaceType); + @SuppressWarnings("unchecked") + T constructMapping(Class interfaceType, String prefix) { + int problemsCount = problems.size(); + Object mappingObject = constructGroup(interfaceType); + if (problemsCount != problems.size()) { + return (T) mappingObject; + } + try { + if (mappingObject instanceof ConfigMappingClassMapper) { + mappingObject = ((ConfigMappingClassMapper) mappingObject).map(); + config.getConfigValidator().validateMapping(mappingObject.getClass(), prefix, mappingObject); + } else { + config.getConfigValidator().validateMapping(interfaceType, prefix, mappingObject); + } + } catch (ConfigValidationException e) { + problems.addAll(Arrays.asList(e.getProblems())); + } + return (T) mappingObject; } public T constructGroup(Class interfaceType) { @@ -102,6 +118,10 @@ public Converter getConverterInstance(Class BEAN_STYLE_GETTERS = new Function() { @Override public String apply(final String name) { @@ -147,8 +163,8 @@ List getProblems() { return problems; } - Map, Map> getRootsMap() { - return roots; + Map, Map> getMappings() { + return mappings; } // TODO - We shouldn't be mutating the EnvSource. @@ -277,7 +293,7 @@ void reportUnknown(final Set ignoredPaths) { } Set prefixes = new HashSet<>(); - for (Map value : this.roots.values()) { + for (Map value : this.mappings.values()) { prefixes.addAll(value.keySet()); } if (prefixes.contains("")) { diff --git a/implementation/src/main/java/io/smallrye/config/ConfigValidationException.java b/implementation/src/main/java/io/smallrye/config/ConfigValidationException.java index 07c0a94b7..7215c8b45 100644 --- a/implementation/src/main/java/io/smallrye/config/ConfigValidationException.java +++ b/implementation/src/main/java/io/smallrye/config/ConfigValidationException.java @@ -36,6 +36,10 @@ private static String list(Problem[] problems) { return b.toString(); } + Problem[] getProblems() { + return problems; + } + public int getProblemCount() { return problems.length; } diff --git a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java index ab42c2358..f017be204 100644 --- a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java +++ b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java @@ -80,10 +80,10 @@ public class SmallRyeConfig implements Config, Serializable { private final Map>> optionalConverters = new ConcurrentHashMap<>(); private final ConfigValidator configValidator; - private final Map, Map> mappings; + private final Map, Map> mappings; SmallRyeConfig(SmallRyeConfigBuilder builder) { - this.configSources = new ConfigSources(builder, this); + this.configSources = new ConfigSources(builder); this.converters = buildConverters(builder); this.configValidator = builder.getValidator(); @@ -115,7 +115,7 @@ private Map> buildConverters(final SmallRyeConfigBuilder buil return converters; } - Map, Map> buildMappings(final SmallRyeConfigBuilder builder) + Map, Map> buildMappings(final SmallRyeConfigBuilder builder) throws ConfigValidationException { SmallRyeConfigBuilder.MappingBuilder mappingsBuilder = builder.getMappingsBuilder(); if (mappingsBuilder.getMappings().isEmpty()) { @@ -139,7 +139,7 @@ public ConfigMappingContext get() { throw new ConfigValidationException(problems.toArray(ConfigValidationException.Problem.NO_PROBLEMS)); } - return context.getRootsMap(); + return context.getMappings(); } private void matchPropertiesWithEnv() { @@ -678,7 +678,11 @@ public > Optional> getOptionalValues( return Optional.of(getMapIndexedValues(keys, keyConverter, valueConverter, mapFactory, collectionFactory)); } - Map, Map> getMappings() { + ConfigValidator getConfigValidator() { + return configValidator; + } + + Map, Map> getMappings() { return mappings; } @@ -699,24 +703,17 @@ public T getConfigMapping(Class type, String prefix) { return getConfigMapping(type); } - Map mappingsForType = mappings.get(getConfigMappingClass(type)); + Map mappingsForType = mappings.get(getConfigMappingClass(type)); if (mappingsForType == null) { throw ConfigMessages.msg.mappingNotFound(type.getName()); } - ConfigMappingObject configMappingObject = mappingsForType.get(prefix); + Object configMappingObject = mappingsForType.get(prefix); if (configMappingObject == null) { throw ConfigMessages.msg.mappingPrefixNotFound(type.getName(), prefix); } - Object value = configMappingObject; - if (configMappingObject instanceof ConfigMappingClassMapper) { - value = ((ConfigMappingClassMapper) configMappingObject).map(); - } - - configValidator.validateMapping(type, prefix, value); - - return type.cast(value); + return type.cast(configMappingObject); } /** @@ -874,7 +871,7 @@ private static class ConfigSources implements Serializable { * that this constructor must be used when the Config object is being initialized, because interceptors also * require initialization. */ - ConfigSources(final SmallRyeConfigBuilder builder, final SmallRyeConfig config) { + ConfigSources(final SmallRyeConfigBuilder builder) { // Add all sources except for ConfigurableConfigSource types. These are initialized later List sources = buildSources(builder); // Add the default values sources separately, so we can keep a reference to it and add mappings defaults diff --git a/implementation/src/test/java/io/smallrye/config/ObjectCreatorTest.java b/implementation/src/test/java/io/smallrye/config/ObjectCreatorTest.java index 95c2ed404..84554170e 100644 --- a/implementation/src/test/java/io/smallrye/config/ObjectCreatorTest.java +++ b/implementation/src/test/java/io/smallrye/config/ObjectCreatorTest.java @@ -336,7 +336,7 @@ void unnamedKeys() { .build(); ConfigMappingContext context = new ConfigMappingContext(config, new HashMap<>()); - context.applyRootPath("unnamed"); + context.applyPrefix("unnamed"); UnnamedKeys mapping = new UnnamedKeysImpl(context); assertEquals("unnamed", mapping.map().get(null).value()); @@ -394,7 +394,7 @@ void mapDefaults() { .build(); ConfigMappingContext context = new ConfigMappingContext(config, new HashMap<>()); - context.applyRootPath("map"); + context.applyPrefix("map"); MapDefaults mapping = new MapDefaultsImpl(context); assertEquals("value", mapping.defaults().get("one")); @@ -505,7 +505,7 @@ void namingStrategy() { .build(); ConfigMappingContext context = new ConfigMappingContext(config, new HashMap<>()); - context.applyRootPath("naming"); + context.applyPrefix("naming"); Naming naming = new NamingImpl(context); assertEquals("value", naming.nestedValue().value()); diff --git a/validator/src/test/java/io/smallrye/config/validator/ValidateConfigTest.java b/validator/src/test/java/io/smallrye/config/validator/ValidateConfigTest.java index a9955035b..627f5b196 100644 --- a/validator/src/test/java/io/smallrye/config/validator/ValidateConfigTest.java +++ b/validator/src/test/java/io/smallrye/config/validator/ValidateConfigTest.java @@ -67,7 +67,7 @@ static void restoreMessageLocale() { @Test void validateConfigMapping() { - SmallRyeConfig config = new SmallRyeConfigBuilder() + SmallRyeConfigBuilder builder = new SmallRyeConfigBuilder() .withValidator(new BeanValidationConfigValidatorImpl()) .withSources(config( "server.host", "localhost", @@ -98,11 +98,9 @@ void validateConfigMapping() { "server.info.admins.root[1].username", "admin", "server.info.firewall.accepted[0]", "127.0.0.1", "server.info.firewall.accepted[1]", "8.8.8")) - .withMapping(Server.class) - .build(); + .withMapping(Server.class); - ConfigValidationException validationException = assertThrows(ConfigValidationException.class, - () -> config.getConfigMapping(Server.class, "server")); + ConfigValidationException validationException = assertThrows(ConfigValidationException.class, builder::build); List validations = new ArrayList<>(); for (int i = 0; i < validationException.getProblemCount(); i++) { validations.add(validationException.getProblem(i).getMessage()); @@ -134,16 +132,14 @@ void validateConfigMapping() { @Test void validateNamingStrategy() { - SmallRyeConfig config = new SmallRyeConfigBuilder() + SmallRyeConfigBuilder builder = new SmallRyeConfigBuilder() .withValidator(new BeanValidationConfigValidatorImpl()) .withSources(config( "server.the_host", "localhost", "server.the_port", "8080")) - .withMapping(ServerNamingStrategy.class) - .build(); + .withMapping(ServerNamingStrategy.class); - ConfigValidationException validationException = assertThrows(ConfigValidationException.class, - () -> config.getConfigMapping(ServerNamingStrategy.class, "server")); + ConfigValidationException validationException = assertThrows(ConfigValidationException.class, builder::build); List validations = new ArrayList<>(); for (int i = 0; i < validationException.getProblemCount(); i++) { validations.add(validationException.getProblem(i).getMessage()); @@ -154,13 +150,11 @@ void validateNamingStrategy() { @Test void validateConfigProperties() { - SmallRyeConfig config = new SmallRyeConfigBuilder() + SmallRyeConfigBuilder builder = new SmallRyeConfigBuilder() .withValidator(new BeanValidationConfigValidatorImpl()) - .withMapping(Client.class) - .build(); + .withMapping(Client.class); - ConfigValidationException validationException = assertThrows(ConfigValidationException.class, - () -> config.getConfigMapping(Client.class)); + ConfigValidationException validationException = assertThrows(ConfigValidationException.class, builder::build); assertEquals(1, validationException.getProblemCount()); List validations = new ArrayList<>(); validations.add(validationException.getProblem(0).getMessage()); @@ -169,16 +163,14 @@ void validateConfigProperties() { @Test void validateParent() { - SmallRyeConfig config = new SmallRyeConfigBuilder() + SmallRyeConfigBuilder builder = new SmallRyeConfigBuilder() .withValidator(new BeanValidationConfigValidatorImpl()) .withSources(config( "server.host", "localhost", "server.port", "80")) - .withMapping(ServerParent.class) - .build(); + .withMapping(ServerParent.class); - ConfigValidationException validationException = assertThrows(ConfigValidationException.class, - () -> config.getConfigMapping(ServerParent.class, "server")); + ConfigValidationException validationException = assertThrows(ConfigValidationException.class, builder::build); assertEquals("server.port must be greater than or equal to 8000", validationException.getProblem(0).getMessage()); } @@ -359,14 +351,12 @@ interface Nested { @Test void hierarchy() { - SmallRyeConfig config = new SmallRyeConfigBuilder() + SmallRyeConfigBuilder builder = new SmallRyeConfigBuilder() .withValidator(new BeanValidationConfigValidatorImpl()) .withMapping(Child.class) - .withSources(config("validator.child.number", "1")) - .build(); + .withSources(config("validator.child.number", "1")); - ConfigValidationException validationException = assertThrows(ConfigValidationException.class, - () -> config.getConfigMapping(Child.class)); + ConfigValidationException validationException = assertThrows(ConfigValidationException.class, builder::build); List validations = new ArrayList<>(); for (int i = 0; i < validationException.getProblemCount(); i++) { validations.add(validationException.getProblem(i).getMessage()); @@ -387,13 +377,11 @@ public interface Child extends Parent { @Test void nestedMethodValidation() { - SmallRyeConfig config = new SmallRyeConfigBuilder() + SmallRyeConfigBuilder builder = new SmallRyeConfigBuilder() .withValidator(new BeanValidationConfigValidatorImpl()) - .withMapping(MethodValidation.class) - .build(); + .withMapping(MethodValidation.class); - ConfigValidationException validationException = assertThrows(ConfigValidationException.class, - () -> config.getConfigMapping(MethodValidation.class)); + ConfigValidationException validationException = assertThrows(ConfigValidationException.class, builder::build); List validations = new ArrayList<>(); for (int i = 0; i < validationException.getProblemCount(); i++) { validations.add(validationException.getProblem(i).getMessage());