Clarify when the server will respond with 304 #151

Closed dpancic closed 10 months ago

In GitLab by @stefanprobst on Jan 26, 2022, 16:36

It is currently not very clear to me when the server will return status code 304 in response to a PUT request.

Example:

(1) Retrieve item:

curl --request GET \
  --url https://sshoc-marketplace-api.acdh-dev.oeaw.ac.at/api/tools-services/ef1r3I

which will return:

{
  "id": 63903,
  "category": "tool-or-service",
  "label": "Bread!",
  "version": "",
  "persistentId": "ef1r3I",
  "lastInfoUpdate": "2022-01-26T13:46:53+0000",
  "status": "approved",
  "informationContributor": {
    "id": 1,
    "username": "Administrator",
    "displayName": "Administrator",
    "status": "enabled",
    "registrationDate": "2020-07-27T15:13:06+0000",
    "role": "administrator",
    "email": "administrator@example.com",
    "config": true
  },
  "description": "Bread!",
  "contributors": [],
  "properties": [],
  "externalIds": [],
  "accessibleAt": [],
  "relatedItems": [],
  "media": []
}

Note that the description field has "Bread!".

(2) As contributor, try to change item description via PUT request:

curl --request PUT \
  --url https://sshoc-marketplace-api.acdh-dev.oeaw.ac.at/api/tools-services/ef1r3I \
  --header 'Authorization: ${SSHOC_TOKEN}' \
  --header 'Content-Type: application/json' \
  --data '{
    "label": "Bread!",
    "version": "",
    "description": "Bread! Bread! Bread!",
    "contributors": [],
    "properties": [],
    "externalIds": [],
    "accessibleAt": [],
    "relatedItems": [],
    "media": []
}'

Note that the description field has been changed to "Bread! Bread! Bread!" -- however the server will respond with 304.

In GitLab by @tparkola on Jan 28, 2022, 15:52

What kind of error and message is send in response?

In GitLab by @stefanprobst on Jan 28, 2022, 16:53

there's no error, but a server response with "304 Not Modified", even though the PUT payload had a different description. am i misunderstanding something here?

In GitLab by @stefanprobst on Feb 2, 2022, 17:19

dispatching this from the browser actually gives some CORS error, but i'm still unclear why the server returns 304 in the first place.

In GitLab by @stefanprobst on Feb 3, 2022, 15:54

when getting this 304 in the browser, there actually is a CORS error (missing "Access-Control-Allow-Origin")

In GitLab by @KlausIllmayer on Feb 4, 2022, 14:52

maybe this is the reason: https://gitlab.gwdg.de/sshoc/sshoc-marketplace-backend/-/issues/85#note_536637

In GitLab by @stefanprobst on Feb 4, 2022, 16:10

thanks for pointing this out - didn't know about that. though not sure how i feel about it, since iirc one goal was to make PUt idempotent i.e. that it's possible to PUT the result of a GET operation, which seems no longer possible? i'll omit source and sourceItemId and see if that helps.

other than that, the CORS issue still seems relevant.

In GitLab by @stefanprobst on Feb 4, 2022, 16:43

hmm, after re-reading the comment, still not sure - since the above example does not provide source and sourceItemId anyway...

In GitLab by @stefanprobst on Feb 4, 2022, 17:02

ok, so i tried omitting source and sourceItemId in a PUT request as admin, but that will actually remove them from the entity. maybe this only applies to status=ingested items?

also one thing that would benefit from some docs i think.

I tried to reproduce the problem today but with no success. Currently the response to PUT (described above by @stefanprobst) is 200 and not 304 so it behaves properly. @stefanprobst could you please verify that, and if the problem is not there anymore, just close the issue. Thanks.

yep, i cannot reproduce this issue anymore either - closing as fixed. thanks!

Re-opening as I discovered this problem again:

If I'm trying to approve a re-ingest item directly at API (not using the frontend) I get the 304 Not modified:

Looking for an ingested item that needs to be approved, e.g. call GET /api/item-search?d.status=ingested&f.source={param_source_label}
Take one item that has rich content (I suspect, that some properties must be there, ideally ones with type concept - I didn't have the chance to test items with only label and description set, which would be interesting) and get the content with GET /api/{category}s/{persistentId}/versions/{id}
Now take the response, don't manipulate it and approve it by inserting in the call to the endpoint PUT /api/{category}s/{persistentId}
I then get a 304 Not Modified even if the published item differs to the ingested one, e.g. having a different label and description but everything else is the same. It will not change the published item.

I was wondering, why the same works on level of the frontend (it does the same API calls). I found out, that if I manipulate the response from step 3, e.g. adding a single character to the label it will not give a 304 and instead will approve the item and return 200. This is also true, if you don't manipulate a value but reduce the sent json-structure for a single concept property to its minimum by deleting all unnecessary keys (only having e.g. {"type": {"code": "language"}, "concept": {"code": "swe"}},) which is what the frontend is doing and therefore is not running in the 304-response. There seems to be some unnecessary(?) check that leads to 304 which does not make sense in the approve-workflow.

The check that leads to 304 is done when current item for the user that executes the API call is not DRAFT and the draft flag (in the API call) is set to false. As I understand this is indeed the case in the workflow you described - the item to be approved has INGESTED status, the flag is false (default value) and all properties in the PUT request are the same as the current item. The the system checks that the items are the same and returns 304. To handle that I would say that we should add condition that we also do not compare items if the approve flag is set to true and the status of the current item is not APPROVE. It would mean that in all such cases we simply update the item (because we know the expected status is different from the current one, so we should update the item). Does it make sense to you?

As of discussion 2023-10-24 - there is agreement to proceed with suggested solution.

SSHOC / sshoc-marketplace-backend

Clarify when the server will respond with 304 #151