diff --git a/src/main/java/org/javarosa/core/model/FormDef.java b/src/main/java/org/javarosa/core/model/FormDef.java index 488dc1919..e5fd76df6 100644 --- a/src/main/java/org/javarosa/core/model/FormDef.java +++ b/src/main/java/org/javarosa/core/model/FormDef.java @@ -687,7 +687,7 @@ public void finalizeTriggerables() throws IllegalStateException { // DAGify the triggerables based on dependencies and sort them so that // triggerables come only after the triggerables they depend on // - dagImpl.finalizeTriggerables(getMainInstance(), getEvaluationContext()); + dagImpl.finalizeTriggerables(getMainInstance()); } /** diff --git a/src/main/java/org/javarosa/core/model/QuickTriggerable.java b/src/main/java/org/javarosa/core/model/QuickTriggerable.java index 035f782ee..238178922 100644 --- a/src/main/java/org/javarosa/core/model/QuickTriggerable.java +++ b/src/main/java/org/javarosa/core/model/QuickTriggerable.java @@ -50,7 +50,7 @@ public List apply(FormInstance mainInstance, EvaluationContext return triggerable.apply(mainInstance, ec, qualified); } - public List getTargets() { + public Set getTargets() { return triggerable.getTargets(); } @@ -58,10 +58,8 @@ public boolean contains(Triggerable triggerable) { return this.triggerable.equals(triggerable); } - public Triggerable changeContextRefToIntersectWithTriggerable(Triggerable other) { - // TODO It's fishy to mutate the Triggerable here. We might prefer to return a copy of the original Triggerable - triggerable.changeContextRefToIntersectWithTriggerable(other); - return triggerable; + public void intersectContextWith(Triggerable other) { + triggerable.intersectContextWith(other); } public void setImmediateCascades(Set deps) { diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 30fdb248f..b1c17322c 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -16,15 +16,17 @@ package org.javarosa.core.model; +import static java.util.Collections.emptySet; + import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -33,7 +35,6 @@ import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.Recalculate; import org.javarosa.core.model.condition.Triggerable; -import org.javarosa.core.model.instance.AbstractTreeElement; import org.javarosa.core.model.instance.FormInstance; import org.javarosa.core.model.instance.TreeElement; import org.javarosa.core.model.instance.TreeReference; @@ -55,41 +56,61 @@ public interface EventNotifierAccessor { EventNotifier getEventNotifier(); } - protected final EventNotifierAccessor accessor; - /** - * NOT VALID UNTIL finalizeTriggerables() is called!! + * Stores the event notifier required to create instances of this class. *

- * Topologically ordered list, meaning that for any tA and tB in - * the list, where tA comes before tB, evaluating tA cannot depend on any - * result from evaluating tB. + * The event notifier is used to publish events that serve as a form for + * loose coupling between processes that could be running at the same time + * in a JavaRosa client. */ - protected final List triggerablesDAG = new ArrayList<>(); + private final EventNotifierAccessor accessor; /** - * NOT VALID UNTIL finalizeTriggerables() is called!! + * Stores the unsorted set of all triggerables present in the form. *

- * Associates repeatable nodes with the Condition that determines their - * relevance. + * This set is used during the DAG build process. */ - protected final Map conditionRepeatTargetIndex = new HashMap<>(); + private final Set allTriggerables = new HashSet<>(); /** - * Maps a tree reference to the set of triggerables that need to be - * processed when the value at this reference changes. + * Stores the sorted set of all triggerables using the dependency direction + * as ordering. + *

+ * Triggerables present in this set depend exclusively on preceding + * triggerables. */ - protected final Map> triggerIndex = new HashMap<>(); + private Set triggerablesDAG = emptySet(); + // TODO Make this member fit the expected behavior on calling sites by containing only relevance conditions /** - * List of all the triggerables in the form. Unordered. + * Stores an index for conditions (triggerables declared in + * readonly, required, or relevant + * attributes) belonging to repeat groups. + *

+ * This index is used to determine whether a repeat group instance is + * relevant or not. + *

+ * Warning: Calling site assumes that a repeat group would only have + * one object stored for its reference in this map. This is because, so + * far, relevant is the only attribute that makes sense adding + * to a repeat group, but a form could declare other conditions as well, + * leading to an unexpected scenario. */ - protected final List unorderedTriggerables = new ArrayList<>(); + private Map repeatConditionsPerTargets = new HashMap<>(); + /** + * Stores an index to resolve triggerables by their corresponding trigger's + * reference. + *

+ * Note that there's a m:n relationship between trigger references and + * triggerables. + */ + private final Map> triggerablesPerTrigger = new HashMap<>(); - protected TriggerableDag(EventNotifierAccessor accessor) { + public TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; } - protected Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext evalContext, Set tv, TreeReference anchorRef, Set alreadyEvaluated) { + private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext evalContext, Set tv, TreeReference anchorRef, Set alreadyEvaluated) { // tv should now contain all of the triggerable components which are going // to need to be addressed by this update. // 'triggerables' is topologically-ordered by dependencies, so evaluate @@ -174,7 +195,7 @@ private List evaluateTriggerable(FormInstance mainInstance, Ev } public QuickTriggerable getTriggerableForRepeatGroup(TreeReference repeatRef) { - return conditionRepeatTargetIndex.get(repeatRef.genericize()); + return repeatConditionsPerTargets.get(repeatRef.genericize()); } private Set initializeTriggerables(FormInstance mainInstance, EvaluationContext evalContext, TreeReference rootRef, Set alreadyEvaluated) { @@ -182,8 +203,7 @@ private Set initializeTriggerables(FormInstance mainInstance, Set applicable = new HashSet<>(); for (QuickTriggerable qt : triggerablesDAG) { - for (int j = 0; j < qt.getTargets().size(); j++) { - TreeReference target = qt.getTargets().get(j); + for (TreeReference target : qt.getTargets()) { if (genericRoot.isAncestorOf(target, false)) { applicable.add(qt); break; @@ -191,6 +211,9 @@ private Set initializeTriggerables(FormInstance mainInstance, } } + // TODO Call doEvaluateTriggerables directly instead to avoid a redundant extra indirection level + // return doEvaluateTriggerables(mainInstance, evalContext, applicable, rootRef, alreadyEvaluated); + return evaluateTriggerables(mainInstance, evalContext, applicable, rootRef, alreadyEvaluated); } @@ -270,7 +293,7 @@ public void copyItemsetAnswer(FormInstance mainInstance, EvaluationContext evalC } private QuickTriggerable findTriggerable(Triggerable t) { - for (QuickTriggerable qt : unorderedTriggerables) { + for (QuickTriggerable qt : allTriggerables) { if (qt.contains(t)) { return qt; } @@ -278,136 +301,152 @@ private QuickTriggerable findTriggerable(Triggerable t) { return null; } - public final Triggerable addTriggerable(Triggerable t) { - QuickTriggerable qt = findTriggerable(t); - if (qt != null) { - // one node may control access to many nodes; this means many nodes - // effectively have the same condition - // let's identify when conditions are the same, and store and calculate - // it only once - - // nov-2-2011: ctsims - We need to merge the context nodes together - // whenever we do this (finding the highest - // common ground between the two), otherwise we can end up failing to - // trigger when the ignored context - // exists and the used one doesn't - - // TODO It's fishy to mutate the Triggerable here. We might prefer to return a copy of the original Triggerable - return qt.changeContextRefToIntersectWithTriggerable(t); - - // note, if the contextRef is unnecessarily deep, the condition will be - // evaluated more times than needed - // perhaps detect when 'identical' condition has a shorter contextRef, - // and use that one instead? - - } else { - qt = QuickTriggerable.of(t); - unorderedTriggerables.add(qt); - - Set triggers = t.getTriggers(); - for (TreeReference trigger : triggers) { - List triggered = triggerIndex.get(trigger); - if (triggered == null) { - triggered = new ArrayList<>(); - triggerIndex.put(trigger.clone(), triggered); - } - if (!triggered.contains(qt)) { - triggered.add(qt); - } - } + /** + * Adds the provided triggerable to the DAG. + *

+ * This method has side-effects: + *

    + *
  • If a similar triggerable has been already added, its context gets + * intersected with the provided triggerable to cover both using only + * one entry
    • + *
  • This method builds the index of triggerables per trigger ref + * at {@link #triggerablesPerTrigger}
    • + *
+ */ + Triggerable addTriggerable(Triggerable triggerable) { + QuickTriggerable existingQuickTriggerable = findTriggerable(triggerable); + if (existingQuickTriggerable != null) { + // We found a quick triggerable wrapping a triggerable matching the provided one. + // Since Triggerable.equals() doesn't have into account contexts or targets, the + // safest thing to do would be to create a new quick triggerable for the incoming + // triggerable, but we could cover both triggerables if we compute the intersection + // between their respective context and reuse the one we've already stored. + existingQuickTriggerable.intersectContextWith(triggerable); + return existingQuickTriggerable.getTriggerable(); + } - return t; + QuickTriggerable newQuickTriggerable = QuickTriggerable.of(triggerable); + allTriggerables.add(newQuickTriggerable); + + // Build the triggerable per trigger index + Set triggers = triggerable.getTriggers(); + for (TreeReference trigger : triggers) { + if (!triggerablesPerTrigger.containsKey(trigger)) + triggerablesPerTrigger.put(trigger, new HashSet<>()); + triggerablesPerTrigger.get(trigger).add(newQuickTriggerable); } + + return triggerable; } /** * Finalize the DAG associated with the form's triggered conditions. This * will create the appropriate ordering and dependencies to ensure the * conditions will be evaluated in the appropriate orders. - * - * @throws IllegalStateException - If the trigger ordering contains an illegal cycle and the - * triggers can't be laid out appropriately */ - public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { - // - // DAGify the triggerables based on dependencies and sort them so that - // triggerables come only after the triggerables they depend on - // - List vertices = new ArrayList<>(unorderedTriggerables); - triggerablesDAG.clear(); - List partialOrdering = new ArrayList<>(); - Set newDestinationSet = new HashSet<>(); - for (QuickTriggerable qt : vertices) { - Set deps = new HashSet<>(); - newDestinationSet.clear(); - fillTriggeredElements(mainInstance, evalContext, qt, deps, newDestinationSet); - - if (deps.contains(qt)) - throwCyclesInDagException(deps); - - if (qt.canCascade()) - for (QuickTriggerable qu : deps) { - QuickTriggerable[] edge = {qt, qu}; - partialOrdering.add(edge); - } + public void finalizeTriggerables(FormInstance mainInstance) throws IllegalStateException { + triggerablesDAG = buildDag(allTriggerables, getDagEdges()); + repeatConditionsPerTargets = getRepeatConditionsPerTargets(mainInstance, triggerablesDAG); + } + + private static Map getRepeatConditionsPerTargets(FormInstance mainInstance, Set triggerables) { + Map repeatConditionsPerTargets = new HashMap<>(); + for (QuickTriggerable triggerable : triggerables) + if (triggerable.isCondition()) + for (TreeReference target : triggerable.getTargets()) + if (mainInstance.getTemplate(target) != null) + repeatConditionsPerTargets.put(target, triggerable); + return repeatConditionsPerTargets; + } - qt.setImmediateCascades(deps); + /** + * Returns the list of edges in the DAG that can be built from all the + * triggerables added to the DAG while parsing the form. + *

+ * This method has side-effects: + *

    + *
  • Throws IllegalStateException when cycles are detected involving + * self-references
    • + *
  • Builds a cache of immediate cascades of each vertex, meaning that + * we will remember the dependant vertices without having to traverse + * the DAG again {@link Triggerable#getImmediateCascades()}
    • + *
+ */ + private Set getDagEdges() { + Set edges = new HashSet<>(); + for (QuickTriggerable source : allTriggerables) { + // Compute the set of edge targets from the source vertex in this + // loop using the triggerable's target tree reference set. + // We will create an edge for all the source's target references + // that, in turn, trigger another triggerable. + Set targets = new HashSet<>(); + for (TreeReference targetRef : source.getTargets()) + targets.addAll(triggerablesPerTrigger.containsKey(targetRef) + ? triggerablesPerTrigger.get(targetRef) + : emptySet()); + + // Account for cycles by self-reference + if (targets.contains(source)) + throwCyclesInDagException(targets); + + for (QuickTriggerable target : targets) + edges.add(new QuickTriggerable[]{source, target}); + + // TODO Move this from Triggerable to TriggerableDag + source.setImmediateCascades(targets); } + return edges; + } - List orderedRoots = new ArrayList<>(); - Set roots = new HashSet<>( - vertices.size()); - while (vertices.size() > 0) { - // determine root nodes - roots.clear(); - roots.addAll(vertices); - for (QuickTriggerable[] edge : partialOrdering) { + /** + * Returns a set with the DAG that can be build using the provided vertices + * and edges. + *

+ * This method has side-effects: + *

    + *
  • Throws IllegalStateException when cycles are detected involving + * more than one node
    • + *
+ */ + private static Set buildDag(Set vertices, Set edges) { + // The dag and the set of remaining vertices will be mutated + // inside the while loop's block + Set dag = new LinkedHashSet<>(); + Set remainingVertices = new HashSet<>(vertices); + + // The set of remaining edges will be replaced inside + // the while loop's block + Set remainingEdges = new HashSet<>(edges); + + while (remainingVertices.size() > 0) { + // Compute the set of roots (nodes that don't show up + // as edge targets) with the remaining vertices + Set roots = new HashSet<>(remainingVertices); + for (QuickTriggerable[] edge : remainingEdges) roots.remove(edge[1]); - } - // if no root nodes while graph still has nodes, graph has cycles if (roots.size() == 0) throwCyclesInDagException(vertices); - // order the root nodes - so the order is fixed - orderedRoots.clear(); - orderedRoots.addAll(roots); - Collections.sort(orderedRoots, QuickTriggerableComparator.INSTANCE); - - // remove root nodes and edges originating from them - // add them to the triggerablesDAG. - for (QuickTriggerable root : orderedRoots) { - triggerablesDAG.add(root); - vertices.remove(root); - } - for (int i = partialOrdering.size() - 1; i >= 0; i--) { - QuickTriggerable[] edge = partialOrdering.get(i); - if (roots.contains(edge[0])) - partialOrdering.remove(i); - } - } - - // - // build the condition index for repeatable nodes - // - - conditionRepeatTargetIndex.clear(); - for (QuickTriggerable qt : triggerablesDAG) { - if (qt.isCondition()) { - List targets = qt.getTargets(); - for (TreeReference target : targets) { - if (mainInstance.getTemplate(target) != null) { - conditionRepeatTargetIndex.put(target, qt); - } - } - } + // "Move" the roots detected during this iteration + // from the remainingVertices to the DAG + remainingVertices.removeAll(roots); + dag.addAll(roots); + + // Compute the new set of remaining edges to continue the iteration + Set newRemainingEdges = new HashSet<>(); + for (QuickTriggerable[] edge : remainingEdges) + if (!roots.contains(edge[0])) + newRemainingEdges.add(edge); + remainingEdges = newRemainingEdges; } + return dag; } - public void throwCyclesInDagException(Collection vertices) { + private static void throwCyclesInDagException(Collection triggerables) { StringBuilder hints = new StringBuilder(); - for (QuickTriggerable qt2 : vertices) { - for (TreeReference r : qt2.getTargets()) { + for (QuickTriggerable qt : triggerables) { + for (TreeReference r : qt.getTargets()) { hints.append("\n").append(r.toString(true)); } } @@ -419,59 +458,6 @@ public void throwCyclesInDagException(Collection vertices) { throw new IllegalStateException(message); } - /** - * Get all of the elements which will need to be evaluated (in order) when - * the triggerable is fired. - */ - public void fillTriggeredElements(FormInstance mainInstance, EvaluationContext evalContext, QuickTriggerable qt, Set destinationSet, Set newDestinationSet) { - - - for (int j = 0; j < qt.getTargets().size(); j++) { - TreeReference target = qt.getTargets().get(j); - Set updatedNodes = new HashSet<>(); - updatedNodes.add(target); - - // For certain types of triggerables, the update will affect - // not only the target, but also the children of the target. - // In that case, we want to add all of those nodes - // to the list of updated elements as well. - if (qt.isCascadingToChildren()) { - addChildrenOfReference(mainInstance, evalContext, - target, updatedNodes); - } - - // Now go through each of these updated nodes (generally - // just 1 for a normal calculation, - // multiple nodes if there's a relevance cascade. - for (TreeReference ref : updatedNodes) { - // Check our index to see if that target is a Trigger - // for other conditions - // IE: if they are an element of a different calculation - // or relevancy calc - - // We can't make this reference generic before now or - // we'll lose the target information, - // so we'll be more inclusive than needed and see if any - // of our triggers are keyed on the predicate-less path - // of this ref - List triggered = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); - - if (triggered != null) { - // If so, walk all of these triggerables that we - // found - for (QuickTriggerable qu : triggered) { - // And add them to the queue if they aren't - // there already - if (!destinationSet.contains(qu)) { - destinationSet.add(qu); - newDestinationSet.add(qu); - } - } - } - } - } - } - /** * Step 2 in evaluating DAG computation updates from a value being changed * in the instance. This step is responsible for taking the root set of @@ -506,46 +492,6 @@ private Set evaluateTriggerables(FormInstance mainInstance, Ev return doEvaluateTriggerables(mainInstance, evalContext, tv, anchorRef, alreadyEvaluated); } - /** - * This is a utility method to get all of the references of a node. It can - * be replaced when we support dependent XPath Steps (IE: /path/to//) - */ - private void addChildrenOfReference(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original, Set toAdd) { - // original has already been added to the 'toAdd' list. - - TreeElement repeatTemplate = mainInstance.getTemplatePath(original); - if (repeatTemplate != null) { - for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { - TreeElement child = repeatTemplate.getChildAt(i); - toAdd.add(child.getRef().genericize()); - addChildrenOfElement(mainInstance, child, toAdd); - } - } else { - List refSet = evalContext.expandReference(original); - for (TreeReference ref : refSet) { - addChildrenOfElement(mainInstance, evalContext.resolveReference(ref), toAdd); - } - } - } - - // Recursive step of utility method - private void addChildrenOfElement(FormInstance mainInstance, AbstractTreeElement el, Set toAdd) { - TreeElement repeatTemplate = mainInstance.getTemplatePath(el.getRef()); - if (repeatTemplate != null) { - for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { - TreeElement child = repeatTemplate.getChildAt(i); - toAdd.add(child.getRef().genericize()); - addChildrenOfElement(mainInstance, child, toAdd); - } - } else { - for (int i = 0; i < el.getNumChildren(); ++i) { - AbstractTreeElement child = el.getChildAt(i); - toAdd.add(child.getRef().genericize()); - addChildrenOfElement(mainInstance, child, toAdd); - } - } - } - /** * Walks the current set of conditions, and evaluates each of them with the * current context. @@ -555,10 +501,10 @@ public Collection initializeTriggerables(FormInstance mainInst } /** - * Invoked to validate a filled-in form. Sweeps through from beginning - * to end, confirming that the entered values satisfy all constraints. - * The FormEntryController is based upon the FormDef, but has its own - * model and controller independent of anything at the UI layer. + * Invoked to validate a filled-in form. Sweeps through from beginning to + * end, confirming that the entered values satisfy all constraints. The + * FormEntryController is based upon the FormDef, but has its own model and + * controller independent of anything at the UI layer. */ public ValidateOutcome validate(FormEntryController formEntryControllerToBeValidated, boolean markCompleted) { @@ -592,7 +538,7 @@ private Set triggerTriggerables(FormInstance mainInstance, Eva TreeReference genericRef = ref.genericize(); // get triggerables which are activated by the generic reference - List triggered = triggerIndex.get(genericRef); + Set triggered = triggerablesPerTrigger.get(genericRef); if (triggered == null) { return alreadyEvaluated; } @@ -607,7 +553,7 @@ public boolean shouldTrustPreviouslyCommittedAnswer() { return false; } - protected final void publishSummary(String lead, TreeReference ref, Collection quickTriggerables) { + final void publishSummary(String lead, TreeReference ref, Collection quickTriggerables) { accessor.getEventNotifier().publishEvent(new Event(lead + ": " + (ref != null ? ref.toShortString() + ": " : "") + quickTriggerables.size() + " triggerables were fired.")); } @@ -617,13 +563,12 @@ public void reportDependencyCycles() { //build graph List targets = new ArrayList<>(); - for (TreeReference trigger : triggerIndex.keySet()) { + for (TreeReference trigger : triggerablesPerTrigger.keySet()) { vertices.add(trigger); - List triggered = triggerIndex.get(trigger); + Set triggered = triggerablesPerTrigger.get(trigger); targets.clear(); for (QuickTriggerable qt : triggered) { - for (int j = 0; j < qt.getTargets().size(); j++) { - TreeReference target = qt.getTargets().get(j); + for (TreeReference target : qt.getTargets()) { if (!targets.contains(target)) targets.add(target); } @@ -696,7 +641,7 @@ public static List readExternalTriggerables(DataInputStream dis, Pr private List getConditions() { List conditions = new ArrayList<>(); - for (QuickTriggerable qt : unorderedTriggerables) { + for (QuickTriggerable qt : allTriggerables) { if (qt.isCondition()) { conditions.add((Condition) qt.getTriggerable()); } @@ -706,7 +651,7 @@ private List getConditions() { private List getRecalculates() { List recalculates = new ArrayList<>(); - for (QuickTriggerable qt : unorderedTriggerables) { + for (QuickTriggerable qt : allTriggerables) { if (qt.isRecalculate()) { recalculates.add((Recalculate) qt.getTriggerable()); } diff --git a/src/main/java/org/javarosa/core/model/condition/Condition.java b/src/main/java/org/javarosa/core/model/condition/Condition.java index 6585a8359..d074a15b4 100644 --- a/src/main/java/org/javarosa/core/model/condition/Condition.java +++ b/src/main/java/org/javarosa/core/model/condition/Condition.java @@ -19,7 +19,6 @@ import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.IOException; -import java.util.List; import java.util.Set; import org.javarosa.core.model.QuickTriggerable; import org.javarosa.core.model.instance.FormInstance; @@ -42,7 +41,7 @@ public class Condition extends Triggerable { public Condition() { } - protected Condition(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, List targets, Set immediateCascades, ConditionAction trueAction, ConditionAction falseAction) { + protected Condition(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, Set targets, Set immediateCascades, ConditionAction trueAction, ConditionAction falseAction) { super(expr, contextRef, originalContextRef, targets, immediateCascades); this.trueAction = trueAction; this.falseAction = falseAction; diff --git a/src/main/java/org/javarosa/core/model/condition/Recalculate.java b/src/main/java/org/javarosa/core/model/condition/Recalculate.java index 18485d773..8f8c6ca50 100644 --- a/src/main/java/org/javarosa/core/model/condition/Recalculate.java +++ b/src/main/java/org/javarosa/core/model/condition/Recalculate.java @@ -19,7 +19,6 @@ import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.IOException; -import java.util.List; import java.util.Set; import org.javarosa.core.model.QuickTriggerable; import org.javarosa.core.model.data.IAnswerData; @@ -41,7 +40,7 @@ public Recalculate() { } - protected Recalculate(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, List targets, Set immediateCascades) { + protected Recalculate(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, Set targets, Set immediateCascades) { super(expr, contextRef, originalContextRef, targets, immediateCascades); } diff --git a/src/main/java/org/javarosa/core/model/condition/Triggerable.java b/src/main/java/org/javarosa/core/model/condition/Triggerable.java index a56b18326..4e7460921 100644 --- a/src/main/java/org/javarosa/core/model/condition/Triggerable.java +++ b/src/main/java/org/javarosa/core/model/condition/Triggerable.java @@ -58,7 +58,7 @@ public abstract class Triggerable implements Externalizable { * References to all of the (non-contextualized) nodes which should be * updated by the result of this triggerable */ - protected List targets; + protected Set targets; /** * Current reference which is the "Basis" of the trigerrables being evaluated. This is the highest @@ -79,7 +79,7 @@ protected Triggerable() { } - protected Triggerable(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, List targets, Set immediateCascades) { + protected Triggerable(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, Set targets, Set immediateCascades) { this.expr = expr; this.targets = targets; this.contextRef = contextRef; @@ -88,11 +88,11 @@ protected Triggerable(XPathConditional expr, TreeReference contextRef, TreeRefer } public static Triggerable condition(XPathConditional expr, ConditionAction trueAction, ConditionAction falseAction, TreeReference contextRef) { - return new Condition(expr, contextRef, contextRef, new ArrayList<>(), new HashSet<>(), trueAction, falseAction); + return new Condition(expr, contextRef, contextRef, new HashSet<>(), new HashSet<>(), trueAction, falseAction); } public static Triggerable recalculate(XPathConditional expr, TreeReference contextRef) { - return new Recalculate(expr, contextRef, contextRef, new ArrayList<>(), new HashSet<>()); + return new Recalculate(expr, contextRef, contextRef, new HashSet<>(), new HashSet<>()); } public abstract Object eval(FormInstance instance, EvaluationContext ec); @@ -167,12 +167,12 @@ public final List apply(FormInstance mainInstance, EvaluationC return affectedNodes; } - public List getTargets() { + public Set getTargets() { return targets; } - public void changeContextRefToIntersectWithTriggerable(Triggerable t) { - contextRef = contextRef.intersect(t.contextRef); + public void intersectContextWith(Triggerable other) { + contextRef = contextRef.intersect(other.contextRef); } public TreeReference getContext() { @@ -196,9 +196,7 @@ public IConditionExpr getExpr() { } public void addTarget(TreeReference target) { - if (targets.indexOf(target) == -1) { - targets.add(target); - } + targets.add(target); } @Override @@ -242,7 +240,7 @@ public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOExcep expr = (XPathConditional) ExtUtil.read(in, new ExtWrapTagged(), pf); contextRef = (TreeReference) ExtUtil.read(in, TreeReference.class, pf); originalContextRef = (TreeReference) ExtUtil.read(in, TreeReference.class, pf); - targets = new ArrayList<>((List) ExtUtil.read(in, new ExtWrapList(TreeReference.class), pf)); + targets = new HashSet<>((List) ExtUtil.read(in, new ExtWrapList(TreeReference.class), pf)); } @Override diff --git a/src/main/java/org/javarosa/form/api/FormEntryController.java b/src/main/java/org/javarosa/form/api/FormEntryController.java index c8bde4e05..e8b155939 100644 --- a/src/main/java/org/javarosa/form/api/FormEntryController.java +++ b/src/main/java/org/javarosa/form/api/FormEntryController.java @@ -112,6 +112,7 @@ public int answerQuestion(FormIndex index, IAnswerData data, boolean midSurvey) throw new RuntimeException("Itemsets do not currently evaluate constraints. Your constraint will not work, please remove it before proceeding."); } else { try { + // TODO Design a test that exercizes this branch. model.getForm().copyItemsetAnswer(q, element, data); } catch (InvalidReferenceException ire) { logger.error("Error", ire); diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 5823c3fd0..787d982aa 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -1,5 +1,15 @@ package org.javarosa.xform.parse; +import static org.javarosa.xform.parse.XFormParser.buildInstanceStructure; +import static org.javarosa.xform.parse.XFormParser.getVagueLocation; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; import org.javarosa.core.model.Constants; import org.javarosa.core.model.DataBinding; import org.javarosa.core.model.FormDef; @@ -10,6 +20,7 @@ import org.javarosa.core.model.QuestionDef; import org.javarosa.core.model.condition.Constraint; import org.javarosa.core.model.condition.EvaluationContext; +import org.javarosa.core.model.instance.AbstractTreeElement; import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.FormInstance; import org.javarosa.core.model.instance.InvalidReferenceException; @@ -17,18 +28,9 @@ import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xform.util.XFormUtils; import org.kxml2.kdom.Element; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.Map; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.javarosa.xform.parse.XFormParser.buildInstanceStructure; -import static org.javarosa.xform.parse.XFormParser.getVagueLocation; - class FormInstanceParser { private static final Logger logger = LoggerFactory.getLogger(FormInstanceParser.class); @@ -40,7 +42,9 @@ class FormInstanceParser { private final List selectOnes; private final List selectMultis; private final List actionTargets; - /** pseudo-data model tree that describes the repeat structure of the instance; useful during instance processing and validation */ + /** + * pseudo-data model tree that describes the repeat structure of the instance; useful during instance processing and validation + */ private FormInstance repeatTree; FormInstanceParser(FormDef formDef, String defaultNamespace, @@ -59,12 +63,12 @@ class FormInstanceParser { FormInstance parseInstance(Element e, boolean isMainInstance, String name, Map namespacePrefixesByUri) { TreeElement root = buildInstanceStructure(e, null, !isMainInstance ? name : null, e.getNamespace(), - namespacePrefixesByUri, null); + namespacePrefixesByUri, null); FormInstance instanceModel = new FormInstance(root); instanceModel.setName(isMainInstance ? formDef.getTitle() : name); final List usedAtts = Collections.unmodifiableList(Arrays.asList("id", "version", "uiVersion", "name", - "prefix", "delimiter")); + "prefix", "delimiter")); String schema = e.getNamespace(); if (schema != null && schema.length() > 0 && !schema.equals(defaultNamespace)) { @@ -87,7 +91,7 @@ FormInstance parseInstance(Element e, boolean isMainInstance, String name, Map missingTemplates = new ArrayList<>(); @@ -179,7 +185,7 @@ private void verifyBindings(FormInstance instance, String mainInstanceNodeName) verifyItemsetSrcDstCompatibility(instance); } - private void verifyActions (FormInstance instance) { + private void verifyActions(FormInstance instance) { //check the target of actions which are manipulating real values for (TreeReference target : actionTargets) { List nodes = new EvaluationContext(instance).expandReference(target, true); @@ -189,12 +195,12 @@ private void verifyActions (FormInstance instance) { } } - private static void checkDuplicateNodesAreRepeatable (TreeElement node) { + private static void checkDuplicateNodesAreRepeatable(TreeElement node) { int mult = node.getMult(); if (mult > 0) { //repeated node if (!node.isRepeatable()) { logger.warn("repeated nodes [{}] detected that have no repeat binding " + - "in the form; DO NOT bind questions to these nodes or their children!", + "in the form; DO NOT bind questions to these nodes or their children!", node.getName()); //we could do a more comprehensive safety check in the future } @@ -205,8 +211,10 @@ private static void checkDuplicateNodesAreRepeatable (TreeElement node) { } } - /** Checks repeat sets for homogeneity */ - private void checkHomogeneity (FormInstance instance) { + /** + * Checks repeat sets for homogeneity + */ + private void checkHomogeneity(FormInstance instance) { for (TreeReference ref : getRepeatableRefs()) { TreeElement template = null; for (TreeReference nref : new EvaluationContext(instance).expandReference(ref)) { @@ -224,7 +232,7 @@ private void checkHomogeneity (FormInstance instance) { } } - private void verifyControlBindings (IFormElement fe, FormInstance instance, List errors) { //throws XmlPullParserException { + private void verifyControlBindings(IFormElement fe, FormInstance instance, List errors) { //throws XmlPullParserException { if (fe.getChildren() == null) return; @@ -235,7 +243,7 @@ private void verifyControlBindings (IFormElement fe, FormInstance instance, List if (child instanceof GroupDef) { ref = child.getBind(); - type = (((GroupDef)child).getRepeat() ? "Repeat" : "Group"); + type = (((GroupDef) child).getRepeat() ? "Repeat" : "Group"); } else if (child instanceof QuestionDef) { ref = child.getBind(); type = "Question"; @@ -259,13 +267,13 @@ private void verifyControlBindings (IFormElement fe, FormInstance instance, List } } - private void verifyRepeatMemberBindings (IFormElement fe, FormInstance instance, GroupDef parentRepeat) { + private void verifyRepeatMemberBindings(IFormElement fe, FormInstance instance, GroupDef parentRepeat) { if (fe.getChildren() == null) return; for (int i = 0; i < fe.getChildren().size(); i++) { IFormElement child = fe.getChildren().get(i); - boolean isRepeat = (child instanceof GroupDef && ((GroupDef)child).getRepeat()); + boolean isRepeat = (child instanceof GroupDef && ((GroupDef) child).getRepeat()); //get bindings of current node and nearest enclosing repeat TreeReference repeatBind = (parentRepeat == null ? TreeReference.rootRef() : FormInstance.unpackReference(parentRepeat.getBind())); @@ -306,11 +314,11 @@ private void verifyRepeatMemberBindings (IFormElement fe, FormInstance instance, } } - verifyRepeatMemberBindings(child, instance, (isRepeat ? (GroupDef)child : parentRepeat)); + verifyRepeatMemberBindings(child, instance, (isRepeat ? (GroupDef) child : parentRepeat)); } } - private void verifyItemsetBindings (FormInstance instance) { + private void verifyItemsetBindings(FormInstance instance) { for (ItemsetBinding itemset : itemsets) { //check proper parent/child relationship if (!itemset.nodesetRef.isAncestorOf(itemset.labelRef, false)) { @@ -350,7 +358,7 @@ else if (itemset.valueRef != null && fi.getTemplatePath(itemset.valueRef) == nul } } - private void verifyItemsetSrcDstCompatibility (FormInstance instance) { + private void verifyItemsetSrcDstCompatibility(FormInstance instance) { for (ItemsetBinding itemset : itemsets) { boolean destRepeatable = (instance.getTemplate(itemset.getDestRef()) != null); if (itemset.copyMode) { @@ -378,68 +386,104 @@ private void verifyItemsetSrcDstCompatibility (FormInstance instance) { } } - private void applyInstanceProperties (FormInstance instance) { + private void applyInstanceProperties(FormInstance instance) { for (DataBinding bind : bindings) { - final TreeReference ref = FormInstance.unpackReference(bind.getReference()); - final List nodes = new EvaluationContext(instance).expandReference(ref, true); - - if (nodes.size() > 0) { - attachBindGeneral(bind); - } - for (TreeReference nref : nodes) { - attachBind(instance.resolveReference(nref), bind); + TreeReference ref = FormInstance.unpackReference(bind.getReference()); + EvaluationContext ec = new EvaluationContext(instance); + List nodeRefs = ec.expandReference(ref, true); + + // Add triggerable targets if needed + if (bind.relevancyCondition != null) { + bind.relevancyCondition.addTarget(ref); + // Since relevancy can affect not only to individual fields, but also to + // groups, we need to register all descendant refs as targets for relevancy + // conditions to allow for chained reactions in triggerables registered in + // any of those descendants + for (TreeReference r : getDescendantRefs(instance, ec, ref)) + bind.relevancyCondition.addTarget(r); + } + if (bind.requiredCondition != null) + bind.requiredCondition.addTarget(ref); + if (bind.readonlyCondition != null) + bind.readonlyCondition.addTarget(ref); + if (bind.calculate != null) + bind.calculate.addTarget(ref); + + // Initialize present nodes under the provided ref + for (TreeReference nodeRef : nodeRefs) { + TreeElement node = instance.resolveReference(nodeRef); + node.setDataType(bind.getDataType()); + + if (bind.relevancyCondition == null) + node.setRelevant(bind.relevantAbsolute); + if (bind.requiredCondition == null) + node.setRequired(bind.requiredAbsolute); + if (bind.readonlyCondition == null) + node.setEnabled(!bind.readonlyAbsolute); + if (bind.constraint != null) + node.setConstraint(new Constraint(bind.constraint, bind.constraintMessage)); + + node.setPreloadHandler(bind.getPreload()); + node.setPreloadParams(bind.getPreloadParams()); + node.setBindAttributes(bind.getAdditionalAttributes()); } } applyControlProperties(instance); } - private static void attachBindGeneral (DataBinding bind) { - TreeReference ref = FormInstance.unpackReference(bind.getReference()); - - if (bind.relevancyCondition != null) { - bind.relevancyCondition.addTarget(ref); - } - if (bind.requiredCondition != null) { - bind.requiredCondition.addTarget(ref); - } - if (bind.readonlyCondition != null) { - bind.readonlyCondition.addTarget(ref); - } - if (bind.calculate != null) { - bind.calculate.addTarget(ref); + private Set getDescendantRefs(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original) { + Set descendantRefs = new HashSet<>(); + TreeElement repeatTemplate = mainInstance.getTemplatePath(original); + if (repeatTemplate != null) { + for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { + TreeElement child = repeatTemplate.getChildAt(i); + descendantRefs.add(child.getRef().genericize()); + descendantRefs.addAll(getDescendantRefs(mainInstance, child)); + } + } else { + // TODO Eventually remove this branch because a parsed form can't force the program flow throw it + List refSet = evalContext.expandReference(original); + for (TreeReference ref : refSet) { + descendantRefs.addAll(getDescendantRefs(mainInstance, evalContext.resolveReference(ref))); + } } + return descendantRefs; } - private static void attachBind(TreeElement node, DataBinding bind) { - node.setDataType(bind.getDataType()); - - if (bind.relevancyCondition == null) { - node.setRelevant(bind.relevantAbsolute); - } - if (bind.requiredCondition == null) { - node.setRequired(bind.requiredAbsolute); - } - if (bind.readonlyCondition == null) { - node.setEnabled(!bind.readonlyAbsolute); - } - if (bind.constraint != null) { - node.setConstraint(new Constraint(bind.constraint, bind.constraintMessage)); + // Recursive step of utility method + private Set getDescendantRefs(FormInstance mainInstance, AbstractTreeElement el) { + Set childrenRefs = new HashSet<>(); + TreeElement repeatTemplate = mainInstance.getTemplatePath(el.getRef()); + if (repeatTemplate != null) { + for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { + TreeElement child = repeatTemplate.getChildAt(i); + childrenRefs.add(child.getRef().genericize()); + childrenRefs.addAll(getDescendantRefs(mainInstance, child)); + } + } else { + for (int i = 0; i < el.getNumChildren(); ++i) { + AbstractTreeElement child = el.getChildAt(i); + childrenRefs.add(child.getRef().genericize()); + childrenRefs.addAll(getDescendantRefs(mainInstance, child)); + } } - - node.setPreloadHandler(bind.getPreload()); - node.setPreloadParams(bind.getPreloadParams()); - node.setBindAttributes(bind.getAdditionalAttributes()); + return childrenRefs; } - /** Checks which repeat bindings have explicit template nodes; returns a list of the bindings that do not */ - private static void checkRepeatsForTemplate (FormInstance instance, FormInstance repeatTree, List missingTemplates) { + + /** + * Checks which repeat bindings have explicit template nodes; returns a list of the bindings that do not + */ + private static void checkRepeatsForTemplate(FormInstance instance, FormInstance repeatTree, List missingTemplates) { if (repeatTree != null) checkRepeatsForTemplate(repeatTree.getRoot(), TreeReference.rootRef(), instance, missingTemplates); } - /** Helper function for checkRepeatsForTemplate */ - private static void checkRepeatsForTemplate (TreeElement repeatTreeNode, TreeReference ref, FormInstance instance, List missing) { + /** + * Helper function for checkRepeatsForTemplate + */ + private static void checkRepeatsForTemplate(TreeElement repeatTreeNode, TreeReference ref, FormInstance instance, List missing) { String name = repeatTreeNode.getName(); int mult = (repeatTreeNode.isRepeatable() ? TreeReference.INDEX_TEMPLATE : 0); ref = ref.extendRef(name, mult); @@ -459,12 +503,12 @@ private static void checkRepeatsForTemplate (TreeElement repeatTreeNode, TreeRef //iterates through instance and removes template nodes that are not valid. a template is invalid if: // it is declared for a node that is not repeatable // it is for a repeat that is a child of another repeat and is not located within the parent's template node - private void removeInvalidTemplates (FormInstance instance, FormInstance repeatTree) { + private void removeInvalidTemplates(FormInstance instance, FormInstance repeatTree) { removeInvalidTemplates(instance.getRoot(), (repeatTree == null ? null : repeatTree.getRoot()), true); } //helper function for removeInvalidTemplates - private boolean removeInvalidTemplates (TreeElement instanceNode, TreeElement repeatTreeNode, boolean templateAllowed) { + private boolean removeInvalidTemplates(TreeElement instanceNode, TreeElement repeatTreeNode, boolean templateAllowed) { int mult = instanceNode.getMult(); boolean repeatable = repeatTreeNode != null && repeatTreeNode.isRepeatable(); @@ -496,7 +540,7 @@ private boolean removeInvalidTemplates (TreeElement instanceNode, TreeElement re } //if repeatables have no template node, duplicate first as template - private void createMissingTemplates (FormInstance instance, List missingTemplates) { + private void createMissingTemplates(FormInstance instance, List missingTemplates) { //it is VERY important that the missing template refs are listed in depth-first or breadth-first order... namely, that //every ref is listed after a ref that could be its parent. checkRepeatsForTemplate currently behaves this way for (TreeReference templRef : missingTemplates) { @@ -529,7 +573,7 @@ private void createMissingTemplates (FormInstance instance, List * Trims repeatable children of newly created template nodes; we trim because the templates are supposed to be devoid of 'data', * and # of repeats for a given repeat node is a kind of data. */ - private static void trimRepeatChildren (TreeElement node) { + private static void trimRepeatChildren(TreeElement node) { for (int i = 0; i < node.getNumChildren(); i++) { TreeElement child = node.getChildAt(i); if (child.isRepeatable()) { @@ -567,7 +611,7 @@ private void applyControlProperties(FormInstance instance) { } } - private List getRepeatableRefs () { + private List getRepeatableRefs() { List refs = new ArrayList<>(repeats); for (ItemsetBinding itemset : itemsets) { @@ -611,7 +655,7 @@ private List getRepeatableRefs () { * result is a FormInstance collapsed where all indexes are 0, and repeatable nodes are flagged as such. * Ignores (invalid) repeats that bind outside the top-level instance data node. Returns null if no repeats. */ - private static FormInstance buildRepeatTree (List repeatRefs, String topLevelName) { + private static FormInstance buildRepeatTree(List repeatRefs, String topLevelName) { TreeElement root = new TreeElement(null, 0); for (TreeReference repeatRef : repeatRefs) { @@ -639,6 +683,6 @@ private static FormInstance buildRepeatTree (List repeatRefs, Str } return (root.getNumChildren() == 0) ? null : - new FormInstance(root.getChild(topLevelName, TreeReference.DEFAULT_MULTIPLICITY)); + new FormInstance(root.getChild(topLevelName, TreeReference.DEFAULT_MULTIPLICITY)); } } diff --git a/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java b/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java index c1316b8b8..7f2a93963 100644 --- a/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java +++ b/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java @@ -59,9 +59,7 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef } else if ("false()".equals(xpathRel)) { binding.relevantAbsolute = false; } else { - Triggerable c = buildCondition(xpathRel, "relevant", ref); - c = formDef.addTriggerable(c); - binding.relevancyCondition = c; + binding.relevancyCondition = formDef.addTriggerable(buildCondition(xpathRel, "relevant", ref)); } } @@ -72,9 +70,7 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef } else if ("false()".equals(xpathReq)) { binding.requiredAbsolute = false; } else { - Triggerable c = buildCondition(xpathReq, "required", ref); - c = formDef.addTriggerable(c); - binding.requiredCondition = c; + binding.requiredCondition = formDef.addTriggerable(buildCondition(xpathReq, "required", ref)); } } @@ -85,9 +81,7 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef } else if ("false()".equals(xpathRO)) { binding.readonlyAbsolute = false; } else { - Triggerable c = buildCondition(xpathRO, "readonly", ref); - c = formDef.addTriggerable(c); - binding.readonlyCondition = c; + binding.readonlyCondition = formDef.addTriggerable(buildCondition(xpathRO, "readonly", ref)); } } @@ -104,15 +98,13 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef final String xpathCalc = element.getAttributeValue(null, "calculate"); if (xpathCalc != null) { - Triggerable r; try { - r = buildCalculate(xpathCalc, ref); + binding.calculate = formDef.addTriggerable(buildCalculate(xpathCalc, ref)); } catch (XPathSyntaxException xpse) { throw new XFormParseException("Invalid calculate for the bind attached to \"" + nodeset + "\" : " + xpse.getMessage() + " in expression " + xpathCalc); } - r = formDef.addTriggerable(r); - binding.calculate = r; + } binding.setPreload(element.getAttributeValue(NAMESPACE_JAVAROSA, "preload")); diff --git a/src/test/java/org/javarosa/core/model/TriggerableDagTest.java b/src/test/java/org/javarosa/core/model/TriggerableDagTest.java index 8bef8d9a2..4a1e5ea9d 100644 --- a/src/test/java/org/javarosa/core/model/TriggerableDagTest.java +++ b/src/test/java/org/javarosa/core/model/TriggerableDagTest.java @@ -277,10 +277,12 @@ public void deleteThirdRepeatGroup_evaluatesTriggerables_dependentOnTheRepeatGro } /** - * Indirectly means that the calculation - `concat(/data/house/name)` - does not take the + * Indirectly means that the calculation - `concat(/data/house/name)` - does + * not take the * `/data/house` nodeset (the repeat group) as an argument * but since it takes one of its children (`name` children), - * the calculation must re-evaluated once after a repeat group deletion because one of the children + * the calculation must re-evaluated once after a repeat group deletion + * because one of the children * has been deleted along with its parent (the repeat group instance). */ @Test @@ -325,8 +327,10 @@ public void deleteThirdRepeatGroup_evaluatesTriggerables_indirectlyDependentOnTh /** * This test was inspired by the issue reported at https://code.google.com/archive/p/opendatakit/issues/888 *

- * We want to focus on the relationship between relevancy and other calculations because relevancy can be - * defined for fields **and groups**, which is a special case of expression evaluation in our DAG. + * We want to focus on the relationship between relevancy and other + * calculations because relevancy can be + * defined for fields **and groups**, which is a special case of expression + * evaluation in our DAG. */ @Test public void verify_relation_between_calculate_expressions_and_relevancy_conditions() throws IOException { @@ -375,12 +379,16 @@ public void verify_relation_between_calculate_expressions_and_relevancy_conditio } /** - * Ignored because the assertions about non-null next-numbers will fail because our DAG - * doesn't evaluate calculations in repeat instances that are previous siblings to the + * Ignored because the assertions about non-null next-numbers will fail + * because our DAG + * doesn't evaluate calculations in repeat instances that are previous + * siblings to the * one that has changed. *

- * The expeculation is that, originally, only forward references might have been considered - * to speed up repeat group intance deletion because it was assumed that back references + * The expeculation is that, originally, only forward references might have + * been considered + * to speed up repeat group intance deletion because it was assumed that + * back references * were a marginal use case. *

* This test explores how the issue affects to value changes too. @@ -453,8 +461,10 @@ public void calculate_expressions_should_be_evaluated_on_previous_repeat_sibling } /** - * Ignored because the count() function inside the predicate of the result_2 calculate - * expression isn't evaled correctly, as opposed to the predicate of the result_1 calculate + * Ignored because the count() function inside the predicate of the result_2 + * calculate + * expression isn't evaled correctly, as opposed to the predicate of the + * result_1 calculate * that uses an interim field as a proxy for the same computation */ @Test @@ -710,9 +720,12 @@ public void parsing_forms_with_self_reference_cycles_in_fields_of_repeat_groups_ } /** - * This test fails to parse the form because it thinks there's a self-reference cycle in /data/group/a, - * but this would be incorrect because each it depends on the same field belonging to the previous - * repeat instance, which wouldn't be a cycle, but an autoincremental feature. + * This test fails to parse the form because it thinks there's a + * self-reference cycle in /data/group/a, + * but this would be incorrect because each it depends on the same field + * belonging to the previous + * repeat instance, which wouldn't be a cycle, but an autoincremental + * feature. */ @Test @Ignore @@ -762,6 +775,36 @@ public void parsing_forms_with_cycles_between_fields_of_the_same_repeat_instance )); } + @Test + public void order_of_the_DAG_is_ensured() throws IOException { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("a", "2"), + t("b"), + t("c") + )), + bind("/data/a").type("int"), + bind("/data/b").type("int").calculate("/data/a * 3"), + bind("/data/c").type("int").calculate("(/data/a + /data/b) * 5") + ) + ), + body(input("/data/a")) + )); + + assertThat(scenario.answerOf("/data/a"), is(intAnswer(2))); + assertThat(scenario.answerOf("/data/b"), is(intAnswer(6))); + assertThat(scenario.answerOf("/data/c"), is(intAnswer(40))); + + scenario.answer("/data/a", 3); + + assertThat(scenario.answerOf("/data/a"), is(intAnswer(3))); + assertThat(scenario.answerOf("/data/b"), is(intAnswer(9))); + // Verify that c gets computed using the updated value of b. + assertThat(scenario.answerOf("/data/c"), is(intAnswer(60))); + } private void assertDagEvents(List dagEvents, String... lines) { assertThat(dagEvents.stream().map(Event::getDisplayMessage).collect(joining("\n")), is(join("\n", lines)));