Made BeanUtils.copyProperties defensive about property type mismatches

Issue: SPR-11209

spring-beans/src/main/java/org/springframework/beans/BeanUtils.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 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.
@@ -199,7 +199,7 @@ public abstract class BeanUtils {
 	 * @return the Method object, or {@code null} if not found
 	 * @see Class#getDeclaredMethod
 	 */
-	public static Method findDeclaredMethod(Class<?> clazz, String methodName, Class<?>[] paramTypes) {
+	public static Method findDeclaredMethod(Class<?> clazz, String methodName, Class<?>... paramTypes) {
 		try {
 			return clazz.getDeclaredMethod(methodName, paramTypes);
 		}
@@ -455,7 +455,7 @@ public abstract class BeanUtils {
 	 * @param beanClasses the classes to check against
 	 * @return the property type, or {@code Object.class} as fallback
 	 */
-	public static Class<?> findPropertyType(String propertyName, Class<?>[] beanClasses) {
+	public static Class<?> findPropertyType(String propertyName, Class<?>... beanClasses) {
 		if (beanClasses != null) {
 			for (Class<?> beanClass : beanClasses) {
 				PropertyDescriptor pd = getPropertyDescriptor(beanClass, propertyName);
@@ -528,7 +528,7 @@ public abstract class BeanUtils {
 	 * @see BeanWrapper
 	 */
 	public static void copyProperties(Object source, Object target) throws BeansException {
-		copyProperties(source, target, null, null);
+		copyProperties(source, target, null, (String[]) null);
 	}
 
 	/**
@@ -548,7 +548,7 @@ public abstract class BeanUtils {
 	public static void copyProperties(Object source, Object target, Class<?> editable)
 			throws BeansException {
 
-		copyProperties(source, target, editable, null);
+		copyProperties(source, target, editable, (String[]) null);
 	}
 
 	/**
@@ -565,7 +565,7 @@ public abstract class BeanUtils {
 	 * @throws BeansException if the copying failed
 	 * @see BeanWrapper
 	 */
-	public static void copyProperties(Object source, Object target, String[] ignoreProperties)
+	public static void copyProperties(Object source, Object target, String... ignoreProperties)
 			throws BeansException {
 
 		copyProperties(source, target, null, ignoreProperties);
@@ -583,7 +583,7 @@ public abstract class BeanUtils {
 	 * @throws BeansException if the copying failed
 	 * @see BeanWrapper
 	 */
-	private static void copyProperties(Object source, Object target, Class<?> editable, String[] ignoreProperties)
+	private static void copyProperties(Object source, Object target, Class<?> editable, String... ignoreProperties)
 			throws BeansException {
 
 		Assert.notNull(source, "Source must not be null");
@@ -601,24 +601,27 @@ public abstract class BeanUtils {
 		List<String> ignoreList = (ignoreProperties != null) ? Arrays.asList(ignoreProperties) : null;
 
 		for (PropertyDescriptor targetPd : targetPds) {
-			if (targetPd.getWriteMethod() != null &&
-					(ignoreProperties == null || (!ignoreList.contains(targetPd.getName())))) {
+			Method writeMethod = targetPd.getWriteMethod();
+			if (writeMethod != null && (ignoreProperties == null || (!ignoreList.contains(targetPd.getName())))) {
 				PropertyDescriptor sourcePd = getPropertyDescriptor(source.getClass(), targetPd.getName());
-				if (sourcePd != null && sourcePd.getReadMethod() != null) {
-					try {
-						Method readMethod = sourcePd.getReadMethod();
-						if (!Modifier.isPublic(readMethod.getDeclaringClass().getModifiers())) {
-							readMethod.setAccessible(true);
+				if (sourcePd != null) {
+					Method readMethod = sourcePd.getReadMethod();
+					if (readMethod != null &&
+							writeMethod.getParameterTypes()[0].isAssignableFrom(readMethod.getReturnType())) {
+						try {
+							if (!Modifier.isPublic(readMethod.getDeclaringClass().getModifiers())) {
+								readMethod.setAccessible(true);
+							}
+							Object value = readMethod.invoke(source);
+							if (!Modifier.isPublic(writeMethod.getDeclaringClass().getModifiers())) {
+								writeMethod.setAccessible(true);
+							}
+							writeMethod.invoke(target, value);
 						}
-						Object value = readMethod.invoke(source);
-						Method writeMethod = targetPd.getWriteMethod();
-						if (!Modifier.isPublic(writeMethod.getDeclaringClass().getModifiers())) {
-							writeMethod.setAccessible(true);
+						catch (Throwable ex) {
+							throw new FatalBeanException(
+									"Could not copy property '" + targetPd.getName() + "' from source to target", ex);
 						}
-						writeMethod.invoke(target, value);
-					}
-					catch (Throwable ex) {
-						throw new FatalBeanException("Could not copy properties from source to target", ex);
 					}
 				}
 			}

spring-beans/src/test/java/org/springframework/beans/BeanUtilsTests.java

@@ -16,8 +16,6 @@
 
 package org.springframework.beans;
 
-import static org.junit.Assert.*;
-
 import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
 import java.lang.reflect.Method;
@@ -25,6 +23,7 @@ import java.util.ArrayList;
 import java.util.List;
 
 import org.junit.Test;
+
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.propertyeditors.CustomDateEditor;
 import org.springframework.core.io.Resource;
@@ -33,6 +32,8 @@ import org.springframework.tests.sample.beans.DerivedTestBean;
 import org.springframework.tests.sample.beans.ITestBean;
 import org.springframework.tests.sample.beans.TestBean;
 
+import static org.junit.Assert.*;
+
 /**
  * Unit tests for {@link BeanUtils}.
  *
@@ -78,8 +79,7 @@ public final class BeanUtilsTests {
 	@Test
 	public void testBeanPropertyIsArray() {
 		PropertyDescriptor[] descriptors = BeanUtils.getPropertyDescriptors(ContainerBean.class);
-		for (int i = 0; i < descriptors.length; i++) {
-			PropertyDescriptor descriptor = descriptors[i];
+		for (PropertyDescriptor descriptor : descriptors) {
 			if ("containedBeans".equals(descriptor.getName())) {
 				assertTrue("Property should be an array", descriptor.getPropertyType().isArray());
 				assertEquals(descriptor.getPropertyType().getComponentType(), ContainedBean.class);
@@ -170,7 +170,7 @@ public final class BeanUtilsTests {
 		assertTrue("Touchy empty", tb2.getTouchy() == null);
 
 		// "spouse", "touchy", "age" should not be copied
-		BeanUtils.copyProperties(tb, tb2, new String[]{"spouse", "touchy", "age"});
+		BeanUtils.copyProperties(tb, tb2, "spouse", "touchy", "age");
 		assertTrue("Name copied", tb2.getName() == null);
 		assertTrue("Age still empty", tb2.getAge() == 0);
 		assertTrue("Touchy still empty", tb2.getTouchy() == null);
@@ -181,7 +181,16 @@ public final class BeanUtilsTests {
 		NameAndSpecialProperty source = new NameAndSpecialProperty();
 		source.setName("name");
 		TestBean target = new TestBean();
-		BeanUtils.copyProperties(source, target, new String[]{"specialProperty"});
+		BeanUtils.copyProperties(source, target, "specialProperty");
+		assertEquals(target.getName(), "name");
+	}
+
+	@Test
+	public void testCopyPropertiesWithInvalidProperty() {
+		InvalidProperty source = new InvalidProperty();
+		source.setName("name");
+		InvalidProperty target = new InvalidProperty();
+		BeanUtils.copyProperties(source, target);
 		assertEquals(target.getName(), "name");
 	}
 
@@ -256,7 +265,8 @@ public final class BeanUtilsTests {
 		assertEquals(String.class, keyDescr.getPropertyType());
 		for (PropertyDescriptor propertyDescriptor : descrs) {
 			if (propertyDescriptor.getName().equals(keyDescr.getName())) {
-				assertEquals(propertyDescriptor.getName() + " has unexpected type", keyDescr.getPropertyType(), propertyDescriptor.getPropertyType());
+				assertEquals(propertyDescriptor.getName() + " has unexpected type",
+						keyDescr.getPropertyType(), propertyDescriptor.getPropertyType());
 			}
 		}
 	}
@@ -291,6 +301,31 @@ public final class BeanUtilsTests {
 	}
 
 
+	@SuppressWarnings("unused")
+	private static class InvalidProperty {
+
+		private String name;
+
+		private String value;
+
+		public void setName(String name) {
+			this.name = name;
+		}
+
+		public String getName() {
+			return this.name;
+		}
+
+		public void setValue(int value) {
+			this.value = Integer.toString(value);
+		}
+
+		public String getValue() {
+			return this.value;
+		}
+	}
+
+
 	@SuppressWarnings("unused")
 	private static class ContainerBean {
 
@@ -346,6 +381,7 @@ public final class BeanUtilsTests {
 		}
 	}
 
+
 	private interface MapEntry<K, V> {
 
 		K getKey();
@@ -357,6 +393,7 @@ public final class BeanUtilsTests {
 		void setValue(V value);
 	}
 
+
 	private static class Bean implements MapEntry<String, String> {
 
 		private String key;