From 902938e95f19b7547421064720b016ba49e3c02e Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 14 Jun 2010 22:58:18 +0000 Subject: [PATCH] smarter guessing of the element type (SPR-7283) --- .../core/convert/TypeDescriptor.java | 89 +++++++------- .../support/GenericConversionService.java | 114 ++++++++---------- .../springframework/util/CollectionUtils.java | 26 +++- .../PropertiesConversionSpelTests.java | 86 +++++++++++++ 4 files changed, 204 insertions(+), 111 deletions(-) create mode 100644 org.springframework.expression/src/test/java/org/springframework/expression/spel/standard/PropertiesConversionSpelTests.java diff --git a/org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java b/org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java index 89297057d28..15b5c83c002 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java +++ b/org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java @@ -26,6 +26,7 @@ import org.springframework.core.GenericCollectionTypeResolver; import org.springframework.core.MethodParameter; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; +import org.springframework.util.CollectionUtils; import org.springframework.util.ObjectUtils; /** @@ -34,7 +35,7 @@ import org.springframework.util.ObjectUtils; * @author Keith Donald * @author Andy Clement * @author Juergen Hoeller - * @since 3.0 + * @since 3.0 */ public class TypeDescriptor { @@ -44,7 +45,7 @@ public class TypeDescriptor { private static final Map, TypeDescriptor> typeDescriptorCache = new HashMap, TypeDescriptor>(); private static final Annotation[] EMPTY_ANNOTATION_ARRAY = new Annotation[0]; - + static { typeDescriptorCache.put(boolean.class, new TypeDescriptor(boolean.class)); typeDescriptorCache.put(Boolean.class, new TypeDescriptor(Boolean.class)); @@ -75,13 +76,13 @@ public class TypeDescriptor { private Object value; private TypeDescriptor elementType; - + private TypeDescriptor mapKeyType; - + private TypeDescriptor mapValueType; private Annotation[] annotations; - + /** * Create a new type descriptor from a method or constructor parameter. @@ -248,15 +249,15 @@ public class TypeDescriptor { * Return the element type as a type descriptor. */ public synchronized TypeDescriptor getElementTypeDescriptor() { - if (elementType == null) { - elementType = forElementType(resolveElementType()); + if (this.elementType == null) { + this.elementType = forElementType(resolveElementType()); } - return elementType; + return this.elementType; } /** - * Return the element type as a type descriptor; if the element type is null (cannot be determined), - * the type descriptor is derived from the element argument. + * Return the element type as a type descriptor. If the element type is null + * (cannot be determined), the type descriptor is derived from the element argument. * @param element the element * @return the element type descriptor */ @@ -273,7 +274,7 @@ public class TypeDescriptor { } /** - * Is this descriptor for a map where the key type and value type are known? + * Is this descriptor for a map where the key type and value type are known? */ public boolean isMapEntryTypeKnown() { return (isMap() && getMapKeyType() != null && getMapValueType() != null); @@ -291,14 +292,15 @@ public class TypeDescriptor { * Returns map key type as a type descriptor. */ public synchronized TypeDescriptor getMapKeyTypeDescriptor() { - if (mapKeyType == null) { - mapKeyType = forElementType(resolveMapKeyType()); + if (this.mapKeyType == null) { + this.mapKeyType = forElementType(resolveMapKeyType()); } - return mapKeyType; + return this.mapKeyType; } /** - * Return the map key type as a type descriptor; if the key type is null (cannot be determined), the type descriptor is derived from the key argument. + * Return the map key type as a type descriptor. If the key type is null + * (cannot be determined), the type descriptor is derived from the key argument. * @param key the key * @return the map key type descriptor */ @@ -306,7 +308,7 @@ public class TypeDescriptor { TypeDescriptor keyType = getMapKeyTypeDescriptor(); return keyType != TypeDescriptor.NULL ? keyType : TypeDescriptor.forObject(key); } - + /** * Determine the generic value type of the wrapped Map parameter/field, if any. * @return the generic type, or null if none @@ -314,19 +316,19 @@ public class TypeDescriptor { public Class getMapValueType() { return getMapValueTypeDescriptor().getType(); } - + /** * Returns map value type as a type descriptor. */ public synchronized TypeDescriptor getMapValueTypeDescriptor() { if (this.mapValueType == null) { - mapValueType = forElementType(resolveMapValueType()); + this.mapValueType = forElementType(resolveMapValueType()); } return this.mapValueType; } /** - * Return the map value type as a type descriptor; if the value type is null + * Return the map value type as a type descriptor. If the value type is null * (cannot be determined), the type descriptor is derived from the value argument. * @param value the value * @return the map value type descriptor @@ -355,7 +357,7 @@ public class TypeDescriptor { return annotation; } } - return null; + return null; } /** @@ -423,14 +425,14 @@ public class TypeDescriptor { ObjectUtils.nullSafeEquals(getMapValueType(), td.getMapValueType()); } else { - return annotatedTypeEquals; + return annotatedTypeEquals; } } - + public int hashCode() { return getType().hashCode(); } - + /** * A textual representation of the type descriptor (eg. Map) for use in messages */ @@ -459,7 +461,7 @@ public class TypeDescriptor { } else if (isCollection()) { Class elementType = getElementType(); - builder.append("<").append(elementType != null ? ClassUtils.getQualifiedName(elementType) : "?").append(">"); + builder.append("<").append(elementType != null ? ClassUtils.getQualifiedName(elementType) : "?").append(">"); } builder.append("]"); return builder.toString(); @@ -468,7 +470,7 @@ public class TypeDescriptor { // internal helpers - + private Class resolveElementType() { if (isArray()) { return getType().getComponentType(); @@ -478,9 +480,9 @@ public class TypeDescriptor { } else { return null; - } + } } - + @SuppressWarnings("unchecked") private Class resolveCollectionElementType() { if (this.field != null) { @@ -490,17 +492,14 @@ public class TypeDescriptor { return GenericCollectionTypeResolver.getCollectionParameterType(this.methodParameter); } else if (this.value instanceof Collection) { - Collection coll = (Collection) this.value; - if (!coll.isEmpty()) { - Object elem = coll.iterator().next(); - if (elem != null) { - return elem.getClass(); - } + Class elementType = CollectionUtils.findCommonElementType((Collection) this.value); + if (elementType != null) { + return elementType; } } - return (this.type != null ? GenericCollectionTypeResolver.getCollectionType((Class) this.type) : null); + return (this.type != null ? GenericCollectionTypeResolver.getCollectionType((Class) this.type) : null); } - + @SuppressWarnings("unchecked") private Class resolveMapKeyType() { if (this.field != null) { @@ -510,12 +509,9 @@ public class TypeDescriptor { return GenericCollectionTypeResolver.getMapKeyParameterType(this.methodParameter); } else if (this.value instanceof Map) { - Map map = (Map) this.value; - if (!map.isEmpty()) { - Object key = map.keySet().iterator().next(); - if (key != null) { - return key.getClass(); - } + Class keyType = CollectionUtils.findCommonElementType(((Map) this.value).keySet()); + if (keyType != null) { + return keyType; } } return (this.type != null && isMap() ? GenericCollectionTypeResolver.getMapKeyType((Class) this.type) : null); @@ -530,17 +526,14 @@ public class TypeDescriptor { return GenericCollectionTypeResolver.getMapValueParameterType(this.methodParameter); } else if (this.value instanceof Map) { - Map map = (Map) this.value; - if (!map.isEmpty()) { - Object val = map.values().iterator().next(); - if (val != null) { - return val.getClass(); - } + Class valueType = CollectionUtils.findCommonElementType(((Map) this.value).values()); + if (valueType != null) { + return valueType; } } return (isMap() && this.type != null ? GenericCollectionTypeResolver.getMapValueType((Class) this.type) : null); } - + private Annotation[] resolveAnnotations() { if (this.field != null) { return this.field.getAnnotations(); 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 8f6ba605b4e..e4519ee2798 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 @@ -31,7 +31,6 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.core.GenericTypeResolver; -import org.springframework.core.convert.ConversionException; import org.springframework.core.convert.ConversionFailedException; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.ConverterNotFoundException; @@ -55,8 +54,6 @@ import org.springframework.util.ClassUtils; */ public class GenericConversionService implements ConversionService, ConverterRegistry { - private static final Log logger = LogFactory.getLog(GenericConversionService.class); - private static final GenericConverter NO_OP_CONVERTER = new GenericConverter() { public Set getConvertibleTypes() { return null; @@ -81,11 +78,16 @@ public class GenericConversionService implements ConversionService, ConverterReg } }; + + private static final Log logger = LogFactory.getLog(GenericConversionService.class); + private final Map, Map, MatchableConverters>> converters = new HashMap, Map, MatchableConverters>>(36); - private final Map converterCache = new ConcurrentHashMap(); - + private final Map converterCache = + new ConcurrentHashMap(); + + // implementing ConverterRegistry public void addConverter(Converter converter) { @@ -119,6 +121,7 @@ public class GenericConversionService implements ConversionService, ConverterReg invalidateCache(); } + // implementing ConversionService public boolean canConvert(Class sourceType, Class targetType) { @@ -132,26 +135,20 @@ public class GenericConversionService implements ConversionService, ConverterReg public boolean canConvert(TypeDescriptor sourceType, TypeDescriptor targetType) { assertNotNull(sourceType, targetType); - if (logger.isDebugEnabled()) { - logger.debug("Checking if I can convert " + sourceType + " to " + targetType); + if (logger.isTraceEnabled()) { + logger.trace("Checking if I can convert " + sourceType + " to " + targetType); } if (sourceType == TypeDescriptor.NULL || targetType == TypeDescriptor.NULL) { - if (logger.isDebugEnabled()) { - logger.debug("Yes, I can convert"); - } + logger.trace("Yes, I can convert"); return true; } GenericConverter converter = getConverter(sourceType, targetType); if (converter != null) { - if (logger.isDebugEnabled()) { - logger.debug("Yes, I can convert"); - } + logger.trace("Yes, I can convert"); return true; } else { - if (logger.isDebugEnabled()) { - logger.debug("No, I cannot convert"); - } + logger.trace("No, I cannot convert"); return false; } } @@ -170,31 +167,18 @@ public class GenericConversionService implements ConversionService, ConverterReg return result; } if (targetType == TypeDescriptor.NULL) { - if (logger.isDebugEnabled()) { - logger.debug("Converted to null"); - } + logger.debug("Converted to null"); return null; } GenericConverter converter = getConverter(sourceType, targetType); if (converter == null) { - ConverterNotFoundException e = new ConverterNotFoundException(sourceType, targetType); - if (logger.isDebugEnabled()) { - logger.debug(e); - } - throw e; + throw new ConverterNotFoundException(sourceType, targetType); } - try { - Object result = ConversionUtils.invokeConverter(converter, source, sourceType, targetType); - if (logger.isDebugEnabled()) { - logger.debug("Converted to " + StylerUtils.style(result)); - } - return result; - } catch (ConversionException e) { - if (logger.isDebugEnabled()) { - logger.debug(e); - } - throw e; + Object result = ConversionUtils.invokeConverter(converter, source, sourceType, targetType); + if (logger.isDebugEnabled()) { + logger.debug("Converted to " + StylerUtils.style(result)); } + return result; } public String toString() { @@ -215,12 +199,14 @@ public class GenericConversionService implements ConversionService, ConverterReg return builder.toString(); } + // subclassing hooks /** - * Hook 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. + * 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 @@ -247,10 +233,10 @@ public class GenericConversionService implements ConversionService, ConverterReg */ protected GenericConverter getConverter(TypeDescriptor sourceType, TypeDescriptor targetType) { ConverterCacheKey key = new ConverterCacheKey(sourceType, targetType); - GenericConverter converter = converterCache.get(key); + GenericConverter converter = this.converterCache.get(key); if (converter != null) { - if (logger.isDebugEnabled()) { - logger.debug("Matched cached converter " + converter); + if (logger.isTraceEnabled()) { + logger.trace("Matched cached converter " + converter); } return converter != NO_MATCH ? converter : null; } @@ -260,7 +246,7 @@ public class GenericConversionService implements ConversionService, ConverterReg if (logger.isTraceEnabled()) { logger.trace("Caching under " + key); } - converterCache.put(key, converter); + this.converterCache.put(key, converter); return converter; } converter = getDefaultConverter(sourceType, targetType); @@ -268,13 +254,13 @@ public class GenericConversionService implements ConversionService, ConverterReg if (logger.isTraceEnabled()) { logger.trace("Caching under " + key); } - converterCache.put(key, converter); + this.converterCache.put(key, converter); return converter; } if (logger.isTraceEnabled()) { logger.trace("Caching NO_MATCH under " + key); } - converterCache.put(key, NO_MATCH); + this.converterCache.put(key, NO_MATCH); return null; } } @@ -290,9 +276,7 @@ public class GenericConversionService implements ConversionService, ConverterReg */ protected GenericConverter getDefaultConverter(TypeDescriptor sourceType, TypeDescriptor targetType) { if (sourceType.isAssignableTo(targetType)) { - if (logger.isDebugEnabled()) { - logger.debug("Matched default NO_OP_CONVERTER"); - } + logger.trace("Matched default NO_OP_CONVERTER"); return NO_OP_CONVERTER; } else { @@ -471,7 +455,9 @@ public class GenericConversionService implements ConversionService, ConverterReg } } - private GenericConverter matchConverter(MatchableConverters matchable, TypeDescriptor sourceFieldType, TypeDescriptor targetFieldType) { + private GenericConverter matchConverter( + MatchableConverters matchable, TypeDescriptor sourceFieldType, TypeDescriptor targetFieldType) { + if (matchable == null) { return null; } @@ -566,20 +552,20 @@ public class GenericConversionService implements ConversionService, ConverterReg logger.trace("Matching " + conditional); } if (conditional.matches(sourceType, targetType)) { - if (logger.isDebugEnabled()) { - logger.debug("Matched converter " + conditional); + if (logger.isTraceEnabled()) { + logger.trace("Matched converter " + conditional); } return conditional; } else { - if (logger.isDebugEnabled()) { - logger.debug("Did not match converter " + conditional); + if (logger.isTraceEnabled()) { + logger.trace("Did not match converter " + conditional); } } } } - if (this.defaultConverter != null && logger.isDebugEnabled()) { - logger.debug("Matched converter " + this.defaultConverter); + if (this.defaultConverter != null && logger.isTraceEnabled()) { + logger.trace("Matched converter " + this.defaultConverter); } return this.defaultConverter; } @@ -587,7 +573,7 @@ public class GenericConversionService implements ConversionService, ConverterReg public String toString() { if (this.conditionalConverters != null) { StringBuilder builder = new StringBuilder(); - for (Iterator it = this.conditionalConverters.iterator(); it.hasNext(); ) { + for (Iterator it = this.conditionalConverters.iterator(); it.hasNext();) { builder.append(it.next()); if (it.hasNext()) { builder.append(", "); @@ -603,7 +589,8 @@ public class GenericConversionService implements ConversionService, ConverterReg } } } - + + private static final class ConverterCacheKey { private final TypeDescriptor sourceType; @@ -615,20 +602,23 @@ public class GenericConversionService implements ConversionService, ConverterReg this.targetType = targetType; } - public boolean equals(Object o) { - if (!(o instanceof ConverterCacheKey)) { + public boolean equals(Object other) { + if (this == other) { + return true; + } + if (!(other instanceof ConverterCacheKey)) { return false; } - ConverterCacheKey key = (ConverterCacheKey) o; - return sourceType.equals(key.sourceType) && targetType.equals(key.targetType); + ConverterCacheKey otherKey = (ConverterCacheKey) other; + return this.sourceType.equals(otherKey.sourceType) && this.targetType.equals(otherKey.targetType); } public int hashCode() { - return sourceType.hashCode() * 29 + targetType.hashCode(); + return this.sourceType.hashCode() * 29 + this.targetType.hashCode(); } public String toString() { - return "[ConverterCacheKey sourceType = " + sourceType + ", targetType = " + targetType + "]"; + return "ConverterCacheKey [sourceType = " + this.sourceType + ", targetType = " + this.targetType + "]"; } } diff --git a/org.springframework.core/src/main/java/org/springframework/util/CollectionUtils.java b/org.springframework.core/src/main/java/org/springframework/util/CollectionUtils.java index 14c8d87e116..249de6439a7 100644 --- a/org.springframework.core/src/main/java/org/springframework/util/CollectionUtils.java +++ b/org.springframework.core/src/main/java/org/springframework/util/CollectionUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2009 the original author or authors. + * Copyright 2002-2010 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. @@ -276,6 +276,30 @@ public abstract class CollectionUtils { return true; } + /** + * Find the common element type of the given Collection, if any. + * @param collection the Collection to check + * @return the common element type, or null if no clear + * common type has been found (or the collection was empty) + */ + public static Class findCommonElementType(Collection collection) { + if (isEmpty(collection)) { + return null; + } + Class candidate = null; + for (Object val : collection) { + if (val != null) { + if (candidate == null) { + candidate = val.getClass(); + } + else if (candidate != val.getClass()) { + return null; + } + } + } + return candidate; + } + /** * Adapts an enumeration to an iterator. * @param enumeration the enumeration diff --git a/org.springframework.expression/src/test/java/org/springframework/expression/spel/standard/PropertiesConversionSpelTests.java b/org.springframework.expression/src/test/java/org/springframework/expression/spel/standard/PropertiesConversionSpelTests.java new file mode 100644 index 00000000000..6c1c47f5087 --- /dev/null +++ b/org.springframework.expression/src/test/java/org/springframework/expression/spel/standard/PropertiesConversionSpelTests.java @@ -0,0 +1,86 @@ +/* + * Copyright 2002-2010 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.expression.spel.standard; + +import static org.junit.Assert.assertEquals; + +import java.util.HashMap; +import java.util.Map; +import java.util.Properties; + +import org.junit.Test; + +import org.springframework.expression.Expression; +import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.expression.spel.support.StandardEvaluationContext; + +/** + * @author Mark Fisher + */ +public class PropertiesConversionSpelTests { + + private static final SpelExpressionParser parser = new SpelExpressionParser(); + + @Test + public void props() { + Properties props = new Properties(); + props.setProperty("x", "1"); + props.setProperty("y", "2"); + props.setProperty("z", "3"); + Expression expression = parser.parseExpression("foo(#props)"); + StandardEvaluationContext context = new StandardEvaluationContext(); + context.setVariable("props", props); + String result = expression.getValue(context, new TestBean(), String.class); + assertEquals("123", result); + } + + @Test + public void mapWithAllStringValues() { + Map map = new HashMap(); + map.put("x", "1"); + map.put("y", "2"); + map.put("z", "3"); + Expression expression = parser.parseExpression("foo(#props)"); + StandardEvaluationContext context = new StandardEvaluationContext(); + context.setVariable("props", map); + String result = expression.getValue(context, new TestBean(), String.class); + assertEquals("123", result); + } + + @Test // questionable, but this passes with 3.0.2.RELEASE + public void mapWithNonStringValue() { + Map map = new HashMap(); + map.put("x", "1"); + map.put("y", 2); + map.put("z", "3"); + Expression expression = parser.parseExpression("foo(#props)"); + StandardEvaluationContext context = new StandardEvaluationContext(); + context.setVariable("props", map); + String result = expression.getValue(context, new TestBean(), String.class); + assertEquals("1null3", result); + } + + + private static class TestBean { + + @SuppressWarnings("unused") + public String foo(Properties props) { + return props.getProperty("x") + props.getProperty("y") + props.getProperty("z"); + } + } + +}