Fixed "enter/leave" sequence bug in the circular reference detection

- Circular Reference Detection: Fixed 'Detected inconsistency in
    enter/leave sequence. Must always be LIFO.' bug that could occur
    due to inconsistent cleanup of the instance memory when a circular
    reference has been detected.

[Fixes #107]

Author: SQiShER

src/integration-test/java/de/danielbechler/diff/issues/issue107/EnterLeaveSequenceInconsistencyIT.groovy

@@ -19,37 +19,20 @@ package de.danielbechler.diff.issues.issue107
 import de.danielbechler.diff.ObjectDiffer
 import de.danielbechler.diff.ObjectDifferBuilder
 import de.danielbechler.diff.circular.CircularReferenceDetector
-import de.danielbechler.diff.inclusion.Inclusion
-import de.danielbechler.diff.inclusion.InclusionResolver
-import de.danielbechler.diff.node.DiffNode
+import de.danielbechler.diff.differ.DifferDispatcher
 import de.danielbechler.diff.node.PrintingVisitor
-import de.danielbechler.diff.selector.MapKeyElementSelector
 import spock.lang.Issue
 import spock.lang.Specification
 import spock.lang.Subject
 
-@Subject(CircularReferenceDetector)
+@Subject([CircularReferenceDetector, DifferDispatcher])
 @Issue("https://github.com/SQiShER/java-object-diff/issues/107")
 class EnterLeaveSequenceInconsistencyIT extends Specification {
 
 	def "Causes: 'Detected inconsistency in enter/leave sequence. Must always be LIFO'"() {
 
-		given: "ObjectDiffer with all properties named 'a' and 'map' excluded"
-		  ObjectDiffer differ = ObjectDifferBuilder.startBuilding()
-				  .inclusion().resolveUsing(new InclusionResolver() {
-			  @Override
-			  Inclusion getInclusion(DiffNode node) {
-				  // TODO some convenience methods to avoid these nasty null-checks would be nice
-				  if (node.propertyName == "a" &&
-						  node.parentNode != null &&
-						  node.parentNode.elementSelector instanceof MapKeyElementSelector &&
-						  node.parentNode.parentNode != null &&
-						  node.parentNode.parentNode.propertyName == "map") {
-					  return Inclusion.EXCLUDED
-				  }
-				  return Inclusion.DEFAULT
-			  }
-		  }).and().build()
+		given:
+		  ObjectDiffer differ = ObjectDifferBuilder.buildDefault()
 
 		and: "a working version in which 'a2' and 'c2' reference each other"
 		  A a2 = new A(s1: "a2")
@@ -75,14 +58,29 @@ class EnterLeaveSequenceInconsistencyIT extends Specification {
 	static class A {
 		def String s1
 		def Map<B, C> map = [:]
+
+		@Override
+		String toString() {
+			"A(${s1})"
+		}
 	}
 
 	static class B {
 		def String s2
+
+		@Override
+		String toString() {
+			"B(${s2})"
+		}
 	}
 
 	static class C {
 		def String s3
 		def A a
+
+		@Override
+		String toString() {
+			"C(${s3})"
+		}
 	}
 }

src/main/java/de/danielbechler/diff/circular/CircularReferenceDetector.java

@@ -70,6 +70,18 @@ public boolean knows(final Object needle)
 		return false;
 	}
 
+	private Entry entryForInstance(final Object instance)
+	{
+		for (final Entry entry : stack)
+		{
+			if (isMatch(instance, entry.getInstance()))
+			{
+				return entry;
+			}
+		}
+		return null;
+	}
+
 	protected boolean isMatch(final Object anObject, final Object anotherObject)
 	{
 		if (referenceMatchingMode == ReferenceMatchingMode.EQUALITY_OPERATOR)
@@ -86,18 +98,6 @@ else if (referenceMatchingMode == ReferenceMatchingMode.EQUALS_METHOD)
 		}
 	}
 
-	private Entry entryForInstance(final Object instance)
-	{
-		for (final Entry entry : stack)
-		{
-			if (isMatch(instance, entry.getInstance()))
-			{
-				return entry;
-			}
-		}
-		return null;
-	}
-
 	public void remove(final Object instance)
 	{
 		if (instance == null)
@@ -152,6 +152,12 @@ public Object getInstance()
 		{
 			return instance;
 		}
+
+		@Override
+		public String toString()
+		{
+			return nodePath.toString() + "{" + instance.toString() + "}";
+		}
 	}
 
 	public static class CircularReferenceException extends RuntimeException

src/main/java/de/danielbechler/diff/differ/DifferDispatcher.java

@@ -27,6 +27,8 @@
 import de.danielbechler.diff.path.NodePath;
 import de.danielbechler.diff.selector.ElementSelector;
 import de.danielbechler.util.Assert;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import static de.danielbechler.diff.circular.CircularReferenceDetector.CircularReferenceException;
 
@@ -35,13 +37,14 @@
  */
 public class DifferDispatcher
 {
+	private static final Logger logger = LoggerFactory.getLogger(DifferDispatcher.class);
 	private final DifferProvider differProvider;
 	private final CircularReferenceDetectorFactory circularReferenceDetectorFactory;
 	private final CircularReferenceExceptionHandler circularReferenceExceptionHandler;
 	private final IsIgnoredResolver isIgnoredResolver;
 	private final IsReturnableResolver isReturnableResolver;
-	private CircularReferenceDetector workingCircularReferenceDetector;
-	private CircularReferenceDetector baseCircularReferenceDetector;
+	CircularReferenceDetector workingCircularReferenceDetector;
+	CircularReferenceDetector baseCircularReferenceDetector;
 
 	public DifferDispatcher(final DifferProvider differProvider,
 							final CircularReferenceDetectorFactory circularReferenceDetectorFactory,
@@ -68,30 +71,6 @@ protected final void resetInstanceMemory()
 		baseCircularReferenceDetector = circularReferenceDetectorFactory.createCircularReferenceDetector();
 	}
 
-	private static DiffNode findNodeMatchingPropertyPath(final DiffNode node, final NodePath nodePath)
-	{
-		if (node == null)
-		{
-			return null;
-		}
-		if (node.matches(nodePath))
-		{
-			return node;
-		}
-		return findNodeMatchingPropertyPath(node.getParentNode(), nodePath);
-	}
-
-	private static DiffNode newCircularNode(final DiffNode parentNode,
-											final Instances instances,
-											final NodePath circleStartPath)
-	{
-		final DiffNode node = new DiffNode(parentNode, instances.getSourceAccessor(), instances.getType());
-		node.setState(DiffNode.State.CIRCULAR);
-		node.setCircleStartPath(circleStartPath);
-		node.setCircleStartNode(findNodeMatchingPropertyPath(parentNode, circleStartPath));
-		return node;
-	}
-
 	/**
 	 * Delegates the call to an appropriate {@link Differ}.
 	 *
@@ -137,7 +116,7 @@ private DiffNode compare(final DiffNode parentNode,
 	private DiffNode compareWithCircularReferenceTracking(final DiffNode parentNode,
 														  final Instances instances)
 	{
-		DiffNode node;
+		DiffNode node = null;
 		try
 		{
 			rememberInstances(parentNode, instances);
@@ -147,7 +126,10 @@ private DiffNode compareWithCircularReferenceTracking(final DiffNode parentNode,
 			}
 			finally
 			{
-				forgetInstances(instances);
+				if (node != null)
+				{
+					forgetInstances(parentNode, instances);
+				}
 			}
 		}
 		catch (final CircularReferenceException e)
@@ -173,8 +155,20 @@ private DiffNode compare(final DiffNode parentNode, final Instances instances)
 		return differ.compare(parentNode, instances);
 	}
 
-	protected void forgetInstances(final Instances instances)
+	protected void forgetInstances(final DiffNode parentNode, final Instances instances)
 	{
+		final NodePath nodePath;
+		if (parentNode != null)
+		{
+			final NodePath parentPath = parentNode.getPath();
+			final ElementSelector elementSelector = instances.getSourceAccessor().getElementSelector();
+			nodePath = NodePath.startBuildingFrom(parentPath).element(elementSelector).build();
+		}
+		else
+		{
+			nodePath = NodePath.withRoot();
+		}
+		logger.debug("[ {} ] Forgetting --- WORKING: {} <=> BASE: {}", nodePath, instances.getWorking(), instances.getBase());
 		workingCircularReferenceDetector.remove(instances.getWorking());
 		baseCircularReferenceDetector.remove(instances.getBase());
 	}
@@ -192,7 +186,49 @@ protected void rememberInstances(final DiffNode parentNode, final Instances inst
 		{
 			nodePath = NodePath.withRoot();
 		}
+		logger.debug("[ {} ] Remembering --- WORKING: {} <=> BASE: {}", nodePath, instances.getWorking(), instances.getBase());
+
+		transactionalPushToCircularReferenceDetectors(nodePath, instances);
+	}
+
+	private void transactionalPushToCircularReferenceDetectors(final NodePath nodePath, final Instances instances)
+	{
 		workingCircularReferenceDetector.push(instances.getWorking(), nodePath);
-		baseCircularReferenceDetector.push(instances.getBase(), nodePath);
+
+		// TODO This needs to be solved more elegantly. If the push for one of these detectors fails,
+		// we need to make sure to revert the push to the other one, if it already happened.
+		try
+		{
+			baseCircularReferenceDetector.push(instances.getBase(), nodePath);
+		}
+		catch (final CircularReferenceException e)
+		{
+			workingCircularReferenceDetector.remove(instances.getWorking()); // rollback
+			throw e;
+		}
+	}
+
+	private static DiffNode findNodeMatchingPropertyPath(final DiffNode node, final NodePath nodePath)
+	{
+		if (node == null)
+		{
+			return null;
+		}
+		if (node.matches(nodePath))
+		{
+			return node;
+		}
+		return findNodeMatchingPropertyPath(node.getParentNode(), nodePath);
+	}
+
+	private static DiffNode newCircularNode(final DiffNode parentNode,
+											final Instances instances,
+											final NodePath circleStartPath)
+	{
+		final DiffNode node = new DiffNode(parentNode, instances.getSourceAccessor(), instances.getType());
+		node.setState(DiffNode.State.CIRCULAR);
+		node.setCircleStartPath(circleStartPath);
+		node.setCircleStartNode(findNodeMatchingPropertyPath(parentNode, circleStartPath));
+		return node;
 	}
 }

src/test/java/de/danielbechler/diff/differ/DifferDispatcherTest.groovy

@@ -0,0 +1,83 @@
+/*
+ * Copyright 2014 Daniel Bechler
+ *
+ * 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 de.danielbechler.diff.differ
+
+import de.danielbechler.diff.access.Accessor
+import de.danielbechler.diff.access.Instances
+import de.danielbechler.diff.access.RootAccessor
+import de.danielbechler.diff.circular.CircularReferenceDetector
+import de.danielbechler.diff.circular.CircularReferenceDetectorFactory
+import de.danielbechler.diff.circular.CircularReferenceExceptionHandler
+import de.danielbechler.diff.filtering.IsReturnableResolver
+import de.danielbechler.diff.inclusion.IsIgnoredResolver
+import de.danielbechler.diff.node.DiffNode
+import spock.lang.Ignore
+import spock.lang.Specification
+import spock.lang.Subject
+
+import static de.danielbechler.diff.circular.CircularReferenceDetector.ReferenceMatchingMode.EQUALITY_OPERATOR
+
+/**
+ * Created by Daniel Bechler.
+ */
+@Subject(DifferDispatcher)
+@Ignore("Man, this class is a pain in the ass to test. Need to find a better way to do it.")
+class DifferDispatcherTest extends Specification {
+
+	def differ = Stub Differ
+	def differProvider = Stub DifferProvider, {
+		retrieveDifferForType(_ as Class<?>) >> differ
+	}
+	def circularReferenceDetectorFactory = Stub CircularReferenceDetectorFactory, {
+		createCircularReferenceDetector() >> new CircularReferenceDetector(EQUALITY_OPERATOR)
+	}
+	def circularReferenceExceptionHandler = Stub(CircularReferenceExceptionHandler)
+	def isIgnoredResolver = Stub IsIgnoredResolver, {
+		isIgnored(_ as DiffNode) >> false
+	}
+	def isReturnableResolver = Stub IsReturnableResolver, {
+		isReturnable(_ as DiffNode) >> true
+	}
+	def differDispatcher = new DifferDispatcher(
+			differProvider,
+			circularReferenceDetectorFactory,
+			circularReferenceExceptionHandler,
+			isIgnoredResolver,
+			isReturnableResolver)
+
+	def "when a circular reference is detected"() {
+		given:
+		  def accessor = Stub Accessor
+		  def accessedInstances = Mock Instances, {
+			  areNull() >> false
+			  getBase() >> new Object()
+			  getWorking() >> new Object()
+		  }
+		  def instances = Mock Instances, {
+			  access(_ as Accessor) >> accessedInstances
+			  getSourceAccessor() >> accessor
+		  }
+		  def node = new DiffNode(RootAccessor.instance, Object)
+
+		when:
+		  differDispatcher.dispatch(node, instances, accessor)
+
+		then:
+		  differDispatcher.workingCircularReferenceDetector.size() == 1
+		  differDispatcher.baseCircularReferenceDetector.size() == 1
+	}
+}

src/test/resources/logback-test.xml

@@ -22,7 +22,7 @@
 		</encoder>
 	</appender>
 
-	<root level="debug">
+	<root level="info">
 		<appender-ref ref="STDOUT"/>
 	</root>