From 01cbfd4f6f750aa87021a91658bf09a2f28e0d03 Mon Sep 17 00:00:00 2001 From: Keith Donald Date: Tue, 24 May 2011 17:53:18 +0000 Subject: [PATCH] added null binding check for primitives for all conversion results; polishing --- .../support/FormattingConversionService.java | 11 ++++- .../FormattingConversionServiceTests.java | 36 ++++++++++++-- .../support/GenericConversionService.java | 48 ++++++++++++------- .../convert/support/IdToEntityConverter.java | 9 ++-- .../GenericConversionServiceTests.java | 11 ++++- 5 files changed, 85 insertions(+), 30 deletions(-) diff --git a/org.springframework.context/src/main/java/org/springframework/format/support/FormattingConversionService.java b/org.springframework.context/src/main/java/org/springframework/format/support/FormattingConversionService.java index f47bceed282..5c4360e8dfe 100644 --- a/org.springframework.context/src/main/java/org/springframework/format/support/FormattingConversionService.java +++ b/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); diff --git a/org.springframework.context/src/test/java/org/springframework/format/support/FormattingConversionServiceTests.java b/org.springframework.context/src/test/java/org/springframework/format/support/FormattingConversionServiceTests.java index a636dd22b14..dbcec2671ec 100644 --- a/org.springframework.context/src/test/java/org/springframework/format/support/FormattingConversionServiceTests.java +++ b/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 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 dates; @@ -257,5 +273,17 @@ public class FormattingConversionServiceTests { this.dates = dates; } } + + public static class NullReturningFormatter implements Formatter { + + public String print(Integer object, Locale locale) { + return null; + } + + public Integer parse(String text, Locale locale) throws ParseException { + return null; + } + + } } diff --git a/org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java b/org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java index 0b8616d887c..fd7a88634d6 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java +++ b/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)) { - logger.debug("No converter found - returning assignable source object as-is"); - return source; - } - else { - throw new ConverterNotFoundException(sourceType, targetType); - } + return handleConverterNotFound(source, 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. *

Default implementation returns null. - * Throws a {@link ConversionFailedException} if the targetType is a primitive type, - * as null 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> classQueue) { - classQueue.addFirst(ifc); - for (Class inheritedIfc : ifc.getInterfaces()) { - addInterfaceHierarchy(inheritedIfc, classQueue); + private void addInterfaceHierarchy(Class interfaceType, LinkedList> classQueue) { + classQueue.addFirst(interfaceType); + for (Class implementedInterface : interfaceType.getInterfaces()) { + 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 { diff --git a/org.springframework.core/src/main/java/org/springframework/core/convert/support/IdToEntityConverter.java b/org.springframework.core/src/main/java/org/springframework/core/convert/support/IdToEntityConverter.java index af97f7083f1..df43fd51a15 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/convert/support/IdToEntityConverter.java +++ b/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 && - this.conversionService.canConvert(sourceType, TypeDescriptor.valueOf(finder.getParameterTypes()[0]))); + return finder != null && 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( - source, sourceType, TypeDescriptor.valueOf(finder.getParameterTypes()[0])); + Object id = this.conversionService.convert(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 && - method.getReturnType().equals(entityClass)) { + if (Modifier.isStatic(method.getModifiers()) && method.getParameterTypes().length == 1 && method.getReturnType().equals(entityClass)) { if (method.getName().equals(finderMethod)) { return method; } diff --git a/org.springframework.core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java b/org.springframework.core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java index 8a98ef6d504..dd40336be53 100644 --- a/org.springframework.core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java +++ b/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));