CARDS-2510: Fix computed questions always recalculating on checkin

modules/data-model/forms/api/src/main/java/io/uhndata/cards/forms/api/ExpressionUtils.java

@@ -68,11 +68,12 @@ public interface ExpressionUtils
      *
      * @param question the question node
      * @param values the other values in the form
+     * @param changedQuestions a list of question identifiers that have changed leading up to this evaluation
      * @return a string representation of the result, may be {@code null} if the expression has unmet dependencies
      */
-    default String evaluate(Node question, Map<String, Object> values)
+    default String evaluate(Node question, Map<String, Object> values, Set<String> changedQuestions)
     {
-        return String.valueOf(evaluate(question, values, Type.STRING));
+        return String.valueOf(evaluate(question, values, Type.STRING, changedQuestions).getResult());
     }
 
     /**
@@ -81,8 +82,48 @@ default String evaluate(Node question, Map<String, Object> values)
      * @param question the question node
      * @param values the other values in the form
      * @param type the expected type of the result
+     * @param changedQuestions a list of question identifiers that have changed leading up to this evaluation
      * @return a representation of the evaluation result, forced into the desired data type; may be {@code null} if the
      *         expression has unmet dependencies or the actual evaluation result cannot be converted to the desired type
      */
-    Object evaluate(Node question, Map<String, Object> values, Type<?> type);
+    ExpressionResult evaluate(Node question, Map<String, Object> values, Type<?> type, Set<String> changedQuestions);
+
+    /**
+     *
+     */
+    final class ExpressionResult
+    {
+        private final boolean missingValue;
+        private final boolean usedChangedValue;
+        private final Object result;
+        private final int arguments;
+
+        public ExpressionResult(boolean missingValue, boolean usedChangedValue, Object result, int arguments)
+        {
+            this.missingValue = missingValue;
+            this.usedChangedValue = usedChangedValue;
+            this.result = result;
+            this.arguments = arguments;
+        }
+
+        public boolean hasMissingValue()
+        {
+            return this.missingValue;
+        }
+
+        public boolean expressionUsedChangedValue()
+        {
+            return this.usedChangedValue;
+        }
+
+        public Object getResult()
+        {
+            return this.result;
+        }
+
+        public int numberOfArguments()
+        {
+            return this.arguments;
+        }
+    }
 }

modules/data-model/forms/api/src/main/java/io/uhndata/cards/forms/api/FormUtils.java

@@ -320,6 +320,15 @@ enum SearchType
      */
     Node getAnswer(Node form, Node question);
 
