Merge pull request #6645 from seadowg/main-instance

seadowg · web-flow · commit b71702326aa8 · 2025-03-19T16:29:58.000Z
Fix metadata parsing for main instances with attributes
diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/FormDownloadException.kt b/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/FormDownloadException.kt
@@ -5,7 +5,7 @@ import org.odk.collect.forms.FormSourceException
 sealed class FormDownloadException : Exception() {
     class DownloadingInterrupted : FormDownloadException()
     class FormWithNoHash : FormDownloadException()
-    class FormParsingError : FormDownloadException()
+    class FormParsingError(val original: Exception) : FormDownloadException()
     class DiskError : FormDownloadException()
     class InvalidSubmission : FormDownloadException()
     class FormSourceError(val exception: FormSourceException) : FormDownloadException()
diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/FormDownloadExceptionMapper.kt b/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/FormDownloadExceptionMapper.kt
@@ -15,11 +15,9 @@ class FormDownloadExceptionMapper(private val context: Context) {
                 )
             }
             is FormDownloadException.FormParsingError -> {
-                context.getLocalizedString(
-                    org.odk.collect.strings.R.string.form_parsing_error
-                ) + " " + context.getLocalizedString(
-                    org.odk.collect.strings.R.string.report_to_project_lead
-                )
+                context.getLocalizedString(org.odk.collect.strings.R.string.form_parsing_error) +
+                    "\n\n${exception.original.getStackTraceString(1)}\n\n" +
+                    context.getLocalizedString(org.odk.collect.strings.R.string.report_to_project_lead)
             }
             is FormDownloadException.DiskError -> {
                 context.getLocalizedString(
@@ -43,4 +41,9 @@ class FormDownloadExceptionMapper(private val context: Context) {
             }
         }
     }
+
+    private fun Exception.getStackTraceString(lines: Int): String {
+        val stackTrace = this.stackTrace
+        return "${this.message}\n${stackTrace.take(lines).joinToString("\n") { it.toString() }}"
+    }
 }
diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/ServerFormDownloader.java b/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/ServerFormDownloader.java
@@ -128,7 +128,7 @@ private void processOneForm(ServerFormDetails fd, OngoingWorkListener stateListe
 
                 Timber.i("Parse finished in %.3f seconds.", (System.currentTimeMillis() - start) / 1000F);
             } catch (RuntimeException e) {
-                throw new FormDownloadException.FormParsingError();
+                throw new FormDownloadException.FormParsingError(e);
             }
         }
 
diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/metadata/FormMetadataParser.kt b/collect_app/src/main/java/org/odk/collect/android/formmanagement/metadata/FormMetadataParser.kt
@@ -19,21 +19,10 @@ object FormMetadataParser {
         val body = doc.getRootElement().getElement(null, "body")
         val title = head.getElement(null, "title").getChild(0).toString()
 
-        lateinit var mainInstanceRoot: Element
-        var submission: Element? = null
-        for (i in 0 until model.childCount) {
-            val child = model.getElement(i) ?: continue
-
-            if (child.name == "instance" && child.attributeCount == 0) {
-                for (j in 0 until child.childCount) {
-                    val mainInstanceChild = child.getElement(j) ?: continue
-                    mainInstanceRoot = mainInstanceChild
-                    break
-                }
-            } else if (child.name == "submission") {
-                submission = child
-            }
-        }
+        val modelElements = getChildren(model)
+        val mainInstance = modelElements.first { it.name == "instance" }
+        val mainInstanceRoot = getChildren(mainInstance).first()
+        val submission = modelElements.firstOrNull { it.name == "submission" }
 
         val id = mainInstanceRoot.getAttributeValue(null, "id")
         val version = mainInstanceRoot.getAttributeValue(null, "version")
@@ -57,6 +46,17 @@ object FormMetadataParser {
         )
     }
 
