From 05d51afe3d1dccf53c459591f568a70b470fb6dc Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Wed, 11 Oct 2023 11:56:12 -0700 Subject: [PATCH 1/2] Use dynamically-created lambdas to call default methods in config proxies This is a re-factor of the code reverted in PR #685, originally by @kilink --- archaius2-core/build.gradle | 2 +- .../netflix/archaius/ConfigProxyFactory.java | 111 +++----- .../archaius/DefaultMethodInvokerFactory.java | 238 ++++++++++++++++++ .../netflix/archaius/ProxyFactoryTest.java | 25 +- archaius2-persisted2/build.gradle | 2 +- archaius2-test/build.gradle | 2 +- 6 files changed, 289 insertions(+), 91 deletions(-) create mode 100644 archaius2-core/src/main/java/com/netflix/archaius/DefaultMethodInvokerFactory.java diff --git a/archaius2-core/build.gradle b/archaius2-core/build.gradle index a11fec65..ac656b06 100644 --- a/archaius2-core/build.gradle +++ b/archaius2-core/build.gradle @@ -19,7 +19,7 @@ apply plugin: 'java-library' dependencies { api project(':archaius2-api') implementation 'org.slf4j:slf4j-api:1.7.36' - implementation 'org.apache.commons:commons-lang3:3.3.2' + implementation 'org.apache.commons:commons-lang3:3.12.0' implementation 'commons-codec:commons-codec:1.16.0' testImplementation 'com.google.code.findbugs:jsr305:3.0.1' } 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 04516926..bb3171a3 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java @@ -10,16 +10,11 @@ import com.netflix.archaius.api.annotations.PropertyName; import com.netflix.archaius.util.Maps; import org.apache.commons.lang3.StringUtils; -import org.apache.commons.lang3.SystemUtils; import org.apache.commons.lang3.text.StrSubstitutor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.inject.Inject; -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; -import java.lang.reflect.Constructor; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Method; import java.lang.reflect.Proxy; @@ -244,33 +239,18 @@ else if ("toString".equals(method.getName())) { } final Class returnType = m.getReturnType(); - - Function defaultSupplier = knownCollections.getOrDefault(returnType, (ignored) -> null); + // The proper type for this would be Function but we can't express that in Java. + final Function defaultSupplier; if (m.getAnnotation(DefaultValue.class) != null) { - if (m.isDefault()) { - throw new IllegalArgumentException("@DefaultValue cannot be defined on a method with a default implementation for method " - + m.getDeclaringClass().getName() + "#" + m.getName()); - } else if ( - Map.class.isAssignableFrom(returnType) || - List.class.isAssignableFrom(returnType) || - Set.class.isAssignableFrom(returnType) ) { - throw new IllegalArgumentException("@DefaultValue cannot be used with collections. Use default method implemenation instead " - + m.getDeclaringClass().getName() + "#" + m.getName()); - } - - String value = m.getAnnotation(DefaultValue.class).value(); - if (returnType == String.class) { - defaultSupplier = memoize((T) config.resolve(value)); - } else { - defaultSupplier = memoize(decoder.decode(returnType, config.resolve(value))); - } - } - - if (m.isDefault()) { + defaultSupplier = createAnnotatedMethodSupplier(m, returnType, config, decoder); + } else if (m.isDefault()) { defaultSupplier = createDefaultMethodSupplier(m, type, proxyObject); + } else { + defaultSupplier = knownCollections.getOrDefault(returnType, (ignored) -> null); } + final PropertyName nameAnnot = m.getAnnotation(PropertyName.class); final String propName = nameAnnot != null && nameAnnot.name() != null ? prefix + nameAnnot.name() @@ -303,54 +283,33 @@ else if ("toString".equals(method.getName())) { return proxyObject; } + private static Function createAnnotatedMethodSupplier(Method m, Class returnType, Config config, Decoder decoder) { + if (m.isDefault()) { + throw new IllegalArgumentException("@DefaultValue cannot be defined on a method with a default implementation for method " + + m.getDeclaringClass().getName() + "#" + m.getName()); + } else if ( + Map.class.isAssignableFrom(returnType) || + List.class.isAssignableFrom(returnType) || + Set.class.isAssignableFrom(returnType) ) { + throw new IllegalArgumentException("@DefaultValue cannot be used with collections. Use default method implemenation instead " + + m.getDeclaringClass().getName() + "#" + m.getName()); + } + + String value = m.getAnnotation(DefaultValue.class).value(); + if (returnType == String.class) { + return memoize((T) config.resolve(value)); + } else { + return memoize(decoder.decode(returnType, config.resolve(value))); + } + } + private static Function memoize(T value) { return (ignored) -> value; } - private static Function createDefaultMethodSupplier(Method method, Class type, T proxyObject) { - final MethodHandle methodHandle; - - try { - if (SystemUtils.IS_JAVA_1_8) { - Constructor constructor = MethodHandles.Lookup.class - .getDeclaredConstructor(Class.class, int.class); - constructor.setAccessible(true); - methodHandle = constructor.newInstance(type, MethodHandles.Lookup.PRIVATE) - .unreflectSpecial(method, type) - .bindTo(proxyObject); - } - else { - // Java 9 onwards - methodHandle = MethodHandles.lookup() - .findSpecial(type, - method.getName(), - MethodType.methodType(method.getReturnType(), method.getParameterTypes()), - type) - .bindTo(proxyObject); - } - } catch (ReflectiveOperationException e) { - throw new RuntimeException("Failed to create temporary object for " + type.getName(), e); - } - - return (args) -> { - try { - if (methodHandle.type().parameterCount() == 0) { - //noinspection unchecked - return (T) methodHandle.invokeWithArguments(); - } else if (args != null) { - //noinspection unchecked - return (T) methodHandle.invokeWithArguments(args); - } else { - // This is a handle to a method WITH arguments, being called with none. This happens when toString() - // is trying to build a representation of a proxy that has a parametrized property AND the interface - // provides a default method for it. There's no good default to return here, so we'll just use null - return null; - } - } catch (Throwable e) { - maybeWrapThenRethrow(e); - return null; // Unreachable, but the compiler doesn't know - } - }; + // type param ? is the return type for the METHOD, but we can't express that in Java + private static Function createDefaultMethodSupplier(Method method, Class type, T proxyObject) { + return DefaultMethodInvokerFactory.getFactory().findOrCreateDefaultMethodInvoker(type, method, proxyObject); } protected MethodInvoker createInterfaceProperty(String propName, final T proxy) { @@ -411,14 +370,4 @@ R getPropertyWithDefault(Class type, String propName) { } }; } - - private static void maybeWrapThenRethrow(Throwable t) { - if (t instanceof RuntimeException) { - throw (RuntimeException) t; - } - if (t instanceof Error) { - throw (Error) t; - } - throw new RuntimeException(t); - } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DefaultMethodInvokerFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/DefaultMethodInvokerFactory.java new file mode 100644 index 00000000..9e53d740 --- /dev/null +++ b/archaius2-core/src/main/java/com/netflix/archaius/DefaultMethodInvokerFactory.java @@ -0,0 +1,238 @@ +package com.netflix.archaius; + +import org.apache.commons.lang3.JavaVersion; +import org.apache.commons.lang3.SystemUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.invoke.CallSite; +import java.lang.invoke.LambdaMetafactory; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiFunction; +import java.util.function.Function; + +abstract class DefaultMethodInvokerFactory { + private static final Logger LOG = LoggerFactory.getLogger(DefaultMethodInvokerFactory.class); + + private static final DefaultMethodInvokerFactory INSTANCE; + static { + if (SystemUtils.isJavaVersionAtMost(JavaVersion.JAVA_1_8)) { + INSTANCE = new LegacyJava8Factory(); + } else if (SystemUtils.isJavaVersionAtMost(JavaVersion.JAVA_16)) { + INSTANCE = new BoundMethodHandlesFactory(); + } else { // Java 17 onwards + INSTANCE = new LambdasFactory(); + } + LOG.info("Choosing {} as invoker factory for default methods", INSTANCE.getClass()); + } + + static DefaultMethodInvokerFactory getFactory() { + return INSTANCE; + } + + /** + * Returns a Function object that acts as a call to invokerBound.method(...). + *

+ * {@literal <}R> is expected to be the METHOD's return type. Unfortunately this constraint can't be enforced by the compiler. + * + * @param methodOwner An interface that declares (or inherits) the wanted method. + * @param method The method to be called. This is expected to be a default method in METHOD_OWNER. + * @param invokerBound An instance of the METHOD_OWNER interface. + */ + abstract Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound); + + /** + * Shared "fall back" implementation using reflection to invoke the method handle. + */ + Function bindMethodHandle(MethodHandle methodHandle, T invokerBound) { + // Exceptions from this call are NOT caught because we want to fail fast if the arguments to the factory are bad. + MethodHandle boundHandle = methodHandle.bindTo(invokerBound); + + return (args) -> { + try { + if (boundHandle.type().parameterCount() == 0) { + + //noinspection unchecked + return (R) boundHandle.invokeWithArguments(); + } else if (args != null) { + + //noinspection unchecked + return (R) boundHandle.invokeWithArguments(args); + } else { + // This is a handle to a method WITH arguments, being called with none. This happens when + // invokerBound.toString() is called on an object that has a parametrized property AND the interface + // provides a default method for it. There's no good default to return here, so we'll just use null + return null; + } + } catch (Throwable e) { + maybeWrapThenRethrow(e); + return null; // Unreachable, but the compiler doesn't know + } + }; + + } + + + /** + * For Java 17 onwards, we wrap the method calls in lambdas and return that. This skips the reflection machinery and + * provides a noticeable performance boost. This implementation is known to fail in java 9 - 11. It *should* work + * from 12 or 13 onwards, but we have only tested it in 17 and 21. + */ + private static class LambdasFactory extends DefaultMethodInvokerFactory { + // We keep a running count of how many times we've seen a given Class. If we cross the threshold we stop creating + // lambdas for that class and fall back to the reflective implementation. This is motivated by an edge case we + // saw in clients, where they create (and leak) large numbers of proxies to the same interface. Because each lambda + // object requires its own anon class object, which lives in metaspace, and since users sometimes size their + // metaspaces to be much smaller than the heap, the leak becomes much more visible and causes OOMs. + private static final Map SEEN_COUNTS = new ConcurrentHashMap<>(); + + // The threshold is simply a best guess. Creating 1000 lambdas for the same interface is almost certainly + // a leak, right? + private static final int MAX_SEEN_THRESHOLD = 1000; + + @Override + Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound) { + MethodHandles.Lookup lookup = MethodHandles.lookup(); + MethodHandle methodHandle; + + try { + methodHandle = lookup.findSpecial( + method.getDeclaringClass(), + method.getName(), + MethodType.methodType(method.getReturnType(), method.getParameterTypes()), + method.getDeclaringClass()); + } catch (NoSuchMethodException | IllegalAccessException e) { + throw new RuntimeException(e); + } + + if (methodHandle.type().parameterCount() <= 2 && + SEEN_COUNTS.getOrDefault(methodOwner.getName(), 0) <= MAX_SEEN_THRESHOLD) { + + // For 1 or 2 argument handles (which translate to 0 and 1 argument *methods*), build a lambda object + // and use that to make the call. This avoids the cost of the reflection machinery and has a significant + // performance boost. + // But first, count how many times we've been here for this owner + SEEN_COUNTS.merge(methodOwner.getName(), 1, (oldValue, ignore) -> oldValue + 1); + + if (methodHandle.type().parameterCount() == 1) { + Function getter = asFunction(lookup, methodHandle); + //noinspection unchecked,DataFlowIssue + return (args) -> (R) getter.apply(invokerBound); + + } else if (methodHandle.type().parameterCount() == 2) { + BiFunction getter = asBiFunction(lookup, methodHandle); + //noinspection unchecked,DataFlowIssue + return (args) -> + args == null ? null : (R) getter.apply(invokerBound, args[0]); + } + } + + // Otherwise, for handles with more than 2 arguments or if we've reached the threshold, fall back to + // reflection + return bindMethodHandle(methodHandle, invokerBound); + } + + /** + * For a given no-args method M, return a (possibly cached) Function equivalent to the lambda + * instance -> instance.M(), + * where instance is an object of an adequate type. + */ + @SuppressWarnings("unchecked") + private static Function asFunction(MethodHandles.Lookup lookup, MethodHandle methodHandle) { + try { + CallSite site = LambdaMetafactory.metafactory(lookup, + "apply", + MethodType.methodType(Function.class), + MethodType.methodType(Object.class, Object.class), + methodHandle, + methodHandle.type()); + return (Function) site.getTarget().invokeExact(); + } catch (Throwable t) { + maybeWrapThenRethrow(t); + return null; // Unreachable, but the compiler doesn't know. + } + } + + @SuppressWarnings("unchecked") + private static BiFunction asBiFunction(MethodHandles.Lookup lookup, MethodHandle methodHandle) { + try { + CallSite site = LambdaMetafactory.metafactory(lookup, + "apply", + MethodType.methodType(BiFunction.class), + MethodType.methodType(Object.class, Object.class, Object.class), + methodHandle, + methodHandle.type()); + return (BiFunction) site.getTarget().invokeExact(); + } catch (Throwable t) { + maybeWrapThenRethrow(t); + return null; // Unreachable, but the compiler doesn't know + } + } + } + + /** + * An implementation safe to use in Java 9 - 16. It looks up a method handle which it then + * binds directly to the requested instance. + */ + private static class BoundMethodHandlesFactory extends DefaultMethodInvokerFactory { + @Override + Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound) { + final MethodHandle methodHandle; + + try { + methodHandle = MethodHandles.lookup() + .findSpecial(methodOwner, + method.getName(), + MethodType.methodType(method.getReturnType(), method.getParameterTypes()), + methodOwner); + } catch (NoSuchMethodException | IllegalAccessException e) { + throw new RuntimeException("Failed to create temporary object for " + methodOwner.getName(), e); + } + + return bindMethodHandle(methodHandle, invokerBound); + } + } + + /** + * For Java 8 we use a hacky mechanism (which got disabled in Java 9) to look up a method handle which we then + * bind directly to the requested function object. + */ + private static class LegacyJava8Factory extends DefaultMethodInvokerFactory { + + @Override + Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound) { + final MethodHandle methodHandle; + + try { + Constructor constructor = MethodHandles.Lookup.class + .getDeclaredConstructor(Class.class, int.class); + constructor.setAccessible(true); + methodHandle = constructor.newInstance(methodOwner, MethodHandles.Lookup.PRIVATE) + .unreflectSpecial(method, methodOwner); + + } catch (NoSuchMethodException | InstantiationException | + IllegalAccessException | InvocationTargetException e) { + throw new RuntimeException("Failed to create temporary object for " + methodOwner.getName(), e); + } + + return bindMethodHandle(methodHandle, invokerBound); + } + } + + private static void maybeWrapThenRethrow(Throwable t) { + if (t instanceof RuntimeException) { + throw (RuntimeException) t; + } + if (t instanceof Error) { + throw (Error) t; + } + throw new RuntimeException(t); + } +} 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 56fbedde..310a19cc 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java @@ -261,8 +261,13 @@ public void testWithArgumentsAndPrefix() { public interface WithArgumentsAndDefaultMethod { @PropertyName(name="${0}.abc.${1}") - default String getProperty(String part0, int part1) { - return "default"; + default String getPropertyWith2Placeholders(String part0, int part1) { + return "defaultFor2"; + } + + @PropertyName(name="${0}.def") + default String getPropertyWith1Placeholder(String part0) { + return "defaultFor1"; } } @@ -271,14 +276,18 @@ public void testWithArgumentsAndDefaultMethod() { SettableConfig config = new DefaultSettableConfig(); config.setProperty("a.abc.1", "value1"); config.setProperty("b.abc.2", "value2"); + config.setProperty("c.def", "value_c"); PropertyFactory factory = DefaultPropertyFactory.from(config); ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); WithArgumentsAndDefaultMethod withArgsAndDefM = proxy.newProxy(WithArgumentsAndDefaultMethod.class); - Assert.assertEquals("value1", withArgsAndDefM.getProperty("a", 1)); - Assert.assertEquals("value2", withArgsAndDefM.getProperty("b", 2)); - Assert.assertEquals("default", withArgsAndDefM.getProperty("a", 2)); + Assert.assertEquals("value1", withArgsAndDefM.getPropertyWith2Placeholders("a", 1)); + Assert.assertEquals("value2", withArgsAndDefM.getPropertyWith2Placeholders("b", 2)); + Assert.assertEquals("defaultFor2", withArgsAndDefM.getPropertyWith2Placeholders("a", 2)); + + Assert.assertEquals("value_c", withArgsAndDefM.getPropertyWith1Placeholder("c")); + Assert.assertEquals("defaultFor1", withArgsAndDefM.getPropertyWith1Placeholder("q")); } public interface ConfigWithMaps { @@ -525,8 +534,10 @@ public void testObjectMethods_ClassWithArgumentsAndDefaultMethod() { // Printing 'null' here is a compromise. The default method in the interface is being called with a bad signature. // There's nothing the proxy could return here that isn't a lie, but at least this is a mostly harmless lie. - Assert.assertEquals("WithArgumentsAndDefaultMethod[${0}.abc.${1}='null']", withArgs.toString()); + Assert.assertEquals("WithArgumentsAndDefaultMethod[${0}.def='null',${0}.abc.${1}='null']", withArgs.toString()); + //noinspection ObviousNullCheck Assert.assertNotNull(withArgs.hashCode()); - Assert.assertTrue(withArgs.equals(withArgs)); + //noinspection EqualsWithItself + Assert.assertEquals(withArgs, withArgs); } } diff --git a/archaius2-persisted2/build.gradle b/archaius2-persisted2/build.gradle index c50f44c6..252f5e5a 100644 --- a/archaius2-persisted2/build.gradle +++ b/archaius2-persisted2/build.gradle @@ -21,7 +21,7 @@ dependencies { implementation 'com.fasterxml.jackson.core:jackson-core:2.4.3' implementation 'com.fasterxml.jackson.core:jackson-databind:2.4.3' implementation 'javax.annotation:javax.annotation-api:1.3.2' - implementation 'org.apache.commons:commons-lang3:3.3.2' + implementation 'org.apache.commons:commons-lang3:3.12.0' implementation 'org.slf4j:slf4j-api:1.7.36' testImplementation project(':archaius2-guice') diff --git a/archaius2-test/build.gradle b/archaius2-test/build.gradle index b8ba3bb9..84bf8d61 100644 --- a/archaius2-test/build.gradle +++ b/archaius2-test/build.gradle @@ -4,7 +4,7 @@ apply plugin: 'java-library' dependencies { api project(':archaius2-core') api 'junit:junit:4.13.2' - implementation 'org.apache.commons:commons-lang3:3.3.2' + implementation 'org.apache.commons:commons-lang3:3.12.0' } eclipse { From 1ae467427a909df40545a53d9873a40c06d805e9 Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Wed, 11 Oct 2023 12:15:10 -0700 Subject: [PATCH 2/2] Cleanup compiler warnings. --- .../netflix/archaius/ConfigProxyFactory.java | 95 ++++++++++--------- .../netflix/archaius/ProxyFactoryTest.java | 31 ++++-- 2 files changed, 73 insertions(+), 53 deletions(-) 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 bb3171a3..ceb391c0 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java @@ -33,11 +33,11 @@ * Factory for binding a configuration interface to properties in a {@link PropertyFactory} * instance. Getter methods on the interface are mapped by naming convention * by the property name may be overridden using the @PropertyName annotation. - * + *

* For example, * 

  * {@code
- * {@literal @}Configuration(prefix="foo")
+ * @Configuration(prefix="foo")
  * interface FooConfiguration {
  *    int getTimeout();     // maps to "foo.timeout"
  *
@@ -50,11 +50,11 @@
  * that the default value type is a string to allow for interpolation.  Alternatively, methods can
  * provide a default method implementation.  Note that {@literal @}DefaultValue cannot be added to a default
  * method as it would introduce ambiguity as to which mechanism wins.
- *
+ * 

  * For example,
  * 
  * {@code
- * {@literal @}Configuration(prefix="foo")
+ * @Configuration(prefix="foo")
  * interface FooConfiguration {
  *    @DefaultValue("1000")
  *    int getReadTimeout();     // maps to "foo.timeout"
@@ -73,7 +73,7 @@
  * 

  * 
  * To override the prefix in {@literal @}Configuration or provide a prefix when there is no 
- * @Configuration annotation simply pass in a prefix in the call to newProxy.
+ * {@literal @}Configuration annotation simply pass in a prefix in the call to newProxy.
  * 
  * 
  * {@code 
@@ -81,14 +81,15 @@
  * }
  * 

  * 
- * By default all properties are dynamic and can therefore change from call to call.  To make the
+ * By default, all properties are dynamic and can therefore change from call to call.  To make the
  * configuration static set the immutable attributes of @Configuration to true.
  * 
  * Note that an application should normally have just one instance of ConfigProxyFactory
  * and PropertyFactory since PropertyFactory caches {@link com.netflix.archaius.api.Property} objects.
  * 
- * @see {@literal }@Configuration
+ * @see Configuration
  */
+@SuppressWarnings("deprecation")
 public class ConfigProxyFactory {
     private static final Logger LOG = LoggerFactory.getLogger(ConfigProxyFactory.class);
 
@@ -99,6 +100,7 @@ public class ConfigProxyFactory {
     private final PropertyRepository propertyRepository;
     private final Config config;
     
+    @SuppressWarnings("DIAnnotationInspectionTool")
     @Inject
     public ConfigProxyFactory(Config config, Decoder decoder, PropertyFactory factory) {
         this.decoder = decoder;
@@ -123,10 +125,6 @@ public ConfigProxyFactory(Config config) {
     /**
      * Create a proxy for the provided interface type for which all getter methods are bound
      * to a Property.
-     * 
-     * @param type
-     * @param config
-     * @return
      */
     public  T newProxy(final Class type) {
         return newProxy(type, null);
@@ -147,22 +145,22 @@ private String derivePrefix(Configuration annot, String prefix) {
         
         return prefix + ".";
     }
-    
+
+    /**
+     * Create a proxy for the provided interface type for which all getter methods are bound
+     * to a Property. The proxy uses the provided prefix, even if there is a {@link Configuration} annotation in TYPE.
+     */
     public  T newProxy(final Class type, final String initialPrefix) {
         Configuration annot = type.getAnnotation(Configuration.class);
-        return newProxy(type, initialPrefix, annot == null ? false : annot.immutable());
+        return newProxy(type, initialPrefix, annot != null && annot.immutable());
     }
     
     /**
-     * Encapsulated the invocation of a single method of the interface
-     *
-     * @param 
+     * Encapsulate the invocation of a single method of the interface
      */
     interface MethodInvoker {
         /**
          * Invoke the method with the provided arguments
-         * @param args
-         * @return
          */
         T invoke(Object[] args);
     }
@@ -194,30 +192,16 @@  T newProxy(final Class type, final String initialPrefix, boolean immutabl
             if (invoker != null) {
                 return invoker.invoke(args);
             }
-            if ("equals".equals(method.getName())) {
-            	return proxy == args[0];
-            }
-            else if ("hashCode".equals(method.getName())) {
-            	return System.identityHashCode(proxy);
-            }
-            else if ("toString".equals(method.getName())) {
-                StringBuilder sb = new StringBuilder();
-                sb.append(type.getSimpleName()).append("[");
-                sb.append(invokers.entrySet().stream().map(entry -> {
-                	StringBuilder sbProperty = new StringBuilder();
-                	sbProperty.append(propertyNames.get(entry.getKey()).substring(prefix.length())).append("='");
-                    try {
-                    	sbProperty.append(entry.getValue().invoke(null));
-                    } catch (Exception e) {
-                    	sbProperty.append(e.getMessage());
-                    }
-                    sbProperty.append("'");
-                    return sbProperty.toString();
-                }).collect(Collectors.joining(",")));
-                sb.append("]");
-                return sb.toString();
-            } else {
-                throw new NoSuchMethodError(method.getName() + " not found on interface " + type.getName());
+
+            switch (method.getName()) {
+                case "equals":
+                    return proxy == args[0];
+                case "hashCode":
+                    return System.identityHashCode(proxy);
+                case "toString":
+                    return proxyToString(type, prefix, invokers, propertyNames);
+                default:
+                    throw new NoSuchMethodError(method.getName() + " not found on interface " + type.getName());
             }
         };
 
@@ -297,7 +281,8 @@ private static  Function createAnnotatedMethodSupplier(Method m,
 
         String value = m.getAnnotation(DefaultValue.class).value();
         if (returnType == String.class) {
-            return memoize((T) config.resolve(value));
+            //noinspection unchecked
+            return memoize((T) config.resolve(value)); // The cast is actually a no-op, T == String here!
         } else {
             return memoize(decoder.decode(returnType, config.resolve(value)));
         }
@@ -370,4 +355,28 @@  R getPropertyWithDefault(Class type, String propName) {
             }
         }; 
     }
+
+    /** Compute a reasonable toString() for a proxy object */
+    private static  String proxyToString(Class type, String prefix, Map> invokers, Map propertyNames) {
+        StringBuilder sb = new StringBuilder();
+        sb.append(type.getSimpleName()).append("[");
+
+        sb.append(invokers.entrySet().stream().map(entry -> {
+            StringBuilder sbProperty = new StringBuilder();
+            sbProperty.append(propertyNames.get(entry.getKey()).substring(prefix.length())).append("='");
+
+            try {
+                sbProperty.append(entry.getValue().invoke(null));
+            } catch (Exception e) {
+                sbProperty.append(e.getMessage());
+            }
+
+            sbProperty.append("'");
+            return sbProperty.toString();
+
+        }).collect(Collectors.joining(",")));
+
+        sb.append("]");
+        return sb.toString();
+    }
 }
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 310a19cc..f07674f2 100644
--- a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java
+++ b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java
@@ -28,6 +28,7 @@
 import com.netflix.archaius.config.DefaultSettableConfig;
 import com.netflix.archaius.config.EmptyConfig;
 
+@SuppressWarnings("deprecation")
 public class ProxyFactoryTest {
     public enum TestEnum {
         NONE,
@@ -46,6 +47,7 @@ public interface ImmutableConfig {
         String getValueWithoutDefault2();
     }
     
+    @SuppressWarnings("unused")
     public interface BaseConfig {
         @DefaultValue("basedefault")
         String getStr();
@@ -60,6 +62,7 @@ default long getLongValueWithDefault() {
         }
     }
     
+    @SuppressWarnings("UnusedReturnValue")
     public interface RootConfig extends BaseConfig {
         @DefaultValue("default")
         @Override
@@ -92,7 +95,7 @@ public interface SubConfig {
     }
     
     public static class SubConfigFromString {
-        private String[] parts;
+        private final String[] parts;
 
         public SubConfigFromString(String value) {
             this.parts = value.split(":");
@@ -203,11 +206,11 @@ public void testAllPropertiesSet() {
             a.getRequiredValue();
             Assert.fail("should have failed with no value for requiredValue");
         }
-        catch (Exception e) {
+        catch (Exception expected) {
         }
     }
     
-    static interface WithArguments {
+    interface WithArguments {
         @PropertyName(name="${0}.abc.${1}")
         @DefaultValue("default")
         String getProperty(String part0, int part1);
@@ -229,7 +232,7 @@ public void testWithArguments() {
     }
 
     @Configuration(prefix = "foo.bar")
-    static interface WithArgumentsAndPrefix {
+    interface WithArgumentsAndPrefix {
         @PropertyName(name="baz.${0}.abc.${1}")
         @DefaultValue("default")
         String getPropertyWithoutPrefix(String part0, int part1);
@@ -259,6 +262,7 @@ public void testWithArgumentsAndPrefix() {
     }
 
 
+    @SuppressWarnings("unused")
     public interface WithArgumentsAndDefaultMethod {
         @PropertyName(name="${0}.abc.${1}")
         default String getPropertyWith2Placeholders(String part0, int part1) {
@@ -407,7 +411,7 @@ public void emptyNonStringValuesIgnoredInCollections() {
         Assert.assertEquals(Arrays.asList(1,2,4), new ArrayList<>(withCollections.getSortedSet()));
     }
     
-    public static interface ConfigWithStringCollections {
+    public interface ConfigWithStringCollections {
         List getList();
         
         Set getSet();
@@ -464,6 +468,7 @@ public void testCollectionsWithoutValue() {
         Assert.assertTrue(withCollections.getSortedSet().isEmpty());
     }
     
+    @SuppressWarnings("unused")
     public interface ConfigWithCollectionsWithDefaultValueAnnotation {
         @DefaultValue("")
         LinkedList getLinkedList();
@@ -494,12 +499,13 @@ public void interfaceDefaultCollections() {
         ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory);
         ConfigWithDefaultStringCollections withCollections = proxy.newProxy(ConfigWithDefaultStringCollections.class);
         
-        Assert.assertEquals(Arrays.asList("default"), new ArrayList<>(withCollections.getList()));
-        Assert.assertEquals(Arrays.asList("default"), new ArrayList<>(withCollections.getSet()));
-        Assert.assertEquals(Arrays.asList("default"), new ArrayList<>(withCollections.getSortedSet()));
+        Assert.assertEquals(Collections.singletonList("default"), new ArrayList<>(withCollections.getList()));
+        Assert.assertEquals(Collections.singletonList("default"), new ArrayList<>(withCollections.getSet()));
+        Assert.assertEquals(Collections.singletonList("default"), new ArrayList<>(withCollections.getSortedSet()));
     }
 
-    public static interface FailingError {
+    @SuppressWarnings("UnusedReturnValue")
+    public interface FailingError {
         default String getValue() { throw new IllegalStateException("error"); }
     }
     
@@ -515,18 +521,22 @@ public void interfaceWithBadDefault() {
     
     @Test
     public void testObjectMethods() {
+        // These tests just ensure that toString, equals and hashCode have implementations that don't fail.
         SettableConfig config = new DefaultSettableConfig();
         PropertyFactory factory = DefaultPropertyFactory.from(config);
         ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory);
         WithArguments withArgs = proxy.newProxy(WithArguments.class);
         
         Assert.assertEquals("WithArguments[${0}.abc.${1}='default']", withArgs.toString());
+        //noinspection ObviousNullCheck
         Assert.assertNotNull(withArgs.hashCode());
-        Assert.assertTrue(withArgs.equals(withArgs));
+        //noinspection EqualsWithItself
+        Assert.assertEquals(withArgs, withArgs);
     }
 
     @Test
     public void testObjectMethods_ClassWithArgumentsAndDefaultMethod() {
+        // These tests just ensure that toString, equals and hashCode have implementations that don't fail.
         SettableConfig config = new DefaultSettableConfig();
         PropertyFactory factory = DefaultPropertyFactory.from(config);
         ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory);
@@ -534,6 +544,7 @@ public void testObjectMethods_ClassWithArgumentsAndDefaultMethod() {
 
         // Printing 'null' here is a compromise. The default method in the interface is being called with a bad signature.
         // There's nothing the proxy could return here that isn't a lie, but at least this is a mostly harmless lie.
+        // This test depends implicitly on the iteration order for HashMap, which could change on future Java releases.
         Assert.assertEquals("WithArgumentsAndDefaultMethod[${0}.def='null',${0}.abc.${1}='null']", withArgs.toString());
         //noinspection ObviousNullCheck
         Assert.assertNotNull(withArgs.hashCode());