From 3d810bb1558071fe1b629c6bf721b835d13d5d9f Mon Sep 17 00:00:00 2001 From: Eran Landau Date: Fri, 2 Sep 2016 13:30:26 -0700 Subject: [PATCH] Proper support for empty values in collections --- .../netflix/archaius/ConfigProxyFactory.java | 32 +++++----- .../property/DefaultPropertyContainer.java | 5 +- .../netflix/archaius/ProxyFactoryTest.java | 58 +++++++++++++++++-- 3 files changed, 69 insertions(+), 26 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 ca8f23e0d..c5ddd1c4e 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java @@ -18,6 +18,7 @@ import java.lang.reflect.Proxy; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.Iterator; import java.util.LinkedHashSet; @@ -247,13 +248,13 @@ T newProxy(final Class type, final String initialPrefix, boolean immutabl if (returnType.equals(Map.class)) { invokers.put(m, createMapProperty(propName, (ParameterizedType)m.getGenericReturnType(), immutable)); } else if (returnType.equals(Set.class)) { - invokers.put(m, createSetProperty(propName, (ParameterizedType)m.getGenericReturnType(), LinkedHashSet::new)); + invokers.put(m, createCollectionProperty(propName, (ParameterizedType)m.getGenericReturnType(), LinkedHashSet::new)); } else if (returnType.equals(SortedSet.class)) { - invokers.put(m, createSetProperty(propName, (ParameterizedType)m.getGenericReturnType(), TreeSet::new)); + invokers.put(m, createCollectionProperty(propName, (ParameterizedType)m.getGenericReturnType(), TreeSet::new)); } else if (returnType.equals(List.class)) { - invokers.put(m, createListProperty(propName, (ParameterizedType)m.getGenericReturnType(), ArrayList::new)); + invokers.put(m, createCollectionProperty(propName, (ParameterizedType)m.getGenericReturnType(), ArrayList::new)); } else if (returnType.equals(LinkedList.class)) { - invokers.put(m, createListProperty(propName, (ParameterizedType)m.getGenericReturnType(), LinkedList::new)); + invokers.put(m, createCollectionProperty(propName, (ParameterizedType)m.getGenericReturnType(), LinkedList::new)); } else if (returnType.isInterface()) { invokers.put(m, createInterfaceProperty(propName, newProxy(returnType, propName, immutable))); } else if (m.getParameterTypes() != null && m.getParameterTypes().length > 0) { @@ -323,7 +324,7 @@ T newProxy(final Class type, final String initialPrefix, boolean immutabl protected MethodInvoker createCustomProperty(final Function converter, final String propName) { final Property prop = propertyFactory .getProperty(propName) - .asType(converter, null); + .asType(converter, ""); return new MethodInvoker() { @Override public T invoke(Object obj, Object[] args) { @@ -337,24 +338,21 @@ public String getKey() { }; } - private MethodInvoker createListProperty(String propName, ParameterizedType type, Supplier listSupplier) { + private MethodInvoker createCollectionProperty(String propName, ParameterizedType type, Supplier listSupplier) { final Class valueType = (Class)type.getActualTypeArguments()[0]; return createCustomProperty(s -> { - List list = listSupplier.get(); - Arrays.asList(s.split("\\s*,\\s*")).forEach(v -> list.add(decoder.decode(valueType, v))); + Collection list = listSupplier.get(); + if (!s.isEmpty()) { + Arrays.asList(s.split("\\s*,\\s*")).forEach(v -> { + if (!v.isEmpty() || valueType == String.class) { + list.add(decoder.decode(valueType, v)); + } + }); + } return list; }, propName); } - private MethodInvoker createSetProperty(String propName, ParameterizedType type, Supplier setSupplier) { - final Class valueType = (Class)type.getActualTypeArguments()[0]; - return createCustomProperty(s -> { - Set set = setSupplier.get(); - Arrays.asList(s.split("\\s*,\\s*")).forEach(v -> set.add(decoder.decode(valueType, v))); - return set; - }, propName); - } - @SuppressWarnings("unchecked") private MethodInvoker createMapProperty(final String propName, final ParameterizedType type, final boolean immutable) { final Class valueType = (Class)type.getActualTypeArguments()[1]; diff --git a/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java b/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java index 3c41a45c0..73d3146ea 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java @@ -206,9 +206,8 @@ public T get() { T newValue = null; try { newValue = resolveCurrent(); - } - catch (Exception e) { - LOG.warn("Unable to get current version of property '{}'. Error: {}", key, e.getMessage()); + } catch (Exception e) { + LOG.warn("Unable to get current version of property '{}'", key, e); } if (cache.compareAndSet(currentValue, newValue, cacheVersion, latestVersion)) { 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 1e6b4763c..fdff03810 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java @@ -252,7 +252,7 @@ public static interface ConfigWithCollections { public void testCollections() { SettableConfig config = new DefaultSettableConfig(); config.setProperty("list", "5,4,3,2,1"); - config.setProperty("set", "5,4,3,2,1"); + config.setProperty("set", "1,2,3,5,4"); config.setProperty("sortedSet", "5,4,3,2,1"); config.setProperty("linkedList", "5,4,3,2,1"); @@ -262,13 +262,59 @@ public void testCollections() { Assert.assertEquals(Arrays.asList(5,4,3,2,1), new ArrayList<>(withCollections.getLinkedList())); Assert.assertEquals(Arrays.asList(5,4,3,2,1), new ArrayList<>(withCollections.getList())); - Assert.assertEquals(Arrays.asList(5,4,3,2,1), new ArrayList<>(withCollections.getSet())); + Assert.assertEquals(Arrays.asList(1,2,3,5,4), new ArrayList<>(withCollections.getSet())); Assert.assertEquals(Arrays.asList(1,2,3,4,5), new ArrayList<>(withCollections.getSortedSet())); config.setProperty("list", "6,7,8,9,10"); Assert.assertEquals(Arrays.asList(6,7,8,9,10), new ArrayList<>(withCollections.getList())); } + + @Test + public void emptyNonStringValuesIgnoredInCollections() { + SettableConfig config = new DefaultSettableConfig(); + config.setProperty("list", ",4, ,2,1"); + config.setProperty("set", ",2, ,5,4"); + config.setProperty("sortedSet", ",4, ,2,1"); + config.setProperty("linkedList", ",4, ,2,1"); + + PropertyFactory factory = DefaultPropertyFactory.from(config); + ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); + ConfigWithCollections withCollections = proxy.newProxy(ConfigWithCollections.class); + + Assert.assertEquals(Arrays.asList(4,2,1), new ArrayList<>(withCollections.getLinkedList())); + Assert.assertEquals(Arrays.asList(4,2,1), new ArrayList<>(withCollections.getList())); + Assert.assertEquals(Arrays.asList(2,5,4), new ArrayList<>(withCollections.getSet())); + Assert.assertEquals(Arrays.asList(1,2,4), new ArrayList<>(withCollections.getSortedSet())); + } + public static interface ConfigWithStringCollections { + List getList(); + + Set getSet(); + + SortedSet getSortedSet(); + + LinkedList getLinkedList(); + } + + @Test + public void emptyStringValuesAreAddedToCollection() { + SettableConfig config = new DefaultSettableConfig(); + config.setProperty("list", ",4, ,2,1"); + config.setProperty("set", ",2, ,5,4"); + config.setProperty("sortedSet", ",4, ,2,1"); + config.setProperty("linkedList", ",4, ,2,1"); + + PropertyFactory factory = DefaultPropertyFactory.from(config); + ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); + ConfigWithStringCollections withCollections = proxy.newProxy(ConfigWithStringCollections.class); + + Assert.assertEquals(Arrays.asList("", "4","", "2","1"), new ArrayList<>(withCollections.getLinkedList())); + Assert.assertEquals(Arrays.asList("", "4","", "2","1"), new ArrayList<>(withCollections.getList())); + Assert.assertEquals(Arrays.asList("" ,"2","5","4"), new ArrayList<>(withCollections.getSet())); + Assert.assertEquals(Arrays.asList("", "1","2","4"), new ArrayList<>(withCollections.getSortedSet())); + } + @Test public void testCollectionsWithoutValue() { SettableConfig config = new DefaultSettableConfig(); @@ -277,10 +323,10 @@ public void testCollectionsWithoutValue() { ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); ConfigWithCollections withCollections = proxy.newProxy(ConfigWithCollections.class); - Assert.assertNull(withCollections.getLinkedList()); - Assert.assertNull(withCollections.getList()); - Assert.assertNull(withCollections.getSet()); - Assert.assertNull(withCollections.getSortedSet()); + Assert.assertTrue(withCollections.getLinkedList().isEmpty()); + Assert.assertTrue(withCollections.getList().isEmpty()); + Assert.assertTrue(withCollections.getSet().isEmpty()); + Assert.assertTrue(withCollections.getSortedSet().isEmpty()); } public static interface ConfigWithCollectionsWithDefaultValueAnnotation {