Remove redundant dependency cycle while parsing forms

src/main/java/org/javarosa/core/model/FormDef.java

@@ -675,14 +675,6 @@ public Triggerable addTriggerable(Triggerable t) {
         return dagImpl.addTriggerable(t);
     }
 
-    /**
-     * Reports any dependency cycles based upon the triggerIndex array.
-     * (Does not require that the DAG be finalized).
-     */
-    public void reportDependencyCycles() {
-        dagImpl.reportDependencyCycles();
-    }
-
     /**
      * Finalizes the DAG associated with the form's triggered conditions. This
      * will create the appropriate ordering and dependencies to ensure the

src/main/java/org/javarosa/core/model/TriggerableDag.java

@@ -339,8 +339,9 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev
             newDestinationSet.clear();
             fillTriggeredElements(mainInstance, evalContext, qt, deps, newDestinationSet);
 
-            // remove any self-reference if we have one...
-            deps.remove(qt);
+            if (deps.contains(qt))
+                throwCycleInDagException(deps);
+
             for (QuickTriggerable qu : deps) {
                 QuickTriggerable[] edge = {qt, qu};
                 partialOrdering.add(edge);
@@ -364,20 +365,8 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev
             }
 
             // if no root nodes while graph still has nodes, graph has cycles
-            if (roots.size() == 0) {
-                StringBuilder hints = new StringBuilder();
-                for (QuickTriggerable qt : vertices) {
-                    for (TreeReference r : qt.t.getTargets()) {
-                        hints.append("\n").append(r.toString(true));
-                    }
-                }
-                String message = "Cycle detected in form's relevant and calculation logic!";
-                if (!hints.toString().equals("")) {
-                    message += "\nThe following nodes are likely involved in the loop:"
-                        + hints;
-                }
-                throw new IllegalStateException(message);
-            }
+            if (roots.size() == 0)
+                throwCycleInDagException(vertices);
 
             // order the root nodes - so the order is fixed
             orderedRoots.clear();
@@ -415,6 +404,21 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev
         }
     }
 
+    public void throwCycleInDagException(Collection<QuickTriggerable> vertices) {
+        StringBuilder hints = new StringBuilder();
+        for (QuickTriggerable qt : vertices) {
+            for (TreeReference r : qt.t.getTargets()) {
+                hints.append("\n").append(r.toString(true));
+            }
+        }
+        String message = "Cycle detected in form's relevant and calculation logic!";
+        if (!hints.toString().equals("")) {
+            message += "\nThe following nodes are likely involved in the loop:"
+                + hints;
+        }
+        throw new IllegalStateException(message);
+    }
+
     /**
      * Get all of the elements which will need to be evaluated (in order) when
      * the triggerable is fired.
@@ -668,71 +672,4 @@ public final ArrayList<Recalculate> getRecalculates() {
         return recalculates;
     }
 
-    public void reportDependencyCycles() {
-        Set<TreeReference> vertices = new HashSet<>();
-        List<TreeReference[]> edges = new ArrayList<>();
-
-        //build graph
-        List<TreeReference> targets = new ArrayList<>();
-        for (TreeReference trigger : triggerIndex.keySet()) {
-            vertices.add(trigger);
-            List<QuickTriggerable> triggered = triggerIndex.get(trigger);
-            targets.clear();
-            for (QuickTriggerable qt : triggered) {
-                Triggerable t = qt.t;
-                for (int j = 0; j < t.getTargets().size(); j++) {
-                    TreeReference target = t.getTargets().get(j);
-                    if (!targets.contains(target))
-                        targets.add(target);
-                }
-            }
-
-            for (TreeReference target : targets) {
-                vertices.add(target);
-
-                TreeReference[] edge = {trigger, target};
-                edges.add(edge);
-            }
-        }
-
-        //find cycles
-        boolean acyclic = true;
-        Set<TreeReference> leaves = new HashSet<>(vertices.size());
-        while (vertices.size() > 0) {
-            //determine leaf nodes
-            leaves.clear();
-            leaves.addAll(vertices);
-            for (TreeReference[] edge : edges) {
-                leaves.remove(edge[0]);
-            }
-
-            //if no leaf nodes while graph still has nodes, graph has cycles
-            if (leaves.size() == 0) {
-                acyclic = false;
-                break;
-            }
-
-            //remove leaf nodes and edges pointing to them
-            for (TreeReference leaf : leaves) {
-                vertices.remove(leaf);
-            }
-            for (int i = edges.size() - 1; i >= 0; i--) {
-                TreeReference[] edge = edges.get(i);
-                if (leaves.contains(edge[1]))
-                    edges.remove(i);
-            }
-        }
-
-        if (!acyclic) {
-            StringBuilder b = new StringBuilder();
-            b.append("XPath Dependency Cycle:\n");
-            for (TreeReference[] edge : edges) {
-                b.append(edge[0].toString()).append(" => ").append(edge[1].toString()).append("\n");
-            }
-            logger.error("XForm Parse Error: {}", b.toString());
-
-            throw new RuntimeException("Dependency cycles amongst the xpath expressions in relevant/calculate");
-        }
-    }
-
 }

src/main/java/org/javarosa/xform/parse/XFormParser.java

@@ -1931,7 +1931,6 @@ private void addBinding(DataBinding binding) {
     private void addMainInstanceToFormDef(Element e, FormInstance instanceModel) {
         loadInstanceData(e, instanceModel.getRoot(), _f);
 
-        checkDependencyCycles();
         _f.setInstance(instanceModel);
         _f.setLocalizer(localizer);
 
@@ -2158,10 +2157,6 @@ public static QuestionDef ghettoGetQuestionDef(int dataType, FormDef f, TreeRefe
         }
     }
 
-    private void checkDependencyCycles() {
-        _f.reportDependencyCycles();
-    }
-
     private void loadXmlInstance(FormDef f, Reader xmlReader) throws IOException {
         loadXmlInstance(f, getXMLDocument(xmlReader));
     }

src/test/java/org/javarosa/core/model/Safe2014DagImplTest.java → src/test/java/org/javarosa/core/model/TriggerableDagTest.java

@@ -34,15 +34,18 @@
 import org.javarosa.core.model.instance.TreeReference;
 import org.javarosa.core.test.Scenario;
 import org.javarosa.debug.Event;
+import org.javarosa.xform.parse.XFormParseException;
 import org.joda.time.LocalTime;
 import org.junit.Before;
 import org.junit.Ignore;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-public class Safe2014DagImplTest {
-    private static Logger logger = LoggerFactory.getLogger(Safe2014DagImplTest.class);
+public class TriggerableDagTest {
+    private static Logger logger = LoggerFactory.getLogger(TriggerableDagTest.class);
 
     private List<Event> dagEvents = new ArrayList<>();
 
@@ -51,6 +54,9 @@ public void setUp() {
         dagEvents.clear();
     }
 
+    @Rule
+    public ExpectedException exceptionRule = ExpectedException.none();
+
     @Test
     public void deleteSecondRepeatGroup_evaluatesTriggerables_dependentOnPrecedingRepeatGroupSiblings() throws IOException {
         Scenario scenario = Scenario.init("Some form", html(
@@ -487,6 +493,41 @@ public void equivalent_predicates_with_function_calls_should_produce_the_same_re
         assertThat(scenario.answerOf("/data/result_2"), is(intAnswer(30)));
     }
 
+
+    @Test
+    public void parsing_forms_with_cycles_by_self_reference_should_fail() throws IOException {
+        exceptionRule.expect(XFormParseException.class);
+        exceptionRule.expectMessage("Cycle detected in form's relevant and calculation logic!");
+
+        Scenario.init("Some form", html(head(
+            title("Some form"),
+            model(
+                mainInstance(t("data id=\"some-form\"",
+                    t("count", "1")
+                )),
+                bind("/data/count").type("int").calculate(". + 1")
+            ))
+        ));
+    }
+
+    @Test
+    public void parsing_forms_with_cycles_should_fail() throws IOException {
+        exceptionRule.expect(XFormParseException.class);
+        exceptionRule.expectMessage("Cycle detected in form's relevant and calculation logic!");
+
+        Scenario.init("Some form", html(head(
+            title("Some form"),
+            model(
+                mainInstance(t("data id=\"some-form\"",
+                    t("a", "1"),
+                    t("b", "1")
+                )),
+                bind("/data/a").type("int").calculate("/data/b + 1"),
+                bind("/data/b").type("int").calculate("/data/a + 1")
+            ))
+        ));
+    }
+
     private void assertDagEvents(List<Event> dagEvents, String... lines) {
         assertThat(dagEvents.stream().map(Event::getDisplayMessage).collect(joining("\n")), is(join("\n", lines)));
     }