change the DELETE fieldsubmission request

MartijnR commented 2 years ago

see email of Nov 1 from @jmcinerney

either POST with body or DELETE without body

[x] update API Docs on swaggerhub

MartijnR commented 2 years ago

information to pass:

repeat path
repeat ordinal
instanceID
ecid, presumably
deprecatedID (probably though not currently used by OC)
OC does not use nested repeats, if there is any chance they may be used in the future, the XML fragment may need to be included to pass the ordinals and paths of all nested repeats
any descendant fields of the deleted repeat?

DELETE request can have body (though it seems frowned upon and the body is not supposed to have any meaning - hence useless) but not as multipart/form-data

options to send data:

URL
body text (JSON e.g)
query parameters (we're sending ecid via query parameter in the POST requests)
headers
multipart/formdata (but only using POST)

If OC only needs to know the path to a repeat (for deleting or adding a repeat) and a single field (for changing/creating a field value), it does seem like a good idea to put the path of that resource in the request URL (and bring back PUT - though PUT does not related to the particular field/repeat but to the form record as a whole).

If in the case of repeats all its descendants have to be part of the delete/add request that approach will not work and we'd have to use something else that can transfer a whole XML fragment.

MartijnR commented 2 years ago

@jmcinerney, @svadla-oc is the OC side aware of all the questions inside a deleted repeat (including inside groups inside that repeat)?

jmcinerney commented 2 years ago

@MartijnR Thanks for this information, I had missed the repeat path in my original email. I think in a delete request we will need:

ecid
instanceID
repeat path
repeat ordinal I think it would make the most sense to have these 4 as path parameters. Semantically this would follow protocol as they would all be required and would be identifying a single resource to delete.

I think for the other mentioned parameters it might be good to run by @pbowen-oc and @svadla-oc to see if there is a future potential use case:

deprecatedID: I can see how this could be used to track changes in a field, but currently we audit all item data changes in our application at the database level.
Future feature of nested repeats would be a good question for Paul. I agree that we would need the XML fragment in that case.
Currently it doesn't seem as though OC needs information about the descendant fields in a deleted repeat in the Enketo request. Our code finds the repeat group as its stored in the database and then nulls out each item data keyed to that group.

I see what you're saying about bringing back the PUT requests. I think this could work if the xml_submission_fragment_file was sent as the body of the request.

MartijnR commented 2 years ago

Thanks!

I meant we would no longer use xml fragments for either POST, PUT and DELETE, but send the path/to/field/or/path/to/repeat as part of the API endpoint as you suggested for DELETE. The rest of the info could be a in query parameter, or in a different part of the path (or the body).

This would not be a good idea if nested repeats would ever need to be supported though.

MartijnR commented 2 years ago

<data>
    <repeat ordinal="1">
        <group>
             <a>2</a>
         </group>
    </repeat>
    <repeat ordinal="5">
        <group>
              <a>5</a>
        </group>
    </repeat>
</data>

If the last /data/repeat/a changes value we would have to provide the ordinal of its ancestor repeat (in the POST or PUT request). The ambiguity of what that ordinal refers to is not great.

MartijnR commented 2 years ago

/path/to/repeat/{ordinal}/node/ecid/{ecid}/instance/{instanceID}/
ordinal is placed directly after the node the ordinal belongs to. The ordinal part of a path can be detected by isNumber(part). If it is a number, it is an ordinal (because XML nodes cannot start with a number). This would also work for nested repeats (e.g. /data/rep1/3/grp/rep2/5/b refers to /data/rep1[@enk:ordinal="3"]/grp/rep2[@enk:ordinal="5"]/b)
order of node-path + ecid + instanceID (+ deprecatedID if necessary) ensures that XML nodenames 'instance' and 'ecid' won't confuse parsers
deprecatedID can be added in the future when necessary

Example: /data/repeat/2/a/ecid/def/instance/abc

path: /data/repeat[@enk:ordinal='2']/a (assuming a defined enk prefix for the enketo/xforms namespace)
instanceID: abc
ecid: def

Common to all endpoints:

Header X-OpenClinica-Version with value 2.0
response codes the same as described in https://app.swaggerhub.com/apis/martijnr/openclinica-fieldsubmission/1.0.0#/fieldsubmission/

POST /fieldsubmission/{path incl ordinals}/ecid/{ecid}/instance/{instanceID}/

First record

optional binary attachments

PUT /fieldsubmission/{path incl ordinals}/ecid/{ecid}/instance/{instanceID}/

Editing an existing record

optional binary attachments

NO LONGER INCLUDE DEPRECATED ID SINCE IT IS NOT USED BY OC

DELETE /fieldsubmission/{path incl ordinals}/ecid/{ecid}/instance/{instanceID}/

Deletes a repeat (and all nodes inside)

POST /fieldsubmission/complete/{path incl ordinals}/ecid/{ecid}/instance/{instanceID}/

Mark a never-completed record as complete

PUT /fieldsubmission/complete/{path incl ordinals}/ecid/{ecid}/instance/{instanceID}/

Mark an edited record as complete

NO LONGER INCLUDE DEPRECATED ID SINCE IT IS NOT USED BY OC

MartijnR commented 2 years ago

@svadla-oc, @jmcinerney

What do you think of the above (even though we'll only implement the DELETE endpoint for now)?
Would you like this to be documented properly like we did with version 1.0? If so, on swaggerhub or elsewhere?
OC adds attributes to XForm models, like oc:queryParent. These were part of the XML fragments we were sending in version 1.0 of the API. Is it okay to no longer have these attributes submitted with the new API?

jmcinerney commented 2 years ago

@MartijnR

Talking this through with Shiva we think we'd need to spend some more time to think about the URL patterns for post/put submissions vs what we do with delete requests. In the OC use case, field submissions are always targeting an item vs. deletes are always at the group level. For now would it be possible to make the delete URL path look something more like: /studies/{studyOid}/ecid/{ecid}/instance/{instanceId}/itemGroup/{itemGroup}/repeat/{repeat} This follows a model of going from most generic to most specific. This would be easier to parse than a URL containing XML like elements.
Swaggerhub would be great. Would it be possible to include the JSON export for this in the project? That way we could generate it if Swaggerhub was not accessible.
It looks like this field is still being referenced in the OC code. It may be redundant data but we're hesitant to change much here until we have more time to really investigate this. It should be fine to remove the deprecated_id though.

At some point down the road we'll want to standardize these URLs and maybe change how we send the data over (JSON?) but for now I think this is a good start and will unblock on our upgrade to Java 11.

MartijnR commented 2 years ago

Thanks @jmcinerney,

It may be redundant data but we're hesitant to change much here until we have more time to really investigate this.

I understand this can this be safely omitted for the DELETE request (only).

Yes, I'll add the swaggerhub JSON files to this repo (for both versions). Good idea.

a. Enketo is not aware of a studyOid. My guess is that this is part of the serverUrl that OC sends to Enketo (and Enketo just appends /fieldsubmission to this). If so, could we do serverUrl+'/fieldsubmission'+'/ecid/{ecid}/instance{instanceID}/itemGroup{itemGroup}/repeat{repeat}/'. Otherwise we have to come up with a way to pass that studyOid value.

b. I see itemGroup is something that is added to the XForm <bind>s, but is not added to the model. That makes it hard for Enketo to access that value at submission time, but I'll look into this. What would you like to see if there is no itemGroup value for a node? Do repeats ever have an itemgroup value?

d. does {repeat} refer to the repeat path with ordinal info like /path/to/myrep/3, or to just the repeat nodeName with ordinal like myrep/3?

MartijnR commented 2 years ago

[x] check if itemGroup info survives Enketo transformation - yes
[x] figure out if that value can be passed to the update event without rewriting too much of Enketo Core. - for questions, we'd have to read and pass the itemgroup in the change event handler in Enketo-Core's form.js. The setVal method in nodeset.js would get an extra parameter. Both these functions would have to be overwritten in this fork - currently not possible for the changehandler so we'd need to change enketo-core to facilitate this. If repeats also have itemgroup values, we'd have to overwrite more things in repeat.js. - It would definitely create some more fragility in this fork (breaking things when updating enketo-core).

jmcinerney commented 2 years ago

Enketo is not aware of a studyOid. My guess is that this is part of the serverUrl that OC sends to Enketo (and Enketo just appends /fieldsubmission to this).

My bad, the transformation of this URL was something that I didn't fully understand until I read your comment. I think given this, the URL would be as you suggested: {serverUrl}/fieldsubmission/ecid/{ecid}/instance/{instanceId}/itemGroup/{itemGroup}/repeat/{repeat}

What would you like to see if there is no itemGroup value for a node? Do repeats ever have an itemgroup value?

As far as I can tell, at least during a deletion, there shouldn't be a node without an itemGroup. Currently when deleting this is the XML payload that is sent over:

<?xml version="1.0" encoding="UTF-8"?>
<data xmlns:enk="http://enketo.org/xforms" xmlns:jr="http://openrosa.org/javarosa" xmlns:oc="http://openclinica.org/xforms" xmlns:orx="http://openrosa.org/xforms" id="951cbb77-afdc-4389-9f4d-94cb01e94294-F_REPEATINGHIS" version="1">
  <MHX enk:ordinal="2"/>
</data>

The parameter we're looking for to be sent over as the itemGroup in this case would be "MHX". As for the actual node, I don't think a DELETE would ever have a value, otherwise it would be sent as a POST/PUT?

does {repeat} refer to the repeat path with ordinal info like /path/to/myrep/3, or to just the repeat nodeName with ordinal like myrep/3?

Just the name, so it would be something like: /itemGroup/{itemGroup}/repeat/{repeat} to /itemGroup/MHX/repeat/2

Let me check in with @svadla-oc and @pbowen-oc about the potential changes needed in Enketo core or changes that would effect upstream merges for this fork.

MartijnR commented 2 years ago

Thanks @jmcinerney,

I was confused by the itemGroup since the XForms have an oc:itemGroup attribute on <bind>s that OC adds, but it looks like that can be ignored, at least for repeats. In that case it might be helpful to let repeat refer to the repeat XML nodeName (e.g. MHX), and ordinal to the repeat ordinal, so like

serverUrl/fieldsubmission/ecid/{ecid}/instance/{instance}/repeat/{repeat nodeName}/ordinal/{repeat ordinal}.

Note that this would bring us back to a plan where we could never support nested repeats

jmcinerney commented 2 years ago

@MartijnR Awesome, this URL structure looks good to me. I think I was confused by the naming, the repeat XML nodeName is the itemGroup name on the OC side.

It sounds like @pbowen-oc is okay with being locked out of support for nested repeats.

MartijnR commented 2 years ago

Great. I documented the whole new API here: https://app.swaggerhub.com/apis-docs/martijnr/openclinica-fieldsubmission/2.0.0

I'll start working on implementing the DELETE request (only).

Some new things that came up:

It appears to be frowned upon (i.e. not allowed by OAS2) to make path parameters optional. This is relevant for the POST and PUT requests that may not have a repeat and ordinal. I think setting the values to 0 is safe (because a nodeName cannot be a number), so e.g. POST /fieldsubmission/ecid/abc/instance/def/repeat/0/ordinal/0/node/mynode.
The body contains the value (for all non-DELETE requests). I set this to use multipart/form-data and the properties "newValue" (for XML node values) and "file" for an uploaded binary file.

MartijnR commented 2 years ago

Todo:

[x] ecid is currently defined in config.json with parameter to pass to submission. We should probably hardcode this instead and (ignore that setting for fieldsubmissions).
[x] pass values to addRepeatRemoval function
[x] fix tests
[x] server side
[x] head request (for max-size) to be added to API and perhaps implementation as well - handled by non-custom code

MartijnR commented 2 years ago

The DELETE request has now been changed according to the Fieldsubmission API 2.0.0 in master and is ready for testing

jmcinerney commented 2 years ago

Thanks @MartijnR! Will be doing some manual testing of this later tonight.

MartijnR commented 2 years ago

[x] encode all the values
[x] repeat => repeat-group
[x] update API docs at https://app.swaggerhub.com/apis-docs/martijnr/openclinica-fieldsubmission/2.0.0
[x] download new API doc json files
[x] check if correct Enketo ID is sent to DELETE /fieldsubmission - http://localhost:8005/single/fs/i/Y7eqyLo8?ecid=1 - I made an error as I forgot to strip the Enketo ID out

later:

implement other requests to follow the 2.0.0 API (and clean up - no more xmlFragment, submissionURL logic etc) => moved to OpenClinica/enketo-oc#90

MartijnR commented 2 years ago

@jmcinerney done! I definitely made a mistake in the submission URL so hopefully this will work now

jmcinerney commented 2 years ago

@MartijnR thanks! The encoded values look good. I'm still seeing an issue with the submission URL though, it looks like it is stripping out the API path prior to fieldsubmission? ie instead of: https://jmac.ngrok.io/OpenClinica/rest2/openrosa/S_RIVERS(TEST)/fieldsubmission/ecid/... I'm seeing https://jmac.ngrok.io/fieldsubmission/ecid/...

I wasn't sure if I was misreading my Tomcat logs, but I checked the Nginx logs and here's a data submission vs a delete: "POST /OpenClinica/rest2/openrosa/S_RIVERS(TEST)/fieldsubmission?ecid=8713c08ce7724bfe60ca6cff2379044968af0d3ee432450d97ab056ace772651 HTTP/1.1" 201

"DELETE /fieldsubmission/ecid/8713c08ce7724bfe60ca6cff2379044968af0d3ee432450d97ab056ace772651/instance/uuid%3Ac0804e9b-274d-4129-8e20-ecf46b23a57a/repeat-group/MHX/ordinal/2 HTTP/1.1" 302 Let me know if there are any additional details I can provide!

MartijnR commented 2 years ago

Sorry about that! I think it is fixed now.

jmcinerney commented 2 years ago

@MartijnR This is looking great now! Haven't fully confirmed it's working end-to-end, there's some work to do on my end now. But everything coming over looks good!

OpenClinica / enketo-oc