Fix attached CSV being shadowed by same-named entity list

collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt

@@ -1,13 +1,15 @@
 package org.odk.collect.android.application.initialization
 
 import org.javarosa.core.model.CoreModelModule
+import org.javarosa.core.reference.ReferenceManager
 import org.javarosa.core.services.PrototypeManager
 import org.javarosa.core.util.JavaRosaCoreModule
 import org.javarosa.model.xform.XFormsModule
 import org.javarosa.xform.parse.XFormParser
 import org.javarosa.xform.parse.XFormParserFactory
 import org.javarosa.xform.util.XFormUtils
 import org.odk.collect.android.dynamicpreload.DynamicPreloadXFormParserFactory
+import org.odk.collect.android.javarosawrapper.ReferenceManagerMediaFileRepository
 import org.odk.collect.android.logic.actions.setgeopoint.CollectSetGeopointActionHandler
 import org.odk.collect.android.projects.ProjectsDataService
 import org.odk.collect.entities.javarosa.intance.LocalEntitiesExternalInstanceParserFactory
@@ -49,7 +51,7 @@ class JavaRosaInitializer(
 
         val localEntitiesExternalInstanceParserFactory = LocalEntitiesExternalInstanceParserFactory(
             { entitiesRepositoryProvider.create(projectsDataService.requireCurrentProject().uuid) },
-            { true }
+            ReferenceManagerMediaFileRepository(ReferenceManager.instance())
         )
 
         XFormUtils.setExternalInstanceParserFactory(localEntitiesExternalInstanceParserFactory)

collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt

@@ -178,6 +178,8 @@ object ServerFormUseCases {
                         entitiesRepository,
                         mediaFile
                     )
+
+                    tempMediaFile.delete()
                 } else {
                     val existingForm = formsRepository.getAllByFormIdAndVersion(
                         formToDownload.formId,

collect_app/src/main/java/org/odk/collect/android/javarosawrapper/ReferenceManagerMediaFileRepository.kt

@@ -0,0 +1,18 @@
+package org.odk.collect.android.javarosawrapper
+
+import org.javarosa.core.reference.InvalidReferenceException
+import org.javarosa.core.reference.ReferenceManager
+import org.odk.collect.forms.FormMediaFileRepository
+import java.io.File
+
+class ReferenceManagerMediaFileRepository(
+    private val referenceManager: ReferenceManager
+) : FormMediaFileRepository {
+    override fun get(uri: String): File? {
+        return try {
+            File(referenceManager.deriveReference(uri).localURI)
+        } catch (_: InvalidReferenceException) {
+            null
+        }
+    }
+}

collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt

@@ -0,0 +1,103 @@
+package org.odk.collect.android.entities
+
+import org.hamcrest.MatcherAssert.assertThat
+import org.hamcrest.Matchers.equalTo
+import org.javarosa.core.model.FormDef
+import org.javarosa.core.reference.ReferenceManager
+import org.javarosa.form.api.FormEntryController
+import org.javarosa.form.api.FormEntryModel
+import org.javarosa.test.BindBuilderXFormsElement.bind
+import org.javarosa.test.Scenario
+import org.javarosa.test.XFormsElement.body
+import org.javarosa.test.XFormsElement.head
+import org.javarosa.test.XFormsElement.html
+import org.javarosa.test.XFormsElement.mainInstance
+import org.javarosa.test.XFormsElement.model
+import org.javarosa.test.XFormsElement.select1Dynamic
+import org.javarosa.test.XFormsElement.t
+import org.javarosa.test.XFormsElement.title
+import org.javarosa.xform.parse.ExternalInstanceParser
+import org.javarosa.xform.util.XFormUtils
+import org.junit.After
+import org.junit.Before
+import org.junit.Test
+import org.odk.collect.android.javarosawrapper.ReferenceManagerMediaFileRepository
+import org.odk.collect.android.utilities.FormUtils
+import org.odk.collect.entities.javarosa.filter.LocalEntitiesFilterStrategy
+import org.odk.collect.entities.javarosa.intance.LocalEntitiesExternalInstanceParserFactory
+import org.odk.collect.entities.storage.Entity
+import org.odk.collect.entities.storage.InMemEntitiesRepository
+import org.odk.collect.shared.TempFiles
+import java.io.File
+
+/**
+ * Regression test for https://github.com/getodk/collect/issues/6425.
+ *
+ * When an attached CSV has the same name as an entity list in the project, an external select
+ * backed by that CSV must read from the CSV - not from the entity list. A select with no such
+ * attachment still falls back to the entity list of the same name.
+ */
+
+class ExternalCsvVsEntityListTest {
+    private val entitiesRepository = InMemEntitiesRepository()
+
+    private val controllerSupplier: (FormDef) -> FormEntryController = { formDef ->
+        FormEntryController(FormEntryModel(formDef)).also {
+            it.addFilterStrategy(LocalEntitiesFilterStrategy(entitiesRepository))
+        }
+    }
+
+    @Before
+    fun setup() {
+        XFormUtils.setExternalInstanceParserFactory(
+            LocalEntitiesExternalInstanceParserFactory(
+                { entitiesRepository },
+                ReferenceManagerMediaFileRepository(ReferenceManager.instance())
+            )
+        )
+    }
+
+    @After
+    fun teardown() {
+        XFormUtils.setExternalInstanceParserFactory { ExternalInstanceParser() }
+        ReferenceManager.instance().reset()
+    }
+
+    @Test
+    fun `an external select reads from its attached CSV even when an entity list of the same name exists`() {
+        entitiesRepository.save("households", Entity.New("springfield", "Springfield"))
+        entitiesRepository.save("people", Entity.New("alice", "Alice"))
+
+        val projectRootDir = TempFiles.createTempDir()
+        val formMediaDir = File(projectRootDir, "forms/clashing-names-media").also { it.mkdirs() }
+        File(formMediaDir, "people.csv").writeText("name,label\njohn,John\n")
+
+        FormUtils.setupReferenceManagerForForm(ReferenceManager.instance(), projectRootDir, formMediaDir)
+
+        val form = html(
+            head(
+                title("Select form"),
+                model(
+                    mainInstance(
+                        t("data id=\"select-form\"", t("household"), t("person"))
+                    ),
+                    t("instance id=\"households\" src=\"jr://file-csv/households.csv\""),
+                    t("instance id=\"people\" src=\"jr://file-csv/people.csv\""),
+                    bind("/data/household").type("string"),
+                    bind("/data/person").type("string")
+                )
+            ),
+            body(
+                select1Dynamic("/data/household", "instance('households')/root/item", "name", "label"),
+                select1Dynamic("/data/person", "instance('people')/root/item", "name", "label")
+            )
+        )
+
+        val scenario = Scenario.init(form, controllerSupplier)
+
+        // The household choices come from the entity list.
+        assertThat(scenario.choicesOf("/data/household").map { it.value }, equalTo(listOf("springfield")))
+        // The person choices come from the attached CSV, not the entity list of the same name.
+        assertThat(scenario.choicesOf("/data/person").map { it.value }, equalTo(listOf("john")))
+    }
+}

entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt

@@ -3,18 +3,17 @@ package org.odk.collect.entities.javarosa.intance
 import org.javarosa.xform.parse.ExternalInstanceParser
 import org.javarosa.xform.parse.ExternalInstanceParserFactory
 import org.odk.collect.entities.storage.EntitiesRepository
+import org.odk.collect.forms.FormMediaFileRepository
 
 class LocalEntitiesExternalInstanceParserFactory(
     private val entitiesRepositoryProvider: () -> EntitiesRepository,
-    private val enabled: () -> Boolean
+    private val mediaFileRepository: FormMediaFileRepository
 ) : ExternalInstanceParserFactory {
     override fun getExternalInstanceParser(): ExternalInstanceParser {
-        val parser = ExternalInstanceParser()
-
-        if (enabled()) {
-            parser.addInstanceProvider(LocalEntitiesInstanceProvider(entitiesRepositoryProvider))
+        return ExternalInstanceParser().apply {
+            addInstanceProvider(
+                LocalEntitiesInstanceProvider(entitiesRepositoryProvider, mediaFileRepository)
+            )
         }
-
-        return parser
     }
 }

entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceProvider.kt

@@ -3,9 +3,12 @@ package org.odk.collect.entities.javarosa.intance
 import org.javarosa.core.model.instance.TreeElement
 import org.javarosa.xform.parse.ExternalInstanceParser
 import org.odk.collect.entities.storage.EntitiesRepository
+import org.odk.collect.forms.FormMediaFileRepository
 
-internal class LocalEntitiesInstanceProvider(private val entitiesRepositoryProvider: () -> EntitiesRepository) :
-    ExternalInstanceParser.InstanceProvider {
+internal class LocalEntitiesInstanceProvider(
+    private val entitiesRepositoryProvider: () -> EntitiesRepository,
+    private val mediaFileRepository: FormMediaFileRepository
+) : ExternalInstanceParser.InstanceProvider {
 
     override fun get(instanceId: String, instanceSrc: String): TreeElement {
         return get(instanceId, instanceSrc, false)
@@ -18,6 +21,14 @@ internal class LocalEntitiesInstanceProvider(private val entitiesRepositoryProvi
     }
 
     override fun isSupported(instanceId: String, instanceSrc: String): Boolean {
+        // Entity lists are stored in the database, not as files on disk (their seed CSV is
+        // deleted after import). So if a media file referenced by the instance's src exists,
+        // it must be a plain attached CSV, which should be used instead of any same-named
+        // entity list. Returning false lets JavaRosa parse the file directly.
+        if (mediaFileRepository.get(instanceSrc)?.exists() == true) {
+            return false
+        }
+
         return createDataAdapter().supportsInstance(instanceId)
     }
 

entities/src/test/java/org/odk/collect/entities/javarosa/LocalEntitiesInstanceProviderTest.kt

@@ -7,6 +7,8 @@ import org.odk.collect.entities.javarosa.intance.LocalEntitiesInstanceProvider
 import org.odk.collect.entities.javarosa.parse.EntitySchema
 import org.odk.collect.entities.storage.Entity
 import org.odk.collect.entities.storage.InMemEntitiesRepository
+import org.odk.collect.shared.TempFiles
+import java.io.File
 
 class LocalEntitiesInstanceProviderTest {
 
@@ -22,7 +24,7 @@ class LocalEntitiesInstanceProviderTest {
             )
         entitiesRepository.save("people", entity)
 
-        val parser = LocalEntitiesInstanceProvider { entitiesRepository }
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
         val instance = parser.get("people", "people.csv")
         assertThat(instance.numChildren, equalTo(1))
 
@@ -42,7 +44,7 @@ class LocalEntitiesInstanceProviderTest {
             )
         entitiesRepository.save("people", entity)
 
-        val parser = LocalEntitiesInstanceProvider { entitiesRepository }
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
         val instance = parser.get("people", "people.csv")
         assertThat(instance.numChildren, equalTo(1))
 
@@ -61,7 +63,7 @@ class LocalEntitiesInstanceProviderTest {
             )
         entitiesRepository.save("people", entity)
 
-        val parser = LocalEntitiesInstanceProvider { entitiesRepository }
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
         val instance = parser.get("people", "people.csv")
         assertThat(instance.numChildren, equalTo(1))
 
@@ -80,7 +82,7 @@ class LocalEntitiesInstanceProviderTest {
             )
         entitiesRepository.save("people", entity)
 
-        val parser = LocalEntitiesInstanceProvider { entitiesRepository }
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
         val instance = parser.get("people", "people.csv")
         assertThat(instance.numChildren, equalTo(1))
 
@@ -102,7 +104,7 @@ class LocalEntitiesInstanceProviderTest {
             )
         entitiesRepository.save("people", entity)
 
-        val parser = LocalEntitiesInstanceProvider { entitiesRepository }
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
         val instance = parser.get("people", "people.csv")
         assertThat(instance.numChildren, equalTo(1))
 
@@ -128,7 +130,7 @@ class LocalEntitiesInstanceProviderTest {
         )
         entitiesRepository.save("people", *entity)
 
-        val parser = LocalEntitiesInstanceProvider { entitiesRepository }
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
         val instance = parser.get("people", "people.csv", true)
         assertThat(instance.numChildren, equalTo(2))
 
@@ -158,7 +160,7 @@ class LocalEntitiesInstanceProviderTest {
         val repository = InMemEntitiesRepository()
         repository.save("people", *entities)
 
-        val parser = LocalEntitiesInstanceProvider { repository }
+        val parser = LocalEntitiesInstanceProvider({ repository }) { null }
         val instance = parser.get("people", "people.csv", false)
 
         val first = instance.getChildAt(0)!!
@@ -170,6 +172,32 @@ class LocalEntitiesInstanceProviderTest {
         assertThat(second.multiplicity, equalTo(1))
     }
 
+    @Test
+    fun `isSupported returns true for an existing entity list when no media file is attached`() {
+        entitiesRepository.save("people", Entity.New("1", "Shiv Roy"))
+
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
+        assertThat(parser.isSupported("people", "jr://file-csv/people.csv"), equalTo(true))
+    }
+
+    @Test
+    fun `isSupported returns false when a media file with the same name is attached`() {
+        entitiesRepository.save("people", Entity.New("1", "Shiv Roy"))
+        val attachedFile = TempFiles.createTempFile()
+
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { attachedFile }
+        assertThat(parser.isSupported("people", "jr://file-csv/people.csv"), equalTo(false))
+    }
+
+    @Test
+    fun `isSupported returns true when the resolved media file does not exist`() {
+        entitiesRepository.save("people", Entity.New("1", "Shiv Roy"))
+        val missingFile = File(TempFiles.getPathInTempDir("missing", ".csv"))
+
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { missingFile }
+        assertThat(parser.isSupported("people", "jr://file-csv/people.csv"), equalTo(true))
+    }
+
     @Test
     fun `includes blank label version when it is null`() {
         val entity =
@@ -179,7 +207,7 @@ class LocalEntitiesInstanceProviderTest {
             )
         entitiesRepository.save("people", entity)
 
-        val parser = LocalEntitiesInstanceProvider { entitiesRepository }
+        val parser = LocalEntitiesInstanceProvider({ entitiesRepository }) { null }
         val instance = parser.get("people", "people.csv")
         assertThat(instance.numChildren, equalTo(1))
 

entities/src/test/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategyTest.kt

@@ -43,7 +43,7 @@ class LocalEntitiesFilterStrategyTest {
     private val entitiesRepository = InMemEntitiesRepository()
     private val fallthroughFilterStrategy = FallthroughFilterStrategy()
     private val instanceProvider =
-        SpyInstanceProvider(LocalEntitiesInstanceProvider(::entitiesRepository))
+        SpyInstanceProvider(LocalEntitiesInstanceProvider(::entitiesRepository) { null })
 
     private val controllerSupplier: (FormDef) -> FormEntryController = { formDef ->
         FormEntryController(FormEntryModel(formDef)).also {

forms/src/main/java/org/odk/collect/forms/FormMediaFileRepository.kt

@@ -0,0 +1,15 @@
+package org.odk.collect.forms
+
+import java.io.File
+
+/**
+ * Resolves a form-media reference (a `jr://…` URI as used by a form's images, audio, video or
+ * CSV external instances) to a local file.
+ */
+fun interface FormMediaFileRepository {
+    /**
+     * @return the local file the [uri] resolves to, or null if the reference can't be resolved.
+     * The returned file is not guaranteed to exist.
+     */
+    fun get(uri: String): File?
+}