This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-lang.git
The following commit(s) were added to refs/heads/master by this push:
new d1e60e04f Align ReflectionDiffBuilder with AbstractReflection and add
cycle detection (#1778)
d1e60e04f is described below
commit d1e60e04f7ddfd51772fbf0859c12407d17e600b
Author: gaurav kumar pandey <[email protected]>
AuthorDate: Mon Aug 24 04:38:56 2026 +0530
Align ReflectionDiffBuilder with AbstractReflection and add cycle detection
(#1778)
---
src/changes/changes.xml | 1 +
.../apache/commons/lang3/builder/DiffBuilder.java | 15 +-
.../lang3/builder/ReflectionDiffBuilder.java | 124 +++++++++++++----
.../lang3/builder/ReflectionDiffBuilderTest.java | 151 +++++++++++++++++++++
4 files changed, 261 insertions(+), 30 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 021fab1b8..264f3a821 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -46,6 +46,7 @@ The <action> type attribute can be add,update,fix,remove.
<body>
<release version="3.21.0" date="YYY-MM-DD" description="This is a feature
and maintenance release. Java 8 or later is required.">
<!-- FIX -->
+ <action type="fix" dev="ggregory" due-to="Gaurav Pandey,
Gary Gregory">Align ReflectionDiffBuilder with AbstractReflection and add cycle
detection to prevent StackOverflowError on cyclic object graphs.</action>
<action type="fix" dev="ggregory" due-to="Javid Khan,
Gary Gregory">Stop ExtendedMessageFormat seekNonWs reading past the pattern
end.</action>
<action type="fix" dev="ggregory" due-to="ThrawnCA">Fix
spelling and grammar in StringUtils #1486.</action>
<action type="fix" dev="ggregory" due-to="Michael
Hausegger, Gary Gregory">Add ConversionTest assertions to increase coverage
#1489.</action>
diff --git a/src/main/java/org/apache/commons/lang3/builder/DiffBuilder.java
b/src/main/java/org/apache/commons/lang3/builder/DiffBuilder.java
index 45e0b2777..368cc0ec8 100644
--- a/src/main/java/org/apache/commons/lang3/builder/DiffBuilder.java
+++ b/src/main/java/org/apache/commons/lang3/builder/DiffBuilder.java
@@ -270,7 +270,20 @@ private DiffBuilder(final T left, final T right, final
ToStringStyle style, fina
this.toStringFormat = toStringFormat;
this.style = style != null ? style : ToStringStyle.DEFAULT_STYLE;
// Don't compare any fields if objects equal
- this.equals = testObjectsEquals && Objects.equals(left, right);
+ if (testObjectsEquals) {
+ if (left == right || ReflectionDiffBuilder.isRegistered(left,
right)) {
+ this.equals = true;
+ } else {
+ try {
+ ReflectionDiffBuilder.register(left, right);
+ this.equals = Objects.equals(left, right);
+ } finally {
+ ReflectionDiffBuilder.unregister(left, right);
+ }
+ }
+ } else {
+ this.equals = false;
+ }
}
private <F> DiffBuilder<T> add(final String fieldName, final
SerializableSupplier<F> left, final SerializableSupplier<F> right, final
Class<F> type) {
diff --git
a/src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java
b/src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java
index baf78bf22..e2209ccfc 100644
--- a/src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java
+++ b/src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java
@@ -19,12 +19,15 @@
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.Arrays;
+import java.util.HashSet;
import java.util.Objects;
+import java.util.Set;
import org.apache.commons.lang3.ArraySorter;
import org.apache.commons.lang3.ArrayUtils;
import org.apache.commons.lang3.ClassUtils;
import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.commons.lang3.tuple.Pair;
/**
* Assists in implementing {@link Diffable#diff(Object)} methods.
@@ -73,9 +76,10 @@
* @see DiffResult
* @see ToStringStyle
* @see DiffBuilder
+ * @see AbstractBuilder#setForceAccessible(boolean)
* @since 3.6
*/
-public class ReflectionDiffBuilder<T> implements Builder<DiffResult<T>> {
+public class ReflectionDiffBuilder<T> extends AbstractReflection implements
Builder<DiffResult<T>> {
/**
* Constructs a new instance.
@@ -83,7 +87,7 @@ public class ReflectionDiffBuilder<T> implements
Builder<DiffResult<T>> {
* @param <T> type of the left and right object.
* @since 3.15.0
*/
- public static final class Builder<T> {
+ public static final class Builder<T> extends AbstractBuilder<Builder<T>> {
private String[] excludeFieldNames = ArrayUtils.EMPTY_STRING_ARRAY;
private DiffBuilder<T> diffBuilder;
@@ -101,7 +105,12 @@ public Builder() {
* @return A new configured {@link ReflectionDiffBuilder}.
*/
public ReflectionDiffBuilder<T> build() {
- return new ReflectionDiffBuilder<>(diffBuilder, excludeFieldNames);
+ return new ReflectionDiffBuilder<>(this);
+ }
+
+ @Override
+ public ReflectionDiffBuilder<T> get() {
+ return build();
}
/**
@@ -128,6 +137,62 @@ public Builder<T> setExcludeFieldNames(final String...
excludeFieldNames) {
}
+ /**
+ * A registry of objects to detect cyclical object references, avoid
infinite loops, and stack overflows.
+ */
+ private static final ThreadLocal<Set<Pair<IDKey, IDKey>>> REGISTRY =
ThreadLocal.withInitial(HashSet::new);
+
+ /**
+ * Gets the registry of object pairs being traversed by the reflection
+ * methods in the current thread.
+ *
+ * @return Set the registry of objects being traversed
+ */
+ static Set<Pair<IDKey, IDKey>> getRegistry() {
+ return REGISTRY.get();
+ }
+
+ /**
+ * Tests whether the registry contains the given object pair.
+ * <p>
+ * Used by the reflection methods to avoid infinite loops.
+ * Objects might be swapped therefore a check is needed if the object pair
+ * is registered in the given or swapped order.
+ * </p>
+ *
+ * @param lhs {@code this} object to lookup in registry
+ * @param rhs The other object to lookup on registry
+ * @return boolean {@code true} if the registry contains the given object.
+ */
+ static boolean isRegistered(final Object lhs, final Object rhs) {
+ return isRegistered(lhs, rhs, getRegistry());
+ }
+
+ /**
+ * Registers the given object pair.
+ * Used by the reflection methods to avoid infinite loops.
+ *
+ * @param lhs {@code this} object to register
+ * @param rhs the other object to register
+ */
+ static void register(final Object lhs, final Object rhs) {
+ register(lhs, rhs, getRegistry());
+ }
+
+ /**
+ * Unregisters the given object pair.
+ *
+ * <p>
+ * Used by the reflection methods to avoid infinite loops.
+ * </p>
+ *
+ * @param lhs {@code this} object to unregister
+ * @param rhs the other object to unregister
+ */
+ static void unregister(final Object lhs, final Object rhs) {
+ unregister(lhs, rhs, getRegistry(), REGISTRY);
+ }
+
/**
* Constructs a new {@link Builder}.
*
@@ -154,16 +219,27 @@ private static String[] toExcludeFieldNames(final
String[] excludeFieldNames) {
*/
private String[] excludeFieldNames;
+ /**
+ * Constructs a new instance.
+ *
+ * @param builder A non-null Builder.
+ * @throws NullPointerException Thrown on null input.
+ */
+ private ReflectionDiffBuilder(final Builder<T> builder) {
+ super(Objects.requireNonNull(builder, "builder"));
+ this.diffBuilder = Objects.requireNonNull(builder.diffBuilder,
"diffBuilder");
+ this.excludeFieldNames =
Objects.requireNonNull(builder.excludeFieldNames, "excludeFieldNames");
+ }
+
/**
* Constructs a new instance.
*
* @param diffBuilder A non-null DiffBuilder.
* @param excludeFieldNames A non-null String array.
- * @throw NullPointerException Thrown on null input.
+ * @throws NullPointerException Thrown on null input.
*/
private ReflectionDiffBuilder(final DiffBuilder<T> diffBuilder, final
String[] excludeFieldNames) {
- this.diffBuilder = Objects.requireNonNull(diffBuilder, "diffBuilder");
- this.excludeFieldNames = Objects.requireNonNull(excludeFieldNames,
"excludeFieldNames");
+
this(ReflectionDiffBuilder.<T>builder().setDiffBuilder(diffBuilder).setExcludeFieldNames(excludeFieldNames));
}
/**
@@ -204,11 +280,11 @@ private void appendFields(final Class<?> clazz) {
for (final Field field : FieldUtils.getAllFields(clazz)) {
if (accept(field)) {
try {
- diffBuilder.append(field.getName(), readField(field,
getLeft()), readField(field, getRight()));
- } catch (final IllegalAccessException e) {
- // this can't happen. Would get a Security exception
instead
- // throw a runtime exception in case the impossible
happens.
- throw new IllegalArgumentException("Unexpected
IllegalAccessException: " + e.getMessage(), e);
+ if (setAccessible(field)) {
+ diffBuilder.append(field.getName(),
Reflection.getUnchecked(field, getLeft()), Reflection.getUnchecked(field,
getRight()));
+ }
+ } catch (final RuntimeException e) {
+ // Ignored as per AccessibleObject / SecurityManager /
InaccessibleObjectException
}
}
}
@@ -222,11 +298,16 @@ private void appendFields(final Class<?> clazz) {
*/
@Override
public DiffResult<T> build() {
- if (getLeft().equals(getRight())) {
+ if (getLeft() == getRight() || isRegistered(getLeft(), getRight())) {
return diffBuilder.build();
}
- appendFields(getLeft().getClass());
- return diffBuilder.build();
+ try {
+ register(getLeft(), getRight());
+ appendFields(getLeft().getClass());
+ return diffBuilder.build();
+ } finally {
+ unregister(getLeft(), getRight());
+ }
}
/**
@@ -247,21 +328,6 @@ private T getRight() {
return diffBuilder.getRight();
}
- /**
- * Reads a {@link Field}, forcing access if needed.
- *
- * @param field The field to use.
- * @param target The object to call on, may be {@code null} for {@code
static} fields.
- * @return The field value.
- * @throws NullPointerException if the field is {@code null}.
- * @throws IllegalAccessException if the field is not made accessible.
- * @throws SecurityException if an underlying accessible object's
method denies the request.
- * @see SecurityManager#checkPermission
- */
- private Object readField(final Field field, final Object target) throws
IllegalAccessException {
- return FieldUtils.readField(field, target, true);
- }
-
/**
* Sets the field names to exclude.
*
diff --git
a/src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java
b/src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java
index c350b4302..20c3aa6ce 100644
---
a/src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java
+++
b/src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java
@@ -21,6 +21,7 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import java.math.BigDecimal;
import java.math.BigInteger;
@@ -545,4 +546,154 @@ void testTransientFieldDifference() {
assertEquals(0, list.getNumberOfDiffs());
}
+ private static final class CycleDiffableNode implements
Diffable<CycleDiffableNode> {
+ @SuppressWarnings("unused")
+ private CycleDiffableNode self;
+ @SuppressWarnings("unused")
+ private final String value;
+
+ CycleDiffableNode(final String value) {
+ this.value = value;
+ }
+
+ @Override
+ public boolean equals(final Object obj) {
+ return EqualsBuilder.reflectionEquals(this, obj);
+ }
+
+ @Override
+ public int hashCode() {
+ return HashCodeBuilder.reflectionHashCode(this);
+ }
+
+ @Override
+ public DiffResult<CycleDiffableNode> diff(final CycleDiffableNode obj)
{
+ return ReflectionDiffBuilder.<CycleDiffableNode>builder()
+ .setDiffBuilder(DiffBuilder.<CycleDiffableNode>builder()
+ .setLeft(this)
+ .setRight(obj)
+ .setStyle(ToStringStyle.SHORT_PREFIX_STYLE)
+ .build())
+ .build()
+ .build();
+ }
+ }
+
+ private static final class MutualDiffableNode implements
Diffable<MutualDiffableNode> {
+ @SuppressWarnings("unused")
+ private MutualDiffableNode other;
+ @SuppressWarnings("unused")
+ private final String name;
+
+ MutualDiffableNode(final String name) {
+ this.name = name;
+ }
+
+ @Override
+ public boolean equals(final Object obj) {
+ return EqualsBuilder.reflectionEquals(this, obj);
+ }
+
+ @Override
+ public int hashCode() {
+ return HashCodeBuilder.reflectionHashCode(this);
+ }
+
+ @Override
+ public DiffResult<MutualDiffableNode> diff(final MutualDiffableNode
obj) {
+ return ReflectionDiffBuilder.<MutualDiffableNode>builder()
+ .setDiffBuilder(DiffBuilder.<MutualDiffableNode>builder()
+ .setLeft(this)
+ .setRight(obj)
+ .setStyle(ToStringStyle.SHORT_PREFIX_STYLE)
+ .build())
+ .build()
+ .build();
+ }
+ }
+
+ @Test
+ void testCycleSelfReferential() {
+ final CycleDiffableNode first = new CycleDiffableNode("a");
+ final CycleDiffableNode second = new CycleDiffableNode("a");
+ first.self = first;
+ second.self = second;
+
+ final DiffResult<CycleDiffableNode> result = first.diff(second);
+ assertEquals(0, result.getNumberOfDiffs());
+ assertTrue(ReflectionDiffBuilder.getRegistry().isEmpty(), "Registry
must be empty after diff");
+ }
+
+ @Test
+ void testCycleSelfReferentialWithDifference() {
+ final CycleDiffableNode first = new CycleDiffableNode("a");
+ final CycleDiffableNode second = new CycleDiffableNode("b");
+ first.self = first;
+ second.self = second;
+
+ final DiffResult<CycleDiffableNode> result = first.diff(second);
+ assertEquals(2, result.getNumberOfDiffs());
+ assertTrue(ReflectionDiffBuilder.getRegistry().isEmpty(), "Registry
must be empty after diff");
+ }
+
+ @Test
+ void testCycleMutuallyReferential() {
+ final MutualDiffableNode a = new MutualDiffableNode("node");
+ final MutualDiffableNode b = new MutualDiffableNode("node");
+ a.other = b;
+ b.other = a;
+
+ final MutualDiffableNode c = new MutualDiffableNode("node");
+ final MutualDiffableNode d = new MutualDiffableNode("node");
+ c.other = d;
+ d.other = c;
+
+ final DiffResult<MutualDiffableNode> result = a.diff(c);
+ assertEquals(0, result.getNumberOfDiffs());
+ assertTrue(ReflectionDiffBuilder.getRegistry().isEmpty(), "Registry
must be empty after diff");
+ }
+
+ @Test
+ void testCycleMutuallyReferentialWithDifference() {
+ final MutualDiffableNode a = new MutualDiffableNode("nodeA");
+ final MutualDiffableNode b = new MutualDiffableNode("nodeB");
+ a.other = b;
+ b.other = a;
+
+ final MutualDiffableNode c = new MutualDiffableNode("nodeA");
+ final MutualDiffableNode d = new MutualDiffableNode("nodeChanged");
+ c.other = d;
+ d.other = c;
+
+ final DiffResult<MutualDiffableNode> result = a.diff(c);
+ assertEquals(1, result.getNumberOfDiffs());
+ assertTrue(ReflectionDiffBuilder.getRegistry().isEmpty(), "Registry
must be empty after diff");
+ }
+
+ @Test
+ void testCycleAsymmetric() {
+ final CycleDiffableNode first = new CycleDiffableNode("a");
+ final CycleDiffableNode second = new CycleDiffableNode("a");
+ first.self = first;
+ second.self = null;
+
+ final DiffResult<CycleDiffableNode> result = first.diff(second);
+ assertEquals(1, result.getNumberOfDiffs());
+ assertEquals("self", result.getDiffs().get(0).getFieldName());
+ assertTrue(ReflectionDiffBuilder.getRegistry().isEmpty(), "Registry
must be empty after diff");
+ }
+
+ @Test
+ void testBuilderGetAndSetForceAccessible() {
+ final TypeTestClass first = new TypeTestClass();
+ final TypeTestClass second = new TypeTestClass();
+ final ReflectionDiffBuilder.Builder<TypeTestClass> builder =
ReflectionDiffBuilder.<TypeTestClass>builder()
+
.setDiffBuilder(DiffBuilder.<TypeTestClass>builder().setLeft(first).setRight(second).build())
+ .setForceAccessible(true);
+ final ReflectionDiffBuilder<TypeTestClass> diffBuilder = builder.get();
+ assertNotNull(diffBuilder);
+ assertTrue(diffBuilder.isForceAccessible());
+ assertEquals(0, diffBuilder.build().getNumberOfDiffs());
+ }
+
}
