Author: bayard Date: Fri Jan 26 23:11:08 2007 New Revision: 500495 URL: http://svn.apache.org/viewvc?view=rev&rev=500495 Log: Applying a modified version of Maarten Coene's patch for #LANG-69. All unit tests pass; opinions would be very welcome though.
Modified: jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ReflectionToStringBuilder.java jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringStyle.java jakarta/commons/proper/lang/trunk/src/test/org/apache/commons/lang/builder/ToStringBuilderTest.java Modified: jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ReflectionToStringBuilder.java URL: http://svn.apache.org/viewvc/jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ReflectionToStringBuilder.java?view=diff&rev=500495&r1=500494&r2=500495 ============================================================================== --- jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ReflectionToStringBuilder.java (original) +++ jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ReflectionToStringBuilder.java Fri Jan 26 23:11:08 2007 @@ -95,57 +95,6 @@ * @version $Id$ */ public class ReflectionToStringBuilder extends ToStringBuilder { - /** - * <p> - * A registry of objects used by <code>reflectionToString</code> methods to detect cyclical object references and - * avoid infinite loops. - * </p> - */ - private static ThreadLocal registry = new ThreadLocal() { - protected synchronized Object initialValue() { - // The HashSet implementation is not synchronized, - // which is just what we need here. - return new HashSet(); - } - }; - - /** - * <p> - * Returns the registry of objects being traversed by the <code>reflectionToString</code> methods in the current - * thread. - * </p> - * - * @return Set the registry of objects being traversed - */ - static Set getRegistry() { - return (Set) registry.get(); - } - - /** - * <p> - * Returns <code>true</code> if the registry contains the given object. Used by the reflection methods to avoid - * infinite loops. - * </p> - * - * @param value - * The object to lookup in the registry. - * @return boolean <code>true</code> if the registry contains the given object. - */ - static boolean isRegistered(Object value) { - return getRegistry().contains(value); - } - - /** - * <p> - * Registers the given object. Used by the reflection methods to avoid infinite loops. - * </p> - * - * @param value - * The object to register. - */ - static void register(Object value) { - getRegistry().add(value); - } /** * <p> @@ -463,22 +412,6 @@ } /** - * <p> - * Unregisters the given object. - * </p> - * - * <p> - * Used by the reflection methods to avoid infinite loops. - * </p> - * - * @param value - * The object to unregister. - */ - static void unregister(Object value) { - getRegistry().remove(value); - } - - /** * Whether or not to append static fields. */ private boolean appendStatics = false; @@ -657,60 +590,29 @@ * The class of object parameter */ protected void appendFieldsIn(Class clazz) { - if (isRegistered(this.getObject())) { - // The object has already been appended, therefore we have an - // object cycle. - // Append a simple Object.toString style string. The field name is - // already appended at this point. - this.appendAsObjectToString(this.getObject()); + if (clazz.isArray()) { + this.reflectionAppendArray(this.getObject()); return; } - try { - this.registerObject(); - if (clazz.isArray()) { - this.reflectionAppendArray(this.getObject()); - return; - } - Field[] fields = clazz.getDeclaredFields(); - AccessibleObject.setAccessible(fields, true); - for (int i = 0; i < fields.length; i++) { - Field field = fields[i]; - String fieldName = field.getName(); - if (this.accept(field)) { - try { - // Warning: Field.get(Object) creates wrappers objects - // for primitive types. - Object fieldValue = this.getValue(field); - if (isRegistered(fieldValue) && !field.getType().isPrimitive()) { - // A known field value has already been appended, - // therefore we have an object cycle, - // append a simple Object.toString style string. - this.getStyle().appendFieldStart(this.getStringBuffer(), fieldName); - this.appendAsObjectToString(fieldValue); - this.getStyle().appendFieldEnd(this.getStringBuffer(), fieldName); - // The recursion out of - // builder.append(fieldName, fieldValue); - // below will append the field - // end marker. - } else { - try { - this.registerObject(); - this.append(fieldName, fieldValue); - } finally { - this.unregisterObject(); - } - } - } catch (IllegalAccessException ex) { - // this can't happen. Would get a Security exception - // instead - // throw a runtime exception in case the impossible - // happens. - throw new InternalError("Unexpected IllegalAccessException: " + ex.getMessage()); - } + Field[] fields = clazz.getDeclaredFields(); + AccessibleObject.setAccessible(fields, true); + for (int i = 0; i < fields.length; i++) { + Field field = fields[i]; + String fieldName = field.getName(); + if (this.accept(field)) { + try { + // Warning: Field.get(Object) creates wrappers objects + // for primitive types. + Object fieldValue = this.getValue(field); + this.append(fieldName, fieldValue); + } catch (IllegalAccessException ex) { + //this can't happen. Would get a Security exception + // instead + //throw a runtime exception in case the impossible + // happens. + throw new InternalError("Unexpected IllegalAccessException: " + ex.getMessage()); } } - } finally { - this.unregisterObject(); } } @@ -791,15 +693,6 @@ /** * <p> - * Registers this builder's source object to avoid infinite loops when processing circular object references. - * </p> - */ - void registerObject() { - register(this.getObject()); - } - - /** - * <p> * Sets whether or not to append static fields. * </p> * @@ -872,12 +765,4 @@ return super.toString(); } - /** - * <p> - * Unregisters this builder's source object to avoid infinite loops when processing circular object references. - * </p> - */ - void unregisterObject() { - unregister(this.getObject()); - } } Modified: jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringStyle.java URL: http://svn.apache.org/viewvc/jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringStyle.java?view=diff&rev=500495&r1=500494&r2=500495 ============================================================================== --- jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringStyle.java (original) +++ jakarta/commons/proper/lang/trunk/src/java/org/apache/commons/lang/builder/ToStringStyle.java Fri Jan 26 23:11:08 2007 @@ -19,7 +19,9 @@ import java.io.Serializable; import java.lang.reflect.Array; import java.util.Collection; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import org.apache.commons.lang.ClassUtils; import org.apache.commons.lang.ObjectUtils; @@ -95,6 +97,78 @@ public static final ToStringStyle SIMPLE_STYLE = new SimpleToStringStyle(); /** + * <p> + * A registry of objects used by <code>reflectionToString</code> methods + * to detect cyclical object references and avoid infinite loops. + * </p> + */ + private static ThreadLocal registry = new ThreadLocal() { + protected synchronized Object initialValue() { + // The HashSet implementation is not synchronized, + // which is just what we need here. + return new HashSet(); + } + }; + + /** + * <p> + * Returns the registry of objects being traversed by the <code>reflectionToString</code> + * methods in the current thread. + * </p> + * + * @return Set the registry of objects being traversed + */ + static Set getRegistry() { + return (Set) registry.get(); + } + + /** + * <p> + * Returns <code>true</code> if the registry contains the given object. + * Used by the reflection methods to avoid infinite loops. + * </p> + * + * @param value + * The object to lookup in the registry. + * @return boolean <code>true</code> if the registry contains the given + * object. + */ + static boolean isRegistered(Object value) { + return getRegistry().contains(value); + } + + /** + * <p> + * Registers the given object. Used by the reflection methods to avoid + * infinite loops. + * </p> + * + * @param value + * The object to register. + */ + static void register(Object value) { + if (value != null) { + getRegistry().add(value); + } + } + + /** + * <p> + * Unregisters the given object. + * </p> + * + * <p> + * Used by the reflection methods to avoid infinite loops. + * </p> + * + * @param value + * The object to unregister. + */ + static void unregister(Object value) { + getRegistry().remove(value); + } + + /** * Whether to use the field names, the default is <code>true</code>. */ private boolean useFieldNames = true; @@ -272,6 +346,7 @@ removeLastFieldSeparator(buffer); } appendContentEnd(buffer); + unregister(object); } /** @@ -343,11 +418,14 @@ * @param detail output detail or not */ protected void appendInternal(StringBuffer buffer, String fieldName, Object value, boolean detail) { - if (ReflectionToStringBuilder.isRegistered(value) + if (isRegistered(value) && !(value instanceof Number || value instanceof Boolean || value instanceof Character)) { - ObjectUtils.appendIdentityToString(buffer, value); - - } else if (value instanceof Collection) { + appendCyclicObject(buffer, fieldName, value); + return; + } + register(value); +try { + if (value instanceof Collection) { if (detail) { appendDetail(buffer, fieldName, (Collection) value); } else { @@ -425,12 +503,31 @@ } } else { - if (detail) { - appendDetail(buffer, fieldName, value); - } else { - appendSummary(buffer, fieldName, value); - } + if (detail) { + appendDetail(buffer, fieldName, value); + } else { + appendSummary(buffer, fieldName, value); + } } + } finally { + unregister(value); + } + } + + /** + * <p>Append to the <code>toString</code> an <code>Object</code> + * value that has been detected to participate in a cycle. This + * implementation will print the standard string value of the value.</p> + * + * @param buffer the <code>StringBuffer</code> to populate + * @param fieldName the field name, typically not used as already appended + * @param value the value to add to the <code>toString</code>, + * not <code>null</code> + * + * @since 2.2 + */ + protected void appendCyclicObject(StringBuffer buffer, String fieldName, Object value) { + ObjectUtils.appendIdentityToString(buffer, value); } /** @@ -1301,6 +1398,7 @@ */ protected void appendClassName(StringBuffer buffer, Object object) { if (useClassName && object != null) { + register(object); if (useShortClassName) { buffer.append(getShortClassName(object.getClass())); } else { @@ -1317,6 +1415,7 @@ */ protected void appendIdentityHashCode(StringBuffer buffer, Object object) { if (this.isUseIdentityHashCode() && object!=null) { + register(object); buffer.append('@'); buffer.append(Integer.toHexString(System.identityHashCode(object))); } Modified: jakarta/commons/proper/lang/trunk/src/test/org/apache/commons/lang/builder/ToStringBuilderTest.java URL: http://svn.apache.org/viewvc/jakarta/commons/proper/lang/trunk/src/test/org/apache/commons/lang/builder/ToStringBuilderTest.java?view=diff&rev=500495&r1=500494&r2=500495 ============================================================================== --- jakarta/commons/proper/lang/trunk/src/test/org/apache/commons/lang/builder/ToStringBuilderTest.java (original) +++ jakarta/commons/proper/lang/trunk/src/test/org/apache/commons/lang/builder/ToStringBuilderTest.java Fri Jan 26 23:11:08 2007 @@ -168,7 +168,7 @@ assertEquals(baseStr + "[{<null>,5,{3,6}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionLongArray() { @@ -177,7 +177,7 @@ assertEquals(baseStr + "[{1,2,-3,4}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionIntArray() { @@ -186,7 +186,7 @@ assertEquals(baseStr + "[{1,2,-3,4}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionShortArray() { @@ -195,7 +195,7 @@ assertEquals(baseStr + "[{1,2,-3,4}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionyteArray() { @@ -204,7 +204,7 @@ assertEquals(baseStr + "[{1,2,-3,4}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionCharArray() { @@ -213,7 +213,7 @@ assertEquals(baseStr + "[{A,2,_,D}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionDoubleArray() { @@ -222,7 +222,7 @@ assertEquals(baseStr + "[{1.0,2.9876,-3.00001,4.3}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionFloatArray() { @@ -231,7 +231,7 @@ assertEquals(baseStr + "[{1.0,2.9876,-3.00001,4.3}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionBooleanArray() { @@ -240,7 +240,7 @@ assertEquals(baseStr + "[{true,false,false}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } // Reflection Array Array tests @@ -251,7 +251,7 @@ assertEquals(baseStr + "[{{1.0,2.29686},<null>,{NaN}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } @@ -261,7 +261,7 @@ assertEquals(baseStr + "[{{1,2},<null>,{5}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionIntArrayArray() { @@ -270,7 +270,7 @@ assertEquals(baseStr + "[{{1,2},<null>,{5}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionhortArrayArray() { @@ -279,7 +279,7 @@ assertEquals(baseStr + "[{{1,2},<null>,{5}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionByteArrayArray() { @@ -288,7 +288,7 @@ assertEquals(baseStr + "[{{1,2},<null>,{5}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionCharArrayArray() { @@ -297,7 +297,7 @@ assertEquals(baseStr + "[{{A,B},<null>,{p}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionDoubleArrayArray() { @@ -306,7 +306,7 @@ assertEquals(baseStr + "[{{1.0,2.29686},<null>,{NaN}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionBooleanArrayArray() { @@ -316,7 +316,7 @@ assertEquals(baseStr + "[{{true,false},<null>,{false}}]", ToStringBuilder.reflectionToString(array)); array = null; assertReflectionArray("<null>", array); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } // Reflection hierarchy tests @@ -326,7 +326,7 @@ String baseStr = this.toBaseString(base); assertEquals(baseStr + "[elementData={<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>},size=0,modCount=0]", ToStringBuilder.reflectionToString(base, null, true)); assertEquals(baseStr + "[size=0]", ToStringBuilder.reflectionToString(base, null, false)); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionHierarchy() { @@ -353,7 +353,7 @@ assertEquals(baseStr + "[b=b,a=a]", ToStringBuilder.reflectionToString(baseB, null, false, List.class)); assertEquals(baseStr + "[b=b,a=a]", ToStringBuilder.reflectionToString(baseB, null, false, ReflectionTestFixtureA.class)); assertEquals(baseStr + "[b=b]", ToStringBuilder.reflectionToString(baseB, null, false, ReflectionTestFixtureB.class)); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } static class ReflectionTestFixtureA { @@ -394,7 +394,7 @@ assertEquals( this.toBaseString(objects) + "[{" + this.toBaseString(objects) + "}]", ToStringBuilder.reflectionToString(objects)); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } /** @@ -411,7 +411,7 @@ assertEquals( this.toBaseString(objectsLevel2) + "[{{" + this.toBaseString(objectsLevel2) + "}}]", ToStringBuilder.reflectionToString(objectsLevel2)); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } public void testReflectionArrayArrayCycle() throws Exception { @@ -433,7 +433,7 @@ + basicToString + "}}]", ToStringBuilder.reflectionToString(objects)); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } /** @@ -514,9 +514,9 @@ public void testSimpleReflectionObjectCycle() throws Exception { SimpleReflectionTestFixture simple = new SimpleReflectionTestFixture(); simple.o = simple; - assertTrue(ReflectionToStringBuilder.getRegistry().isEmpty()); + assertTrue(ToStringStyle.getRegistry().isEmpty()); assertEquals(this.toBaseString(simple) + "[o=" + this.toBaseString(simple) + "]", simple.toString()); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } /** @@ -526,9 +526,9 @@ */ public void testSelfInstanceVarReflectionObjectCycle() throws Exception { SelfInstanceVarReflectionTestFixture test = new SelfInstanceVarReflectionTestFixture(); - assertTrue(ReflectionToStringBuilder.getRegistry().isEmpty()); + assertTrue(ToStringStyle.getRegistry().isEmpty()); assertEquals(this.toBaseString(test) + "[typeIsSelf=" + this.toBaseString(test) + "]", test.toString()); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } /** @@ -539,9 +539,9 @@ */ public void testSelfInstanceTwoVarsReflectionObjectCycle() throws Exception { SelfInstanceTwoVarsReflectionTestFixture test = new SelfInstanceTwoVarsReflectionTestFixture(); - assertTrue(ReflectionToStringBuilder.getRegistry().isEmpty()); + assertTrue(ToStringStyle.getRegistry().isEmpty()); assertEquals(this.toBaseString(test) + "[typeIsSelf=" + this.toBaseString(test) + ",otherType=" + test.getOtherType().toString() + "]", test.toString()); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } @@ -558,9 +558,9 @@ assertEquals( this.toBaseString(a) + "[b=" + this.toBaseString(b) + "[a=" + this.toBaseString(a) + "]]", a.toString()); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } - + /** * Test a nasty combination of arrays and Objects pointing to each other. * objects[0] -> SimpleReflectionTestFixture[ o -> objects ] @@ -586,11 +586,15 @@ + this.toBaseString(simple) + "}]", ToStringBuilder.reflectionToString(simple)); - this.validateEmptyReflectionRegistry(); + this.validateEmptyToStringStyleRegistry(); } - void validateEmptyReflectionRegistry() { - assertTrue(ReflectionToStringBuilder.getRegistry().isEmpty()); + void validateEmptyToStringStyleRegistry() { + if (!ToStringStyle.getRegistry().isEmpty()) { + System.out.println(ToStringStyle.getRegistry()); + } + + assertTrue(ToStringStyle.getRegistry().isEmpty()); } // End: Reflection cycle tests @@ -831,6 +835,25 @@ assertEquals(baseStr + "[<null>]", new ToStringBuilder(base).append((Object) array).toString()); } + public void testObjectCycle() { + ObjectCycle a = new ObjectCycle(); + ObjectCycle b = new ObjectCycle(); + a.obj = b; + b.obj = a; + + String expected = toBaseString(a) + "[" + toBaseString(b) + "[" + toBaseString(a) + "]]"; + assertEquals(expected, a.toString()); + validateEmptyToStringStyleRegistry(); + } + + static class ObjectCycle { + Object obj; + + public String toString() { + return new ToStringBuilder(this).append(obj).toString(); + } + } + public void testSimpleReflectionStatics() { SimpleReflectionStaticFieldsFixture instance1 = new SimpleReflectionStaticFieldsFixture(); assertEquals( @@ -947,26 +970,5 @@ public void testReflectionNull() { assertEquals("<null>", ReflectionToStringBuilder.toString(null)); } - - /* Unit test for #36061 - public void testObjectCycle() { - ObjectCycle a = new ObjectCycle(); - ObjectCycle b = new ObjectCycle(); - a.obj = b; - b.obj = a; - - String expected = toBaseString(a) + "[" + toBaseString(b) + "[" + toBaseString(a) + "]]"; - assertEquals(expected, a.toString()); - validateEmptyReflectionRegistry(); - } - - static class ObjectCycle { - Object obj; - - public String toString() { - return new ToStringBuilder(this).append(obj).toString(); - } - } - */ } --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]