From ae2e2fafab39c0b93f0f0915eda6e35cfc41c29d Mon Sep 17 00:00:00 2001 From: Roberto Cortez Date: Tue, 12 Sep 2023 08:30:35 +0100 Subject: [PATCH] Improve EnvConfigSource - Reduce allocations - Reduce lookups --- .../config/common/utils/StringUtil.java | 10 +- .../config/common/utils/StringUtilTest.java | 27 +++++ .../io/smallrye/config/EnvConfigSource.java | 113 +++++++++++++++--- .../PropertyNamesConfigSourceInterceptor.java | 68 ----------- .../io/smallrye/config/SmallRyeConfig.java | 3 +- .../smallrye/config/EnvConfigSourceTest.java | 4 +- .../smallrye/config/SmallRyeConfigTest.java | 2 +- 7 files changed, 133 insertions(+), 94 deletions(-) diff --git a/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java b/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java index e501593a4..d19909cba 100644 --- a/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java +++ b/common/src/main/java/io/smallrye/config/common/utils/StringUtil.java @@ -139,8 +139,11 @@ public static boolean equalsIgnoreCaseReplacingNonAlphanumericByUnderscores(fina } public static String replaceNonAlphanumericByUnderscores(final String name) { + return replaceNonAlphanumericByUnderscores(name, new StringBuilder(name.length())); + } + + public static String replaceNonAlphanumericByUnderscores(final String name, final StringBuilder sb) { int length = name.length(); - StringBuilder sb = new StringBuilder(length); for (int i = 0; i < length; i++) { char c = name.charAt(i); if (isAsciiLetterOrDigit(c)) { @@ -156,10 +159,13 @@ public static String replaceNonAlphanumericByUnderscores(final String name) { } public static String toLowerCaseAndDotted(final String name) { + return toLowerCaseAndDotted(name, new StringBuilder(name.length())); + } + + public static String toLowerCaseAndDotted(final String name, final StringBuilder sb) { int length = name.length(); int beginSegment = 0; boolean quotesOpen = false; - StringBuilder sb = new StringBuilder(); for (int i = 0; i < length; i++) { char c = name.charAt(i); if ('_' == c) { diff --git a/common/src/test/java/io/smallrye/config/common/utils/StringUtilTest.java b/common/src/test/java/io/smallrye/config/common/utils/StringUtilTest.java index f51bde3df..05b7371dc 100644 --- a/common/src/test/java/io/smallrye/config/common/utils/StringUtilTest.java +++ b/common/src/test/java/io/smallrye/config/common/utils/StringUtilTest.java @@ -120,11 +120,38 @@ void replaceNonAlphanumericByUnderscores() { StringUtil.replaceNonAlphanumericByUnderscores("test.language.\"de.etr\"".toUpperCase())); } + @Test + void replaceNonAlphanumericByUnderscoresWithStringBuilder() { + StringBuilder builder = new StringBuilder(); + builder.setLength(0); + assertEquals("FOO_BAR", StringUtil.replaceNonAlphanumericByUnderscores("foo.bar", builder).toUpperCase()); + builder.setLength(0); + assertEquals("FOO_BAR_BAZ", StringUtil.replaceNonAlphanumericByUnderscores("foo.bar.baz", builder).toUpperCase()); + builder.setLength(0); + assertEquals("FOO", StringUtil.replaceNonAlphanumericByUnderscores("foo", builder).toUpperCase()); + builder.setLength(0); + assertEquals("TEST_LANGUAGE__DE_ETR__", + StringUtil.replaceNonAlphanumericByUnderscores("test.language.\"de.etr\"".toUpperCase())); + } + @Test void toLowerCaseAndDotted() { assertEquals("test.language.\"de.etr\"", StringUtil.toLowerCaseAndDotted("TEST_LANGUAGE__DE_ETR__")); } + @Test + void toLowerCaseAndDottedWithStringBuilder() { + StringBuilder builder = new StringBuilder(); + builder.setLength(0); + assertEquals("foo.bar", StringUtil.toLowerCaseAndDotted("FOO_BAR", builder)); + builder.setLength(0); + assertEquals("foo.bar.baz", StringUtil.toLowerCaseAndDotted("FOO_BAR_BAZ", builder)); + builder.setLength(0); + assertEquals("foo", StringUtil.toLowerCaseAndDotted("FOO", builder)); + builder.setLength(0); + assertEquals("test.language.\"de.etr\"", StringUtil.toLowerCaseAndDotted("TEST_LANGUAGE__DE_ETR__")); + } + @Test void isNumeric() { assertTrue(StringUtil.isNumeric("0")); diff --git a/implementation/src/main/java/io/smallrye/config/EnvConfigSource.java b/implementation/src/main/java/io/smallrye/config/EnvConfigSource.java index 715bc2bb7..f7be80e35 100644 --- a/implementation/src/main/java/io/smallrye/config/EnvConfigSource.java +++ b/implementation/src/main/java/io/smallrye/config/EnvConfigSource.java @@ -18,23 +18,68 @@ import static io.smallrye.config.common.utils.ConfigSourceUtil.CONFIG_ORDINAL_KEY; import static io.smallrye.config.common.utils.StringUtil.replaceNonAlphanumericByUnderscores; import static java.security.AccessController.doPrivileged; -import static java.util.Collections.unmodifiableMap; +import static java.util.Collections.emptySet; import java.io.Serializable; import java.security.PrivilegedAction; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; -import io.smallrye.config.common.MapBackedConfigSource; +import io.smallrye.config.common.AbstractConfigSource; +import io.smallrye.config.common.utils.StringUtil; /** - * @author Jeff Mesnil (c) 2017 Red Hat inc. + * A {@link org.eclipse.microprofile.config.spi.ConfigSource} to access Environment Variables following the mapping + * rules defined by the MicroProfile Config specification. + *

+ * + * For a given property name foo.bar.baz, is matched to an environment variable with the following rules: + * + *

    + *
  1. Exact match (foo.bar.baz)
    2. + *
  2. Replace each character that is neither alphanumeric nor _ with _ + * (foo_bar_baz)
    3. + *
  3. Replace each character that is neither alphanumeric nor _ with _; then convert the name to + * upper case (FOO_BAR_BAZ)
    4. + *
+ *

+ * + * Additionally, this implementation provides candidate matching dotted property name from the Environment + * Variable name. These are required when a consumer relies on the list of properties to find additional + * configurations. The MicroProfile Config specification defines a set of conversion rules to look up and find + * values from environment variables even when using their dotted version, but it is unclear about property names. + *
+ * Because an environment variable name may only be represented by a subset of characters, it is not possible + * to represent exactly a dotted version name from an environment variable name, so consumers must be aware of such + * limitations. + *

+ * + * Due to its high ordinality (300), the {@link EnvConfigSource} may be queried on every property name + * lookup, just to not find a value and proceed to the next source. The conversion rules make such lookups inefficient + * and unnecessary. In order to reduce lookups, this implementation provides the following mechanisms: + *

+ * Keeps three forms of the environment variables names (original, lowercased and uppercased), to avoid having to + * transform the property name on each lookup. + *
+ * A dotted property name lookup is first matched to the existing names to avoid mapping and replacing rules in the + * name. Property names with other special characters always require replacing rules: + * + *

    + *
  1. A lookup to foo.bar requires FOO_BAR converted to the dotted name foo.bar
    2. + *
  2. A lookup to foo-bar requires FOO_BAR no match, mapping applied
    3. + *
  3. A lookup to foo.baz no match
    4. + *
*/ -public class EnvConfigSource extends MapBackedConfigSource { +public class EnvConfigSource extends AbstractConfigSource { private static final long serialVersionUID = -4525015934376795496L; private static final int DEFAULT_ORDINAL = 300; + private final Map properties; + private final Set names; + protected EnvConfigSource() { this(DEFAULT_ORDINAL); } @@ -43,35 +88,52 @@ protected EnvConfigSource(final int ordinal) { this(getEnvProperties(), ordinal); } - public EnvConfigSource(final Map propertyMap, final int ordinal) { - super("EnvConfigSource", propertyMap, getEnvOrdinal(propertyMap, ordinal)); + public EnvConfigSource(final Map properties, final int ordinal) { + super("EnvConfigSource", getEnvOrdinal(properties, ordinal)); + this.properties = new HashMap<>(properties.size() * 3); + this.names = new HashSet<>(properties.size() * 2); + StringBuilder builder = new StringBuilder(); + for (Map.Entry entry : properties.entrySet()) { + this.properties.put(entry.getKey(), entry.getValue()); + this.properties.put(entry.getKey().toLowerCase(), entry.getValue()); + this.properties.put(entry.getKey().toUpperCase(), entry.getValue()); + this.names.add(entry.getKey()); + this.names.add(StringUtil.toLowerCaseAndDotted(entry.getKey(), builder)); + builder.setLength(0); + } + } + + @Override + public Map getProperties() { + return this.properties; + } + + @Override + public Set getPropertyNames() { + return this.names; } @Override public String getValue(final String propertyName) { - return getValue(propertyName, getProperties()); + return getValue(propertyName, getProperties(), names); } - private static String getValue(final String name, final Map properties) { - if (name == null) { + private static String getValue(final String propertyName, final Map properties, final Set names) { + if (propertyName == null) { return null; } // exact match - String value = properties.get(name); + String value = properties.get(propertyName); if (value != null) { return value; } - // replace non-alphanumeric characters by underscores - String sanitizedName = replaceNonAlphanumericByUnderscores(name); - value = properties.get(sanitizedName); - if (value != null) { - return value; + if (isDottedFormat(propertyName) && !names.contains(propertyName)) { + return null; } - // replace non-alphanumeric characters by underscores and convert to uppercase - return properties.get(sanitizedName.toUpperCase()); + return properties.get(replaceNonAlphanumericByUnderscores(propertyName)); } /** @@ -80,17 +142,30 @@ private static String getValue(final String name, final Map prop * instantiated in the heap. */ private static Map getEnvProperties() { - return unmodifiableMap(doPrivileged((PrivilegedAction>) () -> new HashMap<>(System.getenv()))); + return doPrivileged((PrivilegedAction>) () -> new HashMap<>(System.getenv())); } private static int getEnvOrdinal(final Map properties, final int ordinal) { - final String value = getValue(CONFIG_ORDINAL_KEY, properties); + String value = getValue(CONFIG_ORDINAL_KEY, properties, emptySet()); + if (value == null) { + value = getValue(CONFIG_ORDINAL_KEY.toUpperCase(), properties, emptySet()); + } if (value != null) { return Converters.INTEGER_CONVERTER.convert(value); } return ordinal; } + private static boolean isDottedFormat(final String propertyName) { + for (int i = 0; i < propertyName.length(); i++) { + char c = propertyName.charAt(i); + if (!('a' <= c && c <= 'z') && !('0' <= c && c <= '9') && c != '.') { + return false; + } + } + return true; + } + Object writeReplace() { return new Ser(); } diff --git a/implementation/src/main/java/io/smallrye/config/PropertyNamesConfigSourceInterceptor.java b/implementation/src/main/java/io/smallrye/config/PropertyNamesConfigSourceInterceptor.java index 7eab09fc5..3e58d24c2 100644 --- a/implementation/src/main/java/io/smallrye/config/PropertyNamesConfigSourceInterceptor.java +++ b/implementation/src/main/java/io/smallrye/config/PropertyNamesConfigSourceInterceptor.java @@ -1,15 +1,9 @@ package io.smallrye.config; -import static io.smallrye.config.common.utils.StringUtil.replaceNonAlphanumericByUnderscores; -import static io.smallrye.config.common.utils.StringUtil.toLowerCaseAndDotted; - import java.util.HashSet; import java.util.Iterator; -import java.util.List; import java.util.Set; -import org.eclipse.microprofile.config.spi.ConfigSource; - /** * This interceptor adds additional entries to {@link org.eclipse.microprofile.config.Config#getPropertyNames}. */ @@ -18,11 +12,6 @@ class PropertyNamesConfigSourceInterceptor implements ConfigSourceInterceptor { private final Set properties = new HashSet<>(); - public PropertyNamesConfigSourceInterceptor(final ConfigSourceInterceptorContext context, - final List sources) { - this.properties.addAll(generateDottedProperties(context, sources)); - } - @Override public ConfigValue getValue(final ConfigSourceInterceptorContext context, final String name) { return context.proceed(name); @@ -42,61 +31,4 @@ public Iterator iterateNames(final ConfigSourceInterceptorContext contex void addProperties(final Set properties) { this.properties.addAll(properties); } - - /** - * Generate dotted properties from Env properties. - *
- * These are required when a consumer relies on the list of properties to find additional - * configurations. The list of properties is not normalized due to environment variables, which follow specific - * naming rules. The MicroProfile Config specification defines a set of conversion rules to look up and find - * values from environment variables even when using their dotted version, but this does not apply to the - * properties list. - *
- * Because an environment variable name may only be represented by a subset of characters, it is not possible - * to represent exactly a dotted version name from an environment variable name. Additional dotted properties - * mapped from environment variables are only added if a relationship cannot be found between all properties - * using the conversions look up rules of the MicroProfile Config specification. Example: - *
- * If foo.bar is present and FOO_BAR is also present, no property is required. - * If foo-bar is present and FOO_BAR is also present, no property is required. - * If FOO_BAR is present a property foo.bar is required. - */ - private static Set generateDottedProperties(final ConfigSourceInterceptorContext current, - final List sources) { - // Collect all known properties - Set properties = new HashSet<>(); - Iterator iterateNames = current.iterateNames(); - while (iterateNames.hasNext()) { - properties.add(iterateNames.next()); - } - - // Collect only properties from the EnvSources - Set envProperties = new HashSet<>(); - for (ConfigSource source : sources) { - if (source instanceof EnvConfigSource) { - envProperties.addAll(source.getPropertyNames()); - } - } - properties.removeAll(envProperties); - - // Collect properties that have the same semantic meaning - Set overrides = new HashSet<>(); - for (String property : properties) { - String semanticProperty = replaceNonAlphanumericByUnderscores(property); - for (String envProperty : envProperties) { - if (envProperty.equalsIgnoreCase(semanticProperty)) { - overrides.add(envProperty); - break; - } - } - } - - // Remove them - Remaining properties can only be found in the EnvSource - generate a dotted version - envProperties.removeAll(overrides); - Set dottedProperties = new HashSet<>(); - for (String envProperty : envProperties) { - dottedProperties.add(toLowerCaseAndDotted(envProperty)); - } - return dottedProperties; - } } diff --git a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java index b2f0a1023..a04234b25 100644 --- a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java +++ b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java @@ -566,8 +566,7 @@ private static class ConfigSources implements Serializable { // the resolved final source or interceptor to use. current = new SmallRyeConfigSourceInterceptorContext(EMPTY, null); current = new SmallRyeConfigSourceInterceptorContext(new SmallRyeConfigSources(sourcesWithPriorities), current); - PropertyNamesConfigSourceInterceptor propertyNamesInterceptor = new PropertyNamesConfigSourceInterceptor(current, - configSources); + PropertyNamesConfigSourceInterceptor propertyNamesInterceptor = new PropertyNamesConfigSourceInterceptor(); current = new SmallRyeConfigSourceInterceptorContext(propertyNamesInterceptor, current); for (ConfigSourceInterceptor interceptor : interceptors) { current = new SmallRyeConfigSourceInterceptorContext(interceptor, current); diff --git a/implementation/src/test/java/io/smallrye/config/EnvConfigSourceTest.java b/implementation/src/test/java/io/smallrye/config/EnvConfigSourceTest.java index 4bfc73331..6810d79e2 100644 --- a/implementation/src/test/java/io/smallrye/config/EnvConfigSourceTest.java +++ b/implementation/src/test/java/io/smallrye/config/EnvConfigSourceTest.java @@ -54,7 +54,7 @@ void conversionOfEnvVariableNames() { assertFalse(cs.getPropertyNames().contains("smallrye_mp_config_prop")); assertEquals(envProp, cs.getValue("smallrye.mp.config.prop")); - assertFalse(cs.getPropertyNames().contains("smallrye.mp.config.prop")); + assertTrue(cs.getPropertyNames().contains("smallrye.mp.config.prop")); assertEquals(envProp, cs.getValue("SMALLRYE.MP.CONFIG.PROP")); assertFalse(cs.getPropertyNames().contains("SMALLRYE.MP.CONFIG.PROP")); @@ -100,7 +100,7 @@ void ordinal() { ConfigSource configSource = config.getConfigSources().iterator().next(); assertTrue(configSource instanceof EnvConfigSource); - assertEquals(configSource.getOrdinal(), 301); + assertEquals(301, configSource.getOrdinal()); } @Test diff --git a/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java b/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java index 9d9f4476d..7fb837910 100644 --- a/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java +++ b/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java @@ -313,7 +313,7 @@ void getPropertyNames() { assertEquals("1234", config.getRawValue("smallrye.mp-config.prop")); assertTrue(((Set) config.getPropertyNames()).contains("SMALLRYE_MP_CONFIG_PROP")); assertTrue(((Set) config.getPropertyNames()).contains("smallrye.mp-config.prop")); - assertFalse(((Set) config.getPropertyNames()).contains("smallrye.mp.config.prop")); + assertTrue(((Set) config.getPropertyNames()).contains("smallrye.mp.config.prop")); } @Test