From 045ee522321ae99085ef2862f10892ba01837f20 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Tue, 14 Jun 2016 20:08:26 +0200 Subject: [PATCH] Invoke target.toString() safely in ReflectionTestUtils ReflectionTestUtils invokes toString() on target objects in order to build exception and logging messages, and prior to this commit problems could occur if the invocation of toString() threw an exception. This commit addresses this issue by ensuring that all invocations of toString() on target objects within ReflectionTestUtils are performed safely within try-catch blocks. Issue: SPR-14363 --- .../test/util/ReflectionTestUtils.java | 46 ++++++++++++------- .../test/util/ReflectionTestUtilsTests.java | 46 ++++++++++++++----- .../test/util/subpackage/LegacyEntity.java | 34 ++++++++++++-- .../subpackage/LegacyEntityException.java | 32 +++++++++++++ .../src/test/resources/log4j.properties | 3 ++ 5 files changed, 128 insertions(+), 33 deletions(-) create mode 100644 spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntityException.java diff --git a/spring-test/src/main/java/org/springframework/test/util/ReflectionTestUtils.java b/spring-test/src/main/java/org/springframework/test/util/ReflectionTestUtils.java index 87400ff5419..4f508511f51 100644 --- a/spring-test/src/main/java/org/springframework/test/util/ReflectionTestUtils.java +++ b/spring-test/src/main/java/org/springframework/test/util/ReflectionTestUtils.java @@ -173,14 +173,14 @@ public class ReflectionTestUtils { Field field = ReflectionUtils.findField(targetClass, name, type); if (field == null) { throw new IllegalArgumentException(String.format( - "Could not find field '%s' of type [%s] on target object [%s] or target class [%s]", name, type, - ultimateTarget, targetClass)); + "Could not find field '%s' of type [%s] on %s or target class [%s]", name, type, + safeToString(ultimateTarget), targetClass)); } if (logger.isDebugEnabled()) { logger.debug(String.format( - "Setting field '%s' of type [%s] on target object [%s] or target class [%s] to value [%s]", name, type, - ultimateTarget, targetClass, value)); + "Setting field '%s' of type [%s] on %s or target class [%s] to value [%s]", name, type, + safeToString(ultimateTarget), targetClass, value)); } ReflectionUtils.makeAccessible(field); ReflectionUtils.setField(field, ultimateTarget, value); @@ -253,14 +253,13 @@ public class ReflectionTestUtils { Field field = ReflectionUtils.findField(targetClass, name); if (field == null) { - throw new IllegalArgumentException( - String.format("Could not find field '%s' on target object [%s] or target class [%s]", name, - ultimateTarget, targetClass)); + throw new IllegalArgumentException(String.format("Could not find field '%s' on %s or target class [%s]", + name, safeToString(ultimateTarget), targetClass)); } if (logger.isDebugEnabled()) { - logger.debug(String.format("Getting field '%s' from target object [%s] or target class [%s]", name, - ultimateTarget, targetClass)); + logger.debug(String.format("Getting field '%s' from %s or target class [%s]", name, + safeToString(ultimateTarget), targetClass)); } ReflectionUtils.makeAccessible(field); return ReflectionUtils.getField(field, ultimateTarget); @@ -327,13 +326,16 @@ public class ReflectionTestUtils { method = ReflectionUtils.findMethod(target.getClass(), setterMethodName, paramTypes); } if (method == null) { - throw new IllegalArgumentException("Could not find setter method '" + setterMethodName + - "' on target [" + target + "] with parameter type [" + type + "]"); + throw new IllegalArgumentException(String.format( + "Could not find setter method '%s' on %s with parameter type [%s]", setterMethodName, + safeToString(target), type)); } if (logger.isDebugEnabled()) { - logger.debug("Invoking setter method '" + setterMethodName + "' on target [" + target + "]"); + logger.debug(String.format("Invoking setter method '%s' on %s with value [%s]", setterMethodName, + safeToString(target), value)); } + ReflectionUtils.makeAccessible(method); ReflectionUtils.invokeMethod(method, target, value); } @@ -372,12 +374,12 @@ public class ReflectionTestUtils { method = ReflectionUtils.findMethod(target.getClass(), getterMethodName); } if (method == null) { - throw new IllegalArgumentException("Could not find getter method '" + getterMethodName + - "' on target [" + target + "]"); + throw new IllegalArgumentException(String.format( + "Could not find getter method '%s' on %s", getterMethodName, safeToString(target))); } if (logger.isDebugEnabled()) { - logger.debug("Invoking getter method '" + getterMethodName + "' on target [" + target + "]"); + logger.debug(String.format("Invoking getter method '%s' on %s", getterMethodName, safeToString(target))); } ReflectionUtils.makeAccessible(method); return ReflectionUtils.invokeMethod(method, target); @@ -412,8 +414,8 @@ public class ReflectionTestUtils { methodInvoker.prepare(); if (logger.isDebugEnabled()) { - logger.debug("Invoking method '" + name + "' on target [" + target + "] with arguments [" + - ObjectUtils.nullSafeToString(args) + "]"); + logger.debug(String.format("Invoking method '%s' on %s with arguments %s", name, safeToString(target), + ObjectUtils.nullSafeToString(args))); } return (T) methodInvoker.invoke(); @@ -424,4 +426,14 @@ public class ReflectionTestUtils { } } + private static String safeToString(Object target) { + try { + return String.format("target object [%s]", target); + } + catch (Exception ex) { + return String.format("target of type [%s] whose toString() method threw [%s]", + (target != null ? target.getClass().getName() : "unknown"), ex); + } + } + } diff --git a/spring-test/src/test/java/org/springframework/test/util/ReflectionTestUtilsTests.java b/spring-test/src/test/java/org/springframework/test/util/ReflectionTestUtilsTests.java index 0216e362f75..3ea39f9b8b4 100644 --- a/spring-test/src/test/java/org/springframework/test/util/ReflectionTestUtilsTests.java +++ b/spring-test/src/test/java/org/springframework/test/util/ReflectionTestUtilsTests.java @@ -48,6 +48,8 @@ public class ReflectionTestUtilsTests { private final Component component = new Component(); + private final LegacyEntity entity = new LegacyEntity(); + @Rule public final ExpectedException exception = ExpectedException.none(); @@ -202,17 +204,6 @@ public class ReflectionTestUtilsTests { setField(person, "likesPets", null, boolean.class); } - /** - * Verifies behavior requested in SPR-9571. - */ - @Test - public void setFieldOnLegacyEntityWithSideEffectsInToString() { - String testCollaborator = "test collaborator"; - LegacyEntity entity = new LegacyEntity(); - setField(entity, "collaborator", testCollaborator, Object.class); - assertTrue(entity.toString().contains(testCollaborator)); - } - @Test public void setStaticFieldViaClass() throws Exception { setField(StaticFields.class, "publicField", "xxx"); @@ -398,4 +389,37 @@ public class ReflectionTestUtilsTests { invokeMethod(component, "configure", new Integer(42), "enigma", "baz", "quux"); } + @Test // SPR-14363 + public void getFieldOnLegacyEntityWithSideEffectsInToString() { + Object collaborator = getField(entity, "collaborator"); + assertNotNull(collaborator); + } + + @Test // SPR-9571 and SPR-14363 + public void setFieldOnLegacyEntityWithSideEffectsInToString() { + String testCollaborator = "test collaborator"; + setField(entity, "collaborator", testCollaborator, Object.class); + assertTrue(entity.toString().contains(testCollaborator)); + } + + @Test // SPR-14363 + public void invokeMethodOnLegacyEntityWithSideEffectsInToString() { + invokeMethod(entity, "configure", new Integer(42), "enigma"); + assertEquals("number should have been configured", new Integer(42), entity.getNumber()); + assertEquals("text should have been configured", "enigma", entity.getText()); + } + + @Test // SPR-14363 + public void invokeGetterMethodOnLegacyEntityWithSideEffectsInToString() { + Object collaborator = invokeGetterMethod(entity, "collaborator"); + assertNotNull(collaborator); + } + + @Test // SPR-14363 + public void invokeSetterMethodOnLegacyEntityWithSideEffectsInToString() { + String testCollaborator = "test collaborator"; + invokeSetterMethod(entity, "collaborator", testCollaborator); + assertTrue(entity.toString().contains(testCollaborator)); + } + } diff --git a/spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntity.java b/spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntity.java index 8c8ddfd4af1..72d4633f098 100644 --- a/spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntity.java +++ b/spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntity.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2016 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. @@ -31,17 +31,41 @@ public class LegacyEntity { @Override public String toString() { - throw new RuntimeException( + throw new LegacyEntityException( "Invoking toString() on the default collaborator causes an undesirable side effect"); - }; + } }; + private Integer number; + private String text; + + + public void configure(Integer number, String text) { + this.number = number; + this.text = text; + } + + public Integer getNumber() { + return this.number; + } + + public String getText() { + return this.text; + } + + public Object getCollaborator() { + return this.collaborator; + } + + public void setCollaborator(Object collaborator) { + this.collaborator = collaborator; + } @Override public String toString() { return new ToStringCreator(this)// - .append("collaborator", this.collaborator)// - .toString(); + .append("collaborator", this.collaborator)// + .toString(); } } diff --git a/spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntityException.java b/spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntityException.java new file mode 100644 index 00000000000..2318e90a5f5 --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/util/subpackage/LegacyEntityException.java @@ -0,0 +1,32 @@ +/* + * Copyright 2002-2016 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.test.util.subpackage; + +/** + * Exception thrown by a {@link LegacyEntity}. + * + * @author Sam Brannen + * @since 4.3.1 + */ +@SuppressWarnings("serial") +public class LegacyEntityException extends RuntimeException { + + public LegacyEntityException(String message) { + super(message); + } + +} diff --git a/spring-test/src/test/resources/log4j.properties b/spring-test/src/test/resources/log4j.properties index 8b88c2e0798..258e7b8425c 100644 --- a/spring-test/src/test/resources/log4j.properties +++ b/spring-test/src/test/resources/log4j.properties @@ -25,6 +25,9 @@ log4j.logger.org.springframework.test.context.web=WARN #log4j.logger.org.springframework.test.context.support.AbstractGenericContextLoader=INFO #log4j.logger.org.springframework.test.context.support.AnnotationConfigContextLoader=INFO +# The following must be kept at DEBUG in order to test SPR-14363. +log4j.logger.org.springframework.test.util=DEBUG + log4j.logger.org.springframework.test.web.servlet.result=DEBUG #log4j.logger.org.springframework.test=TRACE