From e9b7b72c319297f037a353041582749b1f933cef Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?H=C3=A9l=C3=A8ne=20Martin?= <ln@getodk.org>
Date: Mon, 15 Jul 2024 14:50:42 -0700
Subject: [PATCH 1/3] Document current verifyItemsetBindings behavior

---
 .../ExternalSecondaryInstanceParseTest.java   | 57 ++++++++++++++++++-
 .../org/javarosa/xform/parse/mixed-schema.xml |  9 +++
 .../javarosa/xform/parse/mixed-schema2.xml    |  9 +++
 3 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 src/test/resources/org/javarosa/xform/parse/mixed-schema.xml
 create mode 100644 src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml

diff --git a/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java b/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java
index d135c20dd..dedf92145 100644
--- a/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java
+++ b/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java
@@ -6,9 +6,9 @@
 import org.javarosa.core.model.data.helper.Selection;
 import org.javarosa.core.model.instance.AbstractTreeElement;
 import org.javarosa.core.model.instance.TreeReference;
+import org.javarosa.core.util.externalizable.DeserializationException;
 import org.javarosa.test.FormParseInit;
 import org.javarosa.test.Scenario;
-import org.javarosa.core.util.externalizable.DeserializationException;
 import org.javarosa.xpath.expr.XPathPathExpr;
 import org.javarosa.xpath.parser.XPathSyntaxException;
 import org.junit.Test;
@@ -24,6 +24,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.javarosa.core.reference.ReferenceManagerTestUtils.setUpSimpleReferenceManager;
 import static org.javarosa.test.BindBuilderXFormsElement.bind;
+import static org.javarosa.test.ResourcePathHelper.r;
 import static org.javarosa.test.XFormsElement.body;
 import static org.javarosa.test.XFormsElement.head;
 import static org.javarosa.test.XFormsElement.html;
@@ -32,7 +33,6 @@
 import static org.javarosa.test.XFormsElement.select1Dynamic;
 import static org.javarosa.test.XFormsElement.t;
 import static org.javarosa.test.XFormsElement.title;
-import static org.javarosa.test.ResourcePathHelper.r;
 import static org.javarosa.xform.parse.FormParserHelper.deserializeAndCleanUpSerializedForm;
 import static org.javarosa.xform.parse.FormParserHelper.getSerializedFormPath;
 import static org.javarosa.xform.parse.FormParserHelper.parse;
@@ -134,6 +134,59 @@ public void xformParseException_whenItemsetConfiguresValueOrLabelNotInExternalIn
         }
     }
 
