VIVO-120 improve the error messages from OperationUtils.cloneBean()

Add unit tests for the method and restructure the logic so a failure to create the clone is treated as seriously as a failure to load it with values. Previously  a failure to create resulted in a null return (with a log message) while a failure to load resulted in an exception (with a log message).

webapp/src/edu/cornell/mannlib/vedit/util/OperationUtils.java

@@ -10,14 +10,16 @@ import org.apache.commons.logging.LogFactory;
 
 import edu.cornell.mannlib.vedit.beans.EditProcessObject;
 
-
 public class OperationUtils {
 
-	private static final Log log = LogFactory.getLog(OperationUtils.class.getName());
+	private static final Log log = LogFactory.getLog(OperationUtils.class
+			.getName());
 
-    public static void beanSetAndValidate(Object newObj, String field, String value, EditProcessObject epo){
+	public static void beanSetAndValidate(Object newObj, String field,
-        Class cls = (epo.getBeanClass() != null) ? epo.getBeanClass() : newObj.getClass();
+			String value, EditProcessObject epo) {
-        Class[] paramList = new Class[1];
+		Class<?> cls = (epo.getBeanClass() != null) ? epo.getBeanClass() : newObj
+				.getClass();
+		Class<?>[] paramList = new Class[1];
 		paramList[0] = String.class;
 		boolean isInt = false;
 		boolean isBoolean = false;
@@ -38,7 +40,8 @@ public class OperationUtils{
 					isBoolean = true;
 					log.debug("found boolean field " + field);
 				} catch (NoSuchMethodException g) {
-            		log.error("beanSet could not find an appropriate String, int, or boolean setter method for "+field);	
+					log.error("beanSet could not find an appropriate String, int, or boolean setter method for "
+							+ field);
 				}
 
 			}
@@ -60,86 +63,89 @@ public class OperationUtils{
 	}
 
 	/**
-     * Takes a bean and clones it using reflection.
+	 * Takes a bean and clones it using reflection. Any fields without standard
-     * Any fields without standard getter/setter methods will not be copied.
+	 * getter/setter methods will not be copied.
-     * @param bean
-     * @return
 	 */
 	public static Object cloneBean(Object bean) {
+		if (bean == null) {
+			throw new NullPointerException("bean may not be null.");
+		}
 		return cloneBean(bean, bean.getClass(), bean.getClass());
 	}
 
 	/**
-     * Takes a bean and clones it using reflection.
+	 * Takes a bean and clones it using reflection. Any fields without standard
-     * Any fields without standard getter/setter methods will not be copied.
+	 * getter/setter methods will not be copied.
-     * @param bean
-     * @return
 	 */
-    public static Object cloneBean (Object bean, Class beanClass, Class iface){
+	public static Object cloneBean(final Object bean, final Class<?> beanClass,
-        Object newBean = null;
+			final Class<?> iface) {
+		if (bean == null) {
+			throw new NullPointerException("bean may not be null.");
+		}
+		if (beanClass == null) {
+			throw new NullPointerException("beanClass may not be null.");
+		}
+		if (iface == null) {
+			throw new NullPointerException("iface may not be null.");
+		}
+
+		class CloneBeanException extends RuntimeException {
+			public CloneBeanException(String message, Throwable cause) {
+				super(message + " <" + cause.getClass().getSimpleName()
+						+ ">: bean=" + bean + ", beanClass="
+						+ beanClass.getName() + ", iface=" + iface.getName(),
+						cause);
+			}
+		}
+
+		Object newBean;
 		try {
-            newBean = beanClass.newInstance();
+			newBean = beanClass.getConstructor().newInstance();
-            Method[] beanMeths = iface.getMethods();
+		} catch (NoSuchMethodException e) {
-            for (int i=0; i<beanMeths.length ; ++i) {
+			throw new CloneBeanException("bean has no 'nullary' constructor.", e);
-                Method beanMeth = beanMeths[i];
+		} catch (InstantiationException e) {
+			throw new CloneBeanException("tried to create instance of an abstract class.", e);
+		} catch (IllegalAccessException e) {
+			throw new CloneBeanException("bean constructor is not accessible.", e);
+		} catch (InvocationTargetException e) {
+			throw new CloneBeanException("bean constructor threw an exception.", e);
+		} catch (Exception e) {
+			throw new CloneBeanException("failed to instantiate a new bean.", e);
+		}
+
+		for (Method beanMeth : iface.getMethods()) {
 			String methName = beanMeth.getName();
-                if (methName.startsWith("get") 
+			if (!methName.startsWith("get")) {
-                        && beanMeth.getParameterTypes().length == 0 ) {
+				continue;
+			}
+			if (beanMeth.getParameterTypes().length != 0) {
+				continue;
+			}
 			String fieldName = methName.substring(3, methName.length());
-                    Class returnType = beanMeth.getReturnType();
+			Class<?> returnType = beanMeth.getReturnType();
+
+			Method setterMethod;
 			try {
-                        Class[] args = new Class[1];
+				setterMethod = iface.getMethod("set" + fieldName, returnType);
-                        args[0] = returnType;
+			} catch (NoSuchMethodException nsme) {
-                        Method setterMethod = iface.getMethod("set"+fieldName,args);
+				continue;
+			}
+
+			Object fieldVal;
 			try {
-                            Object fieldVal = beanMeth.invoke(bean,(Object[])null);
+				fieldVal = beanMeth.invoke(bean, (Object[]) null);
+			} catch (Exception e) {
+				throw new CloneBeanException("failed to invoke " + beanMeth, e);
+			}
+
 			try {
 				Object[] setArgs = new Object[1];
 				setArgs[0] = fieldVal;
 				setterMethod.invoke(newBean, setArgs);
-                            } catch (IllegalAccessException iae) {
+			} catch (Exception e) {
-                                log.error(OperationUtils.class.getName() +
+				throw new CloneBeanException(
-                                        ".cloneBean() " +
+						"failed to invoke " + setterMethod, e);
-                                        " encountered IllegalAccessException " +
-                                        " invoking " + 
-                                        setterMethod.getName(), iae);
-                                throw new RuntimeException(iae);
-                            } catch (InvocationTargetException ite) {
-                                log.error(OperationUtils.class.getName() +
-                                        ".cloneBean() " +
-                                        " encountered InvocationTargetException" 
-                                        + " invoking " 
-                                        + setterMethod.getName(), ite);
-                                throw new RuntimeException(ite);
 			}
-                        } catch (IllegalAccessException iae) {
-                            log.error(OperationUtils.class.getName() + 
-                                    ".cloneBean() encountered " +
-                                    " IllegalAccessException invoking " + 
-                                    beanMeths[i].getName(), iae);
-                            throw new RuntimeException(iae);
-                        } catch (InvocationTargetException ite) {
-                            log.error(OperationUtils.class.getName() + 
-                                    ".cloneBean() encountered " +
-                                    " InvocationTargetException invoking " + 
-                                    beanMeths[i].getName(), ite);
-                            throw new RuntimeException(ite);
-                        } catch (IllegalArgumentException iae) {
-                            log.error(OperationUtils.class.getName() + 
-                                    ".cloneBean() encountered " +
-                                    " IllegalArgumentException invoking " + 
-                                    beanMeths[i].getName(), iae);
-                            throw new RuntimeException(iae);
-                        }
-                    } catch (NoSuchMethodException nsme){
-                        // ignore this field because there is no setter method
-                    }
-                }
-            }
-        } catch (InstantiationException ie){
-            log.error("edu.cornell.mannlib.vitro.edit.utils.OperationUtils.cloneBean("+bean.getClass().toString()+") could not instantiate new instance of bean.", ie);
-        } catch (IllegalAccessException iae){
-            log.error("edu.cornell.mannlib.vitro.edit.utils.OperationUtils.cloneBean("+bean.getClass().toString()+") encountered illegal access exception instantiating new bean.", iae);
 		}
 		return newBean;
 	}

webapp/test/edu/cornell/mannlib/vedit/util/OperationUtils_CloneBeanTest.java

@@ -0,0 +1,431 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package edu.cornell.mannlib.vedit.util;
+
+import static org.junit.Assert.assertEquals;
+
+import java.lang.reflect.InvocationTargetException;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.junit.Ignore;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+import edu.cornell.mannlib.vitro.testing.AbstractTestClass;
+
+/**
+ * All of these tests are for OperationUtils.cloneBean()
+ */
+public class OperationUtils_CloneBeanTest extends AbstractTestClass {
+
+	// ----------------------------------------------------------------------
+	// Allow the tests to expect a RuntimeException with a particular cause.
+	// ----------------------------------------------------------------------
+
+	@Rule
+	public ExpectedException thrown = ExpectedException.none();
+
+	Matcher<?> causedBy(Class<? extends Throwable> causeClass) {
+		return new CausedByMatcher(causeClass);
+	}
+
+	class CausedByMatcher extends BaseMatcher<Throwable> {
+		private final Class<? extends Throwable> causeClass;
+
+		public CausedByMatcher(Class<? extends Throwable> causeClass) {
+			this.causeClass = causeClass;
+		}
+
+		@Override
+		public boolean matches(Object actualThrowable) {
+			if (!(actualThrowable instanceof RuntimeException)) {
+				return false;
+			}
+			Throwable cause = ((RuntimeException) actualThrowable).getCause();
+			return causeClass.isInstance(cause);
+		}
+
+		@Override
+		public void describeTo(Description d) {
+			d.appendText("RuntimeException caused by " + causeClass.getName());
+		}
+	}
+
+	// ----------------------------------------------------------------------
+	// Test for invalid classes
+	// ----------------------------------------------------------------------
+
+	@Test(expected = NullPointerException.class)
+	public void nullArgument() {
+		OperationUtils.cloneBean(null);
+	}
+
+	@Test(expected = NullPointerException.class)
+	public void nullBean() {
+		OperationUtils
+				.cloneBean(null, SimpleSuccess.class, SimpleSuccess.class);
+	}
+
+	@Test(expected = NullPointerException.class)
+	public void nullBeanClass() {
+		OperationUtils
+				.cloneBean(new SimpleSuccess(), null, SimpleSuccess.class);
+	}
+
+	@Test(expected = NullPointerException.class)
+	public void nullInterfaceClass() {
+		OperationUtils
+				.cloneBean(new SimpleSuccess(), SimpleSuccess.class, null);
+	}
+
+	@Test(expected = IllegalAccessException.class)
+	@Ignore("Why doesn't this throw an exception?")
+	public void privateClass() {
+		OperationUtils.cloneBean(new PrivateClass());
+	}
+
+	@Test
+	public void privateConstructor() {
+		thrown.expect(causedBy(NoSuchMethodException.class));
+		OperationUtils.cloneBean(new PrivateConstructor());
+	}
+
+	@Test
+	public void abstractClass() {
+		thrown.expect(causedBy(InstantiationException.class));
+		OperationUtils.cloneBean(new ConcreteOfAbstractClass(),
+				AbstractClass.class, AbstractClass.class);
+	}
+
+	@Test
+	public void interfaceClass() {
+		thrown.expect(causedBy(InstantiationException.class));
+		OperationUtils.cloneBean(new ConcreteOfInterfaceClass(),
+				InterfaceClass.class, InterfaceClass.class);
+	}
+
+	@Test
+	public void arrayClass() {
+		thrown.expect(causedBy(NoSuchMethodException.class));
+		OperationUtils.cloneBean(new String[0]);
+	}
+
+	@Test
+	public void primitiveTypeClass() {
+		thrown.expect(causedBy(NoSuchMethodException.class));
+		OperationUtils.cloneBean(1, Integer.TYPE, Integer.TYPE);
+	}
+
+	@Test
+	public void voidClass() {
+		thrown.expect(causedBy(NoSuchMethodException.class));
+		OperationUtils.cloneBean(new Object(), Void.TYPE, Void.TYPE);
+	}
+
+	@Test
+	public void noNullaryConstructor() {
+		thrown.expect(causedBy(NoSuchMethodException.class));
+		OperationUtils.cloneBean(new NoNullaryConstructor(1));
+	}
+
+	@Test(expected = ExceptionInInitializerError.class)
+	public void classThrowsExceptionWhenLoaded() {
+		OperationUtils.cloneBean(new ThrowsExceptionWhenLoaded());
+	}
+
+	@Test
+	public void initializerThrowsException() {
+		thrown.expect(causedBy(InvocationTargetException.class));
+		OperationUtils.cloneBean("random object",
+				InitializerThrowsException.class,
+				InitializerThrowsException.class);
+	}
+
+	@Test
+	public void wrongInterfaceClass() {
+		thrown.expect(causedBy(IllegalArgumentException.class));
+		OperationUtils.cloneBean(new WrongConcreteClass(),
+				WrongConcreteClass.class, WrongInterface.class);
+	}
+
+	@Test
+	public void getThrowsException() {
+		thrown.expect(causedBy(InvocationTargetException.class));
+		OperationUtils.cloneBean(new GetMethodThrowsException());
+	}
+
+	@Test
+	public void setThrowsException() {
+		thrown.expect(causedBy(InvocationTargetException.class));
+		OperationUtils.cloneBean(new SetMethodThrowsException());
+	}
+
+	private static class PrivateClass {
+		public PrivateClass() {
+		}
+	}
+
+	private static class PrivateConstructor {
+		private PrivateConstructor() {
+		}
+	}
+
+	public abstract static class AbstractClass {
+		public AbstractClass() {
+		}
+	}
+
+	public static class ConcreteOfAbstractClass extends AbstractClass {
+		public ConcreteOfAbstractClass() {
+		}
+	}
+
+	public abstract static class InterfaceClass {
+		public InterfaceClass() {
+		}
+	}
+
+	public static class ConcreteOfInterfaceClass extends InterfaceClass {
+		public ConcreteOfInterfaceClass() {
+		}
+	}
+
+	public static class NoNullaryConstructor {
+		@SuppressWarnings("unused")
+		public NoNullaryConstructor(int i) {
+			// nothing to do
+		}
+	}
+
+	public static class ThrowsExceptionWhenLoaded {
+		static {
+			if (true)
+				throw new IllegalArgumentException();
+		}
+	}
+
+	public static class InitializerThrowsException {
+		{
+			if (true)
+				throw new IllegalStateException("Initializer throws exception");
+		}
+	}
+
+	public static class WrongConcreteClass {
+		private String junk = "junk";
+
+		public String getJunk() {
+			return this.junk;
+		}
+
+		@SuppressWarnings("unused")
+		private void setJunk(String junk) {
+			this.junk = junk;
+		}
+	}
+
+	public static interface WrongInterface {
+		String getJunk();
+
+		void setJunk(String junk);
+	}
+
+	public static class GetMethodThrowsException {
+		@SuppressWarnings("unused")
+		private String junk = "junk";
+
+		public String getJunk() {
+			throw new UnsupportedOperationException();
+		}
+
+		public void setJunk(String junk) {
+			this.junk = junk;
+		}
+	}
+
+	public static class SetMethodThrowsException {
+		private String junk = "junk";
+
+		public String getJunk() {
+			return this.junk;
+		}
+
+		@SuppressWarnings("unused")
+		public void setJunk(String junk) {
+			throw new UnsupportedOperationException();
+		}
+	}
+
+	// ----------------------------------------------------------------------
+	// Test simple success and innocuous variations
+	// ----------------------------------------------------------------------
+
+	@Test
+	public void simpleSuccess() {
+		expectSuccess(new SimpleSuccess().insertField("label", "a prize"));
+	}
+
+	@Test
+	public void getButNoSet() {
+		expectSuccess(new GetButNoSet().insertField("label", "shouldBeEqual"));
+	}
+
+	@Test
+	public void getTakesParameters() {
+		expectSuccess(new GetTakesParameters().insertField("label", "fine"));
+	}
+
+	@Test
+	public void getReturnsVoid() {
+		expectSuccess(new GetReturnsVoid().insertField("label", "fine"));
+	}
+
+	@Test
+	public void getAndSetDontMatch() {
+		expectSuccess(new GetAndSetDontMatch().insertField("label", "fine"));
+	}
+
+	@Test
+	public void getIsStatic() {
+		expectSuccess(new GetIsStatic().insertField("label", "fine")
+				.insertField("instanceJunk", "the junk"));
+	}
+
+	@Test
+	public void getMethodIsPrivate() {
+		expectSuccess(new GetMethodIsPrivate().insertField("label", "fine"));
+	}
+
+	@Test
+	public void setMethodIsPrivate() {
+		expectSuccess(new SetMethodIsPrivate().insertField("label", "fine"));
+	}
+
+	private void expectSuccess(BeanBase original) {
+		BeanBase cloned = (BeanBase) OperationUtils.cloneBean(original);
+		assertEquals("Simple success", original, cloned);
+	}
+
+	public static abstract class BeanBase {
+		protected final Map<String, Object> fields = new HashMap<>();
+
+		public BeanBase insertField(String key, Object value) {
+			if (value != null) {
+				fields.put(key, value);
+			}
+			return this;
+		}
+
+		@Override
+		public int hashCode() {
+			return fields.hashCode();
+		}
+
+		@Override
+		public boolean equals(Object o) {
+			if (o == this) {
+				return true;
+			}
+			if (o == null) {
+				return false;
+			}
+			if (!this.getClass().equals(o.getClass())) {
+				return false;
+			}
+			return this.fields.equals(((BeanBase) o).fields);
+		}
+
+		@Override
+		public String toString() {
+			return this.getClass().getSimpleName() + fields;
+		}
+	}
+
+	public static class SimpleSuccess extends BeanBase {
+		public String getLabel() {
+			return (String) fields.get("label");
+		}
+
+		public void setLabel(String label) {
+			insertField("label", label);
+		}
+	}
+
+	public static class GetButNoSet extends SimpleSuccess {
+		public String getJunk() {
+			throw new UnsupportedOperationException();
+		}
+	}
+
+	public static class GetTakesParameters extends SimpleSuccess {
+		@SuppressWarnings("unused")
+		public String getJunk(String why) {
+			throw new UnsupportedOperationException();
+		}
+
+		@SuppressWarnings("unused")
+		public void setJunk(String junk) {
+			throw new UnsupportedOperationException();
+		}
+	}
+
+	public static class GetReturnsVoid extends SimpleSuccess {
+		public void getJunk() {
+			throw new UnsupportedOperationException();
+		}
+
+		@SuppressWarnings("unused")
+		public void setJunk(String junk) {
+			throw new UnsupportedOperationException();
+		}
+	}
+
+	public static class GetAndSetDontMatch extends SimpleSuccess {
+		public String getJunk() {
+			throw new UnsupportedOperationException();
+		}
+
+		@SuppressWarnings("unused")
+		public void setJunk(Integer junk) {
+			throw new UnsupportedOperationException();
+		}
+	}
+
+	public static class GetIsStatic extends SimpleSuccess {
+		public static String getJunk() {
+			return ("the junk");
+		}
+
+		public void setJunk(String junk) {
+			insertField("instanceJunk", junk);
+		}
+	}
+
+	public static class GetMethodIsPrivate extends SimpleSuccess {
+		@SuppressWarnings("unused")
+		private String getJunk() {
+			return ("the junk");
+		}
+
+		public void setJunk(String junk) {
+			insertField("instanceJunk", junk);
+		}
+	}
+
+	public static class SetMethodIsPrivate extends SimpleSuccess {
+		public String getJunk() {
+			return ("the junk");
+		}
+
+		@SuppressWarnings("unused")
+		private void setJunk(String junk) {
+			insertField("instanceJunk", junk);
+		}
+	}
+
+}