-
Notifications
You must be signed in to change notification settings - Fork 11
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
Initial engine support for submissions #226
Merged
eyelidlessness
merged 18 commits into
main
from
features/engine/submission-serialization
Oct 21, 2024
Merged
Initial engine support for submissions #226
eyelidlessness
merged 18 commits into
main
from
features/engine/submission-serialization
Oct 21, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 8edf375 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
This is essentially the design originally proposed in #188, with the following modifications: - Add a `SubmissionResult.status` and corresponding `SubmissionResult` variant, to convey submission data with attachments exceeding a client-specified `maxSize` - Add corresponding `MaxSizeViolation` interface for that violation scenario… - … including a todo discussing potential benefit of moving `maxSize` configuration to form init; allowing potential to surface node-level max size violations during form filling, rather than only at submission time - Account for controlled/uncontrolled repeat range variants, which was merged while the design has been in flight - As discussed in the design thread, revise `RootNode.prepareSubmission` to return a `Promise` - Add clarifying JSDoc detail to `RootNode.prepareSubmission`, largely expanding on reasoning behind `SubmissionResult` statuses (and production of submission result in “error” states), and reasoning for revision to return a `Promise`
This would be more suitable for `@getodk/common`, except that it makes assumptions about serializing attributes. Note that we don’t currently serialize attributes, but the base implementation already handles the difference so the only additional work here was documenting and testing the behavior.
…state This intermediate state will be: - client-reactive - derived from other client-facing state/interfaces (so in theory a client could implement this directly; ui-solid has a partial implementation already) This aspect of the client API isn’t part of the core design we decided on, but it will both support that design as well as give some clear assurances that we want the functionality to be (and remain) client-reactive. In the future, we may determine it’s safe to make all submission functionality synchronous, in which case we can make it all client-reactive as well. This would form the basis for that. In contrast, if we decide not to make submission functionality client-reactive at the API level, we may want to consider folding the `submissionXML` accessor into `BaseNodeState`. Challenges with that are noted here. It is certainly conceivable (we have one other derived client-reactive state field: `children`). But it doesn’t feel like the most valuable yak to shave for actually getting the serialization aspect working.
The functionality has been validated in both `web-forms` and `ui-solid`. But automated tests will further help to ensure the behavior, as well as the reactive contract. This commit originally included unit-ish level tests within `@getodk/xforms-engine`. They have been removed for reasons discussed in the later commit expanding scenario coverage.
This drops the custom serialization previously used as a part of the ui-solid demo. (The functionality was also partially broken: it serialized repeat ranges as elements. This helped guide testing in the prior commit.)
This just adds a new layer of inheritance for the same `Comparable*` base functionality, allowing for non-“answer” values to use the same comparison/assertion extension facilities.
eyelidlessness
force-pushed
the
features/engine/submission-serialization
branch
from
September 27, 2024 18:49
ddf6687
to
5da1f1b
Compare
Includes custom assertions for XML comparison. This is also intended to support: - re-using the XForms DSL to assert expected serializations - comparing equivalent XML structures which may have slightly different formatting Tests serialization of: - default values - runtime-assigned values - repeats (ranges produce their children) - non-relevant nodes (i.e. that they are omitted) - normalized unicode (see comment for rationale) A subsequent commit may expand the suite further to exercise the client-reactive aspects of the engine’s submission serialization API.
This is being introduced now to support testing client reactivity of submission serialization, since we’ve discussed that being a desirable quality to have ready should it make sense to leverage. There are probably dozens if not hundreds of other existing/future test cases where we’d want better coverage of client reactivity at the integration level. This should hopefully be general enough to allow for that as bandwidth allows.
Exercises: - Basic leaf node value state changes - Repeats: - Adding instances - State changes in each instance - Removing instances - Relevance changes within multiple repeat instances Note: these tests (and all the others added to this suite as part of the submission engine API implementation) had originally been written as lower level (unit-ish) tests within `@getodk/xforms-engine`. Testing reactivity there unfortunately resurfaced the cursed `InconsistentChildrenStateError`. Which means we have somehow still not eliminated the fundamental root cause of that behavior! I have however validated that the same behavior under test works: - here, as demonstrated by the tests themselves - in both the `web-forms` and `ui-solid` UI clients While I’m not thrilled that the issue continues to hang over us, it is encouraging that previous efforts to narrow its impact have not totally failed us for this particular feature.
- Full support for `SubmissionResult<‘monolithic’>` - Minimal support for `SubmissionResult<‘chunked’>` Open questions: 1. Should we totally stub off the chunked option for now? I would normally say yes, as it’ll give us more feedback as we implement missing functionality. Hesitation is that the minimal support is already somewhat useful/usable until we support submission attachments. 2. Regardless of how extensively we support the chunked option now, do we have enough of the prerequisites in place to add stub test failures which will alert us when those tests need to be updated for more complete chunked submission support?
eyelidlessness
force-pushed
the
features/engine/submission-serialization
branch
from
September 27, 2024 19:01
5da1f1b
to
832e762
Compare
brontolosone
approved these changes
Oct 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
scenario
testingui-solid
's debug view (replacing the slightly broken version of it that was there before)Why is this the best possible solution? Were any other approaches considered?
This is largely consistent with the design in #188, so from a broad design perspective this is mostly answered there.
Several other implementation approaches were considered, which I'll detail along with the description of the actual approach I landed on.
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?
When I first approached testing the XML serialization aspect of this change, and specifically it being client reactive, I had originally wanted to test those at the
xforms-engine
level. I backed out of that when I found that testing behavior with repeats surfaced the dreadedInconsistentChildrenStateError
. In theory this suggests that we could encounter similar in downstream client-reactive situations. I was not able to reproduce the issue in any of:scenario
ui-solid
web-forms
(not updated here, but I did a quick integration locally to verify)Do we need any specific form for testing your changes? If so, please attach one.
N/A.
What's changed
Engine/client API
This is more or less consistent with the design in #188, with the following deviations:
I did go with some of the changes discussed in my later comments there. I don't think we explicitly folded them into the agreed design, but I don't think they'll be controversial either.
Additive: I also introduced a node-level
submissionState
property, which contains a client-reactivesubmissionXML
getter.Client-reactive
submissionState
The
submissionState
approach serves a few purposes:Ensure we stay mindful that we may want submission state to be explicitly client reactive in general
Provide a clear signal when/where a future change is incompatible with that goal
Establish node-level semantics for serialization, appropriate for each node type's hierarchical structure
Allow for custom node-level serialization logic (possibly applicable: mapping submission attachments to unique file names; configurable omission logic for non-relevant repeat instances)
As is done in
ui-solid
, support inspection/debugging use casesWe may want to consider folding the
submissionXML
property into each node's regularcurrentState
object. I think it would be appropriate, but I didn't want to burden this change with the exploratory work around how we'd mix derived state into its source object (i.e.submissionXML
is derived from each node'scurrentState.children
orcurrentState.value
).Engine internal
I don't think there's really much to say here. The bulk of the work is serialization, which is relatively straightforward since our in memory representation maps almost exactly to our serialized representation. Some new internal interfaces were added to support this, but they're largely reflective of what was already in place apart from the new API surface area.
Partial/incomplete functionality
Since we do not currently support uploads/submission attachments, there was little pertinent work to do on the
chunked
option. There is very minimal stubbed support, largely for the minor difference in shape ofSubmissionResult<'chunked'>
. There is currently no committed work on these fronts:While it's possible to implement these and validate them with automated testing, it would be mostly speculative until we have actual
File
data to submit.I did explore all of these, and will be able to reference them from stashes. But I felt it best not to put that much speculative work into functionality which is already ready for real use.
Alternative serialization approaches
I explored several alternative approaches to serialization. These are the most notable.
Creating an engine-internal "mini client", demonstrating that the serialization is effectively possible with existing client APIs. I think there would be other value in this approach, and it could also serve as a home for other hypothetical "possible for a client, valuable from the engine" functionality. Ultimately I dropped the effort when I encountered annoying tooling yak shaves, specifically around TypeScript configuration (it's really not designed for creating an artificial package boundary with cyclic dependencies, which is what the approach requires).
I had discussed specific approaches to mapping attachments to unique file names. The serialization implementation I landed on will need to change to support that use case: it involved a provision for injecting the mapping logic at the serialization call site, which is not consistent with this implementation's use of getters. I don't think the change will be significant, but as explained above I wanted to avoid over-engineering so much speculative, out of scope functionality.
Scenario
With the initial serialization work complete, it was trivial to observe the one existing submission test ported from JavaRosa passing. I also added a slew of tests in the following areas:
Moreover I added tests exercising the (I think) novel aspects of our design in #188:
status: 'pending'
, i.e. production of serialized invalid form state, accompanied by all validity violations present at that timestatus: 'ready'
to explicitly differentiate valid form state ready for submissionI'm on the fence about how much to accommodate the incomplete chunking support at a test level. I would really like to have something in place which will fail when we add that support, but I think that would again require out of scope speculative work to have any value.