+    @Test
+    public void itemsetBindingVerification_doesNotVerifySecondItem() throws IOException, XFormParser.ParseException {
+        configureReferenceManagerCorrectly();
+
+        Scenario.init("Some form", html(
+            head(
+                title("Some form"),
+                model(
+                    mainInstance(t("data id=\"some-form\"",
+                        t("first")
+                    )),
+
+                    t("instance id=\"mixed-schema\" src=\"jr://file/mixed-schema.xml\""),
+
+                    bind("/data/first").type("string")
+                )
+            ),
+            body(
+                // Define a select using value and label references that only exist for the first item
+                select1Dynamic("/data/first", "instance('mixed-schema')/root/item", "name", "label")
+            )));
+    }
+
+    @Test
+    public void itemsetBindingVerification_verifiesFirstItem() throws IOException {
+        configureReferenceManagerCorrectly();
+
+        try {
+            Scenario.init("Some form", html(
+                head(
+                    title("Some form"),
+                    model(
+                        mainInstance(t("data id=\"some-form\"",
+                            t("first")
+                        )),
+
+                        t("instance id=\"mixed-schema\" src=\"jr://file/mixed-schema2.xml\""),
+
+                        bind("/data/first").type("string")
+                    )
+                ),
+                body(
+                    // Define a select using value and label references that only exist for the second item
+                    select1Dynamic("/data/first", "instance('mixed-schema')/root/item", "name", "label")
+                )));
+            fail("Expected XFormParseException because itemset references don't exist in external instance");
+        } catch (XFormParseException e) {
+            // pass
+        } catch (XFormParser.ParseException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
     @Test
     public void csvSecondaryInstanceWithHeaderOnly_parsesWithoutError() throws IOException, XFormParser.ParseException {
         configureReferenceManagerCorrectly();
diff --git a/src/test/resources/org/javarosa/xform/parse/mixed-schema.xml b/src/test/resources/org/javarosa/xform/parse/mixed-schema.xml
new file mode 100644
index 000000000..07f55bee7
--- /dev/null
+++ b/src/test/resources/org/javarosa/xform/parse/mixed-schema.xml
@@ -0,0 +1,9 @@
+<root>
+    <item>
+        <label>A</label>
+        <name>a</name>
+    </item>
+    <item>
+        <foo>bar</foo>
+    </item>
+</root>
diff --git a/src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml b/src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml
new file mode 100644
index 000000000..5dae83d75
--- /dev/null
+++ b/src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml
@@ -0,0 +1,9 @@
+<root>
+    <item>
+        <foo>bar</foo>
+    </item>
+    <item>
+        <label>A</label>
+        <name>a</name>
+    </item>
+</root>

From 27d98465adf3bea9b9c99be1ee5e6254de26cba3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?H=C3=A9l=C3=A8ne=20Martin?= <ln@getodk.org>
Date: Thu, 5 Dec 2024 11:23:28 -0800
Subject: [PATCH 2/3] Verify itemset bindings for internal secondary instances

---
 .../xform/parse/FormInstanceParser.java       |  2 +-
 .../javarosa/core/model/SelectChoiceTest.java | 71 ++++++++++++++++++-
 .../ExternalSecondaryInstanceParseTest.java   | 53 --------------
 .../org/javarosa/xform/parse/mixed-schema.xml |  9 ---
 .../javarosa/xform/parse/mixed-schema2.xml    |  9 ---
 5 files changed, 70 insertions(+), 74 deletions(-)
 delete mode 100644 src/test/resources/org/javarosa/xform/parse/mixed-schema.xml
 delete mode 100644 src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml

diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java
index 2be1bdfef..4c8629cb9 100644
--- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java
+++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java
@@ -348,7 +348,7 @@ private void verifyItemsetBindings(FormInstance instance) {
 
             // Don't try to validate references if the external instance could not be resolved. We allow parsing a form
             // with placeholder external secondary instances for cases where a ReferenceManager can't be configured.
-            if (fi instanceof ExternalDataInstance && !((ExternalDataInstance) fi).isUsingPlaceholder()) {
+            if (!(fi instanceof ExternalDataInstance) || !((ExternalDataInstance) fi).isUsingPlaceholder()) {
                 if (fi.getTemplatePath(itemset.labelRef) == null) {
                     throw new XFormParseException("<label> node for itemset doesn't exist! [" + itemset.labelRef + "]");
                 }
diff --git a/src/test/java/org/javarosa/core/model/SelectChoiceTest.java b/src/test/java/org/javarosa/core/model/SelectChoiceTest.java
index 13fb11801..c272106ef 100644
--- a/src/test/java/org/javarosa/core/model/SelectChoiceTest.java
+++ b/src/test/java/org/javarosa/core/model/SelectChoiceTest.java
@@ -16,16 +16,17 @@
 
 package org.javarosa.core.model;
 
-import kotlin.Pair;
 import org.hamcrest.CoreMatchers;
 import org.javarosa.core.util.externalizable.DeserializationException;
 import org.javarosa.test.Scenario;
+import org.javarosa.xform.parse.XFormParseException;
 import org.javarosa.xform.parse.XFormParser;
 import org.junit.Test;
 
 import java.io.IOException;
 import java.util.List;
 
+import kotlin.Pair;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
@@ -50,6 +51,7 @@
 import static org.javarosa.test.XFormsElement.select1Dynamic;
 import static org.javarosa.test.XFormsElement.t;
 import static org.javarosa.test.XFormsElement.title;
+import static org.junit.Assert.fail;
 
 public class SelectChoiceTest {
     @Test
@@ -113,7 +115,7 @@ public void getChild_returnsEmptyString_whenChoicesAreFromSecondaryInstance_andR
                         t("data id='select-empty'",
                             t("select"))),
                     instance("choices",
-                        t("item", t("label", "Item"), t("property", ""))
+                        t("item", t("label", "Item"), t("name", "item"), t("property", ""))
                     ))),
             body(
                 select1Dynamic("/data/select", "instance('choices')/root/item", "name", "label"))
@@ -274,4 +276,69 @@ public void getAdditionalChildren_returnsEmptyStringValue_forEmptyChildren() thr
             is(contains(new Pair<>("value", "value"), new Pair<>("child", "")))
         );
     }
+
+    @Test
+    public void itemsetBindingVerification_doesNotVerifySecondItem() throws IOException, XFormParser.ParseException {
+        Scenario.init("Some form", html(
+            head(
+                title("Some form"),
+                model(
+                    mainInstance(t("data id=\"some-form\"",
+                        t("first")
+                    )),
+
+                    t("instance id=\"mixed-schema\"",
+                        t("root",
+                            t("item",
+                                t("label", "A"),
+                                t("name", "a")
+                            ),
+                            t("item",
+                                t("foo")))
+                    ),
+
+                    bind("/data/first").type("string")
+                )
+            ),
+            body(
+                // Define a select using value and label references that only exist for the first item
+                select1Dynamic("/data/first", "instance('mixed-schema')/root/item", "name", "label")
+            )));
+    }
+
+    @Test
+    public void itemsetBindingVerification_verifiesFirstItem() throws IOException {
+        try {
+            Scenario.init("Some form", html(
+                head(
+                    title("Some form"),
+                    model(
+                        mainInstance(t("data id=\"some-form\"",
+                            t("first")
+                        )),
+
+                        t("instance id=\"mixed-schema\"",
+                            t("root",
+                                t("item",
+                                    t("foo", "bar")),
+                                t("item",
+                                    t("label", "A"),
+                                    t("value", "a")
+                                ))
+                        ),
+
+                        bind("/data/first").type("string")
+                    )
+                ),
+                body(
+                    // Define a select using value and label references that only exist for the second item
+                    select1Dynamic("/data/first", "instance('mixed-schema')/root/item", "name", "label")
+                )));
+            fail("Expected XFormParseException because itemset references don't exist in external instance");
+        } catch (XFormParseException e) {
+            // pass
+        } catch (XFormParser.ParseException e) {
+            throw new RuntimeException(e);
+        }
+    }
 }
diff --git a/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java b/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java
index dedf92145..f44b3dc49 100644
--- a/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java
+++ b/src/test/java/org/javarosa/xform/parse/ExternalSecondaryInstanceParseTest.java
@@ -134,59 +134,6 @@ public void xformParseException_whenItemsetConfiguresValueOrLabelNotInExternalIn
         }
     }
 
