You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "rnewson (via GitHub)" <gi...@apache.org> on 2023/05/04 16:52:32 UTC
[GitHub] [couchdb] rnewson opened a new pull request, #4574: WIP remove Content-MD5 header support
rnewson opened a new pull request, #4574:
URL: https://github.com/apache/couchdb/pull/4574
Part of a series of changes to expunge MD5 entirely.
--
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] rnewson commented on pull request #4574: WIP remove Content-MD5 header support
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#issuecomment-1539935931
closes https://github.com/apache/couchdb/issues/4566
--
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] rnewson commented on a diff in pull request #4574: WIP remove Content-MD5 header support
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1185940730
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
If I'd removed the (0, _Footers) clause it would have caused a problem, yes. any footer present would be interpreted as a chunk and that's no good.
--
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] rnewson commented on a diff in pull request #4574: WIP remove Content-MD5 header support
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1186437050
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
sure.
--
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] rnewson merged pull request #4574: remove Content-MD5 header support
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson merged PR #4574:
URL: https://github.com/apache/couchdb/pull/4574
--
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] rnewson commented on a diff in pull request #4574: remove Content-MD5 header support
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1188607762
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
I've restored the original 'reject' tests and altered them to assert that those things now work (since we no longer support the Content-MD5 header).
--
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] rnewson commented on a diff in pull request #4574: WIP remove Content-MD5 header support
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1185543644
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
Good question. My view is that this PR (assuming it's right and complete) yields a CouchDB that completely ignores the Content-MD5 header, so it behaves identically whether the header is absent, present with the right value or present with the wrong value. Do we need tests for something that has no effect on CouchDB just because it did previously?
--
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 diff in pull request #4574: WIP remove Content-MD5 header support
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1185558588
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
I was mainly wondering about the trailer and if content md5 was the only trailer we ever accepted. As long we can ignore it and it won't mess up parsing of chunked requests that's fine, no need for extra tests.
--
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 diff in pull request #4574: WIP remove Content-MD5 header support
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1186338626
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
Good point, agree.
Should we at least leave a comment when we consumer the _Footer to mention that at some point we used to accept trailers and we need that section to consume them, otherwise it would break the parsing logic?
Maybe leave at least one test sending some footer junk (doesn't have to be MD5) in case this is the only place we actually tested that logic.
--
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 diff in pull request #4574: WIP remove Content-MD5 header support
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1185291957
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
Should we assert what happens if users keep uploading stuff with md5 both valid and invalid ones?
--
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 diff in pull request #4574: WIP remove Content-MD5 header support
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4574:
URL: https://github.com/apache/couchdb/pull/4574#discussion_r1186338626
##########
src/couch/test/eunit/couchdb_attachments_tests.erl:
##########
@@ -102,13 +102,7 @@ attachments_md5_tests() ->
fun teardown/1,
[
fun should_upload_attachment_without_md5/1,
- fun should_upload_attachment_by_chunks_without_md5/1,
- fun should_upload_attachment_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_header/1,
- fun should_upload_attachment_by_chunks_with_valid_md5_trailer/1,
- fun should_reject_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5/1,
- fun should_reject_chunked_attachment_with_invalid_md5_trailer/1
Review Comment:
Good point, agree.
Should we at least leave a comment when we consume the _Footer to mention that at some point we used to accept trailers and we need that section to consume them, otherwise it would break the parsing logic?
Maybe leave at least one test sending some footer junk (doesn't have to be MD5) in case this is the only place we actually tested that logic.
--
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