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

Mas i1847 putapi #1848

Merged
merged 7 commits into from
Feb 1, 2023
Merged

Mas i1847 putapi #1848

merged 7 commits into from
Feb 1, 2023

Conversation

martinsumner
Copy link
Contributor

For both PUT and DELETE the behaviour of the HTTP API should be consistent with that of the PB API, in particular it should not do a fetch of the object, unless such a fetch is required to support conditional request headers (e.g. 'if-None-Match').

As the if_not_modified and if_none_match are not supported via the HTTP API for non-consistent PUT - simply build object from a new object, as with PB API.
resource_exists/2 is a key part of the flow for both GET and PUT, as conditional HTTP headers require this check for PUTs.  therefore only override resource_exists (and don't fetch) when it is a PUT, and those conditional headers do not exist.
So that delete does not require a fetch
To mimic if_not_modified feature via PB API
To be consistent with other HTTP headers
@martinsumner
Copy link
Contributor Author

basho/riak_test#1370

@martinsumner
Copy link
Contributor Author

Failure in test verify_api_timeout. This test used to work by accident because of the riak_client:get within the webmachine workflow. Not this bypassed, and riak_client:delete is prompted - but this does not look in options for a passed-in timeout, using the default instead.

@martinsumner
Copy link
Contributor Author

The original intention was to standardise the response to delete - such that HTTP and PB would both respond OK as a result of an attempt to delete an object which is not present.

There is a need to be correct with respect to the HTTP standard. However, the HTTP standard is unclear. Some argue, that due to the idempotent requirement - 204 is the correct response to a not_found delete, but equally others have argued for 404 as the correct response. The 204 response is valid, but the 404 is also valid, and is more explicitly describing what has happened.

As there is no certainty as to the correctness of the response, and there are other cases where the response (especially error responses) vary between HTTP and PB - it seems better to be consistent with the previous version, rather than be consistent between the APIs.

Also ensure the timeouts passed in in a delete is respected, and passed through the riak_client to the FSM.
@martinsumner martinsumner merged commit 9c138dc into develop-3.0 Feb 1, 2023
@martinsumner martinsumner deleted the mas-i1847-putapi branch February 1, 2023 10:00
martinsumner added a commit that referenced this pull request Feb 1, 2023
* Never GET before PUT

As the if_not_modified and if_none_match are not supported via the HTTP API for non-consistent PUT - simply build object from a new object, as with PB API.

* More webmachine friendly override

resource_exists/2 is a key part of the flow for both GET and PUT, as conditional HTTP headers require this check for PUTs.  therefore only override resource_exists (and don't fetch) when it is a PUT, and those conditional headers do not exist.

* Attempt to tidy and refactor delete

So that delete does not require a fetch

* Pipe-cleaned delete path

* Add if_not_modified conflict check

To mimic if_not_modified feature via PB API

* Use hyphen not underscore

To be consistent with other HTTP headers

* Revert to 404 on if DELETE not_found

Also ensure the timeouts passed in in a delete is respected, and passed through the riak_client to the FSM.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants