Skip to content

Fix reading form metadata in forms containing multiple meta sections #7141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
seadowg merged 5 commits into from
Mar 18, 2026
Merged

Fix reading form metadata in forms containing multiple meta sections #7141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2b0a6e
Add a failing test
grzesiek2010 Mar 17, 2026
3431f99
Use the top-level meta block to read metadata
grzesiek2010 Mar 16, 2026
cb61789
Remove redundant code
grzesiek2010 Mar 16, 2026
34d6ff3
Improve variable names
grzesiek2010 Mar 16, 2026
03b73ff
Convert InstanceMetadata to kotlin
grzesiek2010 Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions collect_app/src/main/java/org/odk/collect/android/javarosawrapper/InstanceMetadata.java
View file
Open in desktop

This file was deleted.

12 changes: 12 additions & 0 deletions collect_app/src/main/java/org/odk/collect/android/javarosawrapper/InstanceMetadata.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package org.odk.collect.android.javarosawrapper

import org.odk.collect.android.formentry.audit.AuditConfig
import org.odk.collect.android.utilities.FormNameUtils

data class InstanceMetadata(
@JvmField val instanceId: String?,
private val rawInstanceName: String?,
@JvmField val auditConfig: AuditConfig?
) {
@JvmField val instanceName: String? = FormNameUtils.normalizeFormName(rawInstanceName, false)
}
59 changes: 12 additions & 47 deletions ...ect_app/src/main/java/org/odk/collect/android/javarosawrapper/JavaRosaFormController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,76 +1010,41 @@ public ByteArrayPayload getSubmissionXml() throws IOException {
getSubmissionDataReference());
}

/**
* Traverse the submission looking for the first matching tag in depth-first order.
*/
private TreeElement findDepthFirst(TreeElement parent, String name) {
int len = parent.getNumChildren();
for (int i = 0; i < len; ++i) {
TreeElement e = parent.getChildAt(i);
if (name.equals(e.getName())) {
return e;
} else if (e.getNumChildren() != 0) {
TreeElement v = findDepthFirst(e, name);
if (v != null) {
return v;
}
}
}
return null;
}

