diff --git a/src/main/java/org/javarosa/core/model/FormDef.java b/src/main/java/org/javarosa/core/model/FormDef.java index c33962b93..dab8a010d 100644 --- a/src/main/java/org/javarosa/core/model/FormDef.java +++ b/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 diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 7d54ba27c..7b5c27f49 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/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 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 getRecalculates() { return recalculates; } - public void reportDependencyCycles() { - Set vertices = new HashSet<>(); - List edges = new ArrayList<>(); - - //build graph - List targets = new ArrayList<>(); - for (TreeReference trigger : triggerIndex.keySet()) { - vertices.add(trigger); - List 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 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"); - } - } - } diff --git a/src/main/java/org/javarosa/xform/parse/XFormParser.java b/src/main/java/org/javarosa/xform/parse/XFormParser.java index 1fca4dd77..3fa849e49 100644 --- a/src/main/java/org/javarosa/xform/parse/XFormParser.java +++ b/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)); } diff --git a/src/test/java/org/javarosa/core/model/Safe2014DagImplTest.java b/src/test/java/org/javarosa/core/model/TriggerableDagTest.java similarity index 94% rename from src/test/java/org/javarosa/core/model/Safe2014DagImplTest.java rename to src/test/java/org/javarosa/core/model/TriggerableDagTest.java index ac5d94581..b5256eb24 100644 --- a/src/test/java/org/javarosa/core/model/Safe2014DagImplTest.java +++ b/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 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 dagEvents, String... lines) { assertThat(dagEvents.stream().map(Event::getDisplayMessage).collect(joining("\n")), is(join("\n", lines))); }