Merge pull request #940 from dimagi/dfsFix

Corrects Cycle Detection Algo

src/main/java/org/commcare/util/CollectionUtils.java

@@ -1,5 +1,6 @@
 package org.commcare.util;
 
+import java.util.ArrayList;
 import java.util.Vector;
 
 /**
@@ -28,4 +29,19 @@ public static int[] mergeIntegerVectorsInArray(Vector<Integer> first, Vector<Int
         }
         return result;
     }
+
+    /**
+     * Checks if any element in {@code subList} is contained in {@code superList}
+     * @param superList
+     * @param subList
+     * @return true if {@code superList} contains any element of {@code subList}, false otherwise
+     */
+    public static boolean containsAny(ArrayList<String> superList, ArrayList<String> subList) {
+        for (String item : subList) {
+            if(superList.contains(item)){
+                return true;
+            }
+        }
+        return false;
+    }
 }

src/main/java/org/javarosa/core/util/ShortestCycleAlgorithm.java

@@ -1,16 +1,18 @@
 package org.javarosa.core.util;
 
+import org.commcare.util.CollectionUtils;
 import org.javarosa.core.model.instance.TreeReference;
+import org.javarosa.core.services.Logger;
 
 import java.lang.reflect.Array;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Hashtable;
 import java.util.List;
 import java.util.Vector;
 
 /**
- *
  * Modeled after algorithm here https://stackoverflow.com/a/549312
  *
  * Given a Vector of TreeReference pairs where edges[0] references edges[1]
@@ -29,7 +31,7 @@ public class ShortestCycleAlgorithm {
 
     public ShortestCycleAlgorithm(Vector<TreeReference[]> edges) {
         this.edges = edges;
-        for (TreeReference[] references: edges) {
+        for (TreeReference[] references : edges) {
 
             String parentKey = references[1].toString();
             String childKey = references[0].toString();
@@ -40,7 +42,7 @@ public ShortestCycleAlgorithm(Vector<TreeReference[]> edges) {
             }
         }
 
-        for (String node: nodes) {
+        for (String node : nodes) {
             ArrayList<String> shortestPath = depthFirstSearch(node, node, new ArrayList<String>());
             if (shortestPath != null && (shortestCycle == null || shortestPath.size() < shortestCycle.size())) {
                 shortestCycle = shortestPath;
@@ -60,7 +62,7 @@ private void addChild(String parentKey, String childKey) {
 
     // Add the new node to the set of reachable nodes for all already-visited nodes
     private void addReachbleToVisited(List<String> visited, String reachable) {
-        for (String visit: visited) {
+        for (String visit : visited) {
             addReachable(visit, reachable);
         }
     }
@@ -93,7 +95,7 @@ private ArrayList<String> depthFirstSearch(String startNode, String currentNode,
                 // Then this child cannot contain a cycle
                 if (walked.contains(child)) {
                     ArrayList<String> reachables = reachableMap.get(child);
-                    if (reachables == null || !reachables.contains(visited)) {
+                    if (reachables == null || !CollectionUtils.containsAny(reachables, visited)) {
                         continue;
                     }
                 }
@@ -122,9 +124,9 @@ public String getCycleString() {
                 stringBuilder.append(shortestCycle.get(0));
                 stringBuilder.append(".");
             } else {
-                stringBuilder.append( " references " );
+                stringBuilder.append(" references ");
                 stringBuilder.append(shortestCycle.get(i + 1));
-                stringBuilder.append( ", \n" );
+                stringBuilder.append(", \n");
             }
         }
         return stringBuilder.toString();
@@ -135,8 +137,8 @@ public String getCycleString() {
      * edges. Helpful for debugging
      */
     private String toDOTDigraph() {
-        String graph ="";
-        for(TreeReference[] edge : edges){
+        String graph = "";
+        for (TreeReference[] edge : edges) {
             graph += clean(edge[0].toString(false)) + " -> " + clean(edge[1].toString(false)) + ";\n";
         }
 

src/test/java/org/commcare/util/CollectionUtilsTest.java

@@ -0,0 +1,34 @@
+package org.commcare.util;
+
+import org.junit.Test;
+
+import java.util.ArrayList;
+
+import static org.junit.Assert.*;
+
+public class CollectionUtilsTest {
+
+    @Test
+    public void containsAny() {
+        ArrayList<String> superList = new ArrayList();
+        superList.add("Apple");
+        superList.add("Mango");
+        superList.add("Banana");
+
+        assertFalse("Empty List can't contain any fruits from superList",
+                CollectionUtils.containsAny(superList, new ArrayList<>()));
+
+        ArrayList<String> subListOne = new ArrayList<>();
+        subListOne.add("Orange");
+
+        assertFalse("List doesn't contain any fruits from superList",
+                CollectionUtils.containsAny(superList, subListOne));
+
+        ArrayList<String> subListTwo = new ArrayList<>();
+        subListTwo.add("Orange");
+        subListTwo.add("Mango");
+
+        assertTrue("List contains Mango from superList",
+                CollectionUtils.containsAny(superList, subListTwo));
+    }
+}

src/test/java/org/javarosa/core/model/test/CyclicReferenceTests.java

@@ -38,15 +38,21 @@ public void testCyclicReferenceWithGroup() {
      */
     @Test
     public void testCyclicalReferenceRegression() {
+        testCyclicReferenceForPath("/xform_tests/real_form_with_cycle_errors.xml", 4);
+        testCyclicReferenceForPath("/xform_tests/real_form2_with_cycle_errors.xml", 2);
+
+    }
+
+    private void testCyclicReferenceForPath(String formPath, int numberOfCyclicReferences) {
         try {
-            new FormParseInit("/xform_tests/real_form_with_cycle_errors.xml");
+            new FormParseInit(formPath);
         } catch (XFormParseException e) {
             String detailMessage = e.getMessage();
             // Assert that we're using the shortest cycle algorithm
             assertTrue(detailMessage.contains("Logic is cyclical"));
-            // There should only be three newlines since only the three core cyclic references were included
+            // number of newlines should be equal to number of core cyclic references were included
             int newlineCount = detailMessage.length() - detailMessage.replace("\n", "").length();
-            assertTrue(newlineCount == 4);
+            assertTrue(newlineCount == numberOfCyclicReferences);
             return;
         }
         fail("Cyclical reference did not throw XFormParseException");