smarter guessing of the element type (SPR-7283)

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<Class<?>, TypeDescriptor> typeDescriptorCache = new HashMap<Class<?>, 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 <code>null</code> 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<String,Foo>) 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<? extends Collection>) this.type) : null);
+        return (this.type != null ? GenericCollectionTypeResolver.getCollectionType((Class<? extends Collection>) 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<? extends Map>) 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<? extends Map>) this.type) : null);
 	}
-	
+
 	private Annotation[] resolveAnnotations() {
 		if (this.field != null) {
 			return this.field.getAnnotations();

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<ConvertiblePair> 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<Class<?>, Map<Class<?>, MatchableConverters>> converters =
 			new HashMap<Class<?>, Map<Class<?>, MatchableConverters>>(36);
 
-	private final Map<ConverterCacheKey, GenericConverter> converterCache = new ConcurrentHashMap<ConverterCacheKey, GenericConverter>();
-	
+	private final Map<ConverterCacheKey, GenericConverter> converterCache =
+			new ConcurrentHashMap<ConverterCacheKey, GenericConverter>();
+
+
 	// 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 <code>null</code>.
-	 * 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.
+	 * <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
@@ -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<ConditionalGenericConverter> it = this.conditionalConverters.iterator(); it.hasNext(); ) {
+				for (Iterator<ConditionalGenericConverter> 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 + "]";
 		}
 	}
 

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 <code>null</code> 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

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<String, Object> map = new HashMap<String, Object>();
+		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<String, Object> map = new HashMap<String, Object>();
+		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");
+		}
+	}
+
+}