Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Fix RequestHeaders to proper concat repeats #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BigBlueHat
Copy link
Member

@BigBlueHat BigBlueHat commented Jul 29, 2016

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):

GET /annotations/ HTTP/1.1
Host: example.org
Accept: application/ld+json; profile="http://www.w3.org/ns/anno.jsonld"
Prefer: return=representation;include="http://www.w3.org/ns/ldp#PreferMinimalContainer"
Prefer: return=representation;include="http://www.w3.org/ns/oa#PreferContainedIRIs"

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:

curl http://localhost:8080 \
-H 'Accept: application/ld+json; profile="http://www.w3.org/ns/anno.jsonld"' \
-H 'Prefer: return=representation;include="http://www.w3.org/ns/ldp#PreferMinimalContainer"' \
-H 'Prefer: return=representation;include="http://www.w3.org/ns/oa#PreferContainedIRIs"'

And a simple server to test it against:

import json

import wptserve

port = 8080
doc_root = './docroot/'

@wptserve.handlers.handler
def multi(request, response):
    return json.dumps(request.headers, indent=4)

routes = [
    ("GET", "*", multi)
]

print 'http://localhost:{0}/'.format(port)

httpd = wptserve.server.WebTestHttpd(port=port, doc_root=doc_root,
                                     routes=routes)
httpd.start(block=True)

Which, if run against each other, should result in this JSON:

{
    "host": "localhost:8080",
    "prefer": "return=representation;include=\"http://www.w3.org/ns/ldp#PreferMinimalContainer\", return=representation;include=\"http://www.w3.org/ns/ldp#PreferMinimalContainer\"",
    "accept": "application/ld+json; profile=\"http://www.w3.org/ns/anno.jsonld\"",
    "user-agent": "curl/7.41.0"
}

Cheers.
🎩


This change is Reviewable

@BigBlueHat
Copy link
Member Author

@halindrome your input here would be most welcome. 😄

@halindrome
Copy link

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


wptserve/request.py, line 355 [r1] (raw file):

            # if there are other headers with this name, we need them
            values = items.getallmatchingheaders(header)
            if len(values) > 1:

Silly question - but won't there always be at least 1 if there was a key in items?


Comments from Reviewable

@BigBlueHat
Copy link
Member Author

@halindrome right. This is checking for situations where there's more than 1 header returned. Happy Monday! ☕

@halindrome
Copy link

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


wptserve/request.py, line 355 [r1] (raw file):

Previously, halindrome (Shane McCarron) wrote…

Silly question - but won't there always be at least 1 if there was a key in items?

Wow - never mind. I was thinking >= 1.... MORE COFFEE.

Comments from Reviewable

@halindrome
Copy link

LGTM


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@jgraham
Copy link
Member

jgraham commented Aug 1, 2016

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


wptserve/request.py, line 360 [r1] (raw file):

                # loop through the values from getallmatchingheaders
                for value in values:
                    if ':' in value:

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 : in the line or if the line starts with whitespace.


Comments from Reviewable

@BigBlueHat
Copy link
Member Author

@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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants