added null binding check for primitives for all conversion results; polishing

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

org.springframework.context/src/main/java/org/springframework/format/support/FormattingConversionService.java

@@ -36,6 +36,7 @@ import org.springframework.format.Formatter;
 import org.springframework.format.FormatterRegistry;
 import org.springframework.format.Parser;
 import org.springframework.format.Printer;
+import org.springframework.util.StringUtils;
 import org.springframework.util.StringValueResolver;
 
 /**
@@ -124,10 +125,13 @@ public class FormattingConversionService extends GenericConversionService
 
 		@SuppressWarnings("unchecked")
 		public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
+			if (source == null) {
+				return "";
+			}
 			if (!sourceType.isAssignableTo(this.printerObjectType)) {
 				source = this.conversionService.convert(source, sourceType, this.printerObjectType);
 			}
-			return (source != null ? this.printer.print(source, LocaleContextHolder.getLocale()) : "");
+			return this.printer.print(source, LocaleContextHolder.getLocale());
 		}
 		
 		private Class<?> resolvePrinterObjectType(Printer<?> printer) {
@@ -159,7 +163,7 @@ public class FormattingConversionService extends GenericConversionService
 
 		public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
 			String text = (String) source;
-			if (text == null || text.length() == 0) {
+			if (!StringUtils.hasText(text)) {
 				return null;
 			}
 			Object result;
@@ -169,6 +173,9 @@ public class FormattingConversionService extends GenericConversionService
 			catch (ParseException ex) {
 				throw new IllegalArgumentException("Unable to parse '" + text + "'", ex);
 			}
+			if (result == null) {
+				throw new IllegalStateException("Parsers are not allowed to return null");
+			}
 			TypeDescriptor resultType = TypeDescriptor.valueOf(result.getClass());
 			if (!resultType.isAssignableTo(targetType)) {
 				result = this.conversionService.convert(result, resultType, targetType);

org.springframework.context/src/test/java/org/springframework/format/support/FormattingConversionServiceTests.java

@@ -39,9 +39,11 @@ import org.springframework.beans.factory.config.PropertyPlaceholderConfigurer;
 import org.springframework.beans.factory.support.RootBeanDefinition;
 import org.springframework.context.i18n.LocaleContextHolder;
 import org.springframework.context.support.GenericApplicationContext;
+import org.springframework.core.convert.ConversionFailedException;
 import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.core.convert.support.DefaultConversionService;
+import org.springframework.format.Formatter;
 import org.springframework.format.datetime.joda.DateTimeParser;
 import org.springframework.format.datetime.joda.JodaDateTimeFormatAnnotationFormatterFactory;
 import org.springframework.format.datetime.joda.ReadablePartialPrinter;
@@ -203,6 +205,24 @@ public class FormattingConversionServiceTests {
 		assertNull(formattingService.convert("", TypeDescriptor.valueOf(String.class), TypeDescriptor.valueOf(Integer.class)));
 	}
 
+	@Test
+	public void testParseBlankString() throws ParseException {
+		formattingService.addFormatterForFieldType(Number.class, new NumberFormatter());
+		assertNull(formattingService.convert("     ", TypeDescriptor.valueOf(String.class), TypeDescriptor.valueOf(Integer.class)));
+	}
+
+	@Test(expected=ConversionFailedException.class)
+	public void testParseParserReturnsNull() throws ParseException {
+		formattingService.addFormatterForFieldType(Integer.class, new NullReturningFormatter());
+		assertNull(formattingService.convert("1", TypeDescriptor.valueOf(String.class), TypeDescriptor.valueOf(Integer.class)));
+	}
+
+	@Test(expected=ConversionFailedException.class)
+	public void testParseNullPrimitiveProperty() throws ParseException {
+		formattingService.addFormatterForFieldType(Integer.class, new NumberFormatter());
+		assertNull(formattingService.convert(null, TypeDescriptor.valueOf(String.class), TypeDescriptor.valueOf(int.class)));
+	}
+
 	@Test
 	public void testPrintNullDefault() throws ParseException {
 		assertEquals(null, formattingService.convert(null, TypeDescriptor.valueOf(Integer.class), TypeDescriptor.valueOf(String.class)));
@@ -221,11 +241,9 @@ public class FormattingConversionServiceTests {
 
 	public static class Model {
 
-		@SuppressWarnings("unused")
 		@org.springframework.format.annotation.DateTimeFormat(style="S-")
 		public Date date;
 		
-		@SuppressWarnings("unused")
 		@org.springframework.format.annotation.DateTimeFormat(pattern="M-d-yy")
 		public List<Date> dates;
 
@@ -241,11 +259,9 @@ public class FormattingConversionServiceTests {
 
 	public static class ModelWithPlaceholders {
 
-		@SuppressWarnings("unused")
 		@org.springframework.format.annotation.DateTimeFormat(style="${dateStyle}")
 		public Date date;
 
-		@SuppressWarnings("unused")
 		@org.springframework.format.annotation.DateTimeFormat(pattern="${datePattern}")
 		public List<Date> dates;
 
@@ -257,5 +273,17 @@ public class FormattingConversionServiceTests {
 			this.dates = dates;
 		}
 	}
+	
+	public static class NullReturningFormatter implements Formatter<Integer> {
+
+		public String print(Integer object, Locale locale) {
+			return null;
+		}
+
+		public Integer parse(String text, Locale locale) throws ParseException {
+			return null;
+		}
+		
+	}
 
 }

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

@@ -166,6 +166,9 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		if (sourceType == TypeDescriptor.NULL) {
 			Assert.isTrue(source == null, "The value must be null if sourceType == TypeDescriptor.NULL");
 			Object result = convertNullSource(sourceType, targetType);
+			if (result == null) {
+				assertNotPrimitiveTargetType(sourceType, targetType);
+			}
 			if (logger.isDebugEnabled()) {
 				logger.debug("Converted to " + StylerUtils.style(result));
 			}			
@@ -178,15 +181,12 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		Assert.isTrue(source == null || sourceType.getObjectType().isInstance(source));
 		GenericConverter converter = getConverter(sourceType, targetType);
 		if (converter == null) {
-			if (source == null || sourceType.isAssignableTo(targetType)) {
+			return handleConverterNotFound(source, sourceType, targetType);
-				logger.debug("No converter found - returning assignable source object as-is");
-				return source;
-			}
-			else {
-				throw new ConverterNotFoundException(sourceType, targetType);
-			}
 		}
 		Object result = ConversionUtils.invokeConverter(converter, source, sourceType, targetType);
+		if (result == null) {
+			assertNotPrimitiveTargetType(sourceType, targetType);
+		}
 		if (logger.isDebugEnabled()) {
 			logger.debug("Converted to " + StylerUtils.style(result));
 		}
@@ -218,18 +218,12 @@ public class GenericConversionService implements ConversionService, ConverterReg
 	/**
 	 * Template method to convert a null source.
 	 * <p>Default implementation returns <code>null</code>.
-	 * Throws a {@link ConversionFailedException} if the targetType is a primitive type,
-	 * as <code>null</code> cannot be assigned to a primitive type.
 	 * Subclasses may override to return custom null objects for specific target types.
 	 * @param sourceType the sourceType to convert from
 	 * @param targetType the targetType to convert to
 	 * @return the converted null object
 	 */
 	protected Object convertNullSource(TypeDescriptor sourceType, TypeDescriptor targetType) {
-		if (targetType.isPrimitive()) {
-			throw new ConversionFailedException(sourceType, targetType, null,
-					new IllegalArgumentException("A null value cannot be assigned to a primitive type"));
-		}
 		return null;
 	}
 
@@ -462,10 +456,10 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		}
 	}
 
-	private void addInterfaceHierarchy(Class<?> ifc, LinkedList<Class<?>> classQueue) {
+	private void addInterfaceHierarchy(Class<?> interfaceType, LinkedList<Class<?>> classQueue) {
-		classQueue.addFirst(ifc);
+		classQueue.addFirst(interfaceType);
-		for (Class<?> inheritedIfc : ifc.getInterfaces()) {
+		for (Class<?> implementedInterface : interfaceType.getInterfaces()) {
-			addInterfaceHierarchy(inheritedIfc, classQueue);
+			addInterfaceHierarchy(implementedInterface, classQueue);
 		}
 	}
 
@@ -481,6 +475,26 @@ public class GenericConversionService implements ConversionService, ConverterReg
 		return matchable.matchConverter(sourceFieldType, targetFieldType);
 	}
 
+	private Object handleConverterNotFound(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
+		if (source == null) {
+			assertNotPrimitiveTargetType(sourceType, targetType);
+			return source;
+		} else if (sourceType.isAssignableTo(targetType)) {
+			logger.debug("No converter found - returning assignable source object as-is");
+			return source;				
+		}
+		else {
+			throw new ConverterNotFoundException(sourceType, targetType);
+		}		
+	}
+	
+	private void assertNotPrimitiveTargetType(TypeDescriptor sourceType, TypeDescriptor targetType) {
+		if (targetType.isPrimitive()) {
+			throw new ConversionFailedException(sourceType, targetType, null,
+					new IllegalArgumentException("A null value cannot be assigned to a primitive type"));
+		}		
+	}
+	
 	@SuppressWarnings("unchecked")
 	private final class ConverterAdapter implements GenericConverter {
 

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

@@ -51,8 +51,7 @@ final class IdToEntityConverter implements ConditionalGenericConverter {
 
 	public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
 		Method finder = getFinder(targetType.getType());
-		return (finder != null &&
+		return finder != null && this.conversionService.canConvert(sourceType, TypeDescriptor.valueOf(finder.getParameterTypes()[0]));
-				this.conversionService.canConvert(sourceType, TypeDescriptor.valueOf(finder.getParameterTypes()[0])));
 	}
 
 	public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
@@ -60,8 +59,7 @@ final class IdToEntityConverter implements ConditionalGenericConverter {
 			return null;
 		}
 		Method finder = getFinder(targetType.getType());
-		Object id = this.conversionService.convert(
+		Object id = this.conversionService.convert(source, sourceType, TypeDescriptor.valueOf(finder.getParameterTypes()[0]));
-				source, sourceType, TypeDescriptor.valueOf(finder.getParameterTypes()[0]));
 		return ReflectionUtils.invokeMethod(finder, source, id);
 	}
 
@@ -69,8 +67,7 @@ final class IdToEntityConverter implements ConditionalGenericConverter {
 		String finderMethod = "find" + getEntityName(entityClass);
 		Method[] methods = entityClass.getDeclaredMethods();
 		for (Method method : methods) {
-			if (Modifier.isStatic(method.getModifiers()) && method.getParameterTypes().length == 1 &&
+			if (Modifier.isStatic(method.getModifiers()) && method.getParameterTypes().length == 1 && method.getReturnType().equals(entityClass)) {
-					method.getReturnType().equals(entityClass)) {
 				if (method.getName().equals(finderMethod)) {
 					return method;
 				}

org.springframework.core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java

@@ -26,7 +26,6 @@ import static org.junit.Assert.fail;
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedList;
@@ -62,6 +61,16 @@ public class GenericConversionServiceTests {
 		assertEquals(null, conversionService.convert(null, Integer.class));
 	}
 
+	@Test(expected=ConversionFailedException.class)
+	public void convertNullSourcePrimitiveTarget() {
+		assertEquals(null, conversionService.convert(null, int.class));
+	}
+
+	@Test(expected=ConversionFailedException.class)
+	public void convertNullSourcePrimitiveTargetTypeDescriptor() {
+		assertEquals(null, conversionService.convert(null, TypeDescriptor.valueOf(String.class), TypeDescriptor.valueOf(int.class)));
+	}
+
 	@Test
 	public void convertAssignableSource() {
 		assertEquals(Boolean.FALSE, conversionService.convert(false, boolean.class));