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

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);
+		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();
 		}
-		Set<SpelMapping> autoMappings = getAutoMappings(sourceContext, targetContext);
-		for (SpelMapping mapping : autoMappings) {
-			mapping.map(sourceContext, targetContext, failures);
-		}
-		if (!failures.isEmpty()) {
-			throw new MappingException(failures);
-		}
-		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>