diff --git a/archaius2-core/src/main/java/com/netflix/archaius/AbstractRegistryDecoder.java b/archaius2-core/src/main/java/com/netflix/archaius/AbstractRegistryDecoder.java index e674f44e..fb2b0319 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/AbstractRegistryDecoder.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/AbstractRegistryDecoder.java @@ -48,7 +48,10 @@ public T decode(Type type, String encoded) { } return converter.convert(encoded); } catch (Exception e) { - throw new ParseException("Error decoding type `" + type.getTypeName() + "`", e); + throw new ParseException("Unable to decode `" + + encoded + + "` as type `" + type.getTypeName() + "`: " + + e, e); } } @@ -73,9 +76,7 @@ private TypeConverter getOrCreateConverter(Type type) { } /** - * Iterate through all TypeConverter#Factory's and return the first TypeConverter that matches - * @param type - * @return + * Iterate through all TypeConverter#Factory's and return the first TypeConverter that matches the given type. */ private TypeConverter resolve(Type type) { return factories.stream() @@ -85,10 +86,8 @@ private TypeConverter resolve(Type type) { } /** - * @param type - * @param - * @return Return a converter that uses reflection on either a static valueOf or ctor(String) to convert a string value to the - * type. Will return null if neither is found + * Return a converter that uses reflection on either a static valueOf method or a ctor(String) + * to convert a string value to the requested type. Will return null if neither is found */ private static TypeConverter findValueOfTypeConverter(Type type) { if (!(type instanceof Class)) { @@ -98,19 +97,20 @@ private static TypeConverter findValueOfTypeConverter(Type type) { @SuppressWarnings("unchecked") Class cls = (Class) type; - // Next look a valueOf(String) static method + // Look for a valueOf(String) static method. The code *assumes* that such a method will return a T Method method; try { method = cls.getMethod("valueOf", String.class); return value -> { try { + //noinspection unchecked return (T) method.invoke(null, value); } catch (Exception e) { throw new ParseException("Error converting value '" + value + "' to '" + type.getTypeName() + "'", e); } }; } catch (NoSuchMethodException e1) { - // Next look for a T(String) constructor + // Next, look for a T(String) constructor Constructor c; try { c = cls.getConstructor(String.class); diff --git a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java index ba5c7371..674bb619 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java @@ -228,6 +228,7 @@ T newProxy(final Class type, final String initialPrefix, boolean immutabl final InvocationHandler handler = new ConfigProxyInvocationHandler<>(type, prefix, invokers, propertyNames); final T proxyObject = (T) Proxy.newProxyInstance(type.getClassLoader(), new Class[] { type }, handler); + List proxyingExceptions = new LinkedList<>(); // Iterate through all declared methods of the class looking for setter methods. // Each setter will be mapped to a Property for the property name: @@ -236,20 +237,37 @@ T newProxy(final Class type, final String initialPrefix, boolean immutabl if (Modifier.isStatic(method.getModifiers())) { continue; } - MethodInvokerHolder methodInvokerHolder = buildInvokerForMethod(type, prefix, method, proxyObject, immutable); - propertyNames.put(method, methodInvokerHolder.propertyName); + try { + MethodInvokerHolder methodInvokerHolder = buildInvokerForMethod(type, prefix, method, proxyObject, immutable); - if (immutable) { - // Cache the current value of the property and always return that. - // Note that this will fail for parameterized properties! - Object value = methodInvokerHolder.invoker.invoke(new Object[]{}); - invokers.put(method, (args) -> value); - } else { - invokers.put(method, methodInvokerHolder.invoker); + propertyNames.put(method, methodInvokerHolder.propertyName); + + if (immutable) { + // Cache the current value of the property and always return that. + // Note that this will fail for parameterized properties! + Object value = methodInvokerHolder.invoker.invoke(new Object[]{}); + invokers.put(method, (args) -> value); + } else { + invokers.put(method, methodInvokerHolder.invoker); + } + } catch (RuntimeException e) { + // Capture the exception and continue processing the other methods. We'll throw them all at the end. + proxyingExceptions.add(e); } } + if (!proxyingExceptions.isEmpty()) { + String errors = proxyingExceptions.stream() + .map(Throwable::getMessage) + .collect(Collectors.joining("\n\t")); + RuntimeException exception = new RuntimeException( + "Failed to create a configuration proxy for class " + type.getName() + + ":\n\t" + errors, proxyingExceptions.get(0)); + proxyingExceptions.subList(1, proxyingExceptions.size()).forEach(exception::addSuppressed); + throw exception; + } + return proxyObject; } @@ -310,7 +328,7 @@ private MethodInvokerHolder buildInvokerForMethod(Class type, String pref } else if (m.getParameterCount() > 0) { // A parameterized property. Note that this requires a @PropertyName annotation to extract the interpolation positions! if (nameAnnot == null) { - throw new IllegalArgumentException("Missing @PropertyName annotation on " + m.getDeclaringClass().getName() + "#" + m.getName()); + throw new IllegalArgumentException("Missing @PropertyName annotation on method with parameters " + m.getName()); } // A previous version allowed the full name to be specified, even if the prefix was specified. So, for @@ -322,6 +340,7 @@ private MethodInvokerHolder buildInvokerForMethod(Class type, String pref propertyNameTemplate = nameAnnot.name(); } + // TODO: Figure out a way to validate the template. It should have params in the form ${0}, ${1}, etc. propertyValueGetter = createParameterizedProperty(m.getGenericReturnType(), propertyNameTemplate, defaultValueSupplier); } else { @@ -330,8 +349,8 @@ private MethodInvokerHolder buildInvokerForMethod(Class type, String pref } return new MethodInvokerHolder(propertyValueGetter, propName); - } catch (Exception e) { - throw new RuntimeException("Error proxying method " + m.getName(), e); + } catch (RuntimeException e) { + throw new RuntimeException("Failed to create a proxy for method " + m.getName() + ": " + e, e); } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java b/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java index b00153d5..12495c1a 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java @@ -16,6 +16,7 @@ package com.netflix.archaius.config; import com.netflix.archaius.DefaultDecoder; +import com.netflix.archaius.api.ArchaiusType; import com.netflix.archaius.api.Config; import com.netflix.archaius.api.ConfigListener; import com.netflix.archaius.api.Decoder; @@ -28,8 +29,6 @@ import java.lang.reflect.Type; import java.math.BigDecimal; import java.math.BigInteger; -import java.util.ArrayList; -import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.NoSuchElementException; @@ -62,6 +61,7 @@ public AbstractConfig() { this(generateUniqueName("unnamed-")); } + @SuppressWarnings("unused") protected CopyOnWriteArrayList getListeners() { return listeners; } @@ -74,6 +74,7 @@ public String getListDelimiter() { return listDelimiter; } + @SuppressWarnings("unused") public void setListDelimiter(String delimiter) { listDelimiter = delimiter; } @@ -162,13 +163,18 @@ public String getString(String key) { /** * Handle notFound when a defaultValue is provided. - * @param defaultValue - * @return + * This implementation simply returns the defaultValue. Subclasses can override this method to provide + * alternative behavior. */ - protected T notFound(String key, T defaultValue) { + protected T notFound(@SuppressWarnings("unused") String key, T defaultValue) { return defaultValue; } - + + /** + * Handle notFound when no defaultValue is provided. + * This implementation throws a NoSuchElementException. Subclasses can override this method to provide + * alternative behavior. + */ protected T notFound(String key) { throw new NoSuchElementException("'" + key + "' not found"); } @@ -426,38 +432,35 @@ public Byte getByte(String key, Byte defaultValue) { @Override public List getList(String key, Class type) { - String value = getString(key); + Object value = getRawProperty(key); if (value == null) { return notFound(key); } - String[] parts = value.split(getListDelimiter()); - List result = new ArrayList<>(); - for (String part : parts) { - result.add(decoder.decode((Type) type, part)); - } - return result; + + // TODO: handle the case where value is a collection + return decoder.decode(ArchaiusType.forListOf(type), resolve(value.toString())); } + /** + * @inheritDoc + * This implementation always parses the underlying property as a comma-delimited string and returns + * a {@code List}. + */ @Override - @SuppressWarnings("rawtypes") // Required by legacy API - public List getList(String key) { - String value = getString(key); - if (value == null) { - return notFound(key); - } - String[] parts = value.split(getListDelimiter()); - return Arrays.asList(parts); + public List getList(String key) { + return getList(key, String.class); } @Override @SuppressWarnings("rawtypes") // Required by legacy API public List getList(String key, List defaultValue) { - String value = getString(key, null); + Object value = getRawProperty(key); if (value == null) { return notFound(key, defaultValue); } - String[] parts = value.split(","); - return Arrays.asList(parts); + + // TODO: handle the case where value is a collection + return decoder.decode(ArchaiusType.forListOf(String.class), resolve(value.toString())); } @Override diff --git a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java index 2e693bcc..0eb09c2c 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java @@ -1,6 +1,8 @@ package com.netflix.archaius; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -652,6 +654,7 @@ public void testNestedInterfaceWithCustomDecoder() { } @Configuration(prefix = "config") + @SuppressWarnings("unused") public interface ConfigWithStaticMethods { @PropertyName(name = "foo") @DefaultValue("foo-value") @@ -674,4 +677,33 @@ public void testInterfaceWithStaticMethods() { ConfigWithStaticMethods configWithStaticMethods = proxyFactory.newProxy(ConfigWithStaticMethods.class); assertEquals("foo-value-updated", configWithStaticMethods.foo()); } + + @Configuration(prefix = "config") + @SuppressWarnings("unused") + public interface ConfigWithBadSettings { + @DefaultValue("Q") // Q is, surprisingly, not an integer + int getInteger(); + + @DefaultValue("NOTINENUM") // NOTINENUM is not a valid enum element + TestEnum getEnum(); + + // A parametrized method requires a @PropertyName annotation + int getAnIntWithParam(String param); + } + + @Test + public void testInvalidInterface() { + SettableConfig config = new DefaultSettableConfig(); + PropertyFactory factory = DefaultPropertyFactory.from(config); + ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); + + RuntimeException e = assertThrows(RuntimeException.class, () -> proxy.newProxy(ConfigWithBadSettings.class)); + + assertThat(e.getMessage(), + allOf( + containsString("ConfigWithBadSettings"), + containsString("getInteger"), + containsString("getEnum"), + containsString("getAnIntWithParam"))); + } } diff --git a/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java b/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java index b69ce92d..593c00df 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java @@ -15,16 +15,24 @@ */ package com.netflix.archaius.config; +import java.net.URI; +import java.time.Duration; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.Map; import java.util.function.BiConsumer; +import com.netflix.archaius.exceptions.ParseException; import org.junit.jupiter.api.Test; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; public class AbstractConfigTest { @@ -38,6 +46,10 @@ public class AbstractConfigTest { entries.put("long", 42L); entries.put("float", 42.0f); entries.put("double", 42.0d); + entries.put("numberList", "1, 2,3 "); // The embedded spaces should not trip the numeric parsers. + entries.put("stringList", "a,b,c"); + entries.put("uriList", "http://example.com,http://example.org"); + entries.put("underlyingList", Arrays.asList("a", "b", "c")); } @Override @@ -82,6 +94,37 @@ public void getNonExistentProperty() { assertFalse(config.getProperty("non_existent").isPresent()); } + @Test + public void testGetLists() { + assertEquals(Arrays.asList(1, 2, 3), config.getList("numberList", Integer.class)); + assertEquals(Arrays.asList(1L, 2L, 3L), config.getList("numberList", Long.class)); + // Watch out for the trailing space in the original value in the config! + assertEquals(Arrays.asList("1", "2", "3 "), config.getList("numberList", String.class)); + assertEquals(Arrays.asList("a", "b", "c"), config.getList("stringList", String.class)); + assertEquals(Arrays.asList(URI.create("http://example.com"), URI.create("http://example.org")), config.getList("uriList", URI.class)); + + // Watch out for the trailing space in the list in the original value in the config! + assertEquals(Arrays.asList("1", "2", "3 "), config.getList("numberList")); + assertEquals(Arrays.asList("a", "b", "c"), config.getList("stringList")); + assertEquals(Arrays.asList("http://example.com", "http://example.org"), config.getList("uriList")); + + // TODO: Fix this! The current implementation returns the list ["[a", "b", "c]"] + //assertEquals(Arrays.asList("a", "b", "c"), config.getList("underlyingList")); + } + + @Test + public void testBadLists() { + ParseException pe1 = assertThrows(ParseException.class, () -> config.getList("numberList", Boolean.class)); + assertThat(pe1.getMessage(), allOf( + containsString(config.getString("numberList")), + containsString("java.util.List"))); + + ParseException pe2 = assertThrows(ParseException.class, () -> config.getList("stringList", Duration.class)); + assertThat(pe2.getMessage(), allOf( + containsString(config.getString("stringList")), + containsString("java.util.List"))); + } + @Test public void testGetRawNumerics() { // First, get each entry as its expected type and the corresponding wrapper.