getodk / web-forms

ODK Web Forms enables form filling and submission editing of ODK forms in a web browser. It's coming soon! ✨
https://getodk.org
Apache License 2.0
9 stars 5 forks source link

Engine/client API: submission #188

Open eyelidlessness opened 1 month ago

eyelidlessness commented 1 month ago

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:

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:

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:

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.

[^1]: Apparently coined by Jeff Atwood

eyelidlessness commented 3 weeks ago

@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 commented 3 weeks ago

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

I have few thoughts/questions though:

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

eyelidlessness commented 3 weeks ago

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:

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:

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 commented 3 weeks ago
eyelidlessness commented 3 weeks ago

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).

lognaturel commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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).

[^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]: 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.