added conversion service performance optimizations; added mapping cyclical ref handling; removed ConverterInfo in favor of specifying S and T at registration time if necessary

git-svn-id: https://src.springframework.org/svn/spring-framework/trunk@2063 50f2f4bb-b051-0410-bef5-90022cba6387

org.springframework.context/src/main/java/org/springframework/mapping/support/DefaultMapperTargetFactory.java

@@ -17,6 +17,7 @@ package org.springframework.mapping.support;
 
 import org.springframework.beans.BeanUtils;
 import org.springframework.core.convert.TypeDescriptor;
+import org.springframework.util.ClassUtils;
 
 /**
  * Creates a mapping target by calling its default constructor.
@@ -25,7 +26,11 @@ import org.springframework.core.convert.TypeDescriptor;
  */
 class DefaultMapperTargetFactory implements MapperTargetFactory {
 
-	public Object createTarget(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
+	public boolean supports(TypeDescriptor targetType) {
+		return ClassUtils.hasConstructor(targetType.getType(), null);
+	}
+
+	public Object createTarget(TypeDescriptor targetType) {
 		return BeanUtils.instantiate(targetType.getType());
 	}
 

org.springframework.context/src/main/java/org/springframework/mapping/support/MapperConverter.java

@@ -15,6 +15,8 @@
  */
 package org.springframework.mapping.support;
 
+import org.springframework.beans.BeanUtils;
+import org.springframework.core.convert.ConversionFailedException;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.support.GenericConverter;
 import org.springframework.mapping.Mapper;
@@ -44,9 +46,33 @@ public class MapperConverter implements GenericConverter {
 		if (source == null) {
 			return null;
 		}
-		// TODO - could detect cyclical reference here if had a mapping context? (source should not equal currently mapped object) 
-		Object target = this.mappingTargetFactory.createTarget(source, sourceType, targetType);
-		return this.mapper.map(source, target);
+		if (SpelMappingContextHolder.contains(source)) {
+			return source;
+		}
+		if (sourceType.isAssignableTo(targetType) && isCopyByReference(sourceType, targetType)) {
+			return source;
+		}
+		return createAndMap(targetType, source, sourceType);
+	}
+
+	private boolean isCopyByReference(TypeDescriptor sourceType, TypeDescriptor targetType) {
+		if (BeanUtils.isSimpleValueType(targetType.getType()) || Enum.class.isAssignableFrom(targetType.getType())) {
+			return true;
+		} else {
+			return false;
+		}
 	}
 
+	private Object createAndMap(TypeDescriptor targetType, Object source, TypeDescriptor sourceType) {
+		if (this.mappingTargetFactory.supports(targetType)) {
+			Object target = this.mappingTargetFactory.createTarget(targetType);
+			return this.mapper.map(source, target);
+		} else {
+			IllegalStateException cause = new IllegalStateException("["
+					+ this.mappingTargetFactory.getClass().getName() + "] does not support target type ["
+					+ targetType.getName() + "]");
+			throw new ConversionFailedException(sourceType, targetType, source, cause);
+		}
+
+	}
 }

org.springframework.context/src/main/java/org/springframework/mapping/support/MapperTargetFactory.java

@@ -27,6 +27,13 @@ import org.springframework.mapping.Mapper;
  */
 public interface MapperTargetFactory {
 
+	/**
+	 * Does this factory support creating mapping targets of the specified type
+	 * @param targetType the targe type
+	 * @return true if so, false otherwise
+	 */
+	public boolean supports(TypeDescriptor targetType);
+
 	/**
 	 * Create the target object to be mapped to.
 	 * @param source the source object to map from
@@ -34,5 +41,6 @@ public interface MapperTargetFactory {
 	 * @param targetType the target object type descriptor
 	 * @return the target
 	 */
-	public Object createTarget(Object source, TypeDescriptor sourceType, TypeDescriptor targetType);
+	public Object createTarget(TypeDescriptor targetType);
+
 }

org.springframework.context/src/main/java/org/springframework/mapping/support/SpelMapper.java

@@ -22,6 +22,8 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.converter.ConverterRegistry;
 import org.springframework.core.convert.support.DefaultConversionService;
@@ -45,6 +47,8 @@ import org.springframework.mapping.MappingFailure;
  */
 public class SpelMapper implements Mapper<Object, Object> {
 
+	private static final Log logger = LogFactory.getLog(SpelMapper.class);
+
 	private static final MappableTypeFactory mappableTypeFactory = new MappableTypeFactory();
 
 	private static final SpelExpressionParser sourceExpressionParser = new SpelExpressionParser();
@@ -92,26 +96,39 @@ public class SpelMapper implements Mapper<Object, Object> {
 	}
 
 	public Object map(Object source, Object target) {
-		EvaluationContext sourceContext = getMappingContext(source);
-		EvaluationContext targetContext = getMappingContext(target);
-		List<MappingFailure> failures = new LinkedList<MappingFailure>();
-		for (SpelMapping mapping : this.mappings) {
-			mapping.map(sourceContext, targetContext, failures);
-		}
-		Set<SpelMapping> autoMappings = getAutoMappings(sourceContext, targetContext);
-		for (SpelMapping mapping : autoMappings) {
-			mapping.map(sourceContext, targetContext, failures);
-		}
-		if (!failures.isEmpty()) {
-			throw new MappingException(failures);
+		try {
+			SpelMappingContextHolder.push(source);
+			EvaluationContext sourceContext = getEvaluationContext(source);
+			EvaluationContext targetContext = getEvaluationContext(target);
+			List<MappingFailure> failures = new LinkedList<MappingFailure>();
+			for (SpelMapping mapping : this.mappings) {
+				doMap(mapping, sourceContext, targetContext, failures);
+			}
+			Set<SpelMapping> autoMappings = getAutoMappings(sourceContext, targetContext);
+			for (SpelMapping mapping : autoMappings) {
+				doMap(mapping, sourceContext, targetContext, failures);
+			}
+			if (!failures.isEmpty()) {
+				throw new MappingException(failures);
+			}
+			return target;
+		} finally {
+			SpelMappingContextHolder.pop();
 		}
-		return target;
 	}
 
-	private EvaluationContext getMappingContext(Object object) {
+	private EvaluationContext getEvaluationContext(Object object) {
 		return mappableTypeFactory.getMappableType(object).getEvaluationContext(object, this.conversionService);
 	}
 
+	private void doMap(SpelMapping mapping, EvaluationContext sourceContext, EvaluationContext targetContext,
+			List<MappingFailure> failures) {
+		if (logger.isDebugEnabled()) {
+			logger.debug(SpelMappingContextHolder.getLevel() + mapping);
+		}
+		mapping.map(sourceContext, targetContext, failures);
+	}
+
 	private Set<SpelMapping> getAutoMappings(EvaluationContext sourceContext, EvaluationContext targetContext) {
 		if (this.autoMappingEnabled) {
 			Set<SpelMapping> autoMappings = new LinkedHashSet<SpelMapping>();

org.springframework.context/src/main/java/org/springframework/mapping/support/SpelMappingContextHolder.java

@@ -0,0 +1,49 @@
+package org.springframework.mapping.support;
+
+import java.util.Stack;
+
+import org.springframework.core.NamedThreadLocal;
+
+class SpelMappingContextHolder {
+
+	private static final ThreadLocal<Stack<Object>> mappingContextHolder = new NamedThreadLocal<Stack<Object>>(
+			"Mapping context");
+
+	public static void push(Object source) {
+		Stack<Object> context = getContext();
+		if (context == null) {
+			context = new Stack<Object>();
+			mappingContextHolder.set(context);
+		}
+		context.add(source);
+	}
+
+	public static boolean contains(Object source) {
+		return getContext().contains(source);
+	}
+
+	public static void pop() {
+		Stack<Object> context = getContext();
+		if (context != null) {
+			context.pop();
+			if (context.isEmpty()) {
+				mappingContextHolder.set(null);
+			}
+		}
+	}
+
+	public static String getLevel() {
+		int size = getContext().size();
+		StringBuilder builder = new StringBuilder();
+		for (int i = 0; i < size; i++) {
+			builder.append("*");
+		}
+		builder.append(" ");
+		return builder.toString();
+	}
+
+	private static Stack<Object> getContext() {
+		return mappingContextHolder.get();
+	}
+
+}

org.springframework.context/src/test/java/org/springframework/mapping/support/SpelMapperTests.java

@@ -2,13 +2,14 @@ package org.springframework.mapping.support;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
+import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-import org.junit.Ignore;
 import org.junit.Test;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.mapping.MappingException;
@@ -237,9 +238,8 @@ public class SpelMapperTests {
 			assertEquals(1, e.getMappingFailureCount());
 		}
 	}
-	
+
 	@Test
-	@Ignore
 	public void mapCyclic() {
 		Person source = new Person();
 		source.setName("Keith");
@@ -254,6 +254,72 @@ public class SpelMapperTests {
 		assertEquals(source.cyclic, target.cyclic);
 	}
 
+	@Test
+	public void mapCyclicTypicalHibernateDomainModel() {
+		Order source = new Order();
+		source.setNumber(1);
+		LineItem item = new LineItem();
+		item.setAmount(new BigDecimal("30.00"));
+		item.setOrder(source);
+		source.setLineItem(item);
+
+		Order target = new Order();
+
+		mapper.map(source, target);
+		assertEquals(1, target.getNumber());
+		assertTrue(item != target.getLineItem());
+		assertEquals(new BigDecimal("30.00"), target.getLineItem().getAmount());
+		assertEquals(source, target.getLineItem().getOrder());
+	}
+
+	public static class Order {
+
+		private int number;
+
+		private LineItem lineItem;
+
+		public int getNumber() {
+			return number;
+		}
+
+		public void setNumber(int number) {
+			this.number = number;
+		}
+
+		public LineItem getLineItem() {
+			return lineItem;
+		}
+
+		public void setLineItem(LineItem lineItem) {
+			this.lineItem = lineItem;
+		}
+
+	}
+
+	public static class LineItem {
+
+		private BigDecimal amount;
+
+		private Order order;
+
+		public BigDecimal getAmount() {
+			return amount;
+		}
+
+		public void setAmount(BigDecimal amount) {
+			this.amount = amount;
+		}
+
+		public Order getOrder() {
+			return order;
+		}
+
+		public void setOrder(Order order) {
+			this.order = order;
+		}
+
+	}
+
 	public static class PersonDto {
 
 		private String fullName;

org.springframework.context/src/test/resources/log4j.xml

@@ -11,11 +11,7 @@
 		</layout>
 	</appender>
 
-	<logger name="org.springframework.beans">
-		<level value="warn" />
-	</logger>
-
-	<logger name="org.springframework.binding">
+	<logger name="org.springframework.mapping">
 		<level value="debug" />
 	</logger>
 

org.springframework.core/src/main/java/org/springframework/core/convert/converter/ConverterInfo.java

@@ -1,41 +0,0 @@
-/*
- * Copyright 2002-2009 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.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.springframework.core.convert.converter;
-
-/**
- * A meta interface a Converter may implement to describe what types he can convert between.
- *
- * Implementing this interface is required when registering converters that do not declare their
- * parameterized types S and T with a {@link org.springframework.core.convert.ConversionService}.
- *
- * @author Keith Donald
- * @since 3.0
- * @see Converter
- */
-public interface ConverterInfo {
-
-	/**
-	 * The source type the converter converts from.
-	 */
-	Class<?> getSourceType();
-
-	/**
-	 * The target type the converter converts to.
-	 */
-	Class<?> getTargetType();
-
-}

org.springframework.core/src/main/java/org/springframework/core/convert/support/DefaultConversionService.java

@@ -16,6 +16,7 @@
 
 package org.springframework.core.convert.support;
 
+import java.util.Locale;
 
 /**
  * Default implementation of a conversion service. Will automatically register <i>from string</i>
@@ -31,15 +32,15 @@ public class DefaultConversionService extends GenericConversionService {
 	 * Create a new default conversion service, installing the default converters.
 	 */
 	public DefaultConversionService() {
-		addConverter(new StringToBooleanConverter());
-		addConverter(new StringToCharacterConverter());
-		addConverter(new StringToLocaleConverter());
-		addConverter(new NumberToCharacterConverter());
-		addConverter(new ObjectToStringConverter());
-		addConverterFactory(new StringToNumberConverterFactory());
-		addConverterFactory(new StringToEnumConverterFactory());
-		addConverterFactory(new NumberToNumberConverterFactory());
-		addConverterFactory(new CharacterToNumberFactory());
+		addConverter(String.class, Boolean.class, new StringToBooleanConverter());
+		addConverter(String.class, Character.class, new StringToCharacterConverter());
+		addConverter(String.class, Locale.class, new StringToLocaleConverter());
+		addConverter(Number.class, Character.class, new NumberToCharacterConverter());
+		addConverter(Object.class, String.class, new ObjectToStringConverter());
+		addConverterFactory(String.class, Number.class, new StringToNumberConverterFactory());
+		addConverterFactory(String.class, Enum.class, new StringToEnumConverterFactory());
+		addConverterFactory(Number.class, Number.class, new NumberToNumberConverterFactory());
+		addConverterFactory(Character.class, Number.class, new CharacterToNumberFactory());
 	}
 
 }

org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java

@@ -33,7 +33,6 @@ import org.springframework.core.convert.ConverterNotFoundException;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.core.convert.converter.ConverterFactory;
-import org.springframework.core.convert.converter.ConverterInfo;
 import org.springframework.core.convert.converter.ConverterRegistry;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
@@ -54,7 +53,7 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		}
 	};
 
-	private final Map<Class, Map<Class, GenericConverter>> sourceTypeConverters = new HashMap<Class, Map<Class, GenericConverter>>();
+	private final Map<Class, Map<Class, GenericConverter>> sourceTypeConverters = new HashMap<Class, Map<Class, GenericConverter>>(36);
 
 	private ConversionService parent;
 
@@ -132,7 +131,7 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		}
 		Class sourceType = typeInfo[0];
 		Class targetType = typeInfo[1];
-		getSourceMap(sourceType).put(targetType, new ConverterAdapter(converter));
+		addConverter(sourceType, targetType, converter);
 	}
 
 	public void addConverterFactory(ConverterFactory<?, ?> converterFactory) {
@@ -143,7 +142,7 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		}
 		Class sourceType = typeInfo[0];
 		Class targetType = typeInfo[1];
-		getSourceMap(sourceType).put(targetType, new ConverterFactoryAdapter(converterFactory));
+		addConverterFactory(sourceType, targetType, converterFactory);
 	}
 
 	public void removeConvertible(Class<?> sourceType, Class<?> targetType) {
@@ -196,6 +195,28 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		getSourceMap(sourceType).put(targetType, converter);
 	}
 
+	/**
+	 * Registers a Converter with the sourceType and targetType to index on specified explicitly.
+	 * This method performs better than {@link #addConverter(Converter)} because there parameterized types S and T don't have to be discovered.
+	 * @param sourceType the source type to convert from
+	 * @param targetType the target type to convert to
+	 * @param converter the converter.
+	 */
+	protected void addConverter(Class<?> sourceType, Class<?> targetType, Converter<?, ?> converter) {
+		addGenericConverter(sourceType, targetType, new ConverterAdapter(converter));
+	}
+
+	/**
+	 * Registers a ConverterFactory with the sourceType and targetType to index on specified explicitly.
+	 * This method performs better than {@link #addConverter(ConverterFactory)} because there parameterized types S and T don't have to be discovered.
+	 * @param sourceType the source type to convert from
+	 * @param targetType the target type to convert to
+	 * @param converter the converter.factory
+	 */
+	protected void addConverterFactory(Class<?> sourceType, Class<?> targetType, ConverterFactory<?, ?> converterFactory) {
+		addGenericConverter(sourceType, targetType, new ConverterFactoryAdapter(converterFactory));
+	}
+
 	/**
 	 * Hook method to convert a null source.
 	 * Default implementation returns <code>null</code>.
@@ -259,15 +280,7 @@ public class GenericConversionService implements ConversionService, ConverterReg
 	}
 
 	private Class[] getRequiredTypeInfo(Object converter, Class genericIfc) {
-		Class[] typeInfo = new Class[2];
-		if (converter instanceof ConverterInfo) {
-			ConverterInfo info = (ConverterInfo) converter;
-			typeInfo[0] = info.getSourceType();
-			typeInfo[1] = info.getTargetType();
-			return typeInfo;
-		} else {
-			return GenericTypeResolver.resolveTypeArguments(converter.getClass(), genericIfc);
-		}
+		return GenericTypeResolver.resolveTypeArguments(converter.getClass(), genericIfc);
 	}
 
 	private GenericConverter findConverterByClassPair(Class sourceType, Class targetType) {

org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConverter.java

@@ -37,7 +37,7 @@ public interface GenericConverter {
 
 	/**
 	 * Convert the source to the targetType described by the TypeDescriptor.
-	 * @param source the source object to convert (never <code>null</code>)
+	 * @param source the source object to convert (may be null)
 	 * @param sourceType context about the source type to convert from
 	 * @param targetType context about the target type to convert to
 	 * @return the converted object

org.springframework.core/src/main/java/org/springframework/core/convert/support/NumberToNumberConverterFactory.java

@@ -47,11 +47,11 @@ final class NumberToNumberConverterFactory implements ConverterFactory<Number, N
 	private final static class NumberToNumber<T extends Number> implements Converter<Number, T> {
 
 		private final Class<T> targetType;
-		
+
 		public NumberToNumber(Class<T> targetType) {
 			this.targetType = targetType;
 		}
-	
+
 		public T convert(Number source) {
 			return NumberUtils.convertNumberToTargetClass(source, this.targetType);
 		}

org.springframework.core/src/main/java/org/springframework/core/convert/support/ObjectToStringConverter.java

@@ -28,6 +28,14 @@ import org.springframework.core.convert.converter.Converter;
  */
 final class ObjectToStringConverter implements Converter<Object, String> {
 
+	public Class<?> getSourceType() {
+		return Object.class;
+	}
+
+	public Class<?> getTargetType() {
+		return String.class;
+	}
+	
 	public String convert(Object source) {
 		return source.toString();
 	}

org.springframework.core/src/main/java/org/springframework/core/convert/support/StringToBooleanConverter.java

@@ -48,8 +48,7 @@ final class StringToBooleanConverter implements Converter<String, Boolean> {
 		trueValues.add("1");
 		falseValues.add("0");
 	}
-
-
+	
 	public Boolean convert(String source) {
 		String value = (source != null ? source.trim() : null);
 		if (!StringUtils.hasLength(value)) {

org.springframework.core/src/main/java/org/springframework/core/convert/support/StringToCharacterConverter.java

@@ -31,7 +31,9 @@ final class StringToCharacterConverter implements Converter<String, Character> {
 			return null;
 		}
 		if (source.length() > 1) {
-			throw new IllegalArgumentException("Can only convert a [String] with length of 1 to a [Character]; string value '" + source + "'  has length of " + source.length());
+			throw new IllegalArgumentException(
+					"Can only convert a [String] with length of 1 to a [Character]; string value '" + source
+							+ "'  has length of " + source.length());
 		}
 		return source.charAt(0);
 	}

org.springframework.core/src/main/java/org/springframework/core/convert/support/StringToEnumConverterFactory.java

@@ -35,7 +35,7 @@ final class StringToEnumConverterFactory implements ConverterFactory<String, Enu
 	private class StringToEnum<T extends Enum> implements Converter<String, T> {
 
 		private final Class<T> enumType;
-		
+
 		public StringToEnum(Class<T> enumType) {
 			this.enumType = enumType;
 		}

org.springframework.core/src/main/java/org/springframework/core/convert/support/StringToNumberConverterFactory.java

@@ -47,11 +47,11 @@ final class StringToNumberConverterFactory implements ConverterFactory<String, N
 	private static final class StringToNumber<T extends Number> implements Converter<String, T> {
 
 		private final Class<T> targetType;
-		
+
 		public StringToNumber(Class<T> targetType) {
 			this.targetType = targetType;
 		}
-	
+
 		public T convert(String source) {
 			if ("".equals(source)) {
 				return null;