+    private fun getChildren(element: Element): List<Element> {
+        return 0.until(element.childCount).fold(emptyList()) { list, i ->
+            val child = element.getElement(i)
+            if (child != null) {
+                list + listOf(child)
+            } else {
+                list
+            }
+        }
+    }
+
     /**
      * Finds the first geopoint reference in the primary instance by:
      * 1. Retrieving all geopoint binds from the model.
diff --git a/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/FormDownloadExceptionMapperTest.kt b/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/FormDownloadExceptionMapperTest.kt
@@ -4,6 +4,7 @@ import android.content.Context
 import androidx.test.core.app.ApplicationProvider
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import org.hamcrest.MatcherAssert.assertThat
+import org.hamcrest.Matchers.equalTo
 import org.hamcrest.Matchers.`is`
 import org.junit.Before
 import org.junit.Test
@@ -33,12 +34,20 @@ class FormDownloadExceptionMapperTest {
 
     @Test
     fun formParsingError_returnsFormParsingErrorMessage() {
-        val expectedString = context.getString(
-            org.odk.collect.strings.R.string.form_parsing_error
-        ) + " " + context.getString(org.odk.collect.strings.R.string.report_to_project_lead)
+        val original = Exception("original message")
+        original.stackTrace = arrayOf(
+            StackTraceElement("Class", "method", "File", 1),
+            StackTraceElement("Class", "method", "File", 2)
+        )
+
         assertThat(
-            mapper.getMessage(FormDownloadException.FormParsingError()),
-            `is`(expectedString)
+            mapper.getMessage(FormDownloadException.FormParsingError(original)),
+            equalTo(
+                context.getString(org.odk.collect.strings.R.string.form_parsing_error) + "\n\n" +
+                    original.message + "\n" +
+                    "Class.method(File:1)" + "\n\n" +
+                    context.getString(org.odk.collect.strings.R.string.report_to_project_lead)
+            )
         )
     }
 
diff --git a/collect_app/src/test/java/org/odk/collect/android/formmanagement/metadata/FormMetadataParserTest.kt b/collect_app/src/test/java/org/odk/collect/android/formmanagement/metadata/FormMetadataParserTest.kt
@@ -6,6 +6,58 @@ import org.junit.Test
 import org.odk.collect.android.formmanagement.metadata.FormMetadataParser.readMetadata
 
 class FormMetadataParserTest {
+
+    @Test
+    fun readMetadata_canParseWithAttributesOnMainInstance() {
+        val metadata = readMetadata(
+            """
+                <?xml version="1.0"?>
+                <h:html xmlns:h="http://www.w3.org/1999/xhtml"
+                        xmlns="http://www.w3.org/2002/xforms"
+                        xmlns:custom="http://example.com/custom">
+                    <h:head>
+                        <h:title>Form</h:title>
+                        <model>
+                            <instance custom:attribute="blah">
+                                <data id="form">
+                                </data>
+                            </instance>
+                        </model>
+                    </h:head>
+                    <h:body>
+                    </h:body>
+                </h:html>
+            """.trimIndent().byteInputStream()
+        )
+
+        assertThat(metadata.id, equalTo("form"))
+    }
+
+    @Test
+    fun readMetadata_canParseWithXformsNamespace() {
+        val metadata = readMetadata(
+            """
+                <?xml version="1.0"?>
+                <h:html xmlns:h="http://www.w3.org/1999/xhtml"
+                        xmlns:custom="http://www.w3.org/2002/xforms">
+                    <h:head>
+                        <h:title>Form</h:title>
+                        <custom:model>
+                            <custom:instance>
+                                <custom:data custom:id="form">
+                                </custom:data>
+                            </custom:instance>
+                        </custom:model>
+                    </h:head>
+                    <h:body>
+                    </h:body>
+                </h:html>
+            """.trimIndent().byteInputStream()
+        )
+
+        assertThat(metadata.id, equalTo("form"))
+    }
+
     @Test
     fun readMetadata_canParseFormsWithComments() {
         readMetadata(
diff --git a/collect_app/src/test/java/org/odk/collect/android/utilities/FormsDownloadResultInterpreterTest.kt b/collect_app/src/test/java/org/odk/collect/android/utilities/FormsDownloadResultInterpreterTest.kt
@@ -25,7 +25,7 @@ class FormsDownloadResultInterpreterTest {
 
     private var resultWithOneError = mapOf<ServerFormDetails, FormDownloadException?>(
         formDetails1 to null,
-        formDetails2 to FormDownloadException.FormParsingError()
+        formDetails2 to FormDownloadException.FormParsingError(RuntimeException())
     )
 
     @Test
diff --git a/strings/src/main/res/values/strings.xml b/strings/src/main/res/values/strings.xml
@@ -748,7 +748,7 @@
     <string name="form_with_no_hash_error">The server did not provide a hash for this form.</string>
 
     <!-- Displayed in error dialog for form downloads. Followed by instructions to talk to person who asked to collect data -->
-    <string name="form_parsing_error">This form cannot be processed.</string>
+    <string name="form_parsing_error">This form cannot be processed:</string>
 
     <!-- Displayed in error dialog for form downloads. Followed by instructions to talk to person who asked to collect data -->
     <string name="form_save_disk_error">A disk error occurred on device while downloading this form.</string>

Original file line number	Diff line number	Diff line change
`@@ -15,11 +15,9 @@ class FormDownloadExceptionMapper(private val context: Context) {`
`15`	`15`	`)`
`16`	`16`	`}`
`17`	`17`	`is FormDownloadException.FormParsingError -> {`
`18`		`- context.getLocalizedString(`
`19`		`- org.odk.collect.strings.R.string.form_parsing_error`
`20`		`- ) + " " + context.getLocalizedString(`
`21`		`- org.odk.collect.strings.R.string.report_to_project_lead`
`22`		`- )`
	`18`	`+ context.getLocalizedString(org.odk.collect.strings.R.string.form_parsing_error) +`
	`19`	`+ "\n\n${exception.original.getStackTraceString(1)}\n\n" +`
	`20`	`+ context.getLocalizedString(org.odk.collect.strings.R.string.report_to_project_lead)`
`23`	`21`	`}`
`24`	`22`	`is FormDownloadException.DiskError -> {`
`25`	`23`	`context.getLocalizedString(`
`@@ -43,4 +41,9 @@ class FormDownloadExceptionMapper(private val context: Context) {`
`43`	`41`	`}`
`44`	`42`	`}`
`45`	`43`	`}`
	`44`	`+`
	`45`	`+ private fun Exception.getStackTraceString(lines: Int): String {`
	`46`	`+ val stackTrace = this.stackTrace`
	`47`	`+ return "${this.message}\n${stackTrace.take(lines).joinToString("\n") { it.toString() }}"`
	`48`	`+ }`
`46`	`49`	`}`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ private void processOneForm(ServerFormDetails fd, OngoingWorkListener stateListe`
`128`	`128`
`129`	`129`	`Timber.i("Parse finished in %.3f seconds.", (System.currentTimeMillis() - start) / 1000F);`
`130`	`130`	`} catch (RuntimeException e) {`
`131`		`- throw new FormDownloadException.FormParsingError();`
	`131`	`+ throw new FormDownloadException.FormParsingError(e);`
`132`	`132`	`}`
`133`	`133`	`}`
`134`	`134`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@ class FormsDownloadResultInterpreterTest {`
`25`	`25`
`26`	`26`	`private var resultWithOneError = mapOf<ServerFormDetails, FormDownloadException?>(`
`27`	`27`	`formDetails1 to null,`
`28`		`- formDetails2 to FormDownloadException.FormParsingError()`
	`28`	`+ formDetails2 to FormDownloadException.FormParsingError(RuntimeException())`
`29`	`29`	`)`
`30`	`30`
`31`	`31`	`@Test`