public InstanceMetadata getSubmissionMetadata() {
FormDef formDef = formEntryController.getModel().getForm();
TreeElement rootElement = formDef.getInstance().getRoot();

TreeElement trueSubmissionElement;
// Determine the information about the submission...
SubmissionProfile p = formDef.getSubmissionProfile();
Copy link
Copy Markdown
Member Author

@grzesiek2010 grzesiek2010 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this needed here in any way? I can’t really imagine a use case, but maybe I’m missing something.

Copy link
Copy Markdown
Member

@seadowg seadowg Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the change here very explicit, Collect previously allowed the "meta block" to be defined anywhere within the primary instance. This PR changes that so that it's only supported if it is a direct child of the primary instance. Is that your understanding @grzesiek2010?

As far as I can see, the spec is actually a little ambiguous on where the meta block should live. All the examples show it as a direct child, but that requirement isn't really stated explicitly. The closes we get to that is probably:

The primary instance also includes a special type of nodes for metadata inside the block. See the Metadata section

"contains" isn't really specific enough to make this change, but I suspect the intention in the spec is that the meta block should be a direct child. I'd like to get second opinions from @lindsay-stevens or @lognaturel.

Copy link
Copy Markdown
Member Author

@grzesiek2010 grzesiek2010 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes that so that it's only supported if it is a direct child of the primary instance. Is that your understanding @grzesiek2010?

Kind of, I mean yes, and that's why I got rid of findDepthFirst function, but here the question was more about using SubmissionProfile. Whether it was needed for something or not.

Copy link
Copy Markdown
Member Author

@grzesiek2010 grzesiek2010 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, the spec is actually a little ambiguous on where the meta block should live. All the examples show it as a direct child, but that requirement isn't really stated explicitly.

This PR is not the first where we assume the main meta block (with instanceId, instanceName, audit) is the direct child, see:
https://github.com/getodk/collect/blob/master/collect_app/src/main/java/org/odk/collect/android/formmanagement/finalization/EditedFormFinalizationProcessor.kt#L23

Copy link
Copy Markdown
Member

@seadowg seadowg Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Ok so if there is a problem with that, it will already exist. I'd still like to hear from others (so we can at least make the spec clearer), but this is good to go.

Copy link
Copy Markdown
Member

@lognaturel lognaturel Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting, I'd never noticed that ambiguity. I assume the original intent was indeed the direct meta child, that's certainly an important assumption for the whole way the multiple Entity declaration spec works. I think we should make the spec more specific.

Copy link
Copy Markdown
Contributor

@lindsay-stevens lindsay-stevens Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pyxform is currently doing as expected but I've filed XLSForm/pyxform#831 to improve test coverage. The spec examples show the main meta element being a child of the primary data instance root element (usually data), but it wouldn't hurt to also say that in the text, and perhaps also that any other meta elements (expected for entities) must only have entities in them (though meta isn't currently a reserved name so that might be a stretch).

Copy link
Copy Markdown
Member

@seadowg seadowg Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I can get a PR up for the spec then.

if (p == null || p.getRef() == null) {
trueSubmissionElement = rootElement;
} else {
IDataReference ref = p.getRef();
trueSubmissionElement = formDef.getInstance().resolveReference(ref);
// resolveReference returns null if the reference is to the root element...
if (trueSubmissionElement == null) {
trueSubmissionElement = rootElement;
}
}

// and find the depth-first meta block in this...
TreeElement e = findDepthFirst(trueSubmissionElement, "meta");
TreeElement meta = rootElement.getFirstChild("meta");

String instanceId = null;
String instanceName = null;
AuditConfig auditConfig = null;

if (e != null) {
List<TreeElement> v;
if (meta != null) {
List<TreeElement> metaElements;

// instance id...
v = e.getChildrenWithName(INSTANCE_ID);
if (v.size() == 1) {
IAnswerData sa = v.get(0).getValue();
metaElements = meta.getChildrenWithName(INSTANCE_ID);
if (metaElements.size() == 1) {
IAnswerData sa = metaElements.get(0).getValue();
if (sa != null) {
instanceId = sa.getDisplayText();
}
}

// instance name...
v = e.getChildrenWithName(INSTANCE_NAME);
if (v.size() == 1) {
IAnswerData sa = v.get(0).getValue();
metaElements = meta.getChildrenWithName(INSTANCE_NAME);
if (metaElements.size() == 1) {
IAnswerData sa = metaElements.get(0).getValue();
if (sa != null) {
instanceName = sa.getDisplayText();
}
}

// timing element...
v = e.getChildrenWithName(AUDIT);
if (v.size() == 1) {
metaElements = meta.getChildrenWithName(AUDIT);
if (metaElements.size() == 1) {

TreeElement auditElement = v.get(0);
TreeElement auditElement = metaElements.get(0);

String locationPriority = auditElement.getBindAttributeValue(XML_OPENDATAKIT_NAMESPACE, "location-priority");
String locationMinInterval = auditElement.getBindAttributeValue(XML_OPENDATAKIT_NAMESPACE, "location-min-interval");
Expand Down
42 changes: 42 additions & 0 deletions collect_app/src/test/java/org/odk/collect/android/javarosawrapper/FormControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ public void whenInstanceFileAndAuditConfigNull_getAuditEventLogger_isNotNull() t
assertThat(formController.getAuditEventLogger(), notNullValue());
}

@Test
public void getSubmissionMetadata_usesTopLevelMeta_whenMultipleMetaSectionsExist() throws Exception {
FormController formController = createFormController(MULTIPLE_META_SECTIONS);
assertThat(formController.getSubmissionMetadata().instanceName, is(notNullValue()));
}

//region indexIsInFieldList
@Test
Expand Down Expand Up @@ -149,6 +154,7 @@ private FormController createFormController(String xform) throws IOException, XF
ByteArrayInputStream inputStream = new ByteArrayInputStream(xform.getBytes());
final FormEntryModel fem = new FormEntryModel(XFormUtils.getFormFromInputStream(inputStream));
final FormEntryController formEntryController = new FormEntryController(fem);
formEntryController.getModel().getForm().initialize(true, null);
return new JavaRosaFormController(Files.createTempDir(), formEntryController, File.createTempFile("instance", ""));
}

Expand Down Expand Up @@ -293,4 +299,40 @@ private FormController createFormController(String xform) throws IOException, XF
" </group>\n" +
" </h:body>\n" +
"</h:html>\n";

private static final String MULTIPLE_META_SECTIONS = "<?xml version=\"1.0\"?>\n" +
"<h:html xmlns=\"http://www.w3.org/2002/xforms\" xmlns:h=\"http://www.w3.org/1999/xhtml\" xmlns:entities=\"http://www.opendatakit.org/xforms/entities\" xmlns:jr=\"http://openrosa.org/javarosa\">\n" +
" <h:head>\n" +
" <h:title>Multiple meta sections</h:title>\n" +
" <model entities:entities-version=\"2024.1.0\">\n" +
" <instance>\n" +
" <data id=\"multiple-meta-sections\">\n" +
" <group>\n" +
" <question>xxx</question>\n" +
" <meta>\n" +
" <entity dataset=\"entity_test\" create=\"1\" id=\"\">\n" +
" <label/>\n" +
" </entity>\n" +
" </meta>\n" +
" </group>\n" +
" <meta>\n" +
" <instanceName/>\n" +
" </meta>\n" +
" </data>\n" +
" </instance>\n" +
" <bind nodeset=\"/data/group/question\" type=\"int\"/>\n" +
" <bind nodeset=\"/data/group/meta/entity/@id\" readonly=\"true()\" type=\"string\"/>\n" +
" <setvalue ref=\"/data/group/meta/entity/@id\" event=\"odk-instance-first-load\" value=\"uuid()\"/>\n" +
" <bind nodeset=\"/data/group/meta/entity/label\" calculate=\"/data/group/question\" readonly=\"true()\" type=\"string\"/>\n" +
" <bind nodeset=\"/data/meta/instanceName\" type=\"string\" calculate=\"uuid()\"/>\n" +
" </model>\n" +
" </h:head>\n" +
" <h:body>\n" +
" <group ref=\"/data/group\">\n" +
" <input ref=\"/data/group/question\">\n" +
" <label>Question</label>\n" +
" </input>\n" +
" </group>\n" +
" </h:body>\n" +
"</h:html>\n";
}