-    @Test
-    public void itemsetBindingVerification_doesNotVerifySecondItem() throws IOException, XFormParser.ParseException {
-        configureReferenceManagerCorrectly();
-
-        Scenario.init("Some form", html(
-            head(
-                title("Some form"),
-                model(
-                    mainInstance(t("data id=\"some-form\"",
-                        t("first")
-                    )),
-
-                    t("instance id=\"mixed-schema\" src=\"jr://file/mixed-schema.xml\""),
-
-                    bind("/data/first").type("string")
-                )
-            ),
-            body(
-                // Define a select using value and label references that only exist for the first item
-                select1Dynamic("/data/first", "instance('mixed-schema')/root/item", "name", "label")
-            )));
-    }
-
-    @Test
-    public void itemsetBindingVerification_verifiesFirstItem() throws IOException {
-        configureReferenceManagerCorrectly();
-
-        try {
-            Scenario.init("Some form", html(
-                head(
-                    title("Some form"),
-                    model(
-                        mainInstance(t("data id=\"some-form\"",
-                            t("first")
-                        )),
-
-                        t("instance id=\"mixed-schema\" src=\"jr://file/mixed-schema2.xml\""),
-
-                        bind("/data/first").type("string")
-                    )
-                ),
-                body(
-                    // Define a select using value and label references that only exist for the second item
-                    select1Dynamic("/data/first", "instance('mixed-schema')/root/item", "name", "label")
-                )));
-            fail("Expected XFormParseException because itemset references don't exist in external instance");
-        } catch (XFormParseException e) {
-            // pass
-        } catch (XFormParser.ParseException e) {
-            throw new RuntimeException(e);
-        }
-    }
-
     @Test
     public void csvSecondaryInstanceWithHeaderOnly_parsesWithoutError() throws IOException, XFormParser.ParseException {
         configureReferenceManagerCorrectly();
diff --git a/src/test/resources/org/javarosa/xform/parse/mixed-schema.xml b/src/test/resources/org/javarosa/xform/parse/mixed-schema.xml
deleted file mode 100644
index 07f55bee7..000000000
--- a/src/test/resources/org/javarosa/xform/parse/mixed-schema.xml
+++ /dev/null
@@ -1,9 +0,0 @@
-<root>
-    <item>
-        <label>A</label>
-        <name>a</name>
-    </item>
-    <item>
-        <foo>bar</foo>
-    </item>
-</root>
diff --git a/src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml b/src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml
deleted file mode 100644
index 5dae83d75..000000000
--- a/src/test/resources/org/javarosa/xform/parse/mixed-schema2.xml
+++ /dev/null
@@ -1,9 +0,0 @@
-<root>
-    <item>
-        <foo>bar</foo>
-    </item>
-    <item>
-        <label>A</label>
-        <name>a</name>
-    </item>
-</root>

From 4ff0588bd426703a3d84eba05cdfb836bde94c8a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?H=C3=A9l=C3=A8ne=20Martin?= <ln@getodk.org>
Date: Wed, 11 Dec 2024 09:35:24 -0800
Subject: [PATCH 3/3] Improve naming

---
 .../javarosa/xform/parse/FormInstanceParser.java   | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java
index 4c8629cb9..53e5e3a0b 100644
--- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java
+++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java
@@ -336,24 +336,24 @@ private void verifyItemsetBindings(FormInstance instance) {
 
             //make sure the labelref is tested against the right instance
             //check if it's not the main instance
-            DataInstance fi = null;
+            DataInstance secondaryInstance;
             if (itemset.labelRef.getInstanceName() != null) {
-                fi = formDef.getNonMainInstance(itemset.labelRef.getInstanceName());
-                if (fi == null) {
+                secondaryInstance = formDef.getNonMainInstance(itemset.labelRef.getInstanceName());
+                if (secondaryInstance == null) {
                     throw new XFormParseException("Instance: " + itemset.labelRef.getInstanceName() + " Does not exists");
                 }
             } else {
-                fi = instance;
+                secondaryInstance = instance;
             }
 
             // Don't try to validate references if the external instance could not be resolved. We allow parsing a form
             // with placeholder external secondary instances for cases where a ReferenceManager can't be configured.
-            if (!(fi instanceof ExternalDataInstance) || !((ExternalDataInstance) fi).isUsingPlaceholder()) {
-                if (fi.getTemplatePath(itemset.labelRef) == null) {
+            if (!(secondaryInstance instanceof ExternalDataInstance) || !((ExternalDataInstance) secondaryInstance).isUsingPlaceholder()) {
+                if (secondaryInstance.getTemplatePath(itemset.labelRef) == null) {
                     throw new XFormParseException("<label> node for itemset doesn't exist! [" + itemset.labelRef + "]");
                 }
                 //check value nodes exist
-                else if (itemset.valueRef != null && fi.getTemplatePath(itemset.valueRef) == null) {
+                else if (itemset.valueRef != null && secondaryInstance.getTemplatePath(itemset.valueRef) == null) {
                     throw new XFormParseException("<value> node for itemset doesn't exist! [" + itemset.valueRef + "]");
                 }
             }