Merge pull request #1417 from dimagi/clearVolatilesForTreeElement

Fix repeat deletion Issue by implementing Clear volatiles for tree element
dimagi · Jul 4, 2024 · 126ec06 · 126ec06
2 parents 9242d2c + 9302ea1
commit 126ec06
Show file tree

Hide file tree

Showing 9 changed files with 82 additions and 2 deletions.
diff --git a/src/main/java/org/commcare/cases/instance/StorageBackedChildElement.java b/src/main/java/org/commcare/cases/instance/StorageBackedChildElement.java
@@ -198,6 +198,14 @@ public TreeReference getRef() {
         return ref;
     }
 
+    @Override
+    public void clearVolatiles() {
+        ref = null;
+        TreeElement cache = cache();
+        if (cache != null) {
+            cache.clearVolatiles();
+        }
+    }
 
     //Context Sensitive Methods
     @Override

diff --git a/src/main/java/org/commcare/cases/instance/StorageInstanceTreeElement.java b/src/main/java/org/commcare/cases/instance/StorageInstanceTreeElement.java
@@ -208,6 +208,16 @@ public TreeReference getRef() {
         return cachedRef;
     }
 
+    @Override
+    public void clearVolatiles() {
+        cachedRef = null;
+        if (elements != null) {
+            for (T element : elements) {
+                element.clearVolatiles();
+            }
+        }
+    }
+
     private void expireCachedRef() {
         cachedRef = null;
     }

diff --git a/src/main/java/org/commcare/cases/query/QuerySensitiveTreeElementWrapper.java b/src/main/java/org/commcare/cases/query/QuerySensitiveTreeElementWrapper.java
@@ -131,6 +131,11 @@ public TreeReference getRef() {
         return wrapped.getRef();
     }
 
+    @Override
+    public void clearVolatiles() {
+        wrapped.clearVolatiles();
+    }
+
     @Override
     public String getName() {
         return wrapped.getName();

diff --git a/src/main/java/org/javarosa/core/model/instance/AbstractTreeElement.java b/src/main/java/org/javarosa/core/model/instance/AbstractTreeElement.java
@@ -100,6 +100,9 @@ public interface AbstractTreeElement<T extends AbstractTreeElement> {
 
     String getNamespace();
 
+    // clear any cache maintained by the TreeElement implementation
+    void clearVolatiles();
+
     /**
      * TODO: Worst method name ever. Don't use this unless you know what's up.
      * @param predicates  possibly list of predicates to be evaluated. predicates will be removed from list if they are

diff --git a/src/main/java/org/javarosa/core/model/instance/ConcreteTreeElement.java b/src/main/java/org/javarosa/core/model/instance/ConcreteTreeElement.java
@@ -337,6 +337,20 @@ public TreeReference getRef() {
         return refCache;
     }
 
+    @Override
+    public void clearVolatiles() {
+        refCache = null;
+        if (children != null) {
+            for (T child : children) {
+                child.clearVolatiles();
+            }
+        }
+        if (attributes != null) {
+            for (T attribute : attributes) {
+                attribute.clearVolatiles();
+            }
+        }
+    }
 
     @Override
     public String getName() {

diff --git a/src/main/java/org/javarosa/core/model/instance/DataInstance.java b/src/main/java/org/javarosa/core/model/instance/DataInstance.java
@@ -289,7 +289,11 @@ public void setCacheHost(CacheHost cacheHost) {
         this.mCacheHost = cacheHost;
     }
 
+    /**
+     * Cleans reference caches maintained by the instance and TreeElements contained in the contextNode
+     */
     public void cleanCache() {
         referenceCache.clear();
+        getBase().clearVolatiles();
     }
 }
diff --git a/src/main/java/org/javarosa/core/model/instance/InstanceBase.java b/src/main/java/org/javarosa/core/model/instance/InstanceBase.java
@@ -141,6 +141,13 @@ public TreeReference getRef() {
         return TreeReference.rootRef();
     }
 
+    @Override
+    public void clearVolatiles() {
+        if (child != null) {
+            child.clearVolatiles();
+        }
+    }
+
     @Override
     public String getName() {
         return null;

diff --git a/src/main/java/org/javarosa/core/model/instance/TreeElement.java b/src/main/java/org/javarosa/core/model/instance/TreeElement.java
@@ -753,7 +753,6 @@ private void expireReferenceCache() {
     //return the tree reference that corresponds to this tree element
     @Override
     public TreeReference getRef() {
-        //TODO: Expire cache somehow;
         synchronized (refCache) {
             if (refCache[0] == null) {
                 refCache[0] = TreeReference.buildRefFromTreeElement(this);
@@ -854,6 +853,21 @@ public String getNamespace() {
         return namespace;
     }
 
+    @Override
+    public void clearVolatiles() {
+        refCache[0] = null;
+        if (children != null) {
+            for (TreeElement child : children) {
+                child.clearVolatiles();
+            }
+        }
+        if (attributes != null) {
+            for (TreeElement attribute : attributes) {
+                attribute.clearVolatiles();
+            }
+        }
+    }
+
     public void setNamespace(String namespace) {
         this.namespace = namespace;
     }

diff --git a/src/test/java/org/javarosa/core/model/test/FormDefTest.java b/src/test/java/org/javarosa/core/model/test/FormDefTest.java
@@ -443,7 +443,7 @@ public void testSimilarBindConditionsAreDistinguished() throws Exception {
 
     // regression test for when we weren't decrementing multiplicities correctly when repeats were deleted
     @Test
-    public void testDeleteRepeatMultiplicities() throws IOException {
+    public void testDeleteRepeatMultiplicities() throws IOException, XPathSyntaxException {
         FormParseInit fpi = new FormParseInit("/multiple_repeats.xml");
         FormEntryController fec = initFormEntry(fpi, "en");
         fec.stepToNextEvent();
@@ -483,6 +483,16 @@ public void testDeleteRepeatMultiplicities() throws IOException {
         assertEquals(root.getChildMultiplicity("question1"), 2);
         assertNotEquals(root.getChild("question1", 1), null);
 
+        EvaluationContext evalCtx = fpi.getFormDef().getEvaluationContext();
+        ExprEvalUtils.assertEqualsXpathEval("check repeat node set correctly",
+                "Second repeat, first iteration: question5", "/data/question4[1]/question5", evalCtx);
+        ExprEvalUtils.assertEqualsXpathEval("check repeat node set correctly",
+                "Second repeat, first iteration: question6", "/data/question4[1]/question6", evalCtx);
+        ExprEvalUtils.assertEqualsXpathEval("check repeat node set correctly",
+                "Second repeat, second iteration: question5", "/data/question4[2]/question5", evalCtx);
+        ExprEvalUtils.assertEqualsXpathEval("check repeat node set correctly",
+                "Second repeat, second iteration: question6", "/data/question4[2]/question6", evalCtx);
+
         fec.deleteRepeat(0);
 
         // Confirm that the deleted repeat is gone and its sibling's multiplicity reduced
@@ -491,6 +501,11 @@ public void testDeleteRepeatMultiplicities() throws IOException {
         // Confirm that the other repeat is unchanged
         assertEquals(root.getChildMultiplicity("question1"), 2);
         assertNotEquals(root.getChild("question1", 1), null);
+
+        ExprEvalUtils.assertEqualsXpathEval("check repeat node set correctly",
+                "Second repeat, second iteration: question5", "/data/question4[1]/question5", evalCtx);
+        ExprEvalUtils.assertEqualsXpathEval("check repeat node set correctly",
+                "Second repeat, second iteration: question6", "/data/question4[1]/question6", evalCtx);
     }
 
     /**