From 4518353b8daeb81e0a3f3178e81186ab3383b236 Mon Sep 17 00:00:00 2001
From: Martijn van de Rijdt <martijn@enketo.org>
Date: Wed, 6 Dec 2017 16:46:35 -0700
Subject: [PATCH 01/11] a start

---
 pyxform/builder.py                  |  4 ++++
 pyxform/question_type_dictionary.py |  3 ++-
 pyxform/survey.py                   | 10 ++++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/pyxform/builder.py b/pyxform/builder.py
index 105cd58c7..1b92da6eb 100644
--- a/pyxform/builder.py
+++ b/pyxform/builder.py
@@ -92,6 +92,10 @@ def create_survey_element_from_dict(self, d):
             d = self._sections[section_name]
             full_survey = self.create_survey_element_from_dict(d)
             return full_survey.children
+        elif d[u"type"] == u"data-xml":
+            # TODO: Build up an array of names (preventing duplicates)
+            # and make that accessible in survey.py
+            print 'found data-xml item with name: '+d[u"name"] 
         else:
             return self._create_question_from_dict(
                 d, copy_json_dict(QUESTION_TYPE_DICT), self._add_none_option)
diff --git a/pyxform/question_type_dictionary.py b/pyxform/question_type_dictionary.py
index 0244acca5..54acae76c 100644
--- a/pyxform/question_type_dictionary.py
+++ b/pyxform/question_type_dictionary.py
@@ -856,5 +856,6 @@ def generate_new_dict():
             "bind": {
                 "type": "binary"
             }
-        }
+        },
+        "data-xml": {}
     }
diff --git a/pyxform/survey.py b/pyxform/survey.py
index fc21daeed..46c6fc96c 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -61,6 +61,8 @@ class Survey(Section):
     def validate(self):
         if self.id_string in [None, 'None']:
             raise PyXFormError('Survey cannot have an empty id_string')
+        # TODO: don't raise error if data-xml has no label
+        # TODO: check for duplicate names of data-xml items
         super(Survey, self).validate()
         self._validate_uniqueness_of_section_names()
 
@@ -138,6 +140,13 @@ def _generate_static_instances(self):
             yield node("instance", node("root", *instance_element_list),
                        id=list_name)
 
+    def _generate_remote_instances(self):
+        # TODO: loop through all data-xml items:
+        # TODO: prevent duplicates, see _generate_pulldata_instances, though
+        # it would be more robust to check existing instances in the model so far.
+        name = 'test1'
+        yield node("instance", id=name, src="jr://file/{}.xml".format(name))
+
     def _generate_pulldata_instances(self):
         pulldata = []
         for i in self.iter_descendants():
@@ -184,6 +193,7 @@ def xml_model(self):
             model_children.append(self.itext())
         model_children += [node("instance", self.xml_instance())]
         model_children += list(self._generate_static_instances())
+        model_children += list(self._generate_remote_instances())
         model_children += list(self._generate_pulldata_instances())
         model_children += list(self._generate_from_file_instances())
         model_children += self.xml_bindings()

From e655b214599c57b1323528f9859eba3e92111613 Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Mon, 18 Dec 2017 21:19:40 +1100
Subject: [PATCH 02/11] fix: bug that crashes pydev debugger due to max
 recursion

- was: __repr__ -> unicode -> __repr__ -> unicode -> __repr__ -> ... :(
- now: __repr__ -> __unicode__ -> :)
---
 pyxform/survey.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pyxform/survey.py b/pyxform/survey.py
index 46c6fc96c..fe87f9729 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -450,7 +450,7 @@ def _to_pretty_xml(self):
         return '<?xml version="1.0"?>\n' + inline_output
 
     def __repr__(self):
-        return unicode(self)
+        return self.__unicode__()
 
     def __unicode__(self):
         return "<pyxform.survey.Survey instance at %s>" % hex(id(self))

From d3b011487cc7f542e1f33319bc3a066eb4bf14b7 Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Tue, 19 Dec 2017 12:30:11 +1100
Subject: [PATCH 03/11] chg: use NotImplementedError rather than Exception for
 same purpose

---
 pyxform/survey_element.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pyxform/survey_element.py b/pyxform/survey_element.py
index b1b5d0997..c1813acf8 100644
--- a/pyxform/survey_element.py
+++ b/pyxform/survey_element.py
@@ -358,7 +358,7 @@ def xml_control(self):
         The control depends on what type of question we're asking, it
         doesn't make sense to implement here in the base class.
         """
-        raise Exception("Control not implemented")
+        raise NotImplementedError("Control not implemented")
 
 
 def hashable(v):

From a216da19a604437d71a067ebae9bca59fa0cac1c Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Tue, 19 Dec 2017 14:19:28 +1100
Subject: [PATCH 04/11] add: show the error messages when a test fails

---
 pyxform/tests_v1/pyxform_test_case.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pyxform/tests_v1/pyxform_test_case.py b/pyxform/tests_v1/pyxform_test_case.py
index aba54e28d..498fa9d1a 100644
--- a/pyxform/tests_v1/pyxform_test_case.py
+++ b/pyxform/tests_v1/pyxform_test_case.py
@@ -240,7 +240,9 @@ def check_content(content):
                 "Expected valid survey but compilation failed. "
                 "Try correcting the error with 'debug=True', "
                 "setting 'errored=True', "
-                "and or optionally 'error__contains=[...]'")
+                "and or optionally 'error__contains=[...]'"
+                "\nError(s): " + "\n".join(errors)
+            )
         elif survey and expecting_invalid_survey:
             raise PyxformTestError("Expected survey to be invalid.")
 

From 83d90610d61ea2836807cb53f460b4131ca1600f Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Tue, 19 Dec 2017 17:21:50 +1100
Subject: [PATCH 05/11] add: xml-data instance from survey item only with no
 form / model effect

- As mentioned in https://github.com/XLSForm/pyxform/issues/107
- Similar to select_one_from_file or pulldata, except instance only
- Included some tidy-up of the common instance generating functions
- Lots of tests
---
 pyxform/builder.py                  |   7 +-
 pyxform/external_instance.py        |  22 +++
 pyxform/instance_info.py            |  11 ++
 pyxform/question_type_dictionary.py |   4 +-
 pyxform/section.py                  |   3 +
 pyxform/survey.py                   | 202 +++++++++++++++++++---------
 pyxform/tests_v1/test_xmldata.py    | 191 ++++++++++++++++++++++++++
 7 files changed, 375 insertions(+), 65 deletions(-)
 create mode 100644 pyxform/external_instance.py
 create mode 100644 pyxform/instance_info.py
 create mode 100644 pyxform/tests_v1/test_xmldata.py

diff --git a/pyxform/builder.py b/pyxform/builder.py
index 1b92da6eb..6cb05bc53 100644
--- a/pyxform/builder.py
+++ b/pyxform/builder.py
@@ -3,6 +3,7 @@
 
 from pyxform import file_utils, utils
 from pyxform.errors import PyXFormError
+from pyxform.external_instance import ExternalInstance
 from pyxform.question import (InputQuestion, MultipleChoiceQuestion,
                               OsmUploadQuestion, Question, RangeQuestion,
                               TriggerQuestion, UploadQuestion)
@@ -92,10 +93,8 @@ def create_survey_element_from_dict(self, d):
             d = self._sections[section_name]
             full_survey = self.create_survey_element_from_dict(d)
             return full_survey.children
-        elif d[u"type"] == u"data-xml":
-            # TODO: Build up an array of names (preventing duplicates)
-            # and make that accessible in survey.py
-            print 'found data-xml item with name: '+d[u"name"] 
+        elif d[u"type"] == u"xml-data":
+            return ExternalInstance(**d)
         else:
             return self._create_question_from_dict(
                 d, copy_json_dict(QUESTION_TYPE_DICT), self._add_none_option)
diff --git a/pyxform/external_instance.py b/pyxform/external_instance.py
new file mode 100644
index 000000000..0f411c4e0
--- /dev/null
+++ b/pyxform/external_instance.py
@@ -0,0 +1,22 @@
+from pyxform.survey_element import SurveyElement
+from pyxform.utils import node
+
+
+class ExternalInstance(SurveyElement):
+
+    def xml_instance(self):
+        """
+        Get the XML representation of the external instance element.
+        """
+        return node(
+            "instance",
+            id=self[u"name"],
+            src="jr://file/{}.xml".format(self[u"name"]))
+
+    def xml_control(self):
+        """
+        No-op since there is no associated form control to place under <body/>.
+
+        Exists here because there's a soft abstractmethod in SurveyElement.
+        """
+        pass
diff --git a/pyxform/instance_info.py b/pyxform/instance_info.py
new file mode 100644
index 000000000..ac2b0a079
--- /dev/null
+++ b/pyxform/instance_info.py
@@ -0,0 +1,11 @@
+
+
+class InstanceInfo(object):
+    """Standardise Instance details relevant during XML generation."""
+
+    def __init__(self, type, context, name, unique_id, instance):
+        self.type = type
+        self.context = context
+        self.name = name
+        self.unique_id = unique_id
+        self.instance = instance
diff --git a/pyxform/question_type_dictionary.py b/pyxform/question_type_dictionary.py
index 54acae76c..4f13b9546 100644
--- a/pyxform/question_type_dictionary.py
+++ b/pyxform/question_type_dictionary.py
@@ -857,5 +857,7 @@ def generate_new_dict():
                 "type": "binary"
             }
         },
-        "data-xml": {}
+        "xml-data": {
+            # Only effect is to add an external instance.
+        }
     }
diff --git a/pyxform/section.py b/pyxform/section.py
index 4d8410f50..2be0b7a1d 100644
--- a/pyxform/section.py
+++ b/pyxform/section.py
@@ -1,3 +1,4 @@
+from pyxform.external_instance import ExternalInstance
 from pyxform.question import SurveyElement
 from pyxform.utils import node
 from pyxform.errors import PyXFormError
@@ -38,6 +39,8 @@ def xml_instance(self, **kwargs):
             if child.get(u"flat"):
                 for grandchild in child.xml_instance_array():
                     result.appendChild(grandchild)
+            elif isinstance(child, ExternalInstance):
+                continue
             else:
                 result.appendChild(child.xml_instance())
         return result
diff --git a/pyxform/survey.py b/pyxform/survey.py
index fe87f9729..a1906303e 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -7,8 +7,11 @@
 from datetime import datetime
 
 from pyxform import constants
+from pyxform.external_instance import ExternalInstance
 from pyxform.errors import PyXFormError
+from pyxform.errors import ValidationError
 from pyxform.instance import SurveyInstance
+from pyxform.instance_info import InstanceInfo
 from pyxform.odk_validate import check_xform
 from pyxform.question import Question
 from pyxform.section import Section
@@ -61,8 +64,6 @@ class Survey(Section):
     def validate(self):
         if self.id_string in [None, 'None']:
             raise PyXFormError('Survey cannot have an empty id_string')
-        # TODO: don't raise error if data-xml has no label
-        # TODO: check for duplicate names of data-xml items
         super(Survey, self).validate()
         self._validate_uniqueness_of_section_names()
 
@@ -114,71 +115,155 @@ def xml(self):
                     **nsmap
                     )
 
-    def _generate_static_instances(self):
+    def _generate_static_instances(self, list_name, choice_list):
         """
         Generates <instance> elements for static data
         (e.g. choices for select type questions)
         """
-        for list_name, choice_list in self.choices.items():
-            instance_element_list = []
-            for idx, choice in zip(range(len(choice_list)), choice_list):
-                choice_element_list = []
-                # Add a unique id to the choice element incase there is itext
-                # it refrences
-                itext_id = '-'.join(['static_instance', list_name, str(idx)])
-                choice_element_list.append(node("itextId", itext_id))
+        instance_element_list = []
+        for idx, choice in enumerate(choice_list):
+            choice_element_list = []
+            # Add a unique id to the choice element in case there is itext
+            # it references
+            itext_id = '-'.join(['static_instance', list_name, str(idx)])
+            choice_element_list.append(node("itextId", itext_id))
+
+            for choicePropertyName, choicePropertyValue in choice.items():
+                if isinstance(choicePropertyValue, basestring) \
+                        and choicePropertyName != 'label':
+                    choice_element_list.append(
+                        node(choicePropertyName,
+                             unicode(choicePropertyValue))
+                    )
+            instance_element_list.append(node("item", *choice_element_list))
+        return InstanceInfo(
+            type=u"choice",
+            context=u"survey",
+            name=list_name,
+            unique_id=list_name,
+            instance=node(
+                "instance",
+                node("root", *instance_element_list),
+                id=list_name
+            )
+        )
+
+    def _generate_external_instances(self, element):
+        if isinstance(element, ExternalInstance):
+            return InstanceInfo(
+                type=u"external",
+                context="[type: {t}, name: {n}]".format(
+                    t=element[u"parent"][u"type"],
+                    n=element[u"parent"][u"name"]
+                ),
+                name=element[u"name"],
+                unique_id=(element[u"name"],),
+                instance=element.xml_instance()
+            )
+
+    def _validate_external_instances(self, instances):
+        """
+        Must have unique names.
 
-                for choicePropertyName, choicePropertyValue in choice.items():
-                    if isinstance(choicePropertyValue, basestring) \
-                            and choicePropertyName != 'label':
-                        choice_element_list.append(
-                            node(choicePropertyName,
-                                 unicode(choicePropertyValue))
+        - Duplications could come from across groups; this checks the form.
+        - Errors are pooled together into a (hopefully) helpful message.
+        """
+        seen = {}
+        for i in instances:
+            element = i.name
+            if seen.get(element) is None:
+                seen[element] = [i]
+            else:
+                seen[element].append(i)
+        errors = []
+        for element, copies in seen.items():
+            if 1 < len(copies):
+                contexts = ", ".join(x.context for x in copies)
+                errors.append(
+                    "Instance names must be unique within a form. "
+                    "The name '{i}' was found {c} time(s), "
+                    "under these contexts: {contexts}".format(
+                        i=element, c=len(copies), contexts=contexts))
+        if 0 < len(errors):
+            raise ValidationError("\n".join(errors))
+
+    def _generate_pulldata_instances(self, element):
+        if 'calculate' in element['bind']:
+            calculate = element['bind']['calculate']
+            if calculate.startswith('pulldata('):
+                pieces = calculate.split('"') \
+                    if '"' in calculate else calculate.split("'")
+                if len(pieces) > 1 and pieces[1]:
+                    file_id = pieces[1]
+                    uri = "jr://file-csv/{}.csv".format(file_id)
+                    return InstanceInfo(
+                        type=u"pulldata",
+                        context="[type: {t}, name: {n}]".format(
+                            t=element[u"parent"][u"type"],
+                            n=element[u"parent"][u"name"]
+                        ),
+                        name=file_id,
+                        unique_id=(file_id, uri),
+                        instance=node(
+                            "instance",
+                            id=file_id,
+                            src=uri
                         )
-                instance_element_list.append(node("item",
-                                                  *choice_element_list))
-            yield node("instance", node("root", *instance_element_list),
-                       id=list_name)
-
-    def _generate_remote_instances(self):
-        # TODO: loop through all data-xml items:
-        # TODO: prevent duplicates, see _generate_pulldata_instances, though
-        # it would be more robust to check existing instances in the model so far.
-        name = 'test1'
-        yield node("instance", id=name, src="jr://file/{}.xml".format(name))
-
-    def _generate_pulldata_instances(self):
-        pulldata = []
-        for i in self.iter_descendants():
-            if 'calculate' in i['bind']:
-                calculate = i['bind']['calculate']
-                if calculate.startswith('pulldata('):
-                    pieces = calculate.split('"') \
-                        if '"' in calculate else calculate.split("'")
-                    if len(pieces) > 1 and pieces[1] not in pulldata:
-                        csv_id = pieces[1]
-                        pulldata.append(csv_id)
-
-                        yield node("instance", id=csv_id,
-                                   src="jr://file-csv/{}.csv".format(csv_id)
-                                   )
-
-    def _generate_from_file_instances(self):
-        for i in self.iter_descendants():
-            itemset = i.get('itemset')
-            if itemset and \
-                    (itemset.endswith('.csv') or itemset.endswith('.xml')):
-                file_id, ext = os.path.splitext(itemset)
-                uri = 'jr://%s/%s' % (
-                    'file' if ext == '.xml' else "file-%s" % ext[1:],
-                    itemset)
-                yield node(
+                    )
+
+    def _generate_from_file_instances(self, element):
+        itemset = element.get('itemset')
+        if itemset and (itemset.endswith('.csv') or itemset.endswith('.xml')):
+            file_id, ext = os.path.splitext(itemset)
+            uri = 'jr://%s/%s' % (
+                'file' if ext == '.xml' else "file-%s" % ext[1:], itemset)
+            return InstanceInfo(
+                type=u"file",
+                context="[type: {t}, name: {n}]".format(
+                    t=element[u"parent"][u"type"],
+                    n=element[u"parent"][u"name"]
+                ),
+                name=file_id,
+                unique_id=(file_id, uri),
+                instance=node(
                     "instance",
                     node("root",
-                         node("item", node("name", "_"), node("label", "_"))),
+                         node("item",
+                              node("name", "_"),
+                              node("label", "_"))),
                     id=file_id,
                     src=uri
                 )
+            )
+
+    def _generate_instances(self):
+        """
+        Get instances from all the different ways that they may be generated.
+
+        An opportunity to validate instances before output to the XML model.
+        """
+        instances = []
+        for k, v in self.choices.items():
+            instances += [
+                self._generate_static_instances(list_name=k, choice_list=v)]
+        for i in self.iter_descendants():
+            ext = self._generate_external_instances(element=i)
+            pull = self._generate_pulldata_instances(element=i)
+            file = self._generate_from_file_instances(element=i)
+            instances += [x for x in [ext, pull, file] if x is not None]
+
+        # Check that external instances have unique names.
+        if 0 < len(instances):
+            ext_only = [x for x in instances if x.type == "external"]
+            self._validate_external_instances(instances=ext_only)
+
+        # Only output the exact same instance once.
+        # Identification varies by instance generation type: id +/- src.
+        seen = []
+        for i in instances:
+            if i.unique_id not in seen:
+                yield i.instance
+            seen.append(i.unique_id)
 
     def xml_model(self):
         """
@@ -192,10 +277,7 @@ def xml_model(self):
         if self._translations:
             model_children.append(self.itext())
         model_children += [node("instance", self.xml_instance())]
-        model_children += list(self._generate_static_instances())
-        model_children += list(self._generate_remote_instances())
-        model_children += list(self._generate_pulldata_instances())
-        model_children += list(self._generate_from_file_instances())
+        model_children += list(self._generate_instances())
         model_children += self.xml_bindings()
 
         if self.submission_url or self.public_key:
diff --git a/pyxform/tests_v1/test_xmldata.py b/pyxform/tests_v1/test_xmldata.py
new file mode 100644
index 000000000..b5adceec5
--- /dev/null
+++ b/pyxform/tests_v1/test_xmldata.py
@@ -0,0 +1,191 @@
+from pyxform.tests_v1.pyxform_test_case import PyxformTestCase
+from pyxform.tests_v1.pyxform_test_case import PyxformTestError
+try:
+    from unittest import skip
+except ImportError:
+    from unittest2 import skip
+
+
+class ExternalInstanceTests(PyxformTestCase):
+
+    def test_external_single_ok(self):
+        """Simplest possible example to include an external instance."""
+        self.assertPyxformXform(
+            md="""
+            | survey |             |           |                  |
+            |        | type        | name      | label            |
+            |        | xml-data    | mydata    |                  |
+            """,
+            model__contains=[
+                '<instance id="mydata" src="jr://file/mydata.xml"/>'
+            ]
+        )
+
+    def test_external_two_in_section_duplicate_raises(self):
+        """Duplicate external instances in the same section raises an error."""
+        with self.assertRaises(PyxformTestError) as ctx:
+            self.assertPyxformXform(
+                md="""
+                | survey |             |           |                  |
+                |        | type        | name      | label            |
+                |        | xml-data    | mydata    |                  |
+                |        | xml-data    | mydata    |                  |
+                """,
+                model__contains=[])
+        # This is caught first by existing validation rule.
+        self.assertIn("There are two survey elements named 'mydata'",
+                      repr(ctx.exception))
+
+    def test_external_two_in_section_unique_ok(self):
+        """Two unique external instances in the same section is OK."""
+        self.assertPyxformXform(
+            md="""
+            | survey |             |           |                  |
+            |        | type        | name      | label            |
+            |        | xml-data    | mydata    |                  |
+            |        | xml-data    | mydata2   |                  |
+            """,
+            model__contains=[
+                '<instance id="mydata" src="jr://file/mydata.xml"/>',
+                '<instance id="mydata2" src="jr://file/mydata2.xml"/>'
+            ]
+        )
+
+    def test_external_multi_group_duplicate_raises(self):
+        """Duplicate external instances anywhere raises an error."""
+        with self.assertRaises(PyxformTestError) as ctx:
+            self.assertPyxformXform(
+                md="""
+                | survey |             |         |                  |
+                |        | type        | name    | label            |
+                |        | xml-data    | mydata  |                  |
+                |        | begin group | g1      |                  |
+                |        | xml-data    | mydata  |                  |
+                |        | end group   | g1      |                  |
+                |        | begin group | g2      |                  |
+                |        | xml-data    | mydata  |                  |
+                |        | end group   | g2      |                  |
+                """,
+                model__contains=[])
+        self.assertIn("Instance names must be unique", repr(ctx.exception))
+
+    def test_external_multi_group_duplicate_raises_helpfully(self):
+        """Duplicate external instances anywhere raises a helpful error."""
+        with self.assertRaises(PyxformTestError) as ctx:
+            self.assertPyxformXform(
+                md="""
+                | survey |             |         |                  |
+                |        | type        | name    | label            |
+                |        | xml-data    | mydata  |                  |
+                |        | begin group | g1      |                  |
+                |        | xml-data    | mydata  |                  |
+                |        | end group   | g1      |                  |
+                |        | begin group | g2      |                  |
+                |        | xml-data    | mydata2 |                  |
+                |        | end group   | g2      |                  |
+                |        | begin group | g3      |                  |
+                |        | xml-data    | mydata3 |                  |
+                |        | end group   | g3      |                  |
+                """,
+                model__contains=[])
+        self.assertIn("The name 'mydata' was found 2 time(s)",
+                      repr(ctx.exception))
+
+    def test_external_multi_group_unique_ok(self):
+        """Unique external instances anywhere is OK."""
+        self.assertPyxformXform(
+            md="""
+            | survey |             |         |                  |
+            |        | type        | name    | label            |
+            |        | xml-data    | mydata  |                  |
+            |        | begin group | g1      |                  |
+            |        | xml-data    | mydata1 |                  |
+            |        | end group   | g1      |                  |
+            |        | begin group | g2      |                  |
+            |        | xml-data    | mydata2 |                  |
+            |        | end group   | g2      |                  |
+            |        | begin group | g3      |                  |
+            |        | xml-data    | mydata3 |                  |
+            |        | end group   | g3      |                  |
+            """,
+            model__contains=[
+                '<instance id="mydata" src="jr://file/mydata.xml"/>',
+                '<instance id="mydata1" src="jr://file/mydata1.xml"/>',
+                '<instance id="mydata2" src="jr://file/mydata2.xml"/>',
+                '<instance id="mydata3" src="jr://file/mydata3.xml"/>'
+            ]
+        )
+
+    def test_instance_names_other_sources_duplicate_raises(self):
+        """Duplicate instances with other sources present raises an error."""
+        with self.assertRaises(PyxformTestError) as ctx:
+            self.assertPyxformXform(
+                md="""
+                | survey |                                      |      |       |
+                |        | type                                 | name | label | calculation  |
+                |        | begin group                          | g1   |       |              |
+                |        | xml-data                             | city |       |              |
+                |        | end group                            | g1   |       |              |
+                |        | xml-data                             | city |       |              |
+                |        | begin group                          | g2   |       |              |
+                |        | select_one_from_file cities.csv      | city | City  |              |
+                |        | end group                            | g2   |       |              |
+                |        | begin group                          | g3   |       |              |
+                |        | select_multiple_from_file cities.csv | city | City  |              |
+                |        | end group                            | g3   |       |              |
+                |        | begin group                          | g4   |       |              |
+                |        | calculate                            | city | City  | pulldata('fruits', 'name', 'name', 'mango') |
+                |        | end group                            | g4   |       |              |
+                """,
+                model__contains=[])
+        self.assertIn("The name 'city' was found 2 time(s)",
+                      repr(ctx.exception))
+
+    def test_instance_names_other_sources_unique_ok(self):
+        """Unique instances with other sources present are OK."""
+        self.assertPyxformXform(
+            md="""
+            | survey |                                      |       |       |
+            |        | type                                 | name  | label | calculation  |
+            |        | begin group                          | g1    |       |              |
+            |        | xml-data                             | city1 |       |              |
+            |        | end group                            | g1    |       |              |
+            |        | begin group                          | g2    |       |              |
+            |        | select_one_from_file cities.csv      | city2 | City2 |              |
+            |        | end group                            | g2    |       |              |
+            |        | begin group                          | g3    |       |              |
+            |        | select_multiple_from_file cities.csv | city3 | City3 |              |
+            |        | end group                            | g3    |       |              |
+            |        | begin group                          | g4    |       |              |
+            |        | calculate                            | city4 | City4 | pulldata('fruits', 'name', 'name', 'mango') |
+            |        | end group                            | g4    |       |              |
+            """,
+            model__contains=[
+                '<instance id="city1" src="jr://file/city1.xml"/>',
+"""
+      <instance id="cities" src="jr://file-csv/cities.csv">
+        <root>
+          <item>
+            <name>_</name>
+            <label>_</label>
+          </item>
+        </root>
+      </instance>
+""",
+                '<instance id="fruits" src="jr://file-csv/fruits.csv"/>'
+            ]
+        )
+
+    @skip("Usage scenarios TBA but it might look something like this.")
+    def test_external_usage_scenario(self):
+        self.assertPyxformXform(
+            md="""
+            | survey |             |           |                  | 
+            |        | type        | name      | label            | bind::type  |  calculation  
+            |        | calculate   | external1 |                  | External    |  oc-item(event1(1), form2, item3(1))
+            |        | note        | ext_note  | The external value is ${external_1} |     |
+            """,
+            model__contains=[
+                # TODO: not sure what
+                ])
+

From a4a57fbe0be669cd90f098aabcf9d22ac76cb57f Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Thu, 21 Dec 2017 12:49:31 +1100
Subject: [PATCH 06/11] chg: misc code quality fix-ups

- Mark staticmethods as such
- Avoid module level variables conflicting with lower scope names
- Move namespace map constant dict into utils; associated with this was
  a minor refactor in pyxform_test_case which was relying on this module
  level variable having been mutated already at some point. Could go
  further and change to tuples but this avoids a regression as-is.
- Replace double-space in regex with space tokens
---
 pyxform/survey.py                     | 43 ++++++++++++++-------------
 pyxform/tests_v1/pyxform_test_case.py |  6 ++--
 pyxform/utils.py                      | 10 ++++++-
 pyxform/xform2json.py                 |  5 ++--
 4 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/pyxform/survey.py b/pyxform/survey.py
index a1906303e..624221f6e 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -16,20 +16,16 @@
 from pyxform.question import Question
 from pyxform.section import Section
 from pyxform.survey_element import SurveyElement
-from pyxform.utils import PatchedText, basestring, node, unicode
+from pyxform.utils import PatchedText, basestring, node, unicode, NSMAP
 
-nsmap = {
-    u"xmlns": u"http://www.w3.org/2002/xforms",
-    u"xmlns:h": u"http://www.w3.org/1999/xhtml",
-    u"xmlns:ev": u"http://www.w3.org/2001/xml-events",
-    u"xmlns:xsd": u"http://www.w3.org/2001/XMLSchema",
-    u"xmlns:jr": u"http://openrosa.org/javarosa",
-    u"xmlns:orx": u"http://openrosa.org/xforms"
-    }
 
-for prefix, uri in nsmap.items():
-    prefix = prefix.replace("xmlns", "").replace(":", "")
-    ETree.register_namespace(prefix, uri)
+def register_nsmap():
+    for prefix, uri in NSMAP.items():
+        prefix_no_xmlns = prefix.replace("xmlns", "").replace(":", "")
+        ETree.register_namespace(prefix_no_xmlns, uri)
+
+
+register_nsmap()
 
 
 class Survey(Section):
@@ -86,12 +82,14 @@ def get_nsmap(self):
                 if len(ns.split('=')) == 2 and ns.split('=')[0] != ''
             ]
             xmlns = u'xmlns:'
+            nsmap = NSMAP.copy()
             nsmap.update(dict([
                 (xmlns + k, v.replace('"', '').replace("'", ""))
                 for k, v in nslist if xmlns + k not in nsmap
             ]))
-
-        return nsmap
+            return nsmap
+        else:
+            return NSMAP
 
     def xml(self):
         """
@@ -115,7 +113,8 @@ def xml(self):
                     **nsmap
                     )
 
-    def _generate_static_instances(self, list_name, choice_list):
+    @staticmethod
+    def _generate_static_instances(list_name, choice_list):
         """
         Generates <instance> elements for static data
         (e.g. choices for select type questions)
@@ -148,7 +147,8 @@ def _generate_static_instances(self, list_name, choice_list):
             )
         )
 
-    def _generate_external_instances(self, element):
+    @staticmethod
+    def _generate_external_instances(element):
         if isinstance(element, ExternalInstance):
             return InstanceInfo(
                 type=u"external",
@@ -161,7 +161,8 @@ def _generate_external_instances(self, element):
                 instance=element.xml_instance()
             )
 
-    def _validate_external_instances(self, instances):
+    @staticmethod
+    def _validate_external_instances(instances):
         """
         Must have unique names.
 
@@ -187,7 +188,8 @@ def _validate_external_instances(self, instances):
         if 0 < len(errors):
             raise ValidationError("\n".join(errors))
 
-    def _generate_pulldata_instances(self, element):
+    @staticmethod
+    def _generate_pulldata_instances(element):
         if 'calculate' in element['bind']:
             calculate = element['bind']['calculate']
             if calculate.startswith('pulldata('):
@@ -211,7 +213,8 @@ def _generate_pulldata_instances(self, element):
                         )
                     )
 
-    def _generate_from_file_instances(self, element):
+    @staticmethod
+    def _generate_from_file_instances(element):
         itemset = element.get('itemset')
         if itemset and (itemset.endswith('.csv') or itemset.endswith('.xml')):
             file_id, ext = os.path.splitext(itemset)
@@ -524,7 +527,7 @@ def _to_pretty_xml(self):
         # http://ronrothman.com/public/leftbraned/xml-dom-minidom-toprettyxml-and-silly-whitespace/
         xml_with_linebreaks = self.xml().toprettyxml(indent='  ')
         text_re = re.compile('(>)\n\s*(\s[^<>\s].*?)\n\s*(\s</)', re.DOTALL)
-        output_re = re.compile('\n.*(<output.*>)\n(  )*')
+        output_re = re.compile('\n.*(<output.*>)\n(\s\s)*')
         pretty_xml = text_re.sub(lambda m: ''.join(m.group(1, 2, 3)), xml_with_linebreaks)
         inline_output = output_re.sub('\g<1>', pretty_xml)
         inline_output = re.compile('<label>\s*\n*\s*\n*\s*</label>')\
diff --git a/pyxform/tests_v1/pyxform_test_case.py b/pyxform/tests_v1/pyxform_test_case.py
index 498fa9d1a..8004df98f 100644
--- a/pyxform/tests_v1/pyxform_test_case.py
+++ b/pyxform/tests_v1/pyxform_test_case.py
@@ -12,8 +12,8 @@
 from pyxform.builder import create_survey_element_from_dict
 from pyxform.errors import PyXFormError
 from pyxform.odk_validate import ODKValidateError, check_xform
-from pyxform.survey import nsmap
 from pyxform.tests_v1.test_utils.md_table import md_table_to_ss_structure
+from pyxform.utils import NSMAP
 from pyxform.xls2json import workbook_to_json
 
 
@@ -152,7 +152,9 @@ def assertPyxformXform(self, **kwargs):
             root = ETree.fromstring(xml.encode('utf-8'))
 
             # Ensure all namespaces are present, even if unused
-            root.attrib.update(**nsmap)
+            final_nsmap = NSMAP.copy()
+            final_nsmap.update(survey.get_nsmap())
+            root.attrib.update(final_nsmap)
 
             xml_nodes['xml'] = root
 
diff --git a/pyxform/utils.py b/pyxform/utils.py
index b993690b4..a21d1138e 100644
--- a/pyxform/utils.py
+++ b/pyxform/utils.py
@@ -25,7 +25,15 @@
 XFORM_TAG_REGEXP = "%(start)s%(char)s*" % {
     "start": TAG_START_CHAR,
     "char": TAG_CHAR
-    }
+}
+NSMAP = {
+    u"xmlns": u"http://www.w3.org/2002/xforms",
+    u"xmlns:h": u"http://www.w3.org/1999/xhtml",
+    u"xmlns:ev": u"http://www.w3.org/2001/xml-events",
+    u"xmlns:xsd": u"http://www.w3.org/2001/XMLSchema",
+    u"xmlns:jr": u"http://openrosa.org/javarosa",
+    u"xmlns:orx": u"http://openrosa.org/xforms"
+}
 
 
 class DetachableElement(Element):
diff --git a/pyxform/xform2json.py b/pyxform/xform2json.py
index 9853facc8..874142721 100644
--- a/pyxform/xform2json.py
+++ b/pyxform/xform2json.py
@@ -6,8 +6,7 @@
 import xml.etree.ElementTree as ETree
 from operator import itemgetter
 from pyxform import builder
-from pyxform.utils import basestring
-from pyxform.survey import nsmap
+from pyxform.utils import basestring, NSMAP
 
 
 # {{{ http://code.activestate.com/recipes/573463/ (r7)
@@ -181,7 +180,7 @@ def __init__(self, root):
 
     def get_dict(self):
         json_str = json.dumps(self._dict)
-        for uri in nsmap.values():
+        for uri in NSMAP.values():
             json_str = json_str.replace('{%s}' % uri, '')
         return json.loads(json_str)
 

From 505d95d5803b7154ca58d3f4355583d452b9c773 Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Thu, 21 Dec 2017 18:13:40 +1100
Subject: [PATCH 07/11] chg: more misc code quality fix-ups

- Replace call to str() with py2/3 compatible util.unicode method
- Rename camelCase method _autonameInputs to snake_case
- Remove unnecessary escapes for regex characters # and |
- Mark staticmethods as such
- Move ODK validate runner section to new method to simplify test method
- Replace e.message with unicode(e) since the former doesn't work in py3
- Replace "text" iteration variable that shadows outer scope variable
---
 pyxform/tests_v1/pyxform_test_case.py | 73 ++++++++++++++-------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/pyxform/tests_v1/pyxform_test_case.py b/pyxform/tests_v1/pyxform_test_case.py
index 8004df98f..dd346d996 100644
--- a/pyxform/tests_v1/pyxform_test_case.py
+++ b/pyxform/tests_v1/pyxform_test_case.py
@@ -13,7 +13,7 @@
 from pyxform.errors import PyXFormError
 from pyxform.odk_validate import ODKValidateError, check_xform
 from pyxform.tests_v1.test_utils.md_table import md_table_to_ss_structure
-from pyxform.utils import NSMAP
+from pyxform.utils import NSMAP, unicode
 from pyxform.xls2json import workbook_to_json
 
 
@@ -28,17 +28,17 @@ def md_to_pyxform_survey(self, md_raw, kwargs=None, autoname=True):
         if kwargs is None:
             kwargs = {}
         if autoname:
-            kwargs = self._autonameInputs(kwargs)
+            kwargs = self._autoname_inputs(kwargs)
         _md = []
         for line in md_raw.split('\n'):
-            if re.match(r'^\s+\#', line):
+            if re.match(r'^\s+#', line):
                 # ignore lines which start with pound sign
                 continue
-            elif re.match(r'^(.*)(\#[^\|]+)$', line):
+            elif re.match(r'^(.*)(#[^|]+)$', line):
                 # keep everything before the # outside of the last occurrence
                 # of |
                 _md.append(
-                    re.match(r'^(.*)(\#[^\|]+)$', line).groups()[0].strip())
+                    re.match(r'^(.*)(#[^|]+)$', line).groups()[0].strip())
             else:
                 _md.append(line.strip())
         md = '\n'.join(_md)
@@ -65,7 +65,8 @@ def _row_to_dict(row):
 
         return self._ss_structure_to_pyxform_survey(sheets, kwargs)
 
-    def _ss_structure_to_pyxform_survey(self, ss_structure, kwargs):
+    @staticmethod
+    def _ss_structure_to_pyxform_survey(ss_structure, kwargs):
         # using existing methods from the builder
         imported_survey_json = workbook_to_json(ss_structure)
         # ideally, when all these tests are working, this would be
@@ -77,7 +78,24 @@ def _ss_structure_to_pyxform_survey(self, ss_structure, kwargs):
 
         return survey
 
-    def _autonameInputs(self, kwargs):
+    @staticmethod
+    def _run_odk_validate(xml):
+        # On Windows, NamedTemporaryFile must be opened exclusively.
+        # So it must be explicitly created, opened, closed, and removed
+        tmp = tempfile.NamedTemporaryFile(suffix='.xml', delete=False)
+        tmp.close()
+        try:
+            with codecs.open(tmp.name, mode="w", encoding="utf-8") as fp:
+                fp.write(xml)
+                fp.close()
+            check_xform(tmp.name)
+        finally:
+            # Clean up the temporary file
+            os.remove(tmp.name)
+            assert not os.path.isfile(tmp.name)
+
+    @staticmethod
+    def _autoname_inputs(kwargs):
         """
         title and name are necessary for surveys, but not always convenient to
         include in test cases, so this will pull a default value
@@ -95,6 +113,7 @@ def _autonameInputs(self, kwargs):
 
 
 class PyxformTestCase(PyxformMarkdown, TestCase):
+
     def assertPyxformXform(self, **kwargs):
         """
         PyxformTestCase.assertPyxformXform() named arguments:
@@ -139,10 +158,10 @@ def assertPyxformXform(self, **kwargs):
 
         try:
             if 'md' in kwargs.keys():
-                kwargs = self._autonameInputs(kwargs)
+                kwargs = self._autoname_inputs(kwargs)
                 survey = self.md_to_pyxform_survey(kwargs.get('md'), kwargs)
             elif 'ss_structure' in kwargs.keys():
-                kwargs = self._autonameInputs(kwargs)
+                kwargs = self._autoname_inputs(kwargs)
                 survey = self._ss_structure_to_pyxform_survey(
                     kwargs.get('ss_structure'), kwargs)
             else:
@@ -172,30 +191,13 @@ def _pull_xml_node_from_root(element_selector):
             if debug:
                 print(xml)
             if run_odk_validate:
-                # On Windows, NamedTemporaryFile must be opened exclusively.
-                # So it must be explicitly created, opened, closed, and removed
-                tmp = tempfile.NamedTemporaryFile(suffix='.xml', delete=False)
-                tmp.close()
-                try:
-                    with codecs.open(
-                            tmp.name, mode="w", encoding="utf-8") as fp:
-                        if '_xml_append' in kwargs:
-                            xml += kwargs['_xml_append']
-                        fp.write(xml)
-                        fp.close()
-                    check_xform(tmp.name)
-                finally:
-                    # Clean up the temporary file
-                    os.remove(tmp.name)
-                    assert not os.path.isfile(tmp.name)
-
+                self._run_odk_validate(xml=xml)
                 if len(odk_validate_error__contains) > 0:
-                    raise PyxformTestError(
-                        "ODKValidateError was not raised"
-                        )
+                    raise PyxformTestError("ODKValidateError was not raised")
+
         except PyXFormError as e:
             survey = False
-            errors = [str(e)]
+            errors = [unicode(e)]
             if debug:
                 print("<xml unavailable />")
                 print("ERROR: '%s'" % errors[0])
@@ -203,9 +205,9 @@ def _pull_xml_node_from_root(element_selector):
             if len(odk_validate_error__contains) is 0:
                 raise PyxformTestError("ODK Validate error was thrown but " +
                                        "'odk_validate_error__contains'" +
-                                       " was empty:" + e.message)
+                                       " was empty:" + unicode(e))
             for v_err in odk_validate_error__contains:
-                self.assertContains(e.message, v_err,
+                self.assertContains(unicode(e), v_err,
                                     msg_prefix='odk_validate_error__contains')
         else:
             survey = True
@@ -216,8 +218,8 @@ def _check_contains(keyword):
 
                 def check_content(content):
                     text_arr = kwargs[contains_str]
-                    for text in text_arr:
-                        self.assertContains(content, text, msg_prefix=keyword)
+                    for i in text_arr:
+                        self.assertContains(content, i, msg_prefix=keyword)
 
                 return contains_str, check_content
 
@@ -254,7 +256,8 @@ def check_content(content):
                 self.assertContains(joined_error, text,
                                     msg_prefix="error__contains")
 
-    def _assert_contains(self, content, text, msg_prefix):
+    @staticmethod
+    def _assert_contains(content, text, msg_prefix):
         if msg_prefix:
             msg_prefix += ": "
 

From f01917f38e7f8a2e7a254c270497edd179bf14fb Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Thu, 21 Dec 2017 18:25:25 +1100
Subject: [PATCH 08/11] add: test suggested by @MartijnR, fix for #88, add
 odk_validate checks

- New test was to check that a xml-data called "fruits" and a pulldata
  referencing "fruits" only output one instance with id="fruits"
- Fix for #88 was to ensure that instances generated for choices were
  not output if an instance with the same id was already being generated
  from another source - see _generate_instances docstring for details
- Run odk_validate for all positive-case tests to ensure that valid
  documents are being produced. To satisfy odk_validate, a model item
  (in this case, a note) was added, which is reasonable since these
  external instances would always be used with a form with >1 real item
- Remove unique_id from InstanceInfo since instances must be unique
  based on the id attribute only, id +/- src
---
 pyxform/instance_info.py         |   3 +-
 pyxform/survey.py                |  44 ++++++++---
 pyxform/tests_v1/test_xmldata.py | 129 +++++++++++++++++++++++++++----
 3 files changed, 148 insertions(+), 28 deletions(-)

diff --git a/pyxform/instance_info.py b/pyxform/instance_info.py
index ac2b0a079..e7b2d3d2b 100644
--- a/pyxform/instance_info.py
+++ b/pyxform/instance_info.py
@@ -3,9 +3,8 @@
 class InstanceInfo(object):
     """Standardise Instance details relevant during XML generation."""
 
-    def __init__(self, type, context, name, unique_id, instance):
+    def __init__(self, type, context, name, instance):
         self.type = type
         self.context = context
         self.name = name
-        self.unique_id = unique_id
         self.instance = instance
diff --git a/pyxform/survey.py b/pyxform/survey.py
index 624221f6e..196e23672 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -139,7 +139,6 @@ def _generate_static_instances(list_name, choice_list):
             type=u"choice",
             context=u"survey",
             name=list_name,
-            unique_id=list_name,
             instance=node(
                 "instance",
                 node("root", *instance_element_list),
@@ -157,7 +156,6 @@ def _generate_external_instances(element):
                     n=element[u"parent"][u"name"]
                 ),
                 name=element[u"name"],
-                unique_id=(element[u"name"],),
                 instance=element.xml_instance()
             )
 
@@ -205,7 +203,6 @@ def _generate_pulldata_instances(element):
                             n=element[u"parent"][u"name"]
                         ),
                         name=file_id,
-                        unique_id=(file_id, uri),
                         instance=node(
                             "instance",
                             id=file_id,
@@ -227,7 +224,6 @@ def _generate_from_file_instances(element):
                     n=element[u"parent"][u"name"]
                 ),
                 name=file_id,
-                unique_id=(file_id, uri),
                 instance=node(
                     "instance",
                     node("root",
@@ -244,16 +240,39 @@ def _generate_instances(self):
         Get instances from all the different ways that they may be generated.
 
         An opportunity to validate instances before output to the XML model.
+
+        Instance names used for the id attribute are generated as follows:
+
+        - xml-data: item name value (for type==xml-data)
+        - pulldata: first arg to calculation->pulldata()
+        - select from file: file name arg to type->itemset
+        - choices: list_name (for type==select_*)
+
+        Validation and business rules for output of instances:
+
+        - xml-data item name must be unique across the XForm and the form is
+          considered invalid if there is a duplicate name. This differs from
+          other item types which allow duplicates if not in the same group.
+        - for all instance sources, if the same instance name is encountered,
+          only the first instance definition will be output, even if the
+          instance definitions are different (i.e. external XML, external CSV,
+          or select_* placeholder instances). The "first instance" is
+          determined by the item position in the survey sheet, then by the
+          list_name in the choices sheet. This is done to allow users to re-use
+          external sources without duplicate instances being generated in the
+          XForm document. However, it does require careful in form design.
         """
         instances = []
+        for i in self.iter_descendants():
+            i_ext = self._generate_external_instances(element=i)
+            i_pull = self._generate_pulldata_instances(element=i)
+            i_file = self._generate_from_file_instances(element=i)
+            instances += [x for x in [i_ext, i_pull, i_file] if x is not None]
+
+        # Append last so the choice instance is excluded on a name clash.
         for k, v in self.choices.items():
             instances += [
                 self._generate_static_instances(list_name=k, choice_list=v)]
-        for i in self.iter_descendants():
-            ext = self._generate_external_instances(element=i)
-            pull = self._generate_pulldata_instances(element=i)
-            file = self._generate_from_file_instances(element=i)
-            instances += [x for x in [ext, pull, file] if x is not None]
 
         # Check that external instances have unique names.
         if 0 < len(instances):
@@ -261,12 +280,13 @@ def _generate_instances(self):
             self._validate_external_instances(instances=ext_only)
 
         # Only output the exact same instance once.
-        # Identification varies by instance generation type: id +/- src.
         seen = []
         for i in instances:
-            if i.unique_id not in seen:
+            if i.name not in seen:
                 yield i.instance
-            seen.append(i.unique_id)
+            else:
+                pass  # TODO: warn user in case it was unintentional duplicate
+            seen.append(i.name)
 
     def xml_model(self):
         """
diff --git a/pyxform/tests_v1/test_xmldata.py b/pyxform/tests_v1/test_xmldata.py
index b5adceec5..1d75e7cef 100644
--- a/pyxform/tests_v1/test_xmldata.py
+++ b/pyxform/tests_v1/test_xmldata.py
@@ -18,7 +18,8 @@ def test_external_single_ok(self):
             """,
             model__contains=[
                 '<instance id="mydata" src="jr://file/mydata.xml"/>'
-            ]
+            ],
+            run_odk_validate=True
         )
 
     def test_external_two_in_section_duplicate_raises(self):
@@ -48,7 +49,8 @@ def test_external_two_in_section_unique_ok(self):
             model__contains=[
                 '<instance id="mydata" src="jr://file/mydata.xml"/>',
                 '<instance id="mydata2" src="jr://file/mydata2.xml"/>'
-            ]
+            ],
+            run_odk_validate=True
         )
 
     def test_external_multi_group_duplicate_raises(self):
@@ -100,11 +102,14 @@ def test_external_multi_group_unique_ok(self):
             |        | xml-data    | mydata  |                  |
             |        | begin group | g1      |                  |
             |        | xml-data    | mydata1 |                  |
+            |        | note        | note1   | It's note-able   |
             |        | end group   | g1      |                  |
             |        | begin group | g2      |                  |
+            |        | note        | note2   | It's note-able   |
             |        | xml-data    | mydata2 |                  |
             |        | end group   | g2      |                  |
             |        | begin group | g3      |                  |
+            |        | note        | note3   | It's note-able   |
             |        | xml-data    | mydata3 |                  |
             |        | end group   | g3      |                  |
             """,
@@ -113,7 +118,8 @@ def test_external_multi_group_unique_ok(self):
                 '<instance id="mydata1" src="jr://file/mydata1.xml"/>',
                 '<instance id="mydata2" src="jr://file/mydata2.xml"/>',
                 '<instance id="mydata3" src="jr://file/mydata3.xml"/>'
-            ]
+            ],
+            run_odk_validate=True
         )
 
     def test_instance_names_other_sources_duplicate_raises(self):
@@ -149,6 +155,7 @@ def test_instance_names_other_sources_unique_ok(self):
             |        | type                                 | name  | label | calculation  |
             |        | begin group                          | g1    |       |              |
             |        | xml-data                             | city1 |       |              |
+            |        | note                                 | note1 | Note  |              |
             |        | end group                            | g1    |       |              |
             |        | begin group                          | g2    |       |              |
             |        | select_one_from_file cities.csv      | city2 | City2 |              |
@@ -158,6 +165,7 @@ def test_instance_names_other_sources_unique_ok(self):
             |        | end group                            | g3    |       |              |
             |        | begin group                          | g4    |       |              |
             |        | calculate                            | city4 | City4 | pulldata('fruits', 'name', 'name', 'mango') |
+            |        | note                                 | note4 | Note  |              |
             |        | end group                            | g4    |       |              |
             """,
             model__contains=[
@@ -173,19 +181,112 @@ def test_instance_names_other_sources_unique_ok(self):
       </instance>
 """,
                 '<instance id="fruits" src="jr://file-csv/fruits.csv"/>'
-            ]
+            ],
+            run_odk_validate=True
         )
 
-    @skip("Usage scenarios TBA but it might look something like this.")
-    def test_external_usage_scenario(self):
+    def test_one_instance_per_external_select(self):
+        """Using a select from file should output 1 instance: #88 bug test"""
+        md = """
+            | survey  |                                      |       |       |                                    |
+            |         | type                                 | name  | label | choice_filter                      |
+            |         | select_one_from_file states.csv      | state | State |                                    |
+            |         | select_one_from_file cities.csv      | city  | City  | state=/select_from_file_test/State |
+            |         | select_one regular                   | test  | Test  |                                    |
+            | choices |                                      |       |       |                                    |
+            |         | list_name                            | name  | label |                                    |
+            |         | states                               | name  | label |                                    |
+            |         | cities                               | name  | label |                                    |
+            |         | regular                              | 1     | Pass  |                                    |
+            |         | regular                              | 2     | Fail  |                                    |
+            """
         self.assertPyxformXform(
-            md="""
-            | survey |             |           |                  | 
-            |        | type        | name      | label            | bind::type  |  calculation  
-            |        | calculate   | external1 |                  | External    |  oc-item(event1(1), form2, item3(1))
-            |        | note        | ext_note  | The external value is ${external_1} |     |
-            """,
+            md=md,
             model__contains=[
-                # TODO: not sure what
-                ])
+"""
+      <instance id="states" src="jr://file-csv/states.csv">
+        <root>
+          <item>
+            <name>_</name>
+            <label>_</label>
+          </item>
+        </root>
+      </instance>
+""",
+"""
+      <instance id="cities" src="jr://file-csv/cities.csv">
+        <root>
+          <item>
+            <name>_</name>
+            <label>_</label>
+          </item>
+        </root>
+      </instance>
+""",
+"""
+      <instance id="regular">
+        <root>
+          <item>
+            <itextId>static_instance-regular-0</itextId>
+            <name>1</name>
+          </item>
+          <item>
+            <itextId>static_instance-regular-1</itextId>
+            <name>2</name>
+          </item>
+        </root>
+      </instance>
+"""
+            ],
+            run_odk_validate=True
+        )
+        survey = self.md_to_pyxform_survey(md_raw=md)
+        xml = survey._to_pretty_xml()
+        unwanted_extra_states = \
+"""
+      <instance id="regular">
+        <root>
+          <item>
+            <itextId>static_instance-states-0</itextId>
+            <name>1</name>
+          </item>
+        </root>
+      </instance>
+"""
+        self.assertNotIn(unwanted_extra_states, xml)
+        unwanted_extra_cities = \
+            """
+                  <instance id="regular">
+                    <root>
+                      <item>
+                        <itextId>static_instance-cities-0</itextId>
+                        <name>1</name>
+                      </item>
+                    </root>
+                  </instance>
+            """
+        self.assertNotIn(unwanted_extra_cities, xml)
 
+    def test_no_duplicate_with_pulldata(self):
+        """Using xml-data and pulldata should not output 2 instances."""
+        md = """
+            | survey |                                      |        |       |              |
+            |        | type                                 | name   | label | calculation  |
+            |        | begin group                          | g1     |       |              |
+            |        | xml-data                             | fruits |       |              |
+            |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
+            |        | note                                 | note   | Fruity! ${f_csv} |   |
+            |        | end group                            | g1     |       |              |
+            """
+        self.assertPyxformXform(
+            md=md,
+            model__contains=[
+                '<instance id="fruits" src="jr://file/fruits.xml"/>',
+            ],
+            run_odk_validate=True
+        )
+        survey = self.md_to_pyxform_survey(md_raw=md)
+        xml = survey._to_pretty_xml()
+        unwanted_extra_fruits = \
+            '<instance id="fruits" src="jr://file-csv/fruits.csv"/>'
+        self.assertNotIn(unwanted_extra_fruits, xml)

From 0290fc18eb709ce486220dad09863e6a8b097058 Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Tue, 2 Jan 2018 10:19:57 +1100
Subject: [PATCH 09/11] add: comment from @lognaturel about search and
 select_one_external

---
 pyxform/survey.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/pyxform/survey.py b/pyxform/survey.py
index 196e23672..dd14be0d0 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -261,6 +261,15 @@ def _generate_instances(self):
           list_name in the choices sheet. This is done to allow users to re-use
           external sources without duplicate instances being generated in the
           XForm document. However, it does require careful in form design.
+
+        There are two other things currently supported by pyxform that involve
+        external files and are not explicitly handled here, but may be relevant
+        to future efforts to harmonise / simplify external data workflows:
+
+        - `search` appearance/function: works a lot like pulldata but the csv
+          isn't made explicit in the form.
+        - `select_one_external`: implicitly relies on a `itemsets.csv` file and
+          uses XPath-like expressions for querying.
         """
         instances = []
         for i in self.iter_descendants():

From 8950324538dd86492d1cf3396d7605d69b95aa18 Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Tue, 2 Jan 2018 15:57:02 +1100
Subject: [PATCH 10/11] chg: raise an error on instance id/src clash instead of
 silent ignore

- previously would just output the first of any duplicate instances and
  ignore the rest with no feedback to user
- changed existing instance methods so that they all have some content
  in the instance, to pass odk validate requirements when re-using
  instance defs across different sources
  - pull_data spec test updated accordingly
- renamed / changed / added to tests to make it clearer what can and
  cannot be done with this feature
---
 pyxform/external_instance.py                  |   9 -
 pyxform/instance_info.py                      |   3 +-
 pyxform/survey.py                             |  67 ++--
 .../tests/test_expected_output/pull_data.xml  |   9 +-
 pyxform/tests_v1/test_xmldata.py              | 314 +++++++++++-------
 5 files changed, 250 insertions(+), 152 deletions(-)

diff --git a/pyxform/external_instance.py b/pyxform/external_instance.py
index 0f411c4e0..c2c54caff 100644
--- a/pyxform/external_instance.py
+++ b/pyxform/external_instance.py
@@ -4,15 +4,6 @@
 
 class ExternalInstance(SurveyElement):
 
-    def xml_instance(self):
-        """
-        Get the XML representation of the external instance element.
-        """
-        return node(
-            "instance",
-            id=self[u"name"],
-            src="jr://file/{}.xml".format(self[u"name"]))
-
     def xml_control(self):
         """
         No-op since there is no associated form control to place under <body/>.
diff --git a/pyxform/instance_info.py b/pyxform/instance_info.py
index e7b2d3d2b..b2d339c06 100644
--- a/pyxform/instance_info.py
+++ b/pyxform/instance_info.py
@@ -3,8 +3,9 @@
 class InstanceInfo(object):
     """Standardise Instance details relevant during XML generation."""
 
-    def __init__(self, type, context, name, instance):
+    def __init__(self, type, context, name, src, instance):
         self.type = type
         self.context = context
         self.name = name
+        self.src = src
         self.instance = instance
diff --git a/pyxform/survey.py b/pyxform/survey.py
index dd14be0d0..d8e98f53c 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -118,6 +118,11 @@ def _generate_static_instances(list_name, choice_list):
         """
         Generates <instance> elements for static data
         (e.g. choices for select type questions)
+
+        Note that per commit message 0578242 and in xls2json.py R539, an
+        instance is only output for select items defined in the choices sheet
+        when the item has a choice_filter, and it is that way for backwards
+        compatibility.
         """
         instance_element_list = []
         for idx, choice in enumerate(choice_list):
@@ -139,6 +144,7 @@ def _generate_static_instances(list_name, choice_list):
             type=u"choice",
             context=u"survey",
             name=list_name,
+            src=None,
             instance=node(
                 "instance",
                 node("root", *instance_element_list),
@@ -146,17 +152,30 @@ def _generate_static_instances(list_name, choice_list):
             )
         )
 
+    @staticmethod
+    def _get_dummy_instance():
+        """Instance content required by ODK Validate for select inputs."""
+        return node("root", node("item", node("name", "_"), node("label", "_")))
+
     @staticmethod
     def _generate_external_instances(element):
         if isinstance(element, ExternalInstance):
+            name = element[u"name"]
+            src = "jr://file/{}.xml".format(name)
             return InstanceInfo(
                 type=u"external",
                 context="[type: {t}, name: {n}]".format(
                     t=element[u"parent"][u"type"],
                     n=element[u"parent"][u"name"]
                 ),
-                name=element[u"name"],
-                instance=element.xml_instance()
+                name=name,
+                src=src,
+                instance=node(
+                    "instance",
+                    Survey._get_dummy_instance(),
+                    id=name,
+                    src=src
+                )
             )
 
     @staticmethod
@@ -203,8 +222,10 @@ def _generate_pulldata_instances(element):
                             n=element[u"parent"][u"name"]
                         ),
                         name=file_id,
+                        src=uri,
                         instance=node(
                             "instance",
+                            Survey._get_dummy_instance(),
                             id=file_id,
                             src=uri
                         )
@@ -224,12 +245,10 @@ def _generate_from_file_instances(element):
                     n=element[u"parent"][u"name"]
                 ),
                 name=file_id,
+                src=uri,
                 instance=node(
                     "instance",
-                    node("root",
-                         node("item",
-                              node("name", "_"),
-                              node("label", "_"))),
+                    Survey._get_dummy_instance(),
                     id=file_id,
                     src=uri
                 )
@@ -254,13 +273,11 @@ def _generate_instances(self):
           considered invalid if there is a duplicate name. This differs from
           other item types which allow duplicates if not in the same group.
         - for all instance sources, if the same instance name is encountered,
-          only the first instance definition will be output, even if the
-          instance definitions are different (i.e. external XML, external CSV,
-          or select_* placeholder instances). The "first instance" is
-          determined by the item position in the survey sheet, then by the
-          list_name in the choices sheet. This is done to allow users to re-use
-          external sources without duplicate instances being generated in the
-          XForm document. However, it does require careful in form design.
+          the following rules are used to allow re-using instances but prevent
+          overwriting conflicting instances:
+          - same id, same src URI: skip adding the second (duplicate) instance
+          - same id, different src URI: raise an error
+          - otherwise: output the instance
 
         There are two other things currently supported by pyxform that involve
         external files and are not explicitly handled here, but may be relevant
@@ -288,14 +305,26 @@ def _generate_instances(self):
             ext_only = [x for x in instances if x.type == "external"]
             self._validate_external_instances(instances=ext_only)
 
-        # Only output the exact same instance once.
-        seen = []
+        seen = {}
         for i in instances:
-            if i.name not in seen:
-                yield i.instance
+            if i.name in seen.keys() and seen[i.name].src != i.src:
+                # Instance id exists with different src URI -> error.
+                msg = "The same instance id will be generated for different " \
+                      "external instance source URIs. Please check the form. " \
+                      "Instance name: '{i}', Existing type: '{e}', " \
+                      "Existing URI: '{iu}', Duplicate type: '{d}', " \
+                      "Duplicate URI: '{du}', Duplicate context: '{c}'.".format(
+                          i=i.name, iu=seen[i.name].src, e=seen[i.name].type,
+                          d=i.type, du=i.src, c=i.context
+                      )
+                raise PyXFormError(msg)
+            elif i.name in seen.keys() and seen[i.name].src == i.src:
+                # Instance id exists with same src URI -> ok, don't duplicate.
+                continue
             else:
-                pass  # TODO: warn user in case it was unintentional duplicate
-            seen.append(i.name)
+                # Instance doesn't exist yet -> add it.
+                yield i.instance
+            seen[i.name] = i
 
     def xml_model(self):
         """
diff --git a/pyxform/tests/test_expected_output/pull_data.xml b/pyxform/tests/test_expected_output/pull_data.xml
index 0e01d4bb7..ccd3d38be 100644
--- a/pyxform/tests/test_expected_output/pull_data.xml
+++ b/pyxform/tests/test_expected_output/pull_data.xml
@@ -12,7 +12,14 @@
           </meta>
         </pull_data>
       </instance>
-      <instance id="fruits" src="jr://file-csv/fruits.csv"/>
+      <instance id="fruits" src="jr://file-csv/fruits.csv">
+        <root>
+          <item>
+            <name>_</name>
+            <label>_</label>
+          </item>
+        </root>
+      </instance>
       <bind calculate="pulldata('fruits', 'name','name', 'mango')" nodeset="/pull_data/fruit" type="string"/>
       <bind nodeset="/pull_data/note_fruite" readonly="true()" type="string"/>
       <bind calculate="concat('uuid:', uuid())" nodeset="/pull_data/meta/instanceID" readonly="true()" type="string"/>
diff --git a/pyxform/tests_v1/test_xmldata.py b/pyxform/tests_v1/test_xmldata.py
index 1d75e7cef..91cda67da 100644
--- a/pyxform/tests_v1/test_xmldata.py
+++ b/pyxform/tests_v1/test_xmldata.py
@@ -1,3 +1,4 @@
+from pyxform.errors import PyXFormError
 from pyxform.tests_v1.pyxform_test_case import PyxformTestCase
 from pyxform.tests_v1.pyxform_test_case import PyxformTestError
 try:
@@ -8,7 +9,7 @@
 
 class ExternalInstanceTests(PyxformTestCase):
 
-    def test_external_single_ok(self):
+    def test_can__output_single_external_xml_item(self):
         """Simplest possible example to include an external instance."""
         self.assertPyxformXform(
             md="""
@@ -17,12 +18,12 @@ def test_external_single_ok(self):
             |        | xml-data    | mydata    |                  |
             """,
             model__contains=[
-                '<instance id="mydata" src="jr://file/mydata.xml"/>'
+                '<instance id="mydata" src="jr://file/mydata.xml">'
             ],
             run_odk_validate=True
         )
 
-    def test_external_two_in_section_duplicate_raises(self):
+    def test_cannot__use_same_external_xml_id_in_same_section(self):
         """Duplicate external instances in the same section raises an error."""
         with self.assertRaises(PyxformTestError) as ctx:
             self.assertPyxformXform(
@@ -37,7 +38,7 @@ def test_external_two_in_section_duplicate_raises(self):
         self.assertIn("There are two survey elements named 'mydata'",
                       repr(ctx.exception))
 
-    def test_external_two_in_section_unique_ok(self):
+    def test_can__use_unique_external_xml_in_same_section(self):
         """Two unique external instances in the same section is OK."""
         self.assertPyxformXform(
             md="""
@@ -47,13 +48,13 @@ def test_external_two_in_section_unique_ok(self):
             |        | xml-data    | mydata2   |                  |
             """,
             model__contains=[
-                '<instance id="mydata" src="jr://file/mydata.xml"/>',
-                '<instance id="mydata2" src="jr://file/mydata2.xml"/>'
+                '<instance id="mydata" src="jr://file/mydata.xml">',
+                '<instance id="mydata2" src="jr://file/mydata2.xml">'
             ],
             run_odk_validate=True
         )
 
-    def test_external_multi_group_duplicate_raises(self):
+    def test_cannot__use_same_external_xml_id_across_groups(self):
         """Duplicate external instances anywhere raises an error."""
         with self.assertRaises(PyxformTestError) as ctx:
             self.assertPyxformXform(
@@ -70,30 +71,10 @@ def test_external_multi_group_duplicate_raises(self):
                 """,
                 model__contains=[])
         self.assertIn("Instance names must be unique", repr(ctx.exception))
+        self.assertIn(
+            "The name \'mydata\' was found 3 time(s)", repr(ctx.exception))
 
-    def test_external_multi_group_duplicate_raises_helpfully(self):
-        """Duplicate external instances anywhere raises a helpful error."""
-        with self.assertRaises(PyxformTestError) as ctx:
-            self.assertPyxformXform(
-                md="""
-                | survey |             |         |                  |
-                |        | type        | name    | label            |
-                |        | xml-data    | mydata  |                  |
-                |        | begin group | g1      |                  |
-                |        | xml-data    | mydata  |                  |
-                |        | end group   | g1      |                  |
-                |        | begin group | g2      |                  |
-                |        | xml-data    | mydata2 |                  |
-                |        | end group   | g2      |                  |
-                |        | begin group | g3      |                  |
-                |        | xml-data    | mydata3 |                  |
-                |        | end group   | g3      |                  |
-                """,
-                model__contains=[])
-        self.assertIn("The name 'mydata' was found 2 time(s)",
-                      repr(ctx.exception))
-
-    def test_external_multi_group_unique_ok(self):
+    def test_can__use_unique_external_xml_across_groups(self):
         """Unique external instances anywhere is OK."""
         self.assertPyxformXform(
             md="""
@@ -114,15 +95,15 @@ def test_external_multi_group_unique_ok(self):
             |        | end group   | g3      |                  |
             """,
             model__contains=[
-                '<instance id="mydata" src="jr://file/mydata.xml"/>',
-                '<instance id="mydata1" src="jr://file/mydata1.xml"/>',
-                '<instance id="mydata2" src="jr://file/mydata2.xml"/>',
-                '<instance id="mydata3" src="jr://file/mydata3.xml"/>'
+                '<instance id="mydata" src="jr://file/mydata.xml">',
+                '<instance id="mydata1" src="jr://file/mydata1.xml">',
+                '<instance id="mydata2" src="jr://file/mydata2.xml">',
+                '<instance id="mydata3" src="jr://file/mydata3.xml">'
             ],
             run_odk_validate=True
         )
 
-    def test_instance_names_other_sources_duplicate_raises(self):
+    def test_cannot__use_same_external_xml_id_with_mixed_types(self):
         """Duplicate instances with other sources present raises an error."""
         with self.assertRaises(PyxformTestError) as ctx:
             self.assertPyxformXform(
@@ -147,29 +128,34 @@ def test_instance_names_other_sources_duplicate_raises(self):
         self.assertIn("The name 'city' was found 2 time(s)",
                       repr(ctx.exception))
 
-    def test_instance_names_other_sources_unique_ok(self):
+    def test_can__use_all_types_together_with_unique_ids(self):
         """Unique instances with other sources present are OK."""
         self.assertPyxformXform(
             md="""
-            | survey |                                      |       |       |
-            |        | type                                 | name  | label | calculation  |
-            |        | begin group                          | g1    |       |              |
-            |        | xml-data                             | city1 |       |              |
-            |        | note                                 | note1 | Note  |              |
-            |        | end group                            | g1    |       |              |
-            |        | begin group                          | g2    |       |              |
-            |        | select_one_from_file cities.csv      | city2 | City2 |              |
-            |        | end group                            | g2    |       |              |
-            |        | begin group                          | g3    |       |              |
-            |        | select_multiple_from_file cities.csv | city3 | City3 |              |
-            |        | end group                            | g3    |       |              |
-            |        | begin group                          | g4    |       |              |
-            |        | calculate                            | city4 | City4 | pulldata('fruits', 'name', 'name', 'mango') |
-            |        | note                                 | note4 | Note  |              |
-            |        | end group                            | g4    |       |              |
+            | survey  |                                      |       |       |              |
+            |         | type                                 | name  | label | calculation  | choice_filter |
+            |         | begin group                          | g1    |       | | |
+            |         | xml-data                             | city1 |       | | |
+            |         | note                                 | note1 | Note  | | |
+            |         | end group                            | g1    |       | | |
+            |         | begin group                          | g2    |       | | |
+            |         | select_one_from_file cities.csv      | city2 | City2 | | |
+            |         | end group                            | g2    |       | | |
+            |         | begin group                          | g3    |       | | |
+            |         | select_multiple_from_file cities.csv | city3 | City3 | | |
+            |         | end group                            | g3    |       | | |
+            |         | begin group                          | g4    |       | | |
+            |         | calculate                            | city4 | City4 | pulldata('fruits', 'name', 'name', 'mango') | |
+            |         | note                                 | note4 | Note  | | |
+            |         | end group                            | g4    |       | | |
+            |         | select_one states                    | test  | Test  | | true() |
+            | choices |                                      |       |       | | |
+            |         | list_name                            | name  | label | | |
+            |         | states                               | 1     | Pass  | | |
+            |         | states                               | 2     | Fail  | | |
             """,
             model__contains=[
-                '<instance id="city1" src="jr://file/city1.xml"/>',
+                '<instance id="city1" src="jr://file/city1.xml">',
 """
       <instance id="cities" src="jr://file-csv/cities.csv">
         <root>
@@ -180,31 +166,107 @@ def test_instance_names_other_sources_unique_ok(self):
         </root>
       </instance>
 """,
-                '<instance id="fruits" src="jr://file-csv/fruits.csv"/>'
+                '<instance id="fruits" src="jr://file-csv/fruits.csv">',
+"""
+      <instance id="states">
+        <root>
+          <item>
+            <itextId>static_instance-states-0</itextId>
+            <name>1</name>
+          </item>
+          <item>
+            <itextId>static_instance-states-1</itextId>
+            <name>2</name>
+          </item>
+        </root>
+      </instance>
+"""
             ],
             run_odk_validate=True
         )
 
-    def test_one_instance_per_external_select(self):
-        """Using a select from file should output 1 instance: #88 bug test"""
+    def test_cannot__use_different_src_same_id__select_then_internal(self):
+        """Duplicate instance from internal choices raises an error: #88."""
         md = """
-            | survey  |                                      |       |       |                                    |
-            |         | type                                 | name  | label | choice_filter                      |
-            |         | select_one_from_file states.csv      | state | State |                                    |
-            |         | select_one_from_file cities.csv      | city  | City  | state=/select_from_file_test/State |
-            |         | select_one regular                   | test  | Test  |                                    |
-            | choices |                                      |       |       |                                    |
-            |         | list_name                            | name  | label |                                    |
-            |         | states                               | name  | label |                                    |
-            |         | cities                               | name  | label |                                    |
-            |         | regular                              | 1     | Pass  |                                    |
-            |         | regular                              | 2     | Fail  |                                    |
+            | survey  |                                      |       |       |               |
+            |         | type                                 | name  | label | choice_filter |
+            |         | select_one_from_file states.csv      | state | State |               |
+            |         | select_one states                    | test  | Test  | state=/select_from_file_test/state |
+            | choices |                                      |       |       |               |
+            |         | list_name                            | name  | label |               |
+            |         | states                               | 1     | Pass  |               |
+            |         | states                               | 2     | Fail  |               |
             """
-        self.assertPyxformXform(
-            md=md,
-            model__contains=[
+        with self.assertRaises(PyXFormError) as ctx:
+            survey = self.md_to_pyxform_survey(md_raw=md)
+            survey._to_pretty_xml()
+        self.assertIn(
+            "Instance name: 'states', "
+            "Existing type: 'file', Existing URI: 'jr://file-csv/states.csv', "
+            "Duplicate type: 'choice', Duplicate URI: 'None', "
+            "Duplicate context: 'survey'.",
+            repr(ctx.exception)
+        )
+
+    def test_cannot__use_different_src_same_id__external_then_pulldata(self):
+        """Duplicate instance from pulldata after xml-data raises an error."""
+        md = """
+            | survey |                                      |        |       |              |
+            |        | type                                 | name   | label | calculation  |
+            |        | begin group                          | g1     |       |              |
+            |        | xml-data                             | fruits |       |              |
+            |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
+            |        | note                                 | note   | Fruity! ${f_csv} |   |
+            |        | end group                            | g1     |       |              |
+            """
+        with self.assertRaises(PyXFormError) as ctx:
+            survey = self.md_to_pyxform_survey(md_raw=md)
+            survey._to_pretty_xml()
+        self.assertIn(
+            "Instance name: 'fruits', "
+            "Existing type: 'external', Existing URI: 'jr://file/fruits.xml', "
+            "Duplicate type: 'pulldata', Duplicate URI: 'jr://file-csv/fruits.csv', "
+            "Duplicate context: '[type: group, name: g1]'.",
+            repr(ctx.exception)
+        )
+
+    def test_cannot__use_different_src_same_id__pulldata_then_external(self):
+        """Duplicate instance from xml-data after pulldata raises an error."""
+        md = """
+            | survey |                                      |        |       |              |
+            |        | type                                 | name   | label | calculation  |
+            |        | begin group                          | g1     |       |              |
+            |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
+            |        | xml-data                             | fruits |       |              |
+            |        | note                                 | note   | Fruity! ${f_csv} |   |
+            |        | end group                            | g1     |       |              |
+            """
+        with self.assertRaises(PyXFormError) as ctx:
+            survey = self.md_to_pyxform_survey(md_raw=md)
+            survey._to_pretty_xml()
+        self.assertIn(
+            "Instance name: 'fruits', "
+            "Existing type: 'pulldata', Existing URI: 'jr://file-csv/fruits.csv', "
+            "Duplicate type: 'external', Duplicate URI: 'jr://file/fruits.xml', "
+            "Duplicate context: '[type: group, name: g1]'.",
+            repr(ctx.exception)
+        )
+
+    def test_can__reuse_csv__selects_then_pulldata(self):
+        """Re-using the same csv external data source id and URI is OK."""
+        md = """
+            | survey |                                              |        |                                    |             |
+            |        | type                                         | name   | label                              | calculation |
+            |        | select_multiple_from_file pain_locations.csv | plocs  | Locations of pain this week.       | |
+            |        | select_one_from_file pain_locations.csv      | pweek  | Location of worst pain this week.  | |
+            |        | select_one_from_file pain_locations.csv      | pmonth | Location of worst pain this month. | |
+            |        | select_one_from_file pain_locations.csv      | pyear  | Location of worst pain this year.  | |
+            |        | calculate                                    | f_csv  | pd | pulldata('pain_locations', 'name', 'name', 'arm') |
+            |        | note                                         | note   | Arm ${f_csv} | |
+            """
+        expected = \
 """
-      <instance id="states" src="jr://file-csv/states.csv">
+      <instance id="pain_locations" src="jr://file-csv/pain_locations.csv">
         <root>
           <item>
             <name>_</name>
@@ -212,9 +274,28 @@ def test_one_instance_per_external_select(self):
           </item>
         </root>
       </instance>
-""",
 """
-      <instance id="cities" src="jr://file-csv/cities.csv">
+        self.assertPyxformXform(
+            md=md, model__contains=[expected], run_odk_validate=True)
+        survey = self.md_to_pyxform_survey(md_raw=md)
+        xml = survey._to_pretty_xml()
+        self.assertEqual(1, xml.count(expected))
+
+    def test_can__reuse_csv__pulldata_then_selects(self):
+        """Re-using the same csv external data source id and URI is OK."""
+        md = """
+            | survey |                                              |        |       |             |
+            |        | type                                         | name   | label | calculation |
+            |        | calculate                                    | f_csv  | pd | pulldata('pain_locations', 'name', 'name', 'arm') |
+            |        | note                                         | note   | Arm ${f_csv} | |
+            |        | select_multiple_from_file pain_locations.csv | plocs  | Locations of pain this week.       | |
+            |        | select_one_from_file pain_locations.csv      | pweek  | Location of worst pain this week.  | |
+            |        | select_one_from_file pain_locations.csv      | pmonth | Location of worst pain this month. | |
+            |        | select_one_from_file pain_locations.csv      | pyear  | Location of worst pain this year.  | |
+            """
+        expected = \
+"""
+      <instance id="pain_locations" src="jr://file-csv/pain_locations.csv">
         <root>
           <item>
             <name>_</name>
@@ -222,71 +303,60 @@ def test_one_instance_per_external_select(self):
           </item>
         </root>
       </instance>
-""",
 """
-      <instance id="regular">
+        self.assertPyxformXform(
+            md=md, model__contains=[expected], run_odk_validate=True)
+
+    def test_can__reuse_xml__selects_then_external(self):
+        """Re-using the same xml external data source id and URI is OK."""
+        md = """
+            | survey |                                              |        |        |
+            |        | type                                         | name   | label  | 
+            |        | select_multiple_from_file pain_locations.xml | plocs  | Locations of pain this week.       |
+            |        | select_one_from_file pain_locations.xml      | pweek  | Location of worst pain this week.  |
+            |        | select_one_from_file pain_locations.xml      | pmonth | Location of worst pain this month. |
+            |        | select_one_from_file pain_locations.xml      | pyear  | Location of worst pain this year.  |
+            |        | xml-data                                     | pain_locations  | |
+            """
+        expected = \
+"""
+      <instance id="pain_locations" src="jr://file/pain_locations.xml">
         <root>
           <item>
-            <itextId>static_instance-regular-0</itextId>
-            <name>1</name>
-          </item>
-          <item>
-            <itextId>static_instance-regular-1</itextId>
-            <name>2</name>
+            <name>_</name>
+            <label>_</label>
           </item>
         </root>
       </instance>
 """
-            ],
-            run_odk_validate=True
-        )
         survey = self.md_to_pyxform_survey(md_raw=md)
         xml = survey._to_pretty_xml()
-        unwanted_extra_states = \
+        self.assertEqual(1, xml.count(expected))
+
+    def test_can__reuse_xml__external_then_selects(self):
+        """Re-using the same xml external data source id and URI is OK."""
+        md = """
+            | survey |                                              |        |        |
+            |        | type                                         | name   | label  | 
+            |        | xml-data                                     | pain_locations  | |
+            |        | select_multiple_from_file pain_locations.xml | plocs  | Locations of pain this week.       |
+            |        | select_one_from_file pain_locations.xml      | pweek  | Location of worst pain this week.  |
+            |        | select_one_from_file pain_locations.xml      | pmonth | Location of worst pain this month. |
+            |        | select_one_from_file pain_locations.xml      | pyear  | Location of worst pain this year.  |
+            """
+        expected = \
 """
-      <instance id="regular">
+      <instance id="pain_locations" src="jr://file/pain_locations.xml">
         <root>
           <item>
-            <itextId>static_instance-states-0</itextId>
-            <name>1</name>
+            <name>_</name>
+            <label>_</label>
           </item>
         </root>
       </instance>
 """
-        self.assertNotIn(unwanted_extra_states, xml)
-        unwanted_extra_cities = \
-            """
-                  <instance id="regular">
-                    <root>
-                      <item>
-                        <itextId>static_instance-cities-0</itextId>
-                        <name>1</name>
-                      </item>
-                    </root>
-                  </instance>
-            """
-        self.assertNotIn(unwanted_extra_cities, xml)
-
-    def test_no_duplicate_with_pulldata(self):
-        """Using xml-data and pulldata should not output 2 instances."""
-        md = """
-            | survey |                                      |        |       |              |
-            |        | type                                 | name   | label | calculation  |
-            |        | begin group                          | g1     |       |              |
-            |        | xml-data                             | fruits |       |              |
-            |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
-            |        | note                                 | note   | Fruity! ${f_csv} |   |
-            |        | end group                            | g1     |       |              |
-            """
         self.assertPyxformXform(
-            md=md,
-            model__contains=[
-                '<instance id="fruits" src="jr://file/fruits.xml"/>',
-            ],
-            run_odk_validate=True
-        )
+            md=md, model__contains=[expected], run_odk_validate=True)
         survey = self.md_to_pyxform_survey(md_raw=md)
         xml = survey._to_pretty_xml()
-        unwanted_extra_fruits = \
-            '<instance id="fruits" src="jr://file-csv/fruits.csv"/>'
-        self.assertNotIn(unwanted_extra_fruits, xml)
+        self.assertEqual(1, xml.count(expected))

From 6f1d80a841620b4705a0e1408652aa19401fce1c Mon Sep 17 00:00:00 2001
From: lindsay stevens <lindsay.stevens.au@gmail.com>
Date: Tue, 2 Jan 2018 16:10:31 +1100
Subject: [PATCH 11/11] chg: rename xml-data to xml-external as per comment on
 #107

See https://github.com/XLSForm/pyxform/issues/107#issuecomment-353414375
---
 pyxform/builder.py                  |  2 +-
 pyxform/question_type_dictionary.py |  2 +-
 pyxform/survey.py                   |  4 +-
 pyxform/tests_v1/test_xmldata.py    | 88 ++++++++++++++---------------
 4 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/pyxform/builder.py b/pyxform/builder.py
index 6cb05bc53..7296061c0 100644
--- a/pyxform/builder.py
+++ b/pyxform/builder.py
@@ -93,7 +93,7 @@ def create_survey_element_from_dict(self, d):
             d = self._sections[section_name]
             full_survey = self.create_survey_element_from_dict(d)
             return full_survey.children
-        elif d[u"type"] == u"xml-data":
+        elif d[u"type"] == u"xml-external":
             return ExternalInstance(**d)
         else:
             return self._create_question_from_dict(
diff --git a/pyxform/question_type_dictionary.py b/pyxform/question_type_dictionary.py
index 4f13b9546..7de77bb01 100644
--- a/pyxform/question_type_dictionary.py
+++ b/pyxform/question_type_dictionary.py
@@ -857,7 +857,7 @@ def generate_new_dict():
                 "type": "binary"
             }
         },
-        "xml-data": {
+        "xml-external": {
             # Only effect is to add an external instance.
         }
     }
diff --git a/pyxform/survey.py b/pyxform/survey.py
index d8e98f53c..7e2370add 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -262,14 +262,14 @@ def _generate_instances(self):
 
         Instance names used for the id attribute are generated as follows:
 
-        - xml-data: item name value (for type==xml-data)
+        - xml-external: item name value (for type==xml-external)
         - pulldata: first arg to calculation->pulldata()
         - select from file: file name arg to type->itemset
         - choices: list_name (for type==select_*)
 
         Validation and business rules for output of instances:
 
-        - xml-data item name must be unique across the XForm and the form is
+        - xml-external item name must be unique across the XForm and the form is
           considered invalid if there is a duplicate name. This differs from
           other item types which allow duplicates if not in the same group.
         - for all instance sources, if the same instance name is encountered,
diff --git a/pyxform/tests_v1/test_xmldata.py b/pyxform/tests_v1/test_xmldata.py
index 91cda67da..d1d48958b 100644
--- a/pyxform/tests_v1/test_xmldata.py
+++ b/pyxform/tests_v1/test_xmldata.py
@@ -13,9 +13,9 @@ def test_can__output_single_external_xml_item(self):
         """Simplest possible example to include an external instance."""
         self.assertPyxformXform(
             md="""
-            | survey |             |           |                  |
-            |        | type        | name      | label            |
-            |        | xml-data    | mydata    |                  |
+            | survey |              |        |       |
+            |        | type         | name   | label |
+            |        | xml-external | mydata |       |
             """,
             model__contains=[
                 '<instance id="mydata" src="jr://file/mydata.xml">'
@@ -28,10 +28,10 @@ def test_cannot__use_same_external_xml_id_in_same_section(self):
         with self.assertRaises(PyxformTestError) as ctx:
             self.assertPyxformXform(
                 md="""
-                | survey |             |           |                  |
-                |        | type        | name      | label            |
-                |        | xml-data    | mydata    |                  |
-                |        | xml-data    | mydata    |                  |
+                | survey |              |        |       |
+                |        | type         | name   | label |
+                |        | xml-external | mydata |       |
+                |        | xml-external | mydata |       |
                 """,
                 model__contains=[])
         # This is caught first by existing validation rule.
@@ -42,10 +42,10 @@ def test_can__use_unique_external_xml_in_same_section(self):
         """Two unique external instances in the same section is OK."""
         self.assertPyxformXform(
             md="""
-            | survey |             |           |                  |
-            |        | type        | name      | label            |
-            |        | xml-data    | mydata    |                  |
-            |        | xml-data    | mydata2   |                  |
+            | survey |              |         |       |
+            |        | type         | name    | label |
+            |        | xml-external | mydata  |       |
+            |        | xml-external | mydata2 |       |
             """,
             model__contains=[
                 '<instance id="mydata" src="jr://file/mydata.xml">',
@@ -59,15 +59,15 @@ def test_cannot__use_same_external_xml_id_across_groups(self):
         with self.assertRaises(PyxformTestError) as ctx:
             self.assertPyxformXform(
                 md="""
-                | survey |             |         |                  |
-                |        | type        | name    | label            |
-                |        | xml-data    | mydata  |                  |
-                |        | begin group | g1      |                  |
-                |        | xml-data    | mydata  |                  |
-                |        | end group   | g1      |                  |
-                |        | begin group | g2      |                  |
-                |        | xml-data    | mydata  |                  |
-                |        | end group   | g2      |                  |
+                | survey |              |        |       |
+                |        | type         | name   | label |
+                |        | xml-external | mydata |       |
+                |        | begin group  | g1     |       |
+                |        | xml-external | mydata |       |
+                |        | end group    | g1     |       |
+                |        | begin group  | g2     |       |
+                |        | xml-external | mydata |       |
+                |        | end group    | g2     |       |
                 """,
                 model__contains=[])
         self.assertIn("Instance names must be unique", repr(ctx.exception))
@@ -78,21 +78,21 @@ def test_can__use_unique_external_xml_across_groups(self):
         """Unique external instances anywhere is OK."""
         self.assertPyxformXform(
             md="""
-            | survey |             |         |                  |
-            |        | type        | name    | label            |
-            |        | xml-data    | mydata  |                  |
-            |        | begin group | g1      |                  |
-            |        | xml-data    | mydata1 |                  |
-            |        | note        | note1   | It's note-able   |
-            |        | end group   | g1      |                  |
-            |        | begin group | g2      |                  |
-            |        | note        | note2   | It's note-able   |
-            |        | xml-data    | mydata2 |                  |
-            |        | end group   | g2      |                  |
-            |        | begin group | g3      |                  |
-            |        | note        | note3   | It's note-able   |
-            |        | xml-data    | mydata3 |                  |
-            |        | end group   | g3      |                  |
+            | survey |              |         |                |
+            |        | type         | name    | label          |
+            |        | xml-external | mydata  |                |
+            |        | begin group  | g1      |                |
+            |        | xml-external | mydata1 |                |
+            |        | note         | note1   | It's note-able |
+            |        | end group    | g1      |                |
+            |        | begin group  | g2      |                |
+            |        | note         | note2   | It's note-able |
+            |        | xml-external | mydata2 |                |
+            |        | end group    | g2      |                |
+            |        | begin group  | g3      |                |
+            |        | note         | note3   | It's note-able |
+            |        | xml-external | mydata3 |                |
+            |        | end group    | g3      |                |
             """,
             model__contains=[
                 '<instance id="mydata" src="jr://file/mydata.xml">',
@@ -111,9 +111,9 @@ def test_cannot__use_same_external_xml_id_with_mixed_types(self):
                 | survey |                                      |      |       |
                 |        | type                                 | name | label | calculation  |
                 |        | begin group                          | g1   |       |              |
-                |        | xml-data                             | city |       |              |
+                |        | xml-external                         | city |       |              |
                 |        | end group                            | g1   |       |              |
-                |        | xml-data                             | city |       |              |
+                |        | xml-external                         | city |       |              |
                 |        | begin group                          | g2   |       |              |
                 |        | select_one_from_file cities.csv      | city | City  |              |
                 |        | end group                            | g2   |       |              |
@@ -135,7 +135,7 @@ def test_can__use_all_types_together_with_unique_ids(self):
             | survey  |                                      |       |       |              |
             |         | type                                 | name  | label | calculation  | choice_filter |
             |         | begin group                          | g1    |       | | |
-            |         | xml-data                             | city1 |       | | |
+            |         | xml-external                         | city1 |       | | |
             |         | note                                 | note1 | Note  | | |
             |         | end group                            | g1    |       | | |
             |         | begin group                          | g2    |       | | |
@@ -209,12 +209,12 @@ def test_cannot__use_different_src_same_id__select_then_internal(self):
         )
 
     def test_cannot__use_different_src_same_id__external_then_pulldata(self):
-        """Duplicate instance from pulldata after xml-data raises an error."""
+        """Duplicate instance from pulldata after xml-external raises an error."""
         md = """
             | survey |                                      |        |       |              |
             |        | type                                 | name   | label | calculation  |
             |        | begin group                          | g1     |       |              |
-            |        | xml-data                             | fruits |       |              |
+            |        | xml-external                         | fruits |       |              |
             |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
             |        | note                                 | note   | Fruity! ${f_csv} |   |
             |        | end group                            | g1     |       |              |
@@ -231,13 +231,13 @@ def test_cannot__use_different_src_same_id__external_then_pulldata(self):
         )
 
     def test_cannot__use_different_src_same_id__pulldata_then_external(self):
-        """Duplicate instance from xml-data after pulldata raises an error."""
+        """Duplicate instance from xml-external after pulldata raises an error."""
         md = """
             | survey |                                      |        |       |              |
             |        | type                                 | name   | label | calculation  |
             |        | begin group                          | g1     |       |              |
             |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
-            |        | xml-data                             | fruits |       |              |
+            |        | xml-external                         | fruits |       |              |
             |        | note                                 | note   | Fruity! ${f_csv} |   |
             |        | end group                            | g1     |       |              |
             """
@@ -316,7 +316,7 @@ def test_can__reuse_xml__selects_then_external(self):
             |        | select_one_from_file pain_locations.xml      | pweek  | Location of worst pain this week.  |
             |        | select_one_from_file pain_locations.xml      | pmonth | Location of worst pain this month. |
             |        | select_one_from_file pain_locations.xml      | pyear  | Location of worst pain this year.  |
-            |        | xml-data                                     | pain_locations  | |
+            |        | xml-external                                 | pain_locations  | |
             """
         expected = \
 """
@@ -338,7 +338,7 @@ def test_can__reuse_xml__external_then_selects(self):
         md = """
             | survey |                                              |        |        |
             |        | type                                         | name   | label  | 
-            |        | xml-data                                     | pain_locations  | |
+            |        | xml-external                                 | pain_locations  | |
             |        | select_multiple_from_file pain_locations.xml | plocs  | Locations of pain this week.       |
             |        | select_one_from_file pain_locations.xml      | pweek  | Location of worst pain this week.  |
             |        | select_one_from_file pain_locations.xml      | pmonth | Location of worst pain this month. |