Merge pull request #734 from Netflix/type-error-handling

Improve handling of error messages on decoding errors.

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/DefaultPropertyFactory.java

@@ -28,8 +28,7 @@ public class DefaultPropertyFactory implements PropertyFactory, ConfigListener {
 
     /**
      * Create a Property factory that is attached to a specific config
-     * @param config
-     * @return
+     * @param config The source of configuration for this factory.
      */
     public static DefaultPropertyFactory from(final Config config) {
         return new DefaultPropertyFactory(config);
@@ -64,6 +63,8 @@ public DefaultPropertyFactory(Config config) {
     }
 
     @Override
+    @Deprecated
+    @SuppressWarnings("deprecation")
     public PropertyContainer getProperty(String propName) {
         return new PropertyContainer() {
             @Override
@@ -130,7 +131,7 @@ public <T> Property<T> asType(Function<String, T> mapper, String defaultValue) {
                         try {
                             return mapper.apply(value);
                         } catch (Exception e) {
-                            LOG.warn("Invalid value '{}' for property '{}'", propName, value);
+                            LOG.error("Invalid value '{}' for property '{}'. Will return the default instead.", propName, value);
                         }
                     }
 
@@ -216,7 +217,7 @@ public T get() {
                 try {
                     newValue = supplier.get();
                 } catch (Exception e) {
-                    LOG.warn("Unable to get current version of property '{}'", keyAndType.key, e);
+                    LOG.error("Unable to get current version of property '{}'", keyAndType.key, e);
                 }
 
                 if (cache.compareAndSet(currentValue, newValue, cacheVersion, latestVersion)) {
@@ -260,16 +261,18 @@ public synchronized void run() {
 
         @Deprecated
         @Override
+        @SuppressWarnings("deprecation")
         public void addListener(PropertyListener<T> listener) {
             oldSubscriptions.put(listener, onChange(listener));
         }
 
         /**
          * Remove a listener previously registered by calling addListener
-         * @param listener
+         * @param listener The listener to be removed
          */
         @Deprecated
         @Override
+        @SuppressWarnings("deprecation")
         public void removeListener(PropertyListener<T> listener) {
             Subscription subscription = oldSubscriptions.remove(listener);
             if (subscription != null) {

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