From 0859318965cf2f6805d58cdcaac7f6a7ec08cca0 Mon Sep 17 00:00:00 2001 From: Roberto Cortez Date: Fri, 6 Dec 2024 16:38:16 +0000 Subject: [PATCH] Use config ordinal to determine which config style to use when populating list, between comma-separated and indexed (#1266) --- .../main/docs/config/indexed-properties.md | 5 +- .../io/smallrye/config/SmallRyeConfig.java | 122 ++++++++++++++---- .../smallrye/config/SmallRyeConfigTest.java | 120 +++++++++++++++-- .../source/yaml/YamlConfigMappingTest.java | 33 +++++ 4 files changed, 244 insertions(+), 36 deletions(-) diff --git a/documentation/src/main/docs/config/indexed-properties.md b/documentation/src/main/docs/config/indexed-properties.md index 51c0daaf0..70c20a995 100644 --- a/documentation/src/main/docs/config/indexed-properties.md +++ b/documentation/src/main/docs/config/indexed-properties.md @@ -20,8 +20,9 @@ The indexed property syntax uses the property name and square brackets with an i A call to `Config#getValues("my.collection", String.class)`, will automatically create and convert a `List` that contains the values `dog`, `cat` and `turtle`. A call to `Config#getValues("my.indexed.collection", String.class)` -returns the exact same result. If SmallRye Config finds the same property name in their indexed and unindexed format, -the indexed value has priority. +returns the exact same result. The indexed property format is prioritized when both styles are found in the same +configuration source. When available in multiple sources, the higher ordinal source wins, like any other configuration +lookup. The indexed property is sorted by its index before being added to the target `Collection`. Any gaps in the indexes do not resolve to the target `Collection`, which means that the `Collection` result will store all values without empty diff --git a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java index 1a4b2a56a..ab42c2358 100644 --- a/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java +++ b/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java @@ -188,7 +188,25 @@ public > C getValues(String name, Converter conver // Try legacy / MP comma separated values return getValue(name, newCollectionConverter(converter, collectionFactory)); } - return getIndexedValues(indexedProperties, converter, collectionFactory); + + // Check ordinality of indexed + int indexedOrdinality = Integer.MIN_VALUE; + C collection = collectionFactory.apply(indexedProperties.size()); + for (String indexedProperty : indexedProperties) { + ConfigValue indexed = getConfigValue(indexedProperty); + if (indexed.getConfigSourceOrdinal() >= indexedOrdinality) { + indexedOrdinality = indexed.getConfigSourceOrdinal(); + } + collection.add(convertValue(indexed, converter)); + } + + // Use indexed if comma separated empty or higher in ordinality + ConfigValue commaSeparated = getConfigValue(name); + if (commaSeparated == null || indexedOrdinality >= commaSeparated.getConfigSourceOrdinal()) { + return collection; + } else { + return getValue(name, newCollectionConverter(converter, collectionFactory)); + } } public > C getIndexedValues(String name, Converter converter, @@ -384,19 +402,32 @@ > Map getMapIndexedValues( @SuppressWarnings("unchecked") public T getValue(String name, Class propertyType) { if (propertyType.isArray()) { - ConfigValue configValue = getConfigValue(name); - if (configValue.getValue() != null) { + List indexedProperties = getIndexedProperties(name); + if (indexedProperties.isEmpty()) { + // Try legacy / MP comma separated values return getValue(name, requireConverter(propertyType)); } - List values = getValues(name, propertyType.getComponentType()); - Object array = Array.newInstance(propertyType.getComponentType(), values.size()); - for (int i = 0, valuesSize = values.size(); i < valuesSize; i++) { - Array.set(array, i, values.get(i)); + // Check ordinality of indexed + int indexedOrdinality = Integer.MIN_VALUE; + Object array = Array.newInstance(propertyType.getComponentType(), indexedProperties.size()); + for (int i = 0; i < indexedProperties.size(); i++) { + final String indexedProperty = indexedProperties.get(i); + ConfigValue indexed = getConfigValue(indexedProperty); + if (indexed.getConfigSourceOrdinal() >= indexedOrdinality) { + indexedOrdinality = indexed.getConfigSourceOrdinal(); + } + Array.set(array, i, convertValue(indexed, requireConverter(propertyType.getComponentType()))); } - return (T) array; - } + // Use indexed if comma separated empty or higher in ordinality + ConfigValue commaSeparated = getConfigValue(name); + if (commaSeparated == null || indexedOrdinality >= commaSeparated.getConfigSourceOrdinal()) { + return (T) array; + } else { + return convertValue(commaSeparated, requireConverter(propertyType)); + } + } return getValue(name, requireConverter(propertyType)); } @@ -502,8 +533,36 @@ public String getRawValue(String name) { } @Override - public Optional getOptionalValue(String name, Class aClass) { - return getValue(name, getOptionalConverter(aClass)); + @SuppressWarnings("unchecked") + public Optional getOptionalValue(String name, Class propertyType) { + if (propertyType.isArray()) { + List indexedProperties = getIndexedProperties(name); + if (indexedProperties.isEmpty()) { + // Try legacy / MP comma separated values + return getValue(name, getOptionalConverter(propertyType)); + } + + // Check ordinality of indexed + int indexedOrdinality = Integer.MIN_VALUE; + Object array = Array.newInstance(propertyType.getComponentType(), indexedProperties.size()); + for (int i = 0; i < indexedProperties.size(); i++) { + final String indexedProperty = indexedProperties.get(i); + ConfigValue indexed = getConfigValue(indexedProperty); + if (indexed.getConfigSourceOrdinal() >= indexedOrdinality) { + indexedOrdinality = indexed.getConfigSourceOrdinal(); + } + Array.set(array, i, convertValue(indexed, requireConverter(propertyType.getComponentType()))); + } + + // Use indexed if comma separated empty or higher in ordinality + ConfigValue commaSeparated = getConfigValue(name); + if (commaSeparated == null || indexedOrdinality >= commaSeparated.getConfigSourceOrdinal()) { + return (Optional) Optional.of(array); + } else { + return getValue(name, getOptionalConverter(propertyType)); + } + } + return getValue(name, getOptionalConverter(propertyType)); } public Optional getOptionalValue(String name, Converter converter) { @@ -521,11 +580,29 @@ public > Optional getOptionalValues(String name, C public > Optional getOptionalValues(String name, Converter converter, IntFunction collectionFactory) { - Optional optionalValue = getOptionalValue(name, newCollectionConverter(converter, collectionFactory)); - if (optionalValue.isPresent()) { - return optionalValue; + List indexedProperties = getIndexedProperties(name); + if (indexedProperties.isEmpty()) { + // Try legacy / MP comma separated values + return getOptionalValue(name, newCollectionConverter(converter, collectionFactory)); + } + + // Check ordinality of indexed + int indexedOrdinality = Integer.MIN_VALUE; + C collection = collectionFactory.apply(indexedProperties.size()); + for (String indexedProperty : indexedProperties) { + ConfigValue indexed = getConfigValue(indexedProperty); + if (indexed.getValue() != null && indexed.getConfigSourceOrdinal() >= indexedOrdinality) { + indexedOrdinality = indexed.getConfigSourceOrdinal(); + } + convertValue(indexed, newOptionalConverter(converter)).ifPresent(collection::add); + } + + // Use indexed if comma separated empty or higher in ordinality + ConfigValue commaSeparated = getConfigValue(name); + if (commaSeparated == null || indexedOrdinality >= commaSeparated.getConfigSourceOrdinal()) { + return collection.isEmpty() ? Optional.empty() : Optional.of(collection); } else { - return getIndexedOptionalValues(name, converter, collectionFactory); + return getOptionalValue(name, newCollectionConverter(converter, collectionFactory)); } } @@ -536,17 +613,13 @@ public > Optional getIndexedOptionalValues(String return Optional.empty(); } - final C collection = collectionFactory.apply(indexedProperties.size()); + C collection = collectionFactory.apply(indexedProperties.size()); for (String indexedProperty : indexedProperties) { - final Optional optionalValue = getOptionalValue(indexedProperty, converter); + Optional optionalValue = getOptionalValue(indexedProperty, converter); optionalValue.ifPresent(collection::add); } - if (!collection.isEmpty()) { - return Optional.of(collection); - } - - return Optional.empty(); + return collection.isEmpty() ? Optional.empty() : Optional.of(collection); } /** @@ -839,7 +912,7 @@ private static class ConfigSources implements Serializable { // Init all late sources List profiles = getProfiles(positiveInterceptors); List sourcesWithPriorities = mapLateSources(sources, negativeInterceptors, - positiveInterceptors, current, profiles, builder, config); + positiveInterceptors, current, profiles, builder); List configSources = getSources(sourcesWithPriorities); // Rebuild the chain with the late sources and new instances of the interceptors @@ -929,8 +1002,7 @@ private static List mapLateSources( final List positiveInterceptors, final ConfigSourceInterceptorContext current, final List profiles, - final SmallRyeConfigBuilder builder, - final SmallRyeConfig config) { + final SmallRyeConfigBuilder builder) { ConfigSourceWithPriority.resetLoadPriority(); List currentSources = new ArrayList<>(); diff --git a/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java b/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java index 4bea991d8..d8276696a 100644 --- a/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java +++ b/implementation/src/test/java/io/smallrye/config/SmallRyeConfigTest.java @@ -8,6 +8,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -28,6 +29,7 @@ import java.util.Properties; import java.util.Set; import java.util.TreeMap; +import java.util.function.Consumer; import java.util.function.IntFunction; import org.eclipse.microprofile.config.Config; @@ -212,11 +214,7 @@ void getOptionalValuesIndexed() { @Test void getOptionalValuesNotIndexed() { SmallRyeConfig config = new SmallRyeConfigBuilder() - .withSources(config( - "server.environments", "dev,qa", - "server.environments[0]", "dev", - "server.environments[1]", "qa", - "server.environments[2]", "prod")) + .withSources(config("server.environments", "dev,qa")) .build(); Optional> environments = config.getOptionalValues("server.environments", String.class); @@ -232,12 +230,18 @@ void getOptionalValuesEmpty() { .withSources(config("server.environments", "")) .build(); + assertFalse( + config.getIndexedOptionalValues("server.environments", config.requireConverter(String.class), ArrayList::new) + .isPresent()); assertFalse(config.getOptionalValues("server.environments", String.class).isPresent()); SmallRyeConfig configIndexed = new SmallRyeConfigBuilder() .withSources(config("server.environments[0]", "")) .build(); + assertFalse(configIndexed + .getIndexedOptionalValues("server.environments", config.requireConverter(String.class), ArrayList::new) + .isPresent()); assertFalse(configIndexed.getOptionalValues("server.environments", String.class).isPresent()); } @@ -415,7 +419,7 @@ void getValuesMap() { Converter stringConverter = config.requireConverter(String.class); Map treeMap = config.getValues("my.prop", stringConverter, stringConverter, t -> new TreeMap<>()); - assertTrue(treeMap instanceof TreeMap); + assertInstanceOf(TreeMap.class, treeMap); Optional> optionalMap = config.getOptionalValues("my.prop", String.class, String.class); assertTrue(optionalMap.isPresent()); @@ -428,7 +432,7 @@ void getValuesMap() { Optional> optionalTreeMap = config.getOptionalValues("my.prop", stringConverter, stringConverter, t -> new TreeMap<>()); assertTrue(optionalTreeMap.isPresent()); - assertTrue(optionalTreeMap.get() instanceof TreeMap); + assertInstanceOf(TreeMap.class, optionalTreeMap.get()); assertTrue(config.getOptionalValues("my.optional", String.class, String.class).isEmpty()); } @@ -477,7 +481,7 @@ void getValuesMapList() { Converter stringConverter = config.requireConverter(String.class); Map> treeMap = config.getValues("my.prop", stringConverter, stringConverter, t -> new TreeMap<>(), ArrayList::new); - assertTrue(treeMap instanceof TreeMap); + assertInstanceOf(TreeMap.class, treeMap); Optional>> optionalMap = config.getOptionalValues("my.prop", String.class, String.class, ArrayList::new); @@ -493,7 +497,7 @@ void getValuesMapList() { Optional>> optionalTreeMap = config.getOptionalValues("my.prop", stringConverter, stringConverter, t -> new TreeMap<>(), ArrayList::new); assertTrue(optionalTreeMap.isPresent()); - assertTrue(optionalTreeMap.get() instanceof TreeMap); + assertInstanceOf(TreeMap.class, optionalTreeMap.get()); assertTrue(config.getOptionalValues("my.optional", String.class, String.class, ArrayList::new).isEmpty()); } @@ -603,4 +607,102 @@ void propertiesSources(@TempDir Path tempDir) throws Exception { assertFalse(config.getConfigSources(EnvConfigSource.class).iterator().hasNext()); assertEquals("1234", config.getRawValue("my.prop")); } + + @Test + void overrideIndexed() { + SmallRyeConfig config = new SmallRyeConfigBuilder() + .withSources(config("list[0]", "one", "list[1]", "two")) + .build(); + + String[] listArray = config.getValue("list", String[].class); + assertEquals(2, listArray.length); + assertEquals("one", listArray[0]); + assertEquals("two", listArray[1]); + List listList = config.getValues("list", String.class); + assertEquals(2, listList.size()); + assertEquals("one", listList.get(0)); + assertEquals("two", listList.get(1)); + Optional listOptionalArray = config.getOptionalValue("list", String[].class); + assertTrue(listOptionalArray.isPresent()); + listOptionalArray.ifPresent(list -> { + assertEquals(2, list.length); + assertEquals("one", list[0]); + assertEquals("two", list[1]); + }); + Optional> listOptionalList = config.getOptionalValues("list", String.class); + assertTrue(listOptionalList.isPresent()); + listOptionalList.ifPresent(new Consumer>() { + @Override + public void accept(final List list) { + assertEquals(2, list.size()); + assertEquals("one", list.get(0)); + assertEquals("two", list.get(1)); + } + }); + + config = new SmallRyeConfigBuilder() + .withSources(config("list[0]", "one", "list[1]", "two")) + .withSources(new PropertiesConfigSource(Map.of("list", "three,four"), "", 1000)) + .build(); + + listArray = config.getValue("list", String[].class); + assertEquals(2, listArray.length); + assertEquals("three", listArray[0]); + assertEquals("four", listArray[1]); + listList = config.getValues("list", String.class); + assertEquals(2, listList.size()); + assertEquals("three", listList.get(0)); + assertEquals("four", listList.get(1)); + listOptionalArray = config.getOptionalValue("list", String[].class); + assertTrue(listOptionalArray.isPresent()); + listOptionalArray.ifPresent(list -> { + assertEquals(2, list.length); + assertEquals("three", list[0]); + assertEquals("four", list[1]); + }); + listOptionalList = config.getOptionalValues("list", String.class); + assertTrue(listOptionalList.isPresent()); + listOptionalList.ifPresent(new Consumer>() { + @Override + public void accept(final List list) { + assertEquals(2, list.size()); + assertEquals("three", list.get(0)); + assertEquals("four", list.get(1)); + } + }); + } + + @Test + void overrideCommaSeparated() { + SmallRyeConfig config = new SmallRyeConfigBuilder() + .withSources(config("list", "one,two")) + .withSources(new PropertiesConfigSource(Map.of("list[0]", "three", "list[1]", "four"), "", 1000)) + .build(); + + String[] listArray = config.getValue("list", String[].class); + assertEquals(2, listArray.length); + assertEquals("three", listArray[0]); + assertEquals("four", listArray[1]); + List listList = config.getValues("list", String.class); + assertEquals(2, listList.size()); + assertEquals("three", listList.get(0)); + assertEquals("four", listList.get(1)); + Optional listOptionalArray = config.getOptionalValue("list", String[].class); + assertTrue(listOptionalArray.isPresent()); + listOptionalArray.ifPresent(list -> { + assertEquals(2, list.length); + assertEquals("three", list[0]); + assertEquals("four", list[1]); + }); + Optional> listOptionalList = config.getOptionalValues("list", String.class); + assertTrue(listOptionalList.isPresent()); + listOptionalList.ifPresent(new Consumer>() { + @Override + public void accept(final List list) { + assertEquals(2, list.size()); + assertEquals("three", list.get(0)); + assertEquals("four", list.get(1)); + } + }); + } } diff --git a/sources/yaml/src/test/java/io/smallrye/config/source/yaml/YamlConfigMappingTest.java b/sources/yaml/src/test/java/io/smallrye/config/source/yaml/YamlConfigMappingTest.java index 261bfa2d9..497d26b68 100644 --- a/sources/yaml/src/test/java/io/smallrye/config/source/yaml/YamlConfigMappingTest.java +++ b/sources/yaml/src/test/java/io/smallrye/config/source/yaml/YamlConfigMappingTest.java @@ -1,5 +1,6 @@ package io.smallrye.config.source.yaml; +import static java.util.stream.Collectors.toSet; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -11,11 +12,14 @@ import java.util.Map; import java.util.Optional; import java.util.OptionalInt; +import java.util.Set; +import java.util.stream.StreamSupport; import org.eclipse.microprofile.config.spi.Converter; import org.junit.jupiter.api.Test; import io.smallrye.config.ConfigMapping; +import io.smallrye.config.PropertiesConfigSource; import io.smallrye.config.SmallRyeConfig; import io.smallrye.config.SmallRyeConfigBuilder; import io.smallrye.config.WithConverter; @@ -1084,4 +1088,33 @@ interface NestedMaps { @WithParentName Map> services(); } + + @Test + void overrideIndexedWithHigher() { + String yaml = "override:\n" + + " values:\n" + + " - one\n" + + " - two\n"; + + SmallRyeConfig config = new SmallRyeConfigBuilder() + .withMapping(OverrideIndexedWithHigher.class) + .withSources(new YamlConfigSource("yaml", yaml)) + .withSources(new PropertiesConfigSource(Map.of("override.values", "three,four"), "", 1000)) + .build(); + + Set properties = StreamSupport.stream(config.getPropertyNames().spliterator(), false).collect(toSet()); + assertTrue(properties.contains("override.values[0]")); + assertTrue(properties.contains("override.values[1]")); + assertTrue(properties.contains("override.values")); + + OverrideIndexedWithHigher mapping = config.getConfigMapping(OverrideIndexedWithHigher.class); + assertEquals(2, mapping.values().size()); + assertTrue(mapping.values().contains("three")); + assertTrue(mapping.values().contains("four")); + } + + @ConfigMapping(prefix = "override") + interface OverrideIndexedWithHigher { + List values(); + } }