From 664d4447630ec0635bd428823d9867af4a979dae Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Tue, 24 Sep 2024 17:31:38 -0700 Subject: [PATCH 1/3] 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. --- .../archaius/AbstractRegistryDecoder.java | 20 ++++---- .../netflix/archaius/ConfigProxyFactory.java | 43 +++++++++++----- .../archaius/config/AbstractConfig.java | 51 ++++++++++--------- .../netflix/archaius/ProxyFactoryTest.java | 32 ++++++++++++ .../archaius/config/AbstractConfigTest.java | 43 ++++++++++++++++ 5 files changed, 143 insertions(+), 46 deletions(-) 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. From 5e9da2003d979349d904d63b65e22ec017779df4 Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Tue, 24 Sep 2024 19:22:26 -0700 Subject: [PATCH 2/3] Fix ProxyTest --- .../com/netflix/archaius/guice/ProxyTest.java | 46 ++++++++----------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/archaius2-guice/src/test/java/com/netflix/archaius/guice/ProxyTest.java b/archaius2-guice/src/test/java/com/netflix/archaius/guice/ProxyTest.java index 5cfe267c..8fa04142 100644 --- a/archaius2-guice/src/test/java/com/netflix/archaius/guice/ProxyTest.java +++ b/archaius2-guice/src/test/java/com/netflix/archaius/guice/ProxyTest.java @@ -21,12 +21,14 @@ import org.junit.jupiter.api.Test; +import java.util.stream.Stream; + import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; public class ProxyTest { - public static interface MyConfig { + public interface MyConfig { @DefaultValue("0") int getInteger(); @@ -43,15 +45,7 @@ public interface MySubConfig { @DefaultValue("0") int getInteger(); } - - @Configuration(prefix="foo") - public interface MyConfigWithPrefix { - @DefaultValue("0") - int getInteger(); - - String getString(); - } - + @Test public void testConfigWithNoPrefix() throws ConfigException { Injector injector = Guice.createInjector( @@ -67,6 +61,7 @@ protected void configureArchaius() { @Provides @Singleton + @SuppressWarnings("unused") public MyConfig getMyConfig(ConfigProxyFactory factory) { return factory.newProxy(MyConfig.class); } @@ -98,6 +93,7 @@ protected void configureArchaius() { @Provides @Singleton + @SuppressWarnings("unused") public MyConfig getMyConfig(ConfigProxyFactory factory) { return factory.newProxy(MyConfig.class, "prefix"); } @@ -121,6 +117,7 @@ public void confirmConfigurationSourceWorksWithProxy() { new ArchaiusModule() { @Provides @Singleton + @SuppressWarnings("unused") public ModuleTestConfig getMyConfig(ConfigProxyFactory factory) { return factory.newProxy(ModuleTestConfig.class, "moduleTest"); } @@ -135,6 +132,7 @@ public ModuleTestConfig getMyConfig(ConfigProxyFactory factory) { public interface DefaultMethodWithAnnotation { @DefaultValue("fromAnnotation") + @SuppressWarnings("unused") default String getValue() { return "fromDefault"; } @@ -142,27 +140,23 @@ default String getValue() { @Test public void annotationAndDefaultImplementationNotAllowed() throws ConfigException { - try { - Injector injector = Guice.createInjector( + Injector injector = Guice.createInjector( new ArchaiusModule() { - @Override - protected void configureArchaius() { - } - @Provides @Singleton + @SuppressWarnings("unused") public DefaultMethodWithAnnotation getMyConfig(ConfigProxyFactory factory) { return factory.newProxy(DefaultMethodWithAnnotation.class); } }); - - injector.getInstance(DefaultMethodWithAnnotation.class); - fail("Exepcted ProvisionException"); - } catch (ProvisionException e) { - e.printStackTrace(); - assertEquals(IllegalArgumentException.class, e.getCause().getCause().getClass()); - } catch (Exception e) { - fail("Expected ProvisionException"); - } + + ProvisionException pe = assertThrows(ProvisionException.class, + () -> injector.getInstance(DefaultMethodWithAnnotation.class)); + + Stream.iterate((Throwable) pe, t -> t != null ? t.getCause() : null) + .limit(10) // avoid infinite loop + .filter(t -> t instanceof IllegalArgumentException) + .findFirst() + .orElseThrow(() -> new AssertionError("Expected an IllegalArgumentException in the cause chain", pe)); } } From 12509b4eccce367d7a924c1e79aa7fa798435d66 Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Tue, 24 Sep 2024 19:33:12 -0700 Subject: [PATCH 3/3] Log decoding issues in DefaultPropertyFactory as errors --- .../netflix/archaius/DefaultPropertyFactory.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java index 1c82c90d..cb86d91f 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java +++ b/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 Property asType(Function 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 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 listener) { Subscription subscription = oldSubscriptions.remove(listener); if (subscription != null) {