code review

spring-binding/src/main/java/org/springframework/binding/convert/ConversionContext.java

@@ -1,30 +0,0 @@
-/*
- * Copyright 2004-2007 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.binding.convert;
-
-/**
- * A context object with two main responsibities:
- * <ol>
- * <li>Exposing information to a converter to influence a type conversion attempt.
- * <li>Providing operations for recording progress or errors during the type conversion process.
- * </ol>
- * Empty for now; subclasses may define their own custom context behavior accessible by a converter with a downcast.
- * 
- * @author Keith Donald
- */
-public interface ConversionContext {
-
-}

spring-binding/src/main/java/org/springframework/binding/convert/converters/AbstractConverter.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.converters;
 
 import org.springframework.binding.convert.ConversionException;
 import org.springframework.binding.convert.Converter;

spring-binding/src/main/java/org/springframework/binding/convert/converters/AbstractFormattingConverter.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.converters;
 
 import org.springframework.binding.format.FormatterRegistry;
 import org.springframework.util.Assert;

spring-binding/src/main/java/org/springframework/binding/convert/converters/TextToBoolean.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.converters;
 
 import org.springframework.util.StringUtils;
 

spring-binding/src/main/java/org/springframework/binding/convert/converters/TextToClass.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.converters;
 
 import java.math.BigDecimal;
 import java.math.BigInteger;

spring-binding/src/main/java/org/springframework/binding/convert/converters/TextToDate.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.converters;
 
 import java.util.Date;
 

spring-binding/src/main/java/org/springframework/binding/convert/converters/TextToLabeledEnum.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.converters;
 
 import org.springframework.core.enums.LabeledEnum;
 import org.springframework.core.enums.LabeledEnumResolver;

spring-binding/src/main/java/org/springframework/binding/convert/converters/TextToNumber.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.converters;
 
 import java.math.BigDecimal;
 import java.math.BigInteger;

spring-binding/src/main/java/org/springframework/binding/convert/service/DefaultConversionService.java

@@ -13,8 +13,14 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.service;
 
+import org.springframework.binding.convert.ConversionService;
+import org.springframework.binding.convert.converters.TextToBoolean;
+import org.springframework.binding.convert.converters.TextToClass;
+import org.springframework.binding.convert.converters.TextToDate;
+import org.springframework.binding.convert.converters.TextToLabeledEnum;
+import org.springframework.binding.convert.converters.TextToNumber;
 import org.springframework.binding.format.FormatterRegistry;
 import org.springframework.binding.format.factories.DateFormatterFactory;
 import org.springframework.binding.format.factories.NumberFormatterFactory;
@@ -28,6 +34,11 @@ import org.springframework.binding.format.impl.FormatterRegistryImpl;
  */
 public class DefaultConversionService extends GenericConversionService {
 
+	/**
+	 * A singleton shared instance. Should never be modified.
+	 */
+	private static final DefaultConversionService SHARED_INSTANCE = new DefaultConversionService();
+
 	/**
 	 * Returns the formatter registry used by this conversion service
 	 */
@@ -58,4 +69,11 @@ public class DefaultConversionService extends GenericConversionService {
 		registry.registerFormatter(new DateFormatterFactory());
 		return registry;
 	}
+
+	/**
+	 * Returns the shared {@link DefaultConversionService} instance.
+	 */
+	public static ConversionService getSharedInstance() {
+		return SHARED_INSTANCE;
+	}
 }

spring-binding/src/main/java/org/springframework/binding/convert/service/GenericConversionService.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.service;
 
 import java.util.Collections;
 import java.util.HashMap;
