-
Notifications
You must be signed in to change notification settings - Fork 32
Fix RequestHeaders to proper concat repeats #87
base: master
Are you sure you want to change the base?
Conversation
@halindrome your input here would be most welcome. 😄 |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. wptserve/request.py, line 355 [r1] (raw file):
Silly question - but won't there always be at least 1 if there was a key in items? Comments from Reviewable |
@halindrome right. This is checking for situations where there's more than 1 header returned. Happy Monday! ☕ |
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. wptserve/request.py, line 355 [r1] (raw file):
|
LGTM Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion. Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions. wptserve/request.py, line 360 [r1] (raw file):
This seems technically wrong, since you don't do anything for continuation lines. I would be happy if you raised a 400 error in such cases i.e. if there is no Comments from Reviewable |
@jgraham sorry. yeah. that was left over from an earlier approach. I'll clean it out and resend shortly. Appologies! |
Previously RequestHeaders only provided the last instance of a repeated header.
098390f
to
1bc7383
Compare
Previously RequestHeaders only provided the last instance of a repeated header.
Sadly, writing a unit test for this has proved...daunting. Python libraries from urllib's 1-3, Requests, and even httplib do not allow one to construct a header sequence like the one below (from the Web Annotation Protocol spec):
Most of these libraries use a dictionary style interface, and when given multiple values for a single header, they send them as a comma separated list (as they should! re: RFC 7230, RFC 2616, etc). However, because of that writing a test that would match the exact request seen above is...non-trivial (with the most likely suggestions being to "just use the sockets library")--which goes beyond what I care to attempt (or have time to...) in order to prove that this works. 😖
So. Instead, here's one loverly curl line for your copy/paste enjoyment:
And a simple server to test it against:
Which, if run against each other, should result in this JSON:
Cheers.
🎩
This change is