From ce302552d721a08ef167804e9cdc0aebf85fce89 Mon Sep 17 00:00:00 2001 From: Patrick Strawderman Date: Sun, 19 Nov 2023 11:38:39 -0800 Subject: [PATCH] Return unmodifiable collections in DefaultCollectionsTypeConverterFactory DefaultCollectionsTypeConverterFactory already returned an unmodifiable Map, so update the collection converters for List, Set, and SortedSet to also wrap the converted result with the appropriate unmodifiable collection. Additionally, if the value to convert is an empty String, don't allocate a collection and simply return the appropriate empty singleton (e.g. Collections.emptyList()). Finally, improve type safety in a few places and avoid usage of raw types. --- ...efaultCollectionsTypeConverterFactory.java | 61 +++++++++++++------ .../archaius/property/PropertyTest.java | 38 ++++++++++++ 2 files changed, 79 insertions(+), 20 deletions(-) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/converters/DefaultCollectionsTypeConverterFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/converters/DefaultCollectionsTypeConverterFactory.java index 7194d826..2532f35b 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/converters/DefaultCollectionsTypeConverterFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/converters/DefaultCollectionsTypeConverterFactory.java @@ -18,6 +18,7 @@ import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; +import java.util.function.Function; import java.util.function.Supplier; public final class DefaultCollectionsTypeConverterFactory implements TypeConverter.Factory { @@ -27,7 +28,7 @@ private DefaultCollectionsTypeConverterFactory() {} @Override public Optional> get(Type type, TypeConverter.Registry registry) { if (type instanceof ParameterizedType) { - ParameterizedType parameterizedType = (ParameterizedType)type; + ParameterizedType parameterizedType = (ParameterizedType) type; if (parameterizedType.getRawType().equals(Map.class)) { return Optional.of(createMapTypeConverter( registry.get(parameterizedType.getActualTypeArguments()[0]).orElseThrow(() -> new ConverterNotFoundException("No converter found")), @@ -37,50 +38,70 @@ public Optional> get(Type type, TypeConverter.Registry registry return Optional.of(createCollectionTypeConverter( parameterizedType.getActualTypeArguments()[0], registry, - LinkedHashSet::new)); + LinkedHashSet::new, + Collections::emptySet, + Collections::unmodifiableSet)); } else if (parameterizedType.getRawType().equals(SortedSet.class)) { return Optional.of(createCollectionTypeConverter( parameterizedType.getActualTypeArguments()[0], registry, - TreeSet::new)); + TreeSet::new, + Collections::emptySortedSet, + Collections::unmodifiableSortedSet)); } else if (parameterizedType.getRawType().equals(List.class)) { return Optional.of(createCollectionTypeConverter( parameterizedType.getActualTypeArguments()[0], registry, - ArrayList::new)); + ArrayList::new, + Collections::emptyList, + Collections::unmodifiableList)); } else if (parameterizedType.getRawType().equals(LinkedList.class)) { return Optional.of(createCollectionTypeConverter( parameterizedType.getActualTypeArguments()[0], registry, - LinkedList::new)); + LinkedList::new, + LinkedList::new, + Function.identity())); } } return Optional.empty(); } - @SuppressWarnings({ "rawtypes", "unchecked" }) - private TypeConverter createCollectionTypeConverter(final Type type, TypeConverter.Registry registry, final Supplier> collectionFactory) { - TypeConverter elementConverter = registry.get(type).orElseThrow(() -> new ConverterNotFoundException("No converter found")); + private static > TypeConverter createCollectionTypeConverter(final Type elementType, + final TypeConverter.Registry registry, + final Supplier collectionFactory, + final Supplier emptyCollectionFactory, + final Function finisher) { + @SuppressWarnings("unchecked") + TypeConverter elementConverter = (TypeConverter) registry.get(elementType) + .orElseThrow(() -> new ConverterNotFoundException("No converter found")); - boolean ignoreEmpty = !String.class.equals(type); + boolean ignoreEmpty = !String.class.equals(elementType); return value -> { - final Collection collection = collectionFactory.get(); - if (!value.isEmpty()) { - Arrays.asList(value.split("\\s*,\\s*")).forEach(v -> { - if (!v.isEmpty() || !ignoreEmpty) { - collection.add(elementConverter.convert(v)); - } - }); + if (value.isEmpty()) { + return emptyCollectionFactory.get(); } - return collection; + final T collection = collectionFactory.get(); + for (String item : value.split("\\s*,\\s*")) { + if (!item.isEmpty() || !ignoreEmpty) { + collection.add(elementConverter.convert(item)); + } + } + return finisher.apply(collection); }; } - private TypeConverter createMapTypeConverter(final TypeConverter keyConverter, final TypeConverter valueConverter, final Supplier mapFactory) { + private static TypeConverter> createMapTypeConverter(final TypeConverter keyConverter, + final TypeConverter valueConverter, + final Supplier> mapFactory) { return s -> { - Map result = mapFactory.get(); + if (s.isEmpty()) { + return Collections.emptyMap(); + } + Map result = mapFactory.get(); + Arrays .stream(s.split("\\s*,\\s*")) .filter(pair -> !pair.isEmpty()) @@ -91,4 +112,4 @@ private TypeConverter createMapTypeConverter(final TypeConverter keyConver return Collections.unmodifiableMap(result); }; } -} +} \ No newline at end of file diff --git a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java index 4309d72f..1c331ffc 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java @@ -19,6 +19,7 @@ import java.math.BigInteger; import java.time.Duration; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -167,6 +168,43 @@ public void testCollectionTypes() { // Test proper handling of unset properties Property> emptyProperty = factory.getMap("fubar", CustomType.class, CustomType.class); Assert.assertNull(emptyProperty.get()); + + config.setProperty("fubar", ""); + Property> emptyListProperty = factory.getList("fubar", String.class); + Assert.assertEquals(Collections.emptyList(), emptyListProperty.get()); + + Property> emptySetProperty = factory.getSet("fubar", String.class); + Assert.assertEquals(Collections.emptySet(), emptySetProperty.get()); + + Property> emptyMapProperty = factory.getMap("fubar", String.class, String.class); + Assert.assertEquals(Collections.emptyMap(), emptyMapProperty.get()); + } + + @Test + public void testCollectionTypesImmutability() { + SettableConfig config = new DefaultSettableConfig(); + DefaultPropertyFactory factory = DefaultPropertyFactory.from(config); + config.setProperty("foo", "10,13,13,20"); + config.setProperty("bar", ""); + config.setProperty("baz", "a=1,b=2"); + + List list = factory.getList("foo", Integer.class).get(); + Assert.assertThrows(UnsupportedOperationException.class, () -> list.add(100)); + + Set set = factory.getSet("foo", Integer.class).get(); + Assert.assertThrows(UnsupportedOperationException.class, () -> set.add(100)); + + Map map = factory.getMap("baz", String.class, Integer.class).get(); + Assert.assertThrows(UnsupportedOperationException.class, () -> map.put("c", 3)); + + List emptyList = factory.getList("bar", Integer.class).get(); + Assert.assertThrows(UnsupportedOperationException.class, () -> emptyList.add(100)); + + Set emptySet = factory.getSet("bar", Integer.class).get(); + Assert.assertThrows(UnsupportedOperationException.class, () -> emptySet.add(100)); + + Map emptyMap = factory.getMap("bar", String.class, Integer.class).get(); + Assert.assertThrows(UnsupportedOperationException.class, () -> emptyMap.put("c", 3)); } @Test