@@ -89,13 +89,13 @@ public class GenericConversionService implements ConversionService {
 		sourceClass = convertToWrapperClassIfNecessary(sourceClass);
 		targetClass = convertToWrapperClassIfNecessary(targetClass);
 		if (targetClass.isAssignableFrom(sourceClass)) {
-			return new ConversionExecutorImpl(sourceClass, targetClass, new NoOpConverter(sourceClass, targetClass));
+			return new StaticConversionExecutor(sourceClass, targetClass, new NoOpConverter(sourceClass, targetClass));
 		}
 		Map sourceTargetConverters = findConvertersForSource(sourceClass);
 		Converter converter = findTargetConverter(sourceTargetConverters, targetClass);
 		if (converter != null) {
 			// we found a converter
-			return new ConversionExecutorImpl(sourceClass, targetClass, converter);
+			return new StaticConversionExecutor(sourceClass, targetClass, converter);
 		} else {
 			if (parent != null) {
 				// try the parent

spring-binding/src/main/java/org/springframework/binding/convert/service/NoOpConverter.java

@@ -13,7 +13,9 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.service;
+
+import org.springframework.binding.convert.converters.AbstractConverter;
 
 
 /**

spring-binding/src/main/java/org/springframework/binding/convert/service/RuntimeBindingConversionExecutor.java

@@ -1,4 +1,4 @@
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.service;
 
 import org.springframework.binding.convert.ConversionException;
 import org.springframework.binding.convert.ConversionExecutor;

spring-binding/src/main/java/org/springframework/binding/convert/service/StaticConversionExecutor.java

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.binding.convert.support;
+package org.springframework.binding.convert.service;
 
 import org.springframework.binding.convert.ConversionException;
 import org.springframework.binding.convert.ConversionExecutor;
@@ -30,7 +30,7 @@ import org.springframework.util.Assert;
  * 
  * @author Keith Donald
  */
-class ConversionExecutorImpl implements ConversionExecutor {
+class StaticConversionExecutor implements ConversionExecutor {
 
 	/**
 	 * The source value type this executor will attempt to convert from.
@@ -53,7 +53,7 @@ class ConversionExecutorImpl implements ConversionExecutor {
 	 * @param targetClass the target type that the converter will convert to
 	 * @param converter the converter that will perform the conversion
 	 */
-	public ConversionExecutorImpl(Class sourceClass, Class targetClass, Converter converter) {
+	public StaticConversionExecutor(Class sourceClass, Class targetClass, Converter converter) {
 		Assert.notNull(sourceClass, "The source class is required");
 		Assert.notNull(targetClass, "The target class is required");
 		Assert.notNull(converter, "The converter is required");
@@ -103,10 +103,10 @@ class ConversionExecutorImpl implements ConversionExecutor {
 	}
 
 	public boolean equals(Object o) {
-		if (!(o instanceof ConversionExecutorImpl)) {
+		if (!(o instanceof StaticConversionExecutor)) {
 			return false;
 		}
-		ConversionExecutorImpl other = (ConversionExecutorImpl) o;
+		StaticConversionExecutor other = (StaticConversionExecutor) o;
 		return sourceClass.equals(other.sourceClass) && targetClass.equals(other.targetClass);
 	}
 

spring-binding/src/main/java/org/springframework/binding/expression/el/DefaultExpressionFactoryUtils.java

@@ -2,8 +2,6 @@ package org.springframework.binding.expression.el;
 
 import javax.el.ExpressionFactory;
 
-import org.springframework.beans.BeanInstantiationException;
-import org.springframework.beans.BeanUtils;
 import org.springframework.util.ClassUtils;
 
 /**
@@ -26,14 +24,12 @@ public class DefaultExpressionFactoryUtils {
 
 	/**
 	 * Creates a new instance of the expression factory configured for this VM.
-	 * @throws IllegalStateException if the ExpressionFactory class cannot be found
-	 * @throws RuntimeException if the ExpressionFactory cannot be instantiated
+	 * @throws IllegalStateException if the ExpressionFactory class cannot be instantiated
 	 */
-	public static ExpressionFactory createExpressionFactory() throws IllegalStateException, RuntimeException {
-		// Fallback in the case of using an older version of the EL API
+	public static ExpressionFactory createExpressionFactory() throws IllegalStateException {
+		Class expressionFactoryClass;
 		try {
-			Class expressionFactoryClass = ClassUtils.forName(getDefaultExpressionFactoryClassName());
-			return (ExpressionFactory) BeanUtils.instantiateClass(expressionFactoryClass);
+			expressionFactoryClass = ClassUtils.forName(getDefaultExpressionFactoryClassName());
 		} catch (ClassNotFoundException e) {
 			throw new IllegalStateException(
 					"The default ExpressionFactory class '"
@@ -46,10 +42,15 @@ public class DefaultExpressionFactoryUtils {
 							+ getDefaultExpressionFactoryClassName()
 							+ "' could not be found in the classpath.  "
 							+ "Please add this to your classpath or set the default ExpressionFactory class name to something that is in the classpath.");
-		} catch (BeanInstantiationException e) {
-			throw new RuntimeException("An instance of the default ExpressionFactory '"
+		}
+		try {
+			return (ExpressionFactory) expressionFactoryClass.newInstance();
+		} catch (Exception e) {
+			IllegalStateException iae = new IllegalStateException("An instance of the default ExpressionFactory '"
 					+ getDefaultExpressionFactoryClassName()
-					+ "' could not be instantiated.  Check your EL implementation configuration.", e);
+					+ "' could not be instantiated.  Check your EL implementation configuration.");
+			iae.initCause(e);
+			throw iae;
 		}
 	}
 }

spring-binding/src/main/java/org/springframework/binding/mapping/MappingResult.java

@@ -38,5 +38,4 @@ public class MappingResult {
 	public String toString() {
 		return new ToStringCreator(this).append("mapping", mapping).append("result", result).toString();
 	}
-
 }

spring-binding/src/main/java/org/springframework/binding/mapping/impl/DefaultMapping.java

@@ -15,15 +15,12 @@
  */
 package org.springframework.binding.mapping.impl;
 
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
 import org.springframework.binding.convert.ConversionException;
 import org.springframework.binding.convert.ConversionExecutor;
 import org.springframework.binding.convert.ConversionService;
 import org.springframework.binding.expression.EvaluationException;
 import org.springframework.binding.expression.Expression;
 import org.springframework.binding.mapping.Mapping;
-import org.springframework.core.style.StylerUtils;
 import org.springframework.util.Assert;
 
 /**
@@ -34,8 +31,6 @@ import org.springframework.util.Assert;
  */
 public class DefaultMapping implements Mapping {
 
-	private static final Log logger = LogFactory.getLog(DefaultMapping.class);
-
 	/**
 	 * The source expression to evaluate against a source object to map from.
 	 */
@@ -94,7 +89,6 @@ public class DefaultMapping implements Mapping {
 	/**
 	 * Sets a specific type conversion executor to use during mapping execution.
 	 * @param typeConverter the type converter
-	 * @see #map(MappingContext)
 	 */
 	public void setTypeConverter(ConversionExecutor typeConverter) {
 		this.typeConverter = typeConverter;
@@ -112,7 +106,7 @@ public class DefaultMapping implements Mapping {
 	 * Execute this mapping.
 	 * @param context the mapping context
 	 */
-	public void map(MappingContext context) {
+	void map(MappingContext context) {
 		context.setCurrentMapping(this);
 		Object sourceValue;
 		try {
@@ -159,13 +153,6 @@ public class DefaultMapping implements Mapping {
 		}
 		try {
 			targetExpression.setValue(context.getTarget(), targetValue);
-			if (logger.isDebugEnabled()) {
-				String sourceType = sourceValue != null ? sourceValue.getClass().getName() : "null";
-				String targetType = targetValue != null ? targetValue.getClass().getName() : "null";
-				logger.debug("Sucessfully mapped source [" + sourceType + "] " + sourceExpression + " value "
-						+ StylerUtils.style(sourceValue) + " to target [" + targetType + "] " + targetExpression
-						+ " value " + StylerUtils.style(targetValue));
-			}
 			context.setSuccessResult(sourceValue, targetValue);
 		} catch (EvaluationException e) {
 			context.setTargetAccessError(sourceValue, e);
@@ -195,5 +182,4 @@ public class DefaultMapping implements Mapping {
 	public String toString() {
 		return sourceExpression + " -> " + targetExpression;
 	}
-
 }

spring-binding/src/main/java/org/springframework/binding/mapping/impl/DefaultMappingContext.java

@@ -3,6 +3,8 @@ package org.springframework.binding.mapping.impl;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.binding.convert.ConversionService;
 import org.springframework.binding.expression.EvaluationException;
 import org.springframework.binding.mapping.Mapping;
@@ -20,6 +22,8 @@ import org.springframework.binding.mapping.results.TypeConversionError;
  */
 class DefaultMappingContext implements MappingContext {
 
+	private static final Log logger = LogFactory.getLog(DefaultMapping.class);
+
 	private Object source;
 
 	private Object target;
@@ -61,27 +65,30 @@ class DefaultMappingContext implements MappingContext {
 	}
 
 	public void setSuccessResult(Object originalValue, Object mappedValue) {
-		mappingResults.add(new MappingResult(currentMapping, new Success(mappedValue, originalValue)));
-		currentMapping = null;
+		add(new MappingResult(currentMapping, new Success(mappedValue, originalValue)));
 	}
 
 	public void setRequiredErrorResult(Object originalValue) {
-		mappingResults.add(new MappingResult(currentMapping, new RequiredError(originalValue)));
-		this.currentMapping = null;
+		add(new MappingResult(currentMapping, new RequiredError(originalValue)));
 	}
 
 	public void setTypeConversionErrorResult(Object originalValue, Class targetType) {
-		mappingResults.add(new MappingResult(currentMapping, new TypeConversionError(originalValue, targetType)));
-		this.currentMapping = null;
+		add(new MappingResult(currentMapping, new TypeConversionError(originalValue, targetType)));
 	}
 
 	public void setSourceAccessError(EvaluationException error) {
-		mappingResults.add(new MappingResult(currentMapping, new SourceAccessError(error)));
-		this.currentMapping = null;
+		add(new MappingResult(currentMapping, new SourceAccessError(error)));
 	}
 
 	public void setTargetAccessError(Object originalValue, EvaluationException error) {
-		mappingResults.add(new MappingResult(currentMapping, new TargetAccessError(originalValue, error)));
+		add(new MappingResult(currentMapping, new TargetAccessError(originalValue, error)));
+	}
+
+	private void add(MappingResult result) {
+		if (logger.isDebugEnabled()) {
+			logger.debug("Adding " + result);
+		}
+		this.mappingResults.add(result);
 		this.currentMapping = null;
 	}
 

spring-binding/src/main/java/org/springframework/binding/mapping/results/SourceAccessError.java

@@ -45,12 +45,12 @@ public class SourceAccessError extends Result {
 		if (error instanceof PropertyNotFoundException) {
 			return "propertyNotFound";
 		} else {
-			return "sourceAccess";
+			return "evaluationException";
 		}
 	}
 
 	public String toString() {
-		return new ToStringCreator(this).append("error", error).toString();
+		return new ToStringCreator(this).append("errorCode", getErrorCode()).append("details", error.getMessage())
+				.toString();
 	}
-
 }

spring-binding/src/main/java/org/springframework/binding/mapping/results/Success.java

@@ -40,7 +40,7 @@ public class Success extends Result {
 	}
 
 	public String toString() {
-		return new ToStringCreator(this).append("mappedValue", mappedValue).append("originalValue", originalValue)
+		return new ToStringCreator(this).append("originalValue", originalValue).append("mappedValue", mappedValue)
 				.toString();
 	}
 }

spring-binding/src/main/java/org/springframework/binding/mapping/results/TargetAccessError.java

@@ -49,11 +49,12 @@ public class TargetAccessError extends Result {
 		if (error instanceof PropertyNotFoundException) {
 			return "propertyNotFound";
 		} else {
-			return "targetAccess";
+			return "evaluationException";
 		}
 	}
 
 	public String toString() {
-		return new ToStringCreator(this).append("error", error).toString();
+		return new ToStringCreator(this).append("errorCode", getErrorCode()).append("details", error.getMessage())
+				.toString();
 	}
 }

spring-binding/src/main/java/org/springframework/binding/method/MethodInvoker.java

@@ -21,7 +21,7 @@ import java.lang.reflect.Method;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.binding.convert.ConversionService;
-import org.springframework.binding.convert.support.DefaultConversionService;
+import org.springframework.binding.convert.service.DefaultConversionService;
 import org.springframework.core.style.StylerUtils;
 import org.springframework.util.CachingMapDecorator;
 
@@ -39,7 +39,7 @@ public class MethodInvoker {
 	/**
 	 * Conversion service for converting arguments to the necessary type if required.
 	 */
-	private ConversionService conversionService = new DefaultConversionService();
+	private ConversionService conversionService = DefaultConversionService.getSharedInstance();
 
 	/**
 	 * A cache of invoked bean methods, keyed weakly.