From bea94d5302e16f72a64377c052595a1976629dde Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 10 Mar 2014 15:59:19 +0100 Subject: [PATCH] CollectionToCollectionConverter avoids collection copy for untyped collection when simply returning source anyway Also uses addAll instead of iteration over untyped collection now, supporting optimized addAll in target collection type, and avoids repeated getElementTypeDescriptor calls. Issue: SPR-11479 --- .../CollectionToCollectionConverter.java | 26 +++-- .../CollectionToCollectionConverterTests.java | 108 ++++++++++-------- 2 files changed, 79 insertions(+), 55 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/convert/support/CollectionToCollectionConverter.java b/spring-core/src/main/java/org/springframework/core/convert/support/CollectionToCollectionConverter.java index 09ca1f8ad5a..a8071eae58a 100644 --- a/spring-core/src/main/java/org/springframework/core/convert/support/CollectionToCollectionConverter.java +++ b/spring-core/src/main/java/org/springframework/core/convert/support/CollectionToCollectionConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -34,16 +34,19 @@ import org.springframework.core.convert.converter.ConditionalGenericConverter; * parameterized type to the target collection's parameterized type if necessary. * * @author Keith Donald + * @author Juergen Hoeller * @since 3.0 */ final class CollectionToCollectionConverter implements ConditionalGenericConverter { private final ConversionService conversionService; + public CollectionToCollectionConverter(ConversionService conversionService) { this.conversionService = conversionService; } + @Override public Set getConvertibleTypes() { return Collections.singleton(new ConvertiblePair(Collection.class, Collection.class)); @@ -60,27 +63,34 @@ final class CollectionToCollectionConverter implements ConditionalGenericConvert if (source == null) { return null; } - boolean copyRequired = !targetType.getType().isInstance(source); Collection sourceCollection = (Collection) source; + + // Shortcut if possible... + boolean copyRequired = !targetType.getType().isInstance(source); if (!copyRequired && sourceCollection.isEmpty()) { - return sourceCollection; + return source; } + TypeDescriptor elementDesc = targetType.getElementTypeDescriptor(); + if (elementDesc == null && !copyRequired) { + return source; + } + + // At this point, we need a collection copy in any case, even if just for finding out about element copies... Collection target = CollectionFactory.createCollection(targetType.getType(), sourceCollection.size()); - if (targetType.getElementTypeDescriptor() == null) { - for (Object element : sourceCollection) { - target.add(element); - } + if (elementDesc == null) { + target.addAll(sourceCollection); } else { for (Object sourceElement : sourceCollection) { Object targetElement = this.conversionService.convert(sourceElement, - sourceType.elementTypeDescriptor(sourceElement), targetType.getElementTypeDescriptor()); + sourceType.elementTypeDescriptor(sourceElement), elementDesc); target.add(targetElement); if (sourceElement != targetElement) { copyRequired = true; } } } + return (copyRequired ? target : source); } diff --git a/spring-core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java b/spring-core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java index ddb99a220b1..00bf054a66d 100644 --- a/spring-core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java +++ b/spring-core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -44,16 +44,19 @@ import static org.junit.Assert.*; /** * @author Keith Donald * @author Juergen Hoeller + * @author Stephane Nicoll */ public class CollectionToCollectionConverterTests { private GenericConversionService conversionService = new GenericConversionService(); + @Before public void setUp() { conversionService.addConverter(new CollectionToCollectionConverter(conversionService)); } + @Test public void scalarList() throws Exception { List list = new ArrayList(); @@ -77,8 +80,6 @@ public class CollectionToCollectionConverterTests { assertEquals(37, result.get(1)); } - public ArrayList scalarListTarget; - @Test public void emptyListToList() throws Exception { conversionService.addConverter(new CollectionToCollectionConverter(conversionService)); @@ -90,8 +91,6 @@ public class CollectionToCollectionConverterTests { assertEquals(list, conversionService.convert(list, sourceType, targetType)); } - public List emptyListTarget; - @Test public void emptyListToListDifferentTargetType() throws Exception { conversionService.addConverter(new CollectionToCollectionConverter(conversionService)); @@ -106,8 +105,6 @@ public class CollectionToCollectionConverterTests { assertTrue(result.isEmpty()); } - public LinkedList emptyListDifferentTarget; - @Test public void collectionToObjectInteraction() throws Exception { List> list = new ArrayList>(); @@ -149,8 +146,6 @@ public class CollectionToCollectionConverterTests { assertEquals((Integer) 23, result.get(1).get(1).get(0)); } - public List>> objectToCollection; - @Test @SuppressWarnings("unchecked") public void stringToCollection() throws Exception { @@ -165,10 +160,46 @@ public class CollectionToCollectionConverterTests { TypeDescriptor targetType = new TypeDescriptor(getClass().getField("objectToCollection")); assertTrue(conversionService.canConvert(sourceType, targetType)); List>> result = (List>>) conversionService.convert(list, sourceType, targetType); - assertEquals((Integer)9, result.get(0).get(0).get(0)); - assertEquals((Integer)12, result.get(0).get(0).get(1)); - assertEquals((Integer)37, result.get(1).get(0).get(0)); - assertEquals((Integer)23, result.get(1).get(0).get(1)); + assertEquals((Integer) 9, result.get(0).get(0).get(0)); + assertEquals((Integer) 12, result.get(0).get(0).get(1)); + assertEquals((Integer) 37, result.get(1).get(0).get(0)); + assertEquals((Integer) 23, result.get(1).get(0).get(1)); + } + + @Test + public void convertEmptyVector_shouldReturnEmptyArrayList() { + Vector vector = new Vector(); + vector.add("Element"); + testCollectionConversionToArrayList(vector); + } + + @Test + public void convertNonEmptyVector_shouldReturnNonEmptyArrayList() { + Vector vector = new Vector(); + vector.add("Element"); + testCollectionConversionToArrayList(vector); + } + + @Test + public void testCollectionsEmptyList() throws Exception { + CollectionToCollectionConverter converter = new CollectionToCollectionConverter(new GenericConversionService()); + TypeDescriptor type = new TypeDescriptor(getClass().getField("list")); + converter.convert(list, type, TypeDescriptor.valueOf(Class.forName("java.util.Collections$EmptyList"))); + } + + @SuppressWarnings("rawtypes") + private void testCollectionConversionToArrayList(Collection aSource) { + Object myConverted = (new CollectionToCollectionConverter(new GenericConversionService())).convert( + aSource, TypeDescriptor.forObject(aSource), TypeDescriptor.forObject(new ArrayList())); + assertTrue(myConverted instanceof ArrayList); + assertEquals(aSource.size(), ((ArrayList) myConverted).size()); + } + + @Test + public void listToCollectionNoCopyRequired() throws NoSuchFieldException { + List input = new ArrayList(Arrays.asList("foo", "bar")); + assertSame(input, conversionService.convert(input, TypeDescriptor.forObject(input), + new TypeDescriptor(getClass().getField("wildCardCollection")))); } @Test @@ -210,8 +241,6 @@ public class CollectionToCollectionConverterTests { assertEquals(resources, conversionService.convert(resources, sourceType, new TypeDescriptor(getClass().getField("resources")))); } - public List strings; - @Test(expected=ConversionFailedException.class) public void nothingInCommon() throws Exception { List resources = new ArrayList(); @@ -221,8 +250,24 @@ public class CollectionToCollectionConverterTests { assertEquals(resources, conversionService.convert(resources, sourceType, new TypeDescriptor(getClass().getField("resources")))); } + + public ArrayList scalarListTarget; + + public List emptyListTarget; + + public LinkedList emptyListDifferentTarget; + + public List>> objectToCollection; + + public List strings; + + public List list = Collections.emptyList(); + + public Collection wildCardCollection = Collections.emptyList(); + public List resources; + public static abstract class BaseResource implements Resource { @Override @@ -286,39 +331,8 @@ public class CollectionToCollectionConverterTests { } } - public static class TestResource extends BaseResource { - - } - - @Test - public void convertEmptyVector_shouldReturnEmptyArrayList() { - Vector vector = new Vector(); - vector.add("Element"); - testCollectionConversionToArrayList(vector); - } - @Test - public void convertNonEmptyVector_shouldReturnNonEmptyArrayList() { - Vector vector = new Vector(); - vector.add("Element"); - testCollectionConversionToArrayList(vector); - } - - @Test - public void testCollectionsEmptyList() throws Exception { - CollectionToCollectionConverter converter = new CollectionToCollectionConverter(new GenericConversionService()); - TypeDescriptor type = new TypeDescriptor(getClass().getField("list")); - converter.convert(list, type, TypeDescriptor.valueOf(Class.forName("java.util.Collections$EmptyList"))); - } - - public List list = Collections.emptyList(); - - @SuppressWarnings("rawtypes") - private void testCollectionConversionToArrayList(Collection aSource) { - Object myConverted = (new CollectionToCollectionConverter(new GenericConversionService())).convert( - aSource, TypeDescriptor.forObject(aSource), TypeDescriptor.forObject(new ArrayList())); - assertTrue(myConverted instanceof ArrayList); - assertEquals(aSource.size(), ((ArrayList) myConverted).size()); + public static class TestResource extends BaseResource { } }