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
Fix for 'Failed to parse headers' warning #1439
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0796637
Add test for spurious header warnings; #1438
timb07 7e78f79
Simplify assert_header_parsing; fixes #1438
timb07 5472700
Revert "Simplify assert_header_parsing; fixes #1438"
timb07 129a895
Fix header test cases to remove unintended header separator
timb07 7b13457
Improve test to check for unparsed_data value
timb07 5c0f06c
Rename header parsing test and fix so it passes; fixes #1438
timb07 7649111
Updated changelog
timb07 9fff9af
Merge branch 'master' into bug1438
sethmlarson f22b64b
Remove platform-specific marker
timb07 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these test cases changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the addition of unparsed_data_check specifically?
I looked at the various Stackoverflow questions where this had come up, and realised we already had a test case where unparsed_data was non-empty, so I thought it would be good to check for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant more the input data, it looks like you've changed the number of CRLF within the test case, what was the motivation for those changes? I'd be more comfortable adding a new testcase for unparsed_data than changing these existing ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the changes in the values of
headers
passed into_test_broken_header_parsing()
and the subsequent generation of the value to pass intoself.start_response_handler()
, here's what two of the test cases were actually checking:Looking in more detail, the
'\r\n'
at the end of each bytestring in this call:combined with
b'\r\n'.join(headers)
in_test_broken_header_parsing()
result in duplicate CRLFs between each of the headers supplied in the method call.The duplicate CRLF before
Another: Header
meant thatAnother: Header
wasn't a part of the headers anymore, and wasn't being checked. In all the present test cases that didn't affect the result of the test, but that was more by coincidence. For instance, in the example above, if the order of the two bytestrings in the list was reversed, the test would fail, that is, no warning would be logged.The best fix seemed to me to remove the CRLFs from the test bytestrings, and ensure the headers are terminated with an explicit CRLFCRLF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's next for this PR? Are you happy with my explanations, or would you prefer me to rework the tests?