Improve handling of error messages on decoding errors.

Ensure that all code paths that can throw a type error will log it properly and with as much information as possible.

archaius2-core/src/main/java/com/netflix/archaius/AbstractRegistryDecoder.java

@@ -48,7 +48,10 @@ public <T> 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 <T>
-     * @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 <code>valueOf</code> method or a <code>ctor(String)</code>
+     * to convert a string value to the requested type. Will return null if neither is found
      */
     private static <T> TypeConverter<T> findValueOfTypeConverter(Type type) {
         if (!(type instanceof Class)) {
@@ -98,19 +97,20 @@ private static <T> TypeConverter<T> findValueOfTypeConverter(Type type) {
         @SuppressWarnings("unchecked")
         Class<T> cls = (Class<T>) 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<T> c;
             try {
                 c = cls.getConstructor(String.class);

archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java

@@ -228,6 +228,7 @@ <T> T newProxy(final Class<T> 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<RuntimeException> proxyingExceptions = new LinkedList<>();
 
         // Iterate through all declared methods of the class looking for setter methods.
         // Each setter will be mapped to a Property<T> for the property name:
@@ -236,20 +237,37 @@ <T> T newProxy(final Class<T> 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 <T> MethodInvokerHolder buildInvokerForMethod(Class<T> 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 <T> MethodInvokerHolder buildInvokerForMethod(Class<T> 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 <T> MethodInvokerHolder buildInvokerForMethod(Class<T> 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);
         }
     }
 

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<ConfigListener> 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> T notFound(String key, T defaultValue) {
+    protected <T> 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> T notFound(String key) {
         throw new NoSuchElementException("'" + key + "' not found");
     }
@@ -426,38 +432,35 @@ public Byte getByte(String key, Byte defaultValue) {
 
     @Override
     public <T> List<T> getList(String key, Class<T> type) {
-        String value = getString(key);
+        Object value = getRawProperty(key);
         if (value == null) {
             return notFound(key);
         }
-        String[] parts = value.split(getListDelimiter());
-        List<T> 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<String>}.
+     */
     @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

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")));
+    }
 }

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<java.lang.Boolean>")));
+
+        ParseException pe2 = assertThrows(ParseException.class, () -> config.getList("stringList", Duration.class));
+        assertThat(pe2.getMessage(), allOf(
+                containsString(config.getString("stringList")),
+                containsString("java.util.List<java.time.Duration>")));
+    }
+
     @Test
     public void testGetRawNumerics() {
         // First, get each entry as its expected type and the corresponding wrapper.