+    /**
+     * Get the first answer for a specific question, if any.
+     *
+     * @param form a Form node
+     * @param question a question node, part of the questionnaire that the form is answering
+     * @return an Answer node, may be {@code null}
+     */
+    NodeState getAnswer(NodeState form, Node question);
+
     /**
      * Get all the answers for a specific question, if any.
      *

modules/data-model/forms/impl/src/main/java/io/uhndata/cards/forms/internal/AnswersEditor.java

@@ -23,6 +23,8 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import javax.jcr.Node;
 import javax.jcr.NodeIterator;
@@ -155,15 +157,15 @@ protected Node getQuestionnaire()
     }
 
     // Returns a QuestionTree if any children of this node contains an unanswered matching question, else null
-    protected QuestionTree getUnansweredMatchingQuestions(final Node currentNode)
+    protected QuestionTree getUnmodifiedMatchingQuestions(final Node currentNode)
     {
         QuestionTree currentTree = null;
 
         try {
             if (isQuestionNodeMatchingType(currentNode)) {
                 // Ignore questions that do not match the question type this editor is looking for
-                // Skip already answered questions
-                if (!this.answerChangeTracker.getModifiedAnswers().contains(currentNode.getIdentifier())) {
+                // Skip questions that were modified in the most recent save
+                if (!this.answerChangeTracker.getMatchedModifiedAnswers().contains(currentNode.getIdentifier())) {
                     currentTree = new QuestionTree(currentNode, true, this.formUtils);
                 }
             } else if (this.questionnaireUtils.isQuestionnaire(currentNode)
@@ -172,7 +174,7 @@ protected QuestionTree getUnansweredMatchingQuestions(final Node currentNode)
                 QuestionTree newTree = new QuestionTree(currentNode, false, this.formUtils);
                 for (NodeIterator i = currentNode.getNodes(); i.hasNext();) {
                     Node child = i.nextNode();
-                    QuestionTree childTree = getUnansweredMatchingQuestions(child);
+                    QuestionTree childTree = getUnmodifiedMatchingQuestions(child);
                     if (childTree != null) {
                         // Child has data that should be stored
                         newTree.getChildren().put(child.getName(), childTree);
@@ -196,7 +198,8 @@ protected abstract static class AbstractAnswerChangeTracker extends DefaultEdito
     {
         private final FormUtils formUtils;
 
-        private final Set<String> modifiedAnswers = new HashSet<>();
+        private final Set<String> matchedModifiedAnswers = new HashSet<>();
+        private final Set<String> unmatchedModifiedAnswers = new HashSet<>();
 
         private boolean inMatchedAnswer;
 
@@ -213,9 +216,9 @@ public AbstractAnswerChangeTracker(final FormUtils formUtils)
         public void enter(NodeState before, NodeState after)
         {
             String questionId = this.formUtils.getQuestionIdentifier(after);
+            this.currentAnswer = questionId;
             if (isMatchedAnswerNode(after, questionId)) {
                 this.inMatchedAnswer = true;
-                this.currentAnswer = questionId;
             }
         }
 
@@ -230,7 +233,9 @@ public void leave(NodeState before, NodeState after)
         public void propertyAdded(PropertyState after)
         {
             if (this.inMatchedAnswer) {
-                this.modifiedAnswers.add(this.currentAnswer);
+                this.matchedModifiedAnswers.add(this.currentAnswer);
+            } else {
+                this.unmatchedModifiedAnswers.add(this.currentAnswer);
             }
         }
 
@@ -264,9 +269,20 @@ public Editor childNodeDeleted(String name, NodeState before)
             return this;
         }
 
-        public Set<String> getModifiedAnswers()
+        public Set<String> getMatchedModifiedAnswers()
+        {
+            return this.matchedModifiedAnswers;
+        }
+
+        public Set<String> getUnmatchedModifiedAnswers()
+        {
+            return this.unmatchedModifiedAnswers;
+        }
+
+        public Set<String> getAllModifiedAnswers()
         {
-            return this.modifiedAnswers;
+            return Stream.concat(this.matchedModifiedAnswers.stream(), this.unmatchedModifiedAnswers.stream())
+                .collect(Collectors.toSet());
         }
     }
 

modules/data-model/forms/impl/src/main/java/io/uhndata/cards/forms/internal/ComputedAnswersEditor.java

@@ -126,9 +126,9 @@ protected void handleLeave(final NodeState form)
             return;
         }
         final QuestionTree computedQuestionsTree =
-            getUnansweredMatchingQuestions(questionnaireNode);
+            getUnmodifiedMatchingQuestions(questionnaireNode);
 
-        // There are missing computed questions, let's create them!
+        // There are computed questions that were not modified in the most recent save, calculate them!
         if (computedQuestionsTree != null) {
             Map<Node, NodeBuilder> questionAndAnswers =
                 computedQuestionsTree.getQuestionAndAnswers(this.currentNodeBuilder);
@@ -145,6 +145,18 @@ protected void handleLeave(final NodeState form)
                 }).collect(Collectors.toConcurrentMap(Pair::getKey, Pair::getValue));
             final List<String> orderedAnswersToCompute = sortDependencies(computedAnswerDependencies);
 
+            final Set<String> changedQuestions = new HashSet<>();
+            for (String modifiedAnswer : this.answerChangeTracker.getAllModifiedAnswers()) {
+                try {
+                    Node questionNode = this.currentSession.getNodeByIdentifier(modifiedAnswer);
+                    String name = questionNode.getName();
+                    changedQuestions.add(name);
+                } catch (Exception e) {
+                    // Could not find the answer: not an answer the user has permissions to edit so can't be modified
+                    // by the user
+                }
+            }
+
             // We have the right order, compute all the missing answers
             orderedAnswersToCompute.stream()
                 // Get the right answer node
@@ -154,35 +166,64 @@ protected void handleLeave(final NodeState form)
                     .findFirst().get())
                 // Evaluate it
                 .forEachOrdered(entry -> {
-                    computeAnswer(entry, answersByQuestionName);
+                    computeAnswer(entry, form, answersByQuestionName, changedQuestions);
                 });
         }
     }
 
-    private void computeAnswer(final Map.Entry<Node, NodeBuilder> entry,
-        final Map<String, Object> answersByQuestionName)
+    // Calculate and save the answer for the provided computed question if it should be evaluated
+    private void computeAnswer(final Map.Entry<Node, NodeBuilder> entry, NodeState form,
+        final Map<String, Object> answersByQuestionName, final Set<String> changedQuestions)
     {
-        final Node question = entry.getKey();
-        final NodeBuilder answer = entry.getValue();
-        Type<?> resultType = getAnswerType(question);
-        Object result = this.expressionUtils.evaluate(question, answersByQuestionName, resultType);
+        try {
+            final Node question = entry.getKey();
+            final NodeBuilder answer = entry.getValue();
+            Type<?> resultType = getAnswerType(question);
 
-        if (result == null || (result instanceof String && "null".equals(result))) {
-            answer.removeProperty(FormUtils.VALUE_PROPERTY);
-        } else {
-            // Type erasure makes the actual type irrelevant, there's only one real implementation method
-            // The implementation can extract the right type from the type object
-            @SuppressWarnings("unchecked")
-            Type<Object> untypedResultType = (Type<Object>) resultType;
-            answer.setProperty(FormUtils.VALUE_PROPERTY, result, untypedResultType);
-        }
-        // Update the computed value in the map of existing answers
-        String questionName = this.questionnaireUtils.getQuestionName(question);
-        if (answersByQuestionName.containsKey(questionName)) {
-            // Question has multiple answers. Ignore this answer, just keep previous.
-            // TODO: Implement better recurrent section handling
-        } else {
-            answersByQuestionName.put(questionName, result);
+            ExpressionUtils.ExpressionResult expressionResult = this.expressionUtils.evaluate(question,
+                answersByQuestionName, resultType, changedQuestions);
+
+            NodeState existingAnswer = this.formUtils.getAnswer(form, question);
+
+            // Do not recompute the question if:
+            // - The question depends on other answers(s)
+            // - AND none of the other answer(s) have changed
+            // - AND this question already has an answer
+            if (
+                expressionResult.numberOfArguments() > 0
+                    && !expressionResult.expressionUsedChangedValue()
+                    && existingAnswer != null
+            ) {
+                return;
+            }
+
+            Object result = expressionResult.getResult();
+            if (result instanceof String && "null".equals(result)) {
+                result = null;
+            }
+
+            if (result == null) {
+                answer.removeProperty(FormUtils.VALUE_PROPERTY);
+            } else {
+                // Type erasure makes the actual type irrelevant, there's only one real implementation method
+                // The implementation can extract the right type from the type object
+                @SuppressWarnings("unchecked")
+                Type<Object> untypedResultType = (Type<Object>) resultType;
+                answer.setProperty(FormUtils.VALUE_PROPERTY, result, untypedResultType);
+            }
+            // Mark that this question has been updated
+            changedQuestions.add(question.getName());
+            // Update the computed value in the map of existing answers
+            String questionName = this.questionnaireUtils.getQuestionName(question);
+            if (answersByQuestionName.containsKey(questionName)) {
+                // Question has multiple answers. Ignore this answer, just keep previous.
+                // TODO: Implement better recurrent section handling
+            } else {
+                answersByQuestionName.put(questionName, result);
+            }
+        } catch (RepositoryException e) {
+            // Should not happen
+            LOGGER.warn("Error calculating computing answer", e);
         }
     }
 

modules/data-model/forms/impl/src/main/java/io/uhndata/cards/forms/internal/ExpressionUtilsImpl.java

@@ -26,6 +26,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import javax.jcr.Node;
 import javax.jcr.RepositoryException;
@@ -85,27 +86,34 @@ private Object toJavaScriptObject(ScriptEngine javascriptEngine, Object javaObje
     }
 
     @Override
-    public Object evaluate(final Node question, final Map<String, Object> values, final Type<?> type)
+    public ExpressionResult evaluate(final Node question, final Map<String, Object> values, final Type<?> type,
+        Set<String> changedQuestions)
     {
         try {
             String expression = getExpressionFromQuestion(question);
             ExpressionUtilsImpl.ParsedExpression parsedExpression = parseExpressionInputs(expression, values);
             if (parsedExpression.hasMissingValue()) {
-                return null;
+                return new ExpressionResult(true, false, null, parsedExpression.getQuestions().size());
             }
 
             ScriptEngine engine = this.manager.getEngineByName("JavaScript");
 
             Bindings env = engine.createBindings();
-            parsedExpression.getQuestions().forEach((key, value) ->
-                env.put(value.getArgument(), toJavaScriptObject(engine, value.getValue())));
+            AtomicBoolean usedChangedValue = new AtomicBoolean(false);
+            parsedExpression.getQuestions().forEach((key, value) -> {
+                env.put(value.getArgument(), toJavaScriptObject(engine, value.getValue()));
+                if (changedQuestions.contains(value.getQuestionName())) {
+                    usedChangedValue.set(true);
+                }
+            });
             Object result = engine.eval("(function(){" + parsedExpression.getExpression() + "})()", env);
-            return ValueFormatter.formatResult(result, type);
+            return new ExpressionResult(false, usedChangedValue.get(), ValueFormatter.formatResult(result, type),
+                parsedExpression.getQuestions().size());
         } catch (ScriptException e) {
             LOGGER.warn("Evaluating the expression for question {} failed: {}", question,
                 e.getMessage(), e);
         }
-        return null;
+        return new ExpressionResult(false, false, null, 0);
     }
 
     private ExpressionUtilsImpl.ParsedExpression parseExpressionInputs(final String expression,
@@ -206,7 +214,8 @@ private void parseNextArgument()
                     }
                 }
 
-                ExpressionArgument arg = new ExpressionArgument("arg" + this.questions.size(), questionValue);
+                ExpressionArgument arg = new ExpressionArgument("arg" + this.questions.size(), questionName,
+                    questionValue);
 
                 this.questions.put(questionName, arg);
             }
@@ -284,12 +293,13 @@ public boolean hasMissingValue()
     private static final class ExpressionArgument
     {
         private final String argument;
-
+        private final String questionName;
         private final Object value;
 
-        ExpressionArgument(String argument, Object value)
+        ExpressionArgument(String argument, String questionName, Object value)
         {
             this.argument = argument;
+            this.questionName = questionName;
             this.value = value;
         }
 
@@ -298,6 +308,11 @@ public String getArgument()
             return this.argument;
         }
 
+        public String getQuestionName()
+        {
+            return this.questionName;
+        }
+
         public Object getValue()
         {
             return this.value;
@@ -327,6 +342,8 @@ static Long formatToLong(final Object rawResult)
         {
             if (rawResult instanceof String) {
                 return Long.valueOf((String) rawResult);
+            } else if (rawResult instanceof Integer) {
+                return Long.valueOf((Integer) rawResult);
             } else if (rawResult instanceof Long) {
                 return (Long) rawResult;
             } else if (rawResult instanceof Double) {