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/07/30 18:57:36 UTC
[GitHub] [couchdb] nickva opened a new pull request #3686: Fix response code for existing att and wrong rev
nickva opened a new pull request #3686:
URL: https://github.com/apache/couchdb/pull/3686
## Overview
This is a fixup for pr https://github.com/apache/couchdb/pull/3685
(Using a separate PR as can't push to contributor's private fork)
Which fixed a bug introduced in https://github.com/apache/couchdb/pull/3673
The behavior we are trying to preserve is:
```
DELETE with a rev or without a rev of an existing doc and attachment should return a 409
DELETE for a non-existent doc should return a 404
DELETE with a non-existent attachment name should return a 404
```
## Testing recommendations
make && make elixir tests=test/elixir/test/attachments_test.exs
## 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
- [x] Code is written and works correctly
- [x] 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] iilyak commented on a change in pull request #3686: Fix response code for existing att and wrong rev
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3686:
URL: https://github.com/apache/couchdb/pull/3686#discussion_r680187891
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -1442,120 +1442,115 @@ couch_doc_open(Db, DocId, Rev, Options0) ->
end
end.
-get_attachment(Req, Db, DocId, FileName) ->
- % Check if attachment exists
- #doc_query_args{
- rev=Rev,
- options=Options
- } = parse_doc_query(Req),
- #doc{
- atts=Atts
- } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+
+get_existing_attachment(Atts, FileName) ->
+ % Check if attachment exists, if not throw not_found
case [A || A <- Atts, couch_att:fetch(name, A) == FileName] of
- [] ->
- {error, missing};
- [Att] ->
- {ok, Doc, Att}
+ [] -> throw({not_found, "Document is missing attachment"});
+ [Att] -> Att
end.
% Attachment request handlers
db_attachment_req(#httpd{method='GET',mochi_req=MochiReq}=Req, Db, DocId, FileNameParts) ->
FileName = list_to_binary(mochiweb_util:join(lists:map(fun binary_to_list/1,
FileNameParts),"/")),
- case get_attachment(Req, Db, DocId, FileName) of
- {error, missing} ->
- throw({not_found, "Document is missing attachment"});
- {ok, Doc, Att} ->
- [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
- Refs = monitor_attachments(Att),
- try
- Etag = case Md5 of
- <<>> -> chttpd:doc_etag(Doc);
- _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
- end,
- ReqAcceptsAttEnc = lists:member(
- atom_to_list(Enc),
- couch_httpd:accepted_encodings(Req)
- ),
- Headers = [
- {"ETag", Etag},
- {"Cache-Control", "must-revalidate"},
- {"Content-Type", binary_to_list(Type)}
- ] ++ case ReqAcceptsAttEnc of
- true when Enc =/= identity ->
- % RFC 2616 says that the 'identify' encoding should not be used in
- % the Content-Encoding header
- [{"Content-Encoding", atom_to_list(Enc)}];
+ #doc_query_args{
+ rev=Rev,
+ options=Options
+ } = parse_doc_query(Req),
+ #doc{
+ atts=Atts
+ } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+ Att = get_existing_attachment(Atts, FileName),
+ [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
+ Refs = monitor_attachments(Att),
+ try
+ Etag = case Md5 of
+ <<>> -> chttpd:doc_etag(Doc);
+ _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
+ end,
+ ReqAcceptsAttEnc = lists:member(
+ atom_to_list(Enc),
+ couch_httpd:accepted_encodings(Req)
+ ),
+ Headers = [
+ {"ETag", Etag},
+ {"Cache-Control", "must-revalidate"},
+ {"Content-Type", binary_to_list(Type)}
+ ] ++ case ReqAcceptsAttEnc of
Review comment:
I've notices that you've just copied the previous implementation before #3686. Therefore I am ok with keeping it as is.
--
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] iilyak commented on a change in pull request #3686: Fix response code for existing att and wrong rev
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3686:
URL: https://github.com/apache/couchdb/pull/3686#discussion_r680184897
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -1442,120 +1442,115 @@ couch_doc_open(Db, DocId, Rev, Options0) ->
end
end.
-get_attachment(Req, Db, DocId, FileName) ->
- % Check if attachment exists
- #doc_query_args{
- rev=Rev,
- options=Options
- } = parse_doc_query(Req),
- #doc{
- atts=Atts
- } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+
+get_existing_attachment(Atts, FileName) ->
+ % Check if attachment exists, if not throw not_found
case [A || A <- Atts, couch_att:fetch(name, A) == FileName] of
- [] ->
- {error, missing};
- [Att] ->
- {ok, Doc, Att}
+ [] -> throw({not_found, "Document is missing attachment"});
+ [Att] -> Att
end.
% Attachment request handlers
db_attachment_req(#httpd{method='GET',mochi_req=MochiReq}=Req, Db, DocId, FileNameParts) ->
FileName = list_to_binary(mochiweb_util:join(lists:map(fun binary_to_list/1,
FileNameParts),"/")),
- case get_attachment(Req, Db, DocId, FileName) of
- {error, missing} ->
- throw({not_found, "Document is missing attachment"});
- {ok, Doc, Att} ->
- [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
- Refs = monitor_attachments(Att),
- try
- Etag = case Md5 of
- <<>> -> chttpd:doc_etag(Doc);
- _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
- end,
- ReqAcceptsAttEnc = lists:member(
- atom_to_list(Enc),
- couch_httpd:accepted_encodings(Req)
- ),
- Headers = [
- {"ETag", Etag},
- {"Cache-Control", "must-revalidate"},
- {"Content-Type", binary_to_list(Type)}
- ] ++ case ReqAcceptsAttEnc of
- true when Enc =/= identity ->
- % RFC 2616 says that the 'identify' encoding should not be used in
- % the Content-Encoding header
- [{"Content-Encoding", atom_to_list(Enc)}];
+ #doc_query_args{
+ rev=Rev,
+ options=Options
+ } = parse_doc_query(Req),
+ #doc{
+ atts=Atts
+ } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+ Att = get_existing_attachment(Atts, FileName),
+ [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
+ Refs = monitor_attachments(Att),
+ try
+ Etag = case Md5 of
+ <<>> -> chttpd:doc_etag(Doc);
+ _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
+ end,
+ ReqAcceptsAttEnc = lists:member(
+ atom_to_list(Enc),
+ couch_httpd:accepted_encodings(Req)
+ ),
+ Headers = [
+ {"ETag", Etag},
+ {"Cache-Control", "must-revalidate"},
+ {"Content-Type", binary_to_list(Type)}
+ ] ++ case ReqAcceptsAttEnc of
Review comment:
or `{Enc, ReqAcceptsAttEnc}` to keep ordering of arguments consistent with line 1494
--
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] iilyak commented on a change in pull request #3686: Fix response code for existing att and wrong rev
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3686:
URL: https://github.com/apache/couchdb/pull/3686#discussion_r680184261
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -1442,120 +1442,115 @@ couch_doc_open(Db, DocId, Rev, Options0) ->
end
end.
-get_attachment(Req, Db, DocId, FileName) ->
- % Check if attachment exists
- #doc_query_args{
- rev=Rev,
- options=Options
- } = parse_doc_query(Req),
- #doc{
- atts=Atts
- } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+
+get_existing_attachment(Atts, FileName) ->
+ % Check if attachment exists, if not throw not_found
case [A || A <- Atts, couch_att:fetch(name, A) == FileName] of
- [] ->
- {error, missing};
- [Att] ->
- {ok, Doc, Att}
+ [] -> throw({not_found, "Document is missing attachment"});
+ [Att] -> Att
end.
% Attachment request handlers
db_attachment_req(#httpd{method='GET',mochi_req=MochiReq}=Req, Db, DocId, FileNameParts) ->
FileName = list_to_binary(mochiweb_util:join(lists:map(fun binary_to_list/1,
FileNameParts),"/")),
- case get_attachment(Req, Db, DocId, FileName) of
- {error, missing} ->
- throw({not_found, "Document is missing attachment"});
- {ok, Doc, Att} ->
- [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
- Refs = monitor_attachments(Att),
- try
- Etag = case Md5 of
- <<>> -> chttpd:doc_etag(Doc);
- _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
- end,
- ReqAcceptsAttEnc = lists:member(
- atom_to_list(Enc),
- couch_httpd:accepted_encodings(Req)
- ),
- Headers = [
- {"ETag", Etag},
- {"Cache-Control", "must-revalidate"},
- {"Content-Type", binary_to_list(Type)}
- ] ++ case ReqAcceptsAttEnc of
- true when Enc =/= identity ->
- % RFC 2616 says that the 'identify' encoding should not be used in
- % the Content-Encoding header
- [{"Content-Encoding", atom_to_list(Enc)}];
+ #doc_query_args{
+ rev=Rev,
+ options=Options
+ } = parse_doc_query(Req),
+ #doc{
+ atts=Atts
+ } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+ Att = get_existing_attachment(Atts, FileName),
+ [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
+ Refs = monitor_attachments(Att),
+ try
+ Etag = case Md5 of
+ <<>> -> chttpd:doc_etag(Doc);
+ _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
+ end,
+ ReqAcceptsAttEnc = lists:member(
+ atom_to_list(Enc),
+ couch_httpd:accepted_encodings(Req)
+ ),
+ Headers = [
+ {"ETag", Etag},
+ {"Cache-Control", "must-revalidate"},
+ {"Content-Type", binary_to_list(Type)}
+ ] ++ case ReqAcceptsAttEnc of
Review comment:
this a really complex condition to understand. It could possibly be expressed as few clauses under `case {ReqAcceptsAttEnc, Enc} of`.
--
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 merged pull request #3686: Fix response code for existing att and wrong rev
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3686:
URL: https://github.com/apache/couchdb/pull/3686
--
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 #3686: Fix response code for existing att and wrong rev
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3686:
URL: https://github.com/apache/couchdb/pull/3686#discussion_r680185818
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -1442,120 +1442,115 @@ couch_doc_open(Db, DocId, Rev, Options0) ->
end
end.
-get_attachment(Req, Db, DocId, FileName) ->
- % Check if attachment exists
- #doc_query_args{
- rev=Rev,
- options=Options
- } = parse_doc_query(Req),
- #doc{
- atts=Atts
- } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+
+get_existing_attachment(Atts, FileName) ->
+ % Check if attachment exists, if not throw not_found
case [A || A <- Atts, couch_att:fetch(name, A) == FileName] of
- [] ->
- {error, missing};
- [Att] ->
- {ok, Doc, Att}
+ [] -> throw({not_found, "Document is missing attachment"});
+ [Att] -> Att
end.
% Attachment request handlers
db_attachment_req(#httpd{method='GET',mochi_req=MochiReq}=Req, Db, DocId, FileNameParts) ->
FileName = list_to_binary(mochiweb_util:join(lists:map(fun binary_to_list/1,
FileNameParts),"/")),
- case get_attachment(Req, Db, DocId, FileName) of
- {error, missing} ->
- throw({not_found, "Document is missing attachment"});
- {ok, Doc, Att} ->
- [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
- Refs = monitor_attachments(Att),
- try
- Etag = case Md5 of
- <<>> -> chttpd:doc_etag(Doc);
- _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
- end,
- ReqAcceptsAttEnc = lists:member(
- atom_to_list(Enc),
- couch_httpd:accepted_encodings(Req)
- ),
- Headers = [
- {"ETag", Etag},
- {"Cache-Control", "must-revalidate"},
- {"Content-Type", binary_to_list(Type)}
- ] ++ case ReqAcceptsAttEnc of
- true when Enc =/= identity ->
- % RFC 2616 says that the 'identify' encoding should not be used in
- % the Content-Encoding header
- [{"Content-Encoding", atom_to_list(Enc)}];
+ #doc_query_args{
+ rev=Rev,
+ options=Options
+ } = parse_doc_query(Req),
+ #doc{
+ atts=Atts
+ } = Doc = couch_doc_open(Db, DocId, Rev, Options),
+ Att = get_existing_attachment(Atts, FileName),
+ [Type, Enc, DiskLen, AttLen, Md5] = couch_att:fetch([type, encoding, disk_len, att_len, md5], Att),
+ Refs = monitor_attachments(Att),
+ try
+ Etag = case Md5 of
+ <<>> -> chttpd:doc_etag(Doc);
+ _ -> "\"" ++ ?b2l(base64:encode(Md5)) ++ "\""
+ end,
+ ReqAcceptsAttEnc = lists:member(
+ atom_to_list(Enc),
+ couch_httpd:accepted_encodings(Req)
+ ),
+ Headers = [
+ {"ETag", Etag},
+ {"Cache-Control", "must-revalidate"},
+ {"Content-Type", binary_to_list(Type)}
+ ] ++ case ReqAcceptsAttEnc of
Review comment:
I agree but that was left as is. The reason the diff looks so large is because almost all of the body in get_attachment got shifted to the left since we removed the
```
case get_attachment(Req, Db, DocId, FileName) of
{error, missing} ->
throw({not_found, "Document is missing attachment"});
{ok, Doc, Att} ->
Body...
end
```
and replaced it with
```
#doc_query_args{
rev=Rev,
options=Options
} = parse_doc_query(Req),
#doc{
atts=Atts
} = Doc = couch_doc_open(Db, DocId, Rev, Options),
Att = get_existing_attachment(Atts, FileName),
Body...
```
--
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