Skip to content
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

StringStubbingResource.render() pukes when request has headers #259

Open
wsanchez opened this issue Oct 18, 2019 · 3 comments
Open

StringStubbingResource.render() pukes when request has headers #259

wsanchez opened this issue Oct 18, 2019 · 3 comments
Labels

Comments

@wsanchez
Copy link
Member

I have some code following the example in the docs for treq.testing.RequestSequence and I find that if the response tuple includes any headers, it causes an exception which shows none my my code in the traceback:

===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/wsanchez/Dropbox/Developer/Twisted/txdockerhub/.tox/coverage-py37/lib/python3.7/site-packages/twisted/web/server.py", line 217, in process
    self.render(resrc)
  File "/Users/wsanchez/Dropbox/Developer/Twisted/txdockerhub/.tox/coverage-py37/lib/python3.7/site-packages/twisted/web/server.py", line 284, in render
    body = resrc.render(self)
  File "/Users/wsanchez/Dropbox/Developer/Twisted/txdockerhub/.tox/coverage-py37/lib/python3.7/site-packages/treq/testing.py", line 317, in render
    request.setHeader(k, v)
  File "/Users/wsanchez/Dropbox/Developer/Twisted/txdockerhub/.tox/coverage-py37/lib/python3.7/site-packages/twisted/web/http.py", line 1305, in setHeader
    self.responseHeaders.setRawHeaders(name, [value])
  File "/Users/wsanchez/Dropbox/Developer/Twisted/txdockerhub/.tox/coverage-py37/lib/python3.7/site-packages/twisted/web/http_headers.py", line 220, in setRawHeaders
    for v in self._encodeValues(values)]
  File "/Users/wsanchez/Dropbox/Developer/Twisted/txdockerhub/.tox/coverage-py37/lib/python3.7/site-packages/twisted/web/http_headers.py", line 220, in <listcomp>
    for v in self._encodeValues(values)]
  File "/Users/wsanchez/Dropbox/Developer/Twisted/txdockerhub/.tox/coverage-py37/lib/python3.7/site-packages/twisted/web/http_headers.py", line 40, in _sanitizeLinearWhitespace
    return b' '.join(headerComponent.splitlines())
builtins.AttributeError: 'list' object has no attribute 'splitlines'

txdockerhub.v2.test.test_client.ClientTests.test_ping_noToken
-------------------------------------------------------------------------------

After looking at treq/testing.py:317, I can see the problem, which is that in line 293 the code creates a dict called headers from the keys and values returned by request.requestHeaders.getAllRawHeaders(). Note that the values in this case are lists of bytes.

Then back in line 317, there's a call to request.setHeader(k, v), where k and v are the keys and values kept in headers. But request.setHeader() expects values to be bytes, not lists.

I think the code should call request.responseHeaders.setRawHeaders(k, v) instead.

@wsanchez wsanchez added the bug label Oct 18, 2019
@twm
Copy link
Contributor

twm commented Oct 19, 2019

I think you're getting tripped up by a weird asymmetry in the RequestSequence API.

treq/src/treq/testing.py

Lines 296 to 298 in 2d28bf5

headers = {}
for k, v in request.requestHeaders.getAllRawHeaders():
headers[k] = v

At this point headers is the request headers. Its type is Dict[bytes, List[bytes]].

treq/src/treq/testing.py

Lines 316 to 318 in 2d28bf5

status_code, headers, body = self._get_response_for(
request.method, absoluteURI, params, headers,
request.content.read())

Now headers is the response headers of type Dict[bytes, bytes] (or really Dict[Union[bytes, Text], List[Union[bytes, Text]]] if you like).

I can't defend any of this. The type mismatch is surprising API design and reassigning headers is downright sneaky. But you can pass response headers — just don't give a list of header values.

(This would have passed silently before twisted/twisted#999. The changes in that PR have found an amazing number of bugs.)

@glyph
Copy link
Member

glyph commented Nov 12, 2019

(Thank you @markrwilliams for braving that spec and making it all work)

@twm
Copy link
Contributor

twm commented Nov 14, 2019

Yes, absolutely. We should do the same with Resource.putChild().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants