You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/11/03 04:51:47 UTC
[GitHub] [couchdb] jiahuili430 opened a new pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
jiahuili430 opened a new pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815
## Overview
Use `_bulk_docs` with "new_edits": false and without `_rev` should respond with 400 Bad Request instead of 500 Server Error.
When "new_edits":false, the docs should always include `_rev` value.
Ref: https://docs.couchdb.org/en/stable/api/database/bulk-api.html?highlight=bulk_docs#db-bulk-docs
## Testing recommendations
<!-- Describe how we can test your changes.
Does it provides any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 closed pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
jiahuili430 closed pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 closed pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
jiahuili430 closed pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815#discussion_r742089495
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Silently dropping documents seems a bit worrying. It's worth keeping in mind that `new_edits=false` option is intended intended for replication [mostly](https://docs.couchdb.org/en/stable/api/document/common.html#put--db-docid) :
> The ``false`` value is intended for use only by the replicator.
So we want to make sure a broken replicator implementation doesn't just drop documents on the floor and continue on without noticing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815#discussion_r742090735
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Also wondering if `element(3, Doc)` makes sense. Is Doc parsed a proper `#doc` record and should be be able to use the record or maybe create a couch_doc API function to extra the revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815#discussion_r742089495
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Silently dropping documents seems a bit worrying. It's worth keeping in mind that `new_edits=false` option is intended intended for replication [mostly](https://docs.couchdb.org/en/stable/api/document/common.html#put--db-docid) :
> The ``false`` value is intended for use only by the replicator.
So we want to make sure a broken replicator implementation doesn't just drop documents on the floor and continue on without noticing
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Also wondering if `element(3, Doc)` makes sense. Is Doc parsed a proper `#doc` record and should be be able to use the record or maybe create a couch_doc API function to extra the revision.
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
+ case length(Docs2) of
+ 0 ->
+ send_json(Req, 400, {[{missing_revs,
+ ?l2b("If `new_edits` is false, a well-formed "
+ "`_rev` must be included in the document.")}]});
+ _ ->
Review comment:
I had noticed in the previous incarnation of the PR https://github.com/apache/couchdb/pull/3812 that we should really double check if _rev is actually needed. `replicated` changes should primarily rely on `_revisions` so that would make that error incorrect. (But do double-check the semantics and interplay between those).
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Silently dropping documents seems a bit worrying. It's worth keeping in mind that `new_edits=false` option is intended intended for replication [mostly](https://docs.couchdb.org/en/stable/api/document/common.html#put--db-docid) :
> The ``false`` value is intended for use only by the replicator.
So we want to make sure a broken replicator implementation doesn't just drop documents on the floor and continue on without noticing
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Also wondering if `element(3, Doc)` makes sense. Is Doc parsed a proper `#doc` record and should be be able to use the record or maybe create a couch_doc API function to extra the revision.
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
+ case length(Docs2) of
+ 0 ->
+ send_json(Req, 400, {[{missing_revs,
+ ?l2b("If `new_edits` is false, a well-formed "
+ "`_rev` must be included in the document.")}]});
+ _ ->
Review comment:
I had noticed in the previous incarnation of the PR https://github.com/apache/couchdb/pull/3812 that we should really double check if _rev is actually needed. `replicated` changes should primarily rely on `_revisions` so that would make that error incorrect. (But do double-check the semantics and interplay between those).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815#discussion_r742099821
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
+ case length(Docs2) of
+ 0 ->
+ send_json(Req, 400, {[{missing_revs,
+ ?l2b("If `new_edits` is false, a well-formed "
+ "`_rev` must be included in the document.")}]});
+ _ ->
Review comment:
I had noticed in the previous incarnation of the PR https://github.com/apache/couchdb/pull/3812 that we should really double check if _rev is actually needed. `replicated` changes should primarily rely on `_revisions` so that would make that error incorrect. (But do double-check the semantics and interplay between those).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 closed pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
jiahuili430 closed pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3815: Using _bulk_docs with new_edits false and without _rev should respond 400 Bad Request
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3815:
URL: https://github.com/apache/couchdb/pull/3815#discussion_r742089495
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Silently dropping documents seems a bit worrying. It's worth keeping in mind that `new_edits=false` option is intended intended for replication [mostly](https://docs.couchdb.org/en/stable/api/document/common.html#put--db-docid) :
> The ``false`` value is intended for use only by the replicator.
So we want to make sure a broken replicator implementation doesn't just drop documents on the floor and continue on without noticing
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
Review comment:
Also wondering if `element(3, Doc)` makes sense. Is Doc parsed a proper `#doc` record and should be be able to use the record or maybe create a couch_doc API function to extra the revision.
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -569,15 +569,23 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
send_json(Req, 417, ErrorsJson)
end;
false ->
- case fabric:update_docs(Db, Docs, [replicated_changes|Options]) of
- {ok, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 201, ErrorsJson);
- {accepted, Errors} ->
- chttpd_stats:incr_writes(length(Docs)),
- ErrorsJson = lists:map(fun update_doc_result_to_json/1, Errors),
- send_json(Req, 202, ErrorsJson)
+ Docs2 = [Doc || Doc <- Docs, element(3, Doc) =/= {0, []}],
+ case length(Docs2) of
+ 0 ->
+ send_json(Req, 400, {[{missing_revs,
+ ?l2b("If `new_edits` is false, a well-formed "
+ "`_rev` must be included in the document.")}]});
+ _ ->
Review comment:
I had noticed in the previous incarnation of the PR https://github.com/apache/couchdb/pull/3812 that we should really double check if _rev is actually needed. `replicated` changes should primarily rely on `_revisions` so that would make that error incorrect. (But do double-check the semantics and interplay between those).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org