Skip to content
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.

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

Include primary instance root attributes, namespace declarations in submission XML #287

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eyelidlessness
Copy link
Member

@eyelidlessness eyelidlessness commented Jan 21, 2025

Closes #286.

I have verified this PR works in these browsers (latest versions):

  • Chrome
  • Firefox
  • Safari (macOS)
  • Safari (iOS)
  • Not applicable

What else has been done to verify that this works as intended?

  • Added tests for attributes
  • Updated tests for XML namespace declarations

Why is this the best possible solution? Were any other approaches considered?

It'd be better to have a more general solution for both attributes and namespace declarations.

Both would involve significant work on both parsing (to capture attributes and namespace prefixes/declarations throughout the primary instance subtree), as well as the primary instance state data model (as we don't have a concept of attributes, and presently ignore non-default-namespace element names).

I'd be remiss if I failed to mention that there would have been an even hacker way to deal with namespace declarations: abuse built-in DOM serialization (as it functionally does the same thing to derive namespaces as this change does more explicitly). I didn't do this because:

  • It'd be a lot less clear what's happening and why
  • I don't think it's standardized in any way, and I wouldn't be surprised if jsdom/any other DOM-compat library would make that technique unreliable

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It's possible that even the simplified solution has ignored some edge case, which would be most likely around namespaces. I think I was fairly thorough, but the parse-stage collection of namespaces in a temporary map would be the most likely suspect.

It's also possible some logical error might have introduced subtle fallibility. I'm hopeful that letting the type system guide the change avoided most if not all of that risk.

Do we need any specific form for testing your changes? If so, please attach one.

I think any form will do?

What's changed

  • Static attributes on the primary instance root element are parsed, and re-serialized on submission
  • Namespace declarations are parsed (or derived) on the primary instance root element, also re-serialized on submission

Copy link

changeset-bot bot commented Jan 21, 2025

🦋 Changeset detected

Latest commit: b576ec4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@getodk/xforms-engine Patch
@getodk/web-forms Patch
@getodk/scenario Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@eyelidlessness eyelidlessness marked this pull request as ready for review January 21, 2025 23:37
@eyelidlessness eyelidlessness changed the title engine (fix): include primary instance root attributes in submission XML Include primary instance root attributes, namespace declarations in submission XML Jan 21, 2025
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

I should have checked what Collect and Enketo do with namespaces in submissions before commenting! Neither includes the default namespace declaration. I don't think it will break any existing systems to include but it does add more noise.

I'm inclined to go with this for now and come back to it if we see any issues. Alternately, we could special case the default namespace, maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass through static id and version attributes on primary instance
2 participants