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

Engine/client API: submission #188

Closed
eyelidlessness opened this issue Aug 16, 2024 · 13 comments
Closed

Engine/client API: submission #188

eyelidlessness opened this issue Aug 16, 2024 · 13 comments

Comments

@eyelidlessness
Copy link
Member

eyelidlessness commented Aug 16, 2024

I'm opening this issue to start discussion of a design for the engine/client interface for form submissions, in support of:

In past API design issues, I've generally tried to provide at least a couple of competing options. This has helped to identify some of the pros and cons of a particular design, even where I have a bias toward one of the solutions.

In this case, we've already discussed as a team most of the shape I expect this design to take, and in prior discussions we seemed to reach a general consensus. So this issue mostly serves as a more formal reference for the concepts we've discussed.

The proposed interface is currently detailed in 505a5ce (on the design/submission branch). I'll briefly outline the concepts here as a point of reference for further discussion:

  • RootNode.prepareSubmission(options?: SubmissionOptions): SubmissionResult: called by clients to prepare a submission

  • SubmissionResult: provides all of the information a client needs to proceed with a submission

    • status: 'pending' | 'ready': specifies whether the provided submission data is ready to be submitted by a client. This is effectively a reflection of the form instance's validity state. At call time, if the submission has any constraint or required violations (or violates any other future validity conditions, such as any implied by data types) its status will be pending. Otherwise it will be ready.

    • violations: if status is pending, this will be a reflection of the same violations produced by RootNode.validationState.violations. If status is ready, this will be null.

    • definition: SubmissionDefinition:

    • data: SubmissionData | [SubmissionData, ...SubmissionData[]] (as determined by SubmissionOptions)

  • SubmissionDefinition: provides submission-pertinent information as provided by the form, or as provided by the engine in accordance with pertinent ODK XForms specifications. Most likely to be of interest to a client:

    • submissionAction—corresponds to <submission action>)
    • submissionMethod: 'post'—corresponds to <submission method>, always normalized to 'post' where forms provide the deprecated form-data-post. Always defined, unless discussion suggests additional nuance not clear from the spec.
  • SubmissionData: a more specific variation of the FormData web standard, which guarantees presence of the form instance data itself, i.e. the XForm Part as defined by the OpenRosa Protocol's Form Submission API.

  • SubmissionOptions.chunked: 'chunked' and SubmissionOptions.maxSize: number: if specified as options to RootNode.prepareSubmission, the engine will chunk the submission data it provides to the client, producing [SubmissionData, ...FormData[]] where form attachments are chunked into subsequent FormData objects for chunked submission. (Otherwise the engine will provide a single ['monolithic' if explicitly specified in SubmissionOptions] SubmissionData object.)

Notes on SubmissionData and FormData

The intent of this aspect of the design is that the engine will provide submission data in a format suitable for clients to initiate any network requests (or hand that responsibility off to a host application)...

  1. As specified by the OpenRosa Protocol's Form Submission API, and

  2. With the standard and idiomatic web standard Fetch API.

It's expected that deferring to ODK specifications and web standards like these will strike the best balance for clients between:

  • Flexible usage: the engine does not dictate how or where submission occurs, only facilitating the aspects core to other aspects of the engine's responsibilities

  • The "pit of success"1: the engine's facilitation is consistent with expected (typical, idiomatic) usage whether implemented by a client, a host application, or some unknown third thing

Alternative: engine performs submission

In the spirit of past engine/client API design proposals, I do want to provide an alternative option so we have something to contrast. The most obvious alternative would be to have the engine perform submission.

This would likely involve the engine's submission API accepting a fetch-like option similar to the configuration it currently uses to retrieve XForm XML definitions (and is expected to do for form attachments or any other form-referenced resources).

This is a perfectly reasonable option, and we've discussed it as a team as well. I don't recall what motivating factors influenced it as a stronger contender. Off the top of my head, an obvious benefit would be the engine handling common failure modes (such as poor network conditions).

I'll caution that if we went with this option, most of what's in the primary proposal would either:

  • Still be present, but internal
  • More likely: be present and exposed to clients, which could then choose between either approach

If we want to go this direction, I would want to strongly consider some layering of responsibilities: the engine providing a single interface (likely consistent with the primary proposal), and then also providing a network/IO facilitation layer which consumes that base API.

Footnotes

  1. Apparently coined by Jeff Atwood

@eyelidlessness
Copy link
Member Author

@sadiqkhoja does this sound like it'll work from your end? It should be pretty much what we discussed previously, but want to confirm it feels right before I start implementation (which I'd like to do).

@lognaturel since you mentioned taking a look at this too, please also feel free to let me know if you have any concerns about this direction.

@sadiqkhoja
Copy link
Contributor

sadiqkhoja commented Aug 23, 2024

This is excellent 🎉. I specially like the option of having submissions to be chunked or not.

I have few thoughts/questions though:

  • If client decides not to use OpenRosa protocol and wish to use REST API for creating Submission, how can this be supported? I guess client would read Submission XML from SubmissionInstanceFile and attachments by iterating over keys of SubmissionData?
  • What do you think of calling the method getSubmission instead of prepareSubmission? This could be just me that when I hear the word prepare, it feels something time taking/asynchronous whereas this method is going to be (almost) instantaneous, right?
  • What if the method throws error when Form is invalid? And remove Status and violations from SubmissionResult?
  • Can engine make HEAD request during initialization phase to determine X-OpenRosa-Accept-Content-Length and set the default maxSize so that client can select chunked option without specifying maxSize?

PS: "Pit of Success" was originally coined by Rico Mariani 🙃

@eyelidlessness
Copy link
Member Author

This is excellent 🎉. I specially like the option of having submissions to be chunked or not.

🎉

If client decides not to use OpenRosa protocol and wish to use REST API for creating Submission, how can this be supported? I guess client would read Submission XML from SubmissionInstanceFile and attachments by iterating over keys of SubmissionData?

I'm less familiar with these REST APIs (I wish someone had mentioned this sooner!), but at a glance it looks like you could do this yes. If you think it makes sense to have another "pit of success" structure tailored to these APIs, I'm happy to do another pass. I don't know that it'd look very different, other than probably splitting the known xml_submission_file field and arbitrary submission attachments into separate fields. But that's just an off the cuff guess.

What do you think of calling the method getSubmission instead of prepareSubmission? This could be just me that when I hear the word prepare, it feels something time taking/asynchronous whereas this method is going to be (almost) instantaneous, right?

I do think it'll tend to be almost instantaneous. Even so, this question is a great prompt to consider changing its signature to return a Promise. There may be a forcing function that requires asynchrony to produce (at least some) submission attachments. I think it's worth considering accepting that likelihood now, rather than planning for a likely breaking change later.

On the question of method name: my reasoning for the prepare prefix is to make it absolutely clear that calling this does not perform the submission action, only produces a value suitable for submission by the client (or to be handed off to a host application). I think getSubmission gets a bit murkier in conveying that, especially if it returns a Promise.

What if the method throws error when Form is invalid? And remove Status and violations from SubmissionResult?

This design intentionally avoids throwing, and this has been a longstanding goal for any fallible aspect of the engine/client API. This is what I've meant when I mention a "Result type" in various conversations about this feature and others.

The problems with exceptions are numerous (and there's tons of literature on the topic). But most pragmatically for our purposes:

  • It's impossible to include any information about what errors might be thrown at the type level. And in TypeScript, this will never change: the TypeScript team has categorically ruled out "checked exceptions" or similar functionality.

  • It's very hard to document what errors might be thrown as a consequence of these type system limitations.

  • It's very easy for any documentation to fall out of sync with the implementation, also as a consequence of the type system limitations.

  • Even if we were to document this case, that risks surprise if any other error is thrown. This is ultimately unknowable (which is largely congruent with the reasons TypeScript will never support "checked exceptions" or similar).

  • In this case, it isn't necessarily an error! There are very good reasons a client might want to access submission data before all validation conditions are satisfied.

In the longer term, it's quite likely more of the engine/client interface will have a similar shape. Result types are not just good monadic theory, they're extraordinarily well suited to many aspects of this boundary in Web Forms.

If anything, I think it's worth considering making that intent more formally clear here, by giving it an explicit Result name. My hesitation is specifically the point that this isn't necessarily an error. Result is certainly capable of representing that, but it's more common to name it Either or similar. At which point, this case may not serve to make the intent any more clear at all.

Can engine make HEAD request during initialization phase to determine X-OpenRosa-Accept-Content-Length and set the default maxSize so that client can select chunked option without specifying maxSize?

If my reading of the OpenRosa Protocol is right, I believe we can do this iff:

  • the form defines a <submission action>
  • the client supplies a URL in its absence

I think it would be a bit odd to support this in the proposed design. Why would the engine handle this specific subset of the OpenRosa Protocol? If it does, why wouldn't it also handle the portion of IO to actually send the submission and attachments? I'm open to considering this, but I want to better understand the intent.

PS: "Pit of Success" was originally coined by Rico Mariani 🙃

Thanks for the correction! (And I was kinda surprised I didn't easily find an earlier reference.)

@sadiqkhoja
Copy link
Contributor

  • I can't make a call about whether we want to tweak the structure to facilitate REST APIs or not, maybe @lognaturel can guide us.
  • Returning Promise sounds good to me.
  • Agree with the reasoning about Result
  • I was hoping if we could make maxSize optional so that clients don't have to deal with it. But you are right engine shouldn't deal with OpenRosa Protocol.

@eyelidlessness
Copy link
Member Author

I can't make a call about whether we want to tweak the structure to facilitate REST APIs or not

I assumed you brought it up because you anticipate using it in the client<->host integration. If that's not the case, maybe you can elaborate on what motivates the question in the first place?

In any case, I expect that you are probably going to be either the initial primary user of these interfaces, or taking point to facilitate their downstream usage. So my instinct is that most of these are your calls to make.

Maybe it would be better to take a step back. How do you anticipate using submission data produced by the engine? Either in the client, or in a handoff to hosts. If anything about this design isn't serving that, now is the best time to find that out.

I was hoping if we could make maxSize optional so that clients don't have to deal with it. But you are right engine shouldn't deal with OpenRosa Protocol.

It is optional. If the engine doesn't know about it, it will produce a single, unchunked (monolithic) object. I suppose we can also expose the chunking logic so that clients can pass the monolithic submission data and the chunking logic to a host application? I'm not sure how much value that would add: I hope the engine's chunking implementation will be pretty simplistic (we're not solving the knapsack problem).

@getodk getodk deleted a comment from masooddahmedd Aug 26, 2024
@getodk getodk deleted a comment from masooddahmedd Aug 26, 2024
@lognaturel
Copy link
Member

lognaturel commented Aug 26, 2024

I have been imagining that for Central, submission would be done in a layer in Central frontend with the OpenRosa API to match Collect but I admit I haven't thought about it deeply. I think the main tradeoff between that and the Central REST API would be around how submission attachments are submitted. Either way we'll need to do some design work around what happens if connection cuts out part way through a submission and I think we can end up with the same user-facing result. @sadiqkhoja do you see a strong reason to use one vs the other?

How do you anticipate using submission data produced by the engine? Either in the client, or in a handoff to hosts.

👍 Yes, I'm interested in hearing thoughts on this too.

There will certainly be use cases outside of ODK Central for APIs of various shapes.

read Submission XML from SubmissionInstanceFile and attachments by iterating over keys of SubmissionData?

This seems ok, I think. @eyelidlessness you mention splitting the known xml_submission_file field and arbitrary submission attachments into separate fields, would this be additive for more direct retrieval?

expose the chunking logic so that clients can pass the monolithic submission data and the chunking logic to a host application

This doesn't feel necessary to me! I agree with what you've both said about getting the chunk size being best handled in an OpenRosa-specific layer.

@eyelidlessness
Copy link
Member Author

@eyelidlessness you mention splitting the known xml_submission_file field and arbitrary submission attachments into separate fields, would this be additive for more direct retrieval?

My instinct was that—if there is a compelling reason to produce different outputs for the different APIs—it would be additive.

I think being well prepared to support the OpenRosa Protocol, while maybe not a hard requirement, seems like the most obvious default. I definitely wouldn't want to make it more difficult, without good reason, in pursuit of other conveniences.

I imagine it'd probably accessed by an enum option—e.g. (totally off the cuff) something like:

const submission = await root.prepareSubmission({ format: 'openrosa' | 'odk-central' });

I'm not sure how I feel about the 'odk-central' enum value here. It'd probably be such a general format that 'other' is a more accurate descriptor.

I also think chunking (and the maxSize option to direct it) is probably only really applicable to OpenRosa Protocol support.


I suppose another reasonable option is to not produce an API-specific FormData here at all, just a bag of submission + attachments (although I'm struggling to think of a different structure which wouldn't be served just as well by the proposed 'monolithic' structure).

We could instead address OpenRosa support separately from this engine API. That could be handled by a separate export, package, recommended code snippet, etc. So instead of being additive, it would be composable—i.e. engine produces more general structure, hypothetical OpenRosa thing is something like fn(GeneralFormat, { maxSize?: number }) => OpenRosaFormat.

That's slightly more onerous than the original proposal, but doesn't feel prohibitively so.


Writing this response, I keep going back and forth. But the more I think about it, the more I think FormData with a known-present submission entry is probably still what we'll end up wanting. Everything other than the known submission entry inherently wants to be a Map-like thing anyway. FormData is that, plus some additional convenience if you want to use use it directly in a fetch.

@eyelidlessness
Copy link
Member Author

I suppose the other obvious, totally general structure would just be [SubmissionInstanceFile, ...File[]]. Since File has a name. Wrapping that in a FormData is trivial. Chunking can be handled separately/wherever.

@sadiqkhoja
Copy link
Contributor

My inclination is to hand over submission data produced by the engine to the host application, which will enable it to enhance/compose the data further. A feature that I think a lot of is to render two separate Forms together, on submit combined the data of both Form into one, and then send it to the server.

I agree that good support for OpenRosa Protocol should be our primary goal; this will be helpful for non-Central servers to adopt Web Forms. At the same time, I would like to see support for REST APIs because we are building such a powerful tool that people outside of the ODK ecosystem can benefit from this too - definitely this doesn't need to happen now but it would be great if we have a point of extension/addition for that. Hence I like the idea of format option, we can start with openrosa but depending on the need more options like xml string or DOM can be added later on.

@lognaturel
Copy link
Member

We discussed this briefly and will stick with FormData and the maxSize parameter. We'll leave the door open to a format option in the future as we learn more about what users want to do.

@eyelidlessness
Copy link
Member Author

eyelidlessness commented Sep 6, 2024

When we last discussed this, we also talked about the possibility of allowing clients to specify maxSize in initializeForm (likely via the config option). The intent would be to allow the engine to detect violations earlier, i.e. at the time a submission attachment to-be is assigned to a given node's value state. This would in turn allow the engine to surface validation for that case in the same way as it does for form-defined conditions (constraint, required).

I still think this is a good idea! That said, I took a little time this morning to update the affected validation interfaces, and it seemed complicated enough that we might want to give it more thought when we actually introduce functionality producing submission attachments. These are the main complications I encountered:

  1. If we treat a maxSize violation as additive to the current API, it negates the convenient property of constraint/required violations being mutually exclusive by design. This would drive us to either reconsider the aspect of validation APIs where a given leaf node can have at most one violation at any given time.

  2. Where constraint and required have corresponding form-defined violation messaging semantics (jr:constraintMsg and jr:requiredMsg respectively), we don't have a spec-based equivalent for maxSize. We use this 1:1 mapping to drive parsing and the types that follow from there, so we'd want to think about either introducing an beyond-spec equivalent, or how we'd want to special case the exception. I would want to consider a beyond-spec equivalent (something like wf:maxSizeMsg, introducing a wf namespace as a way to explore spec extensions?), at which point I'd also want to consider whether it makes sense to similarly derive maxSize itself from another corresponding extension in the form definition itself.

I think these are great areas to explore! But my inclination now is to defer them for the first pass on submission API implementation, so we can give them more dedicated thought, likely in the context where we introduce real submission attachment functionality we can test against.

@eyelidlessness
Copy link
Member Author

I think I'd also like to remove instanceID and deprecatedID from SubmissionDefinition. I plan to populate that interface at parse time, where instanceID will not yet be defined. Of course, the values will still be populated in the submission XML.


On that note, here I'll also capture some clarification from a quick chat with @lognaturel:

  1. It is assumed that a form definition will define one of the following, as a child of the form's primary instance root:

    • <orx:meta><orx:instanceID/><!-- ... other meta ... --></orx:meta>
    • <meta><instanceID/><!-- ... other meta ... --></meta> (in the default XForms namespace, also commonly prefixed xf)

    Enketo has special logic to implicitly generate such a structure where historically it may have been absent. We are free to produce an error if not defined in the form, as (quoting @lognaturel) "it isn't really an ODK XForm".1

  2. While orx is the preferred namespace for this aspect of the form definition, the default/xf namespace is also expected.1 We do not expect the two namespaces to be mixed in the same subtree, and will likely produce an error if encountered.

  3. Populating the instanceID is expected to be handled by the form definition. This will commonly be achieved by <bind nodeset="/data/orx:meta/orx:instanceID" calculate="concat('uuid:', uuid())" />. Also worth mentioning: the ODK XForms spec's first example includes <bind nodeset="/data/orx:meta/orx:instanceID" preload="uid" type="xsd:string"/> (see also preload attributes).2

    Regardless of the form definition mechanism, it was clarified that it will not be the engine's responsibility to populate this value (beyond supporting the other functionality used by the form definition, whether XPath uuid() support, jr:preload).

  4. For edits: deprecatedID is populated with the instanceID value present in a submission's XML.3 This should presumably occur before a new instanceID is generated by whatever form definition logic initializes it (i.e. this must occur before initializing calculate computations, and would apply to pending support for preloads/actions/events).

Footnotes

  1. Since we will have accommodation for both namespaces, I think it's probably trivial to also reproduce Enketo's behavior here. I'm kind of inclined to do that, unless there's a strong reluctance among the team. 2

  2. Presumably that preload is intended as jr:preload.

  3. Inferring from other aspects of the chat, I assume that if no submission instanceID is present, we should produce an error when editing the submission is attempted.

eyelidlessness added a commit that referenced this issue Sep 25, 2024
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`
eyelidlessness added a commit that referenced this issue Sep 26, 2024
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`
eyelidlessness added a commit that referenced this issue Sep 26, 2024
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`
eyelidlessness added a commit that referenced this issue Sep 27, 2024
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`
@lognaturel lognaturel added this to the Next milestone Oct 15, 2024
@eyelidlessness
Copy link
Member Author

More or less done in #226! Anything else was punted for later, related to pending functionality (e.g. submission attachments).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

5 participants
@eyelidlessness @sadiqkhoja @lognaturel and others