return code 409 instead of 500 for revert conflict #3538
Conversation
itaidavid
added
the
include-changelog
|
Closes #3482 |
| @@ -1751,7 +1751,8 @@ func handleAPIError(w http.ResponseWriter, err error) bool { | |||
| errors.Is(err, actions.ErrParamConflict): | |||
| writeError(w, http.StatusBadRequest, err) | |||
|
|
|||
| case errors.Is(err, graveler.ErrNotUnique): | |||
| case errors.Is(err, graveler.ErrNotUnique), | |||
There was a problem hiding this comment.
I wonder why there's 2 conflict like graveler errors.. Just making sure we don't override some other flow error handling that returned graveler.ErrConflictFound
There was a problem hiding this comment.
That is a good question. Actually, the controller handles graveler.ErrConflictFound specifically for Merge and returns http.StatusConflict, so we actually aligning a more general flow.
I can't say I analyzed each path from our API that may result with graveler.ErrConflictFound, but I think it is pretty safe, as the handling is done within handleAPIError so the client (no matter which client) gets an error eitherways. I don't think anyone should expect the very general http.StatusInternalServerError explicitly, and fail if another error code is received (excluding the cases where specific error codes gets specific handling, in which case we are only helping the client, IMHO)
WDYT?
There was a problem hiding this comment.
graveler.ErrNotUnique is returned when attempting to create repo, branch or tag that already exist. Looking at http error codes, it does seem that StatusConflict is the most appropriate code to use. It looks like the usage of both graveler.ErrNotUnique and graveler.ErrConflictFound, and their mapping to http.StatusConflict, are in place
esti/rollback_test.go
Outdated
| @@ -244,3 +245,49 @@ func TestRevert(t *testing.T) { | |||
| body := string(getObjResp.Body) | |||
| require.Equal(t, objContent2, body, fmt.Sprintf("path: %s, expected: %s, actual:%s", objPath2, objContent2, body)) | |||
| } | |||
|
|
|||
| func TestRevertConflict(t *testing.T) { | |||
There was a problem hiding this comment.
minor: Could be a controller level test, it would reduce the runtime.
There was a problem hiding this comment.
I'm all in favour of adding a controller test, but do you think the Esti test should be removed?
As I see it, these E2E tests are the important ones, in terms of CI/CD, as they really cover a true use-case.
WDYT?
There was a problem hiding this comment.
Added controller UT and remove Esti test
|
handleAPIErrormappedgraveler.ErrConflictFoundto the defaulthttp.StatusInternalServerError (500)instead of the expectedhttp.StatusConflict (409)Fixed that and added an
Estitest to create a conflict, attempt a revert and verify the status code