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

Return 409 when put attachment with nonexistent rev #595

Merged
merged 2 commits into from Jun 16, 2017

Conversation

Projects
None yet
3 participants
@jaydoane
Contributor

jaydoane commented Jun 13, 2017

Overview

Currently we return a 500 and something like
{"error":"{not_found,missing}","reason":"{1,<<\"000\">>}"}
when an attempt is made to put an attachment document with a
non-existent revision.

This changes the behavior to return a 409 and
{"error":"not_found","reason":"missing_rev"}"

Testing recommendations

Test is included with PR

GitHub issue number

Related Pull Requests

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@eiri

We need to adopt this change in test/javascript/tests/attachments.js test (line 120, case "// with nonexistent rev")

@eiri

eiri approved these changes Jun 13, 2017

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Jun 13, 2017

Member

The tests are fine locally and 404 is better than 500, so the change itself looks good to me.

The only thing is that I'm not 100% sure that we should return 404 and not 409 here. I understand we are crashing with not_found, but update of attachment is doc's update and we are throwing 409 on attempt to update doc with non-existing revision. So from consistency perspective we might want to do this here as well.

I'm tentatively +1, but get a second opinion before merging, please.

Local tests:

make eunit apps=chttpd
...
=======================================================
  All 150 tests passed.
make javascript
...
=======================================================
JavaScript tests complete.
  Failed: 0.  Skipped or passed: 86.
Member

eiri commented Jun 13, 2017

The tests are fine locally and 404 is better than 500, so the change itself looks good to me.

The only thing is that I'm not 100% sure that we should return 404 and not 409 here. I understand we are crashing with not_found, but update of attachment is doc's update and we are throwing 409 on attempt to update doc with non-existing revision. So from consistency perspective we might want to do this here as well.

I'm tentatively +1, but get a second opinion before merging, please.

Local tests:

make eunit apps=chttpd
...
=======================================================
  All 150 tests passed.
make javascript
...
=======================================================
JavaScript tests complete.
  Failed: 0.  Skipped or passed: 86.
@jaydoane

This comment has been minimized.

Show comment
Hide comment
@jaydoane

jaydoane Jun 13, 2017

Contributor

@eiri thanks for the review. I chose 404 since it seemed like that was consistent with other "not_found" errors, but I understand your concern. I agree we should have at least a second opinion.

Contributor

jaydoane commented Jun 13, 2017

@eiri thanks for the review. I chose 404 since it seemed like that was consistent with other "not_found" errors, but I understand your concern. I agree we should have at least a second opinion.

@nickva

This comment has been minimized.

Show comment
Hide comment
@nickva

nickva Jun 14, 2017

Contributor

I think I lean more toward @eiri's idea of making it a 409.

The document itself is there it's just that the revision doesn't match. This is similar to updating a regular document (not an attachment) with an revision which doesn't match where we get a 409.

Now if database is altogether missing then we do get a 404. But I think this is closer to the former than the later.

Also +1. Nice work!

Contributor

nickva commented Jun 14, 2017

I think I lean more toward @eiri's idea of making it a 409.

The document itself is there it's just that the revision doesn't match. This is similar to updating a regular document (not an attachment) with an revision which doesn't match where we get a 409.

Now if database is altogether missing then we do get a 404. But I think this is closer to the former than the later.

Also +1. Nice work!

jaydoane added some commits Jun 13, 2017

Return 409 to PUT attachment with non-existent rev
Currently we return a 500 and something like
{"error":"{not_found,missing}","reason":"{1,<<\"000\">>}"}
when an attempt is made to put an attachment document with a
non-existent revision.

This changes the behavior to return a 409 and
{"error":"not_found","reason":"missing_rev"}"

@jaydoane jaydoane changed the title from Return 404 when put attachment with nonexistent rev to Return 409 when put attachment with nonexistent rev Jun 16, 2017

@jaydoane

This comment has been minimized.

Show comment
Hide comment
@jaydoane

jaydoane Jun 16, 2017

Contributor

@eiri and @nickva I've updated this PR to return 409 instead of 404. If you are ok with this, can one of you merge this for me?

Contributor

jaydoane commented Jun 16, 2017

@eiri and @nickva I've updated this PR to return 409 instead of 404. If you are ok with this, can one of you merge this for me?

@nickva nickva merged commit d3bae3f into apache:master Jun 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jaydoane jaydoane deleted the cloudant:404-put-attachment-nonexistent-rev branch Jun 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment