Return unmodifiable collections in DefaultCollectionsTypeConverterFac…

…tory 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.
Netflix · Nov 19, 2023 · ce30255 · ce30255
1 parent 0332cef
commit ce30255
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 20 deletions.
diff --git a/...src/main/java/com/netflix/archaius/converters/DefaultCollectionsTypeConverterFactory.java b/...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<TypeConverter<?>> 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<TypeConverter<?>> 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 <T> TypeConverter<?> createCollectionTypeConverter(final Type type, TypeConverter.Registry registry, final Supplier<Collection<T>> collectionFactory) {
-        TypeConverter elementConverter = registry.get(type).orElseThrow(() -> new ConverterNotFoundException("No converter found"));
+    private static <E, T extends Collection<E>> TypeConverter<T> createCollectionTypeConverter(final Type elementType,
+                                                                                               final TypeConverter.Registry registry,
+                                                                                               final Supplier<T> collectionFactory,
+                                                                                               final Supplier<T> emptyCollectionFactory,
+                                                                                               final Function<T, T> finisher) {
+        @SuppressWarnings("unchecked")
+        TypeConverter<E> elementConverter = (TypeConverter<E>) 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<Map> mapFactory) {
+    private static <K, V> TypeConverter<Map<K, V>> createMapTypeConverter(final TypeConverter<K> keyConverter,
+                                                                          final TypeConverter<V> valueConverter,
+                                                                          final Supplier<Map<K, V>> mapFactory) {
         return s -> {
-            Map result = mapFactory.get();
+            if (s.isEmpty()) {
+                return Collections.emptyMap();
+            }
+            Map<K, V> 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);
         };
     }
-}
+}
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
@@ -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<Map<CustomType, CustomType>> emptyProperty = factory.getMap("fubar", CustomType.class, CustomType.class);
         Assert.assertNull(emptyProperty.get());
+
+        config.setProperty("fubar", "");
+        Property<List<String>> emptyListProperty = factory.getList("fubar", String.class);
+        Assert.assertEquals(Collections.emptyList(), emptyListProperty.get());
+
+        Property<Set<String>> emptySetProperty = factory.getSet("fubar", String.class);
+        Assert.assertEquals(Collections.emptySet(), emptySetProperty.get());
+
+        Property<Map<String, String>> 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<Integer> list = factory.getList("foo", Integer.class).get();
+        Assert.assertThrows(UnsupportedOperationException.class, () -> list.add(100));
+
+        Set<Integer> set = factory.getSet("foo", Integer.class).get();
+        Assert.assertThrows(UnsupportedOperationException.class, () -> set.add(100));
+
+        Map<String, Integer> map = factory.getMap("baz", String.class, Integer.class).get();
+        Assert.assertThrows(UnsupportedOperationException.class, () -> map.put("c", 3));
+
+        List<Integer> emptyList = factory.getList("bar", Integer.class).get();
+        Assert.assertThrows(UnsupportedOperationException.class, () -> emptyList.add(100));
+
+        Set<Integer> emptySet = factory.getSet("bar", Integer.class).get();
+        Assert.assertThrows(UnsupportedOperationException.class, () -> emptySet.add(100));
+
+        Map<String, Integer> emptyMap = factory.getMap("bar", String.class, Integer.class).get();
+        Assert.assertThrows(UnsupportedOperationException.class, () -> emptyMap.put("c", 3));
     }
 
     @Test