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

Fixes for _local doc update and _bulk_docs operations with new_edits false #1683

Merged
merged 4 commits into from Oct 29, 2018

Conversation

@eiri
Copy link
Member

@eiri eiri commented Oct 25, 2018

Overview

This PR is mostly a refactoring and addresses the following issues:

  • Attempt to create a local document with an invalid revision throwing badarg exception.
  • The documents with _local prefix written into a wrong btree on a bulk operation with new_edits set to false.
  • It is possible to create a document with an empty id on a bulk operation with new_edits set to false

Testing recommendations

New tests are expected to pass.

For manual testing here is an expected change of behaviour:

Readable error on an invalid rev for a local doc creation

curl -X PUT http://127.0.0.1:15984/koi/_local/alice?rev=0-abcdf -d '{"name":"Alice"}'
{"error":"error","reason":"Invalid rev format"}

Correct creation of a local doc on a bulk update

$ curl -X POST http://127.0.0.1:15984/koi/_bulk_docs -d '{"new_edits":false, "docs": [{"_id":"_local/alice", "_rev":"0-1", "name":"Alice"}]}'
[]
$ curl http://127.0.0.1:15984/koi/_local/alice
{"_id":"_local/alice","_rev":"0-2","name":"Alice"}

Generation of a missing id on a bulk update

$ curl -X POST http://127.0.0.1:15984/koi/_bulk_docs -d '{"new_edits":false, "docs": [{"_rev":"1-abcdefgh", "name":"Alice"}]}'
[]
$ curl http://127.0.0.1:15984/koi/_all_docs -G -d include_docs=true
{"total_rows":1,"offset":0,"rows":[
{"id":"ce4c2d25c9d580c1c5e06a05bb000443","key":"ce4c2d25c9d580c1c5e06a05bb000443","value":{"rev":"1-abcdefgh"},"doc":{"_id":"ce4c2d25c9d580c1c5e06a05bb000443","_rev":"1-abcdefgh","name":"Alice"}}
]}

Related Issues or Pull Requests

This closes #1628

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@eiri eiri force-pushed the cloudant:fix-_local-_bulk_docs branch from eda3d1c to 93524dc Oct 25, 2018
}
end, Docs).
lists:foldl(fun({Client, NewDoc}, Acc) ->
try

This comment has been minimized.

@iilyak

iilyak Oct 25, 2018
Contributor

WARNING: Most likely very subjective proposal. How about creating a function increment_revision(#doc{}) -> #doc{} | undefined and put try/catch there. Then we can rewrite foldl as follows:

lists:foldl(fun({Client, NewDoc}, Acc) ->
   case increment_revision(NewDoc) of
      #doc{revs = {0, [NewRev | _]}} = Doc ->
          send_result(Client, Doc, {ok, {0, integer_to_binary(NewRev)}}),
          [Doc | Acc];
      _ -> 
          send_result(Client, NewDoc, {error, <<"Invalid rev format">>}),
          Acc
   end
end, [], Docs).
end, Bucket)
end, BucketList),

ValidatePred = fun

This comment has been minimized.

@iilyak

iilyak Oct 26, 2018
Contributor

💯

Doc = #doc{
id = <<"_local/alice">>,
revs = {0, [<<"1">>]},
meta = [{ref, make_ref()}]

This comment has been minimized.

@iilyak

iilyak Oct 26, 2018
Contributor

interesting approach

@iilyak
iilyak approved these changes Oct 26, 2018
Copy link
Contributor

@iilyak iilyak left a comment

+1. Nice work

New tests pass.

Had to update commands for manual tests to include ContentType and auth. Everything works as advertised.

@eiri eiri force-pushed the cloudant:fix-_local-_bulk_docs branch from 93524dc to 7ba8279 Oct 29, 2018
@eiri
Copy link
Member Author

@eiri eiri commented Oct 29, 2018

@iilyak I've extracted increment_local_doc_revs into a separate function as you suggested. Can you take a look, please, if this is what you had in mind?

@iilyak
Copy link
Contributor

@iilyak iilyak commented Oct 29, 2018

Perfect. Thank you @eiri.

@eiri eiri force-pushed the cloudant:fix-_local-_bulk_docs branch from 7ba8279 to 6ee3d95 Oct 29, 2018
@eiri eiri merged commit f350d5f into apache:master Oct 29, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eiri eiri deleted the cloudant:fix-_local-_bulk_docs branch Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants