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..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 @@ -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,7 +51,7 @@ class JavaRosaInitializer( val localEntitiesExternalInstanceParserFactory = LocalEntitiesExternalInstanceParserFactory( { entitiesRepositoryProvider.create(projectsDataService.requireCurrentProject().uuid) }, - { true } + ReferenceManagerMediaFileRepository(ReferenceManager.instance()) ) XFormUtils.setExternalInstanceParserFactory(localEntitiesExternalInstanceParserFactory) 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, 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/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..e16cb0831ea --- /dev/null +++ b/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"))) + } +} 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..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 @@ -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 } } 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/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 { 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? +}