From 6491abc55fb2b7105ff6df86fd5f2176e1d18b52 Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Tue, 16 Aug 2022 15:20:46 -0700 Subject: [PATCH] Fixed interaction of default methods and parametrized properties. --- .../netflix/archaius/ConfigProxyFactory.java | 79 +++++++++---------- .../netflix/archaius/ProxyFactoryTest.java | 40 +++++++++- 2 files changed, 76 insertions(+), 43 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 62a756a4f..b9e62a50a 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java @@ -30,7 +30,7 @@ import java.util.Optional; import java.util.Set; import java.util.SortedSet; -import java.util.function.Supplier; +import java.util.function.Function; import java.util.stream.Collectors; /** @@ -171,36 +171,14 @@ interface MethodInvoker { T invoke(Object[] args); } - /** - * Abstract method invoker that encapsulates a property - * @param - */ - private static abstract class PropertyMethodInvoker extends AbstractProperty implements MethodInvoker { - private final Supplier next; - - public PropertyMethodInvoker(String key, Supplier next) { - super(key); - this.next = next; - } - - @Override - public T invoke(Object[] args) { - T result = get(); - if (result == null) { - return next.get(); - } - return result; - } - } - - private static Map> knownCollections = new HashMap<>(); + private static Map> knownCollections = new HashMap<>(); static { - knownCollections.put(Map.class, Collections::emptyMap); - knownCollections.put(Set.class, Collections::emptySet); - knownCollections.put(SortedSet.class, Collections::emptySortedSet); - knownCollections.put(List.class, Collections::emptyList); - knownCollections.put(LinkedList.class, LinkedList::new); + knownCollections.put(Map.class, (ignored) -> Collections.emptyMap()); + knownCollections.put(Set.class, (ignored) -> Collections.emptySet()); + knownCollections.put(SortedSet.class, (ignored) -> Collections.emptySortedSet()); + knownCollections.put(List.class, (ignored) -> Collections.emptyList()); + knownCollections.put(LinkedList.class, (ignored) -> new LinkedList()); } @SuppressWarnings({ "rawtypes", "unchecked" }) @@ -266,7 +244,7 @@ else if ("toString".equals(method.getName())) { final Class returnType = m.getReturnType(); - Supplier defaultSupplier = knownCollections.getOrDefault(returnType, () -> null); + Function defaultSupplier = knownCollections.getOrDefault(returnType, (ignored) -> null); if (m.getAnnotation(DefaultValue.class) != null) { if (m.isDefault()) { @@ -324,11 +302,11 @@ else if ("toString".equals(method.getName())) { return proxyObject; } - private static Supplier memoize(T value) { - return () -> value; + private static Function memoize(T value) { + return (ignored) -> value; } - private static Supplier createDefaultMethodSupplier(Method method, Class type, T proxyObject) { + private static Function createDefaultMethodSupplier(Method method, Class type, T proxyObject) { final MethodHandle methodHandle; try { @@ -345,7 +323,7 @@ private static Supplier createDefaultMethodSupplier(Method method, Class< methodHandle = MethodHandles.lookup() .findSpecial(type, method.getName(), - MethodType.methodType(method.getReturnType(), new Class[0]), + MethodType.methodType(method.getReturnType(), method.getParameterTypes()), type) .bindTo(proxyObject); } @@ -353,10 +331,20 @@ private static Supplier createDefaultMethodSupplier(Method method, Class< throw new RuntimeException("Failed to create temporary object for " + type.getName(), e); } - return () -> { + return (args) -> { try { - //noinspection unchecked - return (T) methodHandle.invokeWithArguments(); + 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) { throw new RuntimeException(e); } @@ -368,17 +356,26 @@ protected MethodInvoker createInterfaceProperty(String propName, final T return (args) -> proxy; } - protected MethodInvoker createScalarProperty(final Type type, final String propName, Supplier next) { + protected MethodInvoker createScalarProperty(final Type type, final String propName, Function next) { LOG.debug("Creating scalar property `{}` for type `{}`", propName, type.getClass()); final Property prop = propertyRepository.get(propName, type); - return args -> Optional.ofNullable(prop.get()).orElseGet(next); + return args -> Optional.ofNullable(prop.get()).orElseGet(() -> next.apply(null)); } - protected MethodInvoker createParameterizedProperty(final Class returnType, final String propName, final String nameAnnot, Supplier next) { + protected MethodInvoker createParameterizedProperty(final Class returnType, final String propName, final String nameAnnot, Function next) { LOG.debug("Creating parameterized property `{}` for type `{}`", propName, returnType); return new MethodInvoker() { @Override public T invoke(Object[] args) { + if (args == null) { + // Why would args be null if this is a parametrized property? Because toString() abuses its + // access to this internal representation :-/ + // We'll fall back to trying to call the provider for the default value. That works properly if + // it comes from an annotation or the known collections. Our wrapper for default interface methods + // catches this case and just returns a null, which is probably the least bad response. + return next.apply(null); + } + // Determine the actual property name by replacing with arguments using the argument index // to the method. For example, // @PropertyName(name="foo.${1}.${0}") @@ -392,7 +389,7 @@ public T invoke(Object[] args) { String propName = new StrSubstitutor(values, "${", "}", '$').replace(nameAnnot); T result = getPropertyWithDefault(returnType, propName); if (result == null) { - result = next.get(); + result = next.apply(args); } return result; } 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 9e6143e40..b21ba16b7 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java @@ -217,7 +217,29 @@ public void testWithArguments() { Assert.assertEquals("value2", withArgs.getProperty("b", 2)); Assert.assertEquals("default", withArgs.getProperty("a", 2)); } - + + public interface WithArgumentsAndDefaultMethod { + @PropertyName(name="${0}.abc.${1}") + default String getProperty(String part0, int part1) { + return "default"; + } + } + + @Test + public void testWithArgumentsAndDefaultMethod() { + SettableConfig config = new DefaultSettableConfig(); + config.setProperty("a.abc.1", "value1"); + config.setProperty("b.abc.2", "value2"); + + 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)); + } + public interface ConfigWithMaps { @PropertyName(name="map") default Map getStringToLongMap() { return Collections.singletonMap("default", 0L); } @@ -448,7 +470,21 @@ public void testObjectMethods() { ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); WithArguments withArgs = proxy.newProxy(WithArguments.class); - Assert.assertEquals("WithArguments[${0}.abc.${1}='null']", withArgs.toString()); + Assert.assertEquals("WithArguments[${0}.abc.${1}='default']", withArgs.toString()); + Assert.assertNotNull(withArgs.hashCode()); + Assert.assertTrue(withArgs.equals(withArgs)); + } + + @Test + public void testObjectMethods_ClassWithArgumentsAndDefaultMethod() { + SettableConfig config = new DefaultSettableConfig(); + PropertyFactory factory = DefaultPropertyFactory.from(config); + ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); + WithArgumentsAndDefaultMethod withArgs = proxy.newProxy(WithArgumentsAndDefaultMethod.class); + + // 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.assertNotNull(withArgs.hashCode()); Assert.assertTrue(withArgs.equals(withArgs)); }