From a064714eaae3d06f4022afa49a4441696d72f95d Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Tue, 9 Jun 2026 12:31:09 +0200 Subject: [PATCH 1/4] Fix attached CSV being shadowed by entity list of the same name --- .../initialization/JavaRosaInitializer.kt | 3 +++ .../ReferenceManagerMediaFileRepository.kt | 18 ++++++++++++++++++ ...calEntitiesExternalInstanceParserFactory.kt | 6 +++++- .../intance/LocalEntitiesInstanceProvider.kt | 15 +++++++++++++-- .../collect/forms/FormMediaFileRepository.kt | 15 +++++++++++++++ 5 files changed, 54 insertions(+), 3 deletions(-) create mode 100644 collect_app/src/main/java/org/odk/collect/android/javarosawrapper/ReferenceManagerMediaFileRepository.kt create mode 100644 forms/src/main/java/org/odk/collect/forms/FormMediaFileRepository.kt diff --git a/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt b/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt index a062e4e7f32..9850656dde0 100644 --- a/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt +++ b/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt @@ -1,6 +1,7 @@ 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 @@ -8,6 +9,7 @@ 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,6 +51,7 @@ class JavaRosaInitializer( val localEntitiesExternalInstanceParserFactory = LocalEntitiesExternalInstanceParserFactory( { entitiesRepositoryProvider.create(projectsDataService.requireCurrentProject().uuid) }, + ReferenceManagerMediaFileRepository(ReferenceManager.instance()), { true } ) diff --git a/collect_app/src/main/java/org/odk/collect/android/javarosawrapper/ReferenceManagerMediaFileRepository.kt b/collect_app/src/main/java/org/odk/collect/android/javarosawrapper/ReferenceManagerMediaFileRepository.kt new file mode 100644 index 00000000000..cd0219e1461 --- /dev/null +++ b/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 + } + } +} diff --git a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt b/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt index c536e90f0ec..3612d441ee0 100644 --- a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt +++ b/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt @@ -3,16 +3,20 @@ 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 mediaFileRepository: FormMediaFileRepository, private val enabled: () -> Boolean ) : ExternalInstanceParserFactory { override fun getExternalInstanceParser(): ExternalInstanceParser { val parser = ExternalInstanceParser() if (enabled()) { - parser.addInstanceProvider(LocalEntitiesInstanceProvider(entitiesRepositoryProvider)) + parser.addInstanceProvider( + LocalEntitiesInstanceProvider(entitiesRepositoryProvider, mediaFileRepository) + ) } return parser diff --git a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceProvider.kt b/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceProvider.kt index 24b7f53e0b7..bcb9ba65677 100644 --- a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceProvider.kt +++ b/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) } diff --git a/forms/src/main/java/org/odk/collect/forms/FormMediaFileRepository.kt b/forms/src/main/java/org/odk/collect/forms/FormMediaFileRepository.kt new file mode 100644 index 00000000000..75051cad6c3 --- /dev/null +++ b/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? +} From c069f97c716dff3ab1747a2ef4fd905feff0b44a Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Tue, 9 Jun 2026 12:31:55 +0200 Subject: [PATCH 2/4] Add new tests and fix existing ones --- .../entities/ExternalCsvVsEntityListTest.kt | 104 ++++++++++++++++++ .../LocalEntitiesInstanceProviderTest.kt | 44 ++++++-- .../filter/LocalEntitiesFilterStrategyTest.kt | 2 +- 3 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt diff --git a/collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt b/collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt new file mode 100644 index 00000000000..55b3df8c604 --- /dev/null +++ b/collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt @@ -0,0 +1,104 @@ +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()), + { true } + ) + ) + } + + @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"))) + } +} diff --git a/entities/src/test/java/org/odk/collect/entities/javarosa/LocalEntitiesInstanceProviderTest.kt b/entities/src/test/java/org/odk/collect/entities/javarosa/LocalEntitiesInstanceProviderTest.kt index 55a14128a90..5ea3105d8b0 100644 --- a/entities/src/test/java/org/odk/collect/entities/javarosa/LocalEntitiesInstanceProviderTest.kt +++ b/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)) diff --git a/entities/src/test/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategyTest.kt b/entities/src/test/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategyTest.kt index ae5ec7f7906..c8065cdb6c5 100644 --- a/entities/src/test/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategyTest.kt +++ b/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 { From 81f96e13ed91896192453a6224f13d806813db62 Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Tue, 9 Jun 2026 12:44:41 +0200 Subject: [PATCH 3/4] Remove redundant enabled flag from LocalEntitiesExternalInstanceParserFactory --- .../application/initialization/JavaRosaInitializer.kt | 3 +-- .../android/entities/ExternalCsvVsEntityListTest.kt | 3 +-- .../LocalEntitiesExternalInstanceParserFactory.kt | 11 +++-------- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt b/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt index 9850656dde0..1d0d23c338c 100644 --- a/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt +++ b/collect_app/src/main/java/org/odk/collect/android/application/initialization/JavaRosaInitializer.kt @@ -51,8 +51,7 @@ class JavaRosaInitializer( val localEntitiesExternalInstanceParserFactory = LocalEntitiesExternalInstanceParserFactory( { entitiesRepositoryProvider.create(projectsDataService.requireCurrentProject().uuid) }, - ReferenceManagerMediaFileRepository(ReferenceManager.instance()), - { true } + ReferenceManagerMediaFileRepository(ReferenceManager.instance()) ) XFormUtils.setExternalInstanceParserFactory(localEntitiesExternalInstanceParserFactory) diff --git a/collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt b/collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt index 55b3df8c604..e16cb0831ea 100644 --- a/collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt +++ b/collect_app/src/test/java/org/odk/collect/android/entities/ExternalCsvVsEntityListTest.kt @@ -52,8 +52,7 @@ class ExternalCsvVsEntityListTest { XFormUtils.setExternalInstanceParserFactory( LocalEntitiesExternalInstanceParserFactory( { entitiesRepository }, - ReferenceManagerMediaFileRepository(ReferenceManager.instance()), - { true } + ReferenceManagerMediaFileRepository(ReferenceManager.instance()) ) ) } diff --git a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt b/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt index 3612d441ee0..7d087e43735 100644 --- a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt +++ b/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesExternalInstanceParserFactory.kt @@ -7,18 +7,13 @@ import org.odk.collect.forms.FormMediaFileRepository class LocalEntitiesExternalInstanceParserFactory( private val entitiesRepositoryProvider: () -> EntitiesRepository, - private val mediaFileRepository: FormMediaFileRepository, - private val enabled: () -> Boolean + private val mediaFileRepository: FormMediaFileRepository ) : ExternalInstanceParserFactory { override fun getExternalInstanceParser(): ExternalInstanceParser { - val parser = ExternalInstanceParser() - - if (enabled()) { - parser.addInstanceProvider( + return ExternalInstanceParser().apply { + addInstanceProvider( LocalEntitiesInstanceProvider(entitiesRepositoryProvider, mediaFileRepository) ) } - - return parser } } From 01dbdc159143ead8885ae9a1387270f705abbc14 Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Tue, 9 Jun 2026 23:46:53 +0200 Subject: [PATCH 4/4] Don't keep entity list CSVs in the form media directory after processing --- .../odk/collect/android/formmanagement/ServerFormUseCases.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt b/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt index 80716b7f7c0..eb5df19f1a4 100644 --- a/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormUseCases.kt +++ b/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,