HTTP responses: two fixes and some tests (#96)

* Fix: Always call Request.write()

The spec says 'content' is an optional key, defaulting to b''.
But before this commit, if 'content' wasn't specified, Request.write()
was not called. In conjunction with setting 'more_content' to True,
this would result in nothing being written on the transport. If
'content' was set to b'' instead, the HTTP preamble and any headers were
written as expected. That smells like a bug, so I'm making sure we're
always calling Request.write().

* Require status key in first message to response channel

Previous to this commit, it was possible to not pass in a 'status' key.
This would result in any passed in headers being ignored as well.

Instead of relying on user data ('status' being present or not), this
commit now enforces that the first message to a response channel is
indead a HTTP Response-style message, and hence contains status. It will
complain loudly if that isn't the case.

* Helper for getting HTTP Response for a given channel message

To test Daphne's message-to-HTTP part, we need an easy way to fetch the
HTTP response for a given response channel message. I borrowed the
approach from Andrew's existing code. I feel like we might be able to do
with less scaffolding at some point, but didn't have time to
investigate. It's good enough for now.

* Add assert method to check a response for spec conformance

Similarly to the method for checking HTTP requests for spec conformance,
we're adding a method to do the same for HTTP responses. This one is a bit
less exciting because we're testing raw HTTP responses.

* Add Hypothesis tests for HTTP responses

Similarly to what I did for HTTP requests, this commit adds a couple
test that try to check different parts of the ASGI spec. Because going
from message to HTTP response is more straightforward than going from
HTTP request to channel message, there's not a whole lot going on here.

daphne/http_protocol.py

@@ -227,11 +227,12 @@ class WebRequest(http.Request):
         """
         Writes a received HTTP response back out to the transport.
         """
-        if "status" in message:
+        if not self._got_response_start:
-            if self._got_response_start:
-                raise ValueError("Got multiple Response messages for %s!" % self.reply_channel)
             self._got_response_start = True
-            # Write code
+            if 'status' not in message:
+                raise ValueError("Specifying a status code is required for a Response message.")
+
+            # Set HTTP status code
             self.setResponseCode(message['status'])
             # Write headers
             for header, value in message.get("headers", {}):
@@ -240,9 +241,13 @@ class WebRequest(http.Request):
                     header = header.encode("latin1")
                 self.responseHeaders.addRawHeader(header, value)
             logger.debug("HTTP %s response started for %s", message['status'], self.reply_channel)
+        else:
+            if 'status' in message:
+                raise ValueError("Got multiple Response messages for %s!" % self.reply_channel)
+
         # Write out body
-        if "content" in message:
+        http.Request.write(self, message.get('content', b''))
-            http.Request.write(self, message['content'])
+
         # End if there's no more content
         if not message.get("more_content", False):
             self.finish()

daphne/tests/factories.py

@@ -14,7 +14,23 @@ def message_for_request(method, path, params=None, headers=None, body=None):
     that through daphne and returns the emitted channel message.
     """
     request = _build_request(method, path, params, headers, body)
-    return _run_through_daphne(request, 'http.request')
+    message, factory, transport = _run_through_daphne(request, 'http.request')
+    return message
+
+
+def response_for_message(message):
+    """
+    Returns the raw HTTP response that Daphne constructs when sending a reply
+    to a HTTP request.
+
+    The current approach actually first builds a HTTP request (similar to
+    message_for_request) because we need a valid reply channel. I'm sure
+    this can be streamlined, but it works for now.
+    """
+    request = _build_request('GET', '/')
+    request_message, factory, transport = _run_through_daphne(request, 'http.request')
+    factory.dispatch_reply(request_message['reply_channel'], message)
+    return transport.value()
 
 
 def _build_request(method, path, params=None, headers=None, body=None):
@@ -57,8 +73,8 @@ def _build_request(method, path, params=None, headers=None, body=None):
             quoted_path += b'?' + parse.urlencode(params)
 
     request = method.encode('ascii') + b' ' + quoted_path + b" HTTP/1.1\r\n"
-    for k, v in headers:
+    for name, value in headers:
-        request += k.encode('ascii') + b': ' + v.encode('ascii') + b"\r\n"
+        request += header_line(name, value)
 
     request += b'\r\n'
 
@@ -68,6 +84,13 @@ def _build_request(method, path, params=None, headers=None, body=None):
     return request
 
 
+def header_line(name, value):
+    """
+    Given a header name and value, returns the line to use in a HTTP request or response.
+    """
+    return name.encode('ascii') + b': ' + value.encode('ascii') + b"\r\n"
+
+
 def _run_through_daphne(request, channel_name):
     """
     Returns Daphne's channel message for a given request.
@@ -78,11 +101,11 @@ def _run_through_daphne(request, channel_name):
     channel_layer = ChannelLayer()
     factory = HTTPFactory(channel_layer)
     proto = factory.buildProtocol(('127.0.0.1', 0))
-    tr = proto_helpers.StringTransport()
+    transport = proto_helpers.StringTransport()
-    proto.makeConnection(tr)
+    proto.makeConnection(transport)
     proto.dataReceived(request)
     _, message = channel_layer.receive([channel_name])
-    return message
+    return message, factory, transport
 
 
 def content_length_header(body):

daphne/tests/test_http_response.py

@@ -7,9 +7,84 @@ from __future__ import unicode_literals
 from unittest import TestCase
 
 from asgiref.inmemory import ChannelLayer
+from hypothesis import given
 from twisted.test import proto_helpers
 
-from ..http_protocol import HTTPFactory
+from daphne.http_protocol import HTTPFactory
+from . import factories, http_strategies, testcases
+
+
+class TestHTTPResponseSpec(testcases.ASGITestCase):
+
+    def test_minimal_response(self):
+        """
+        Smallest viable example. Mostly verifies that our response building works.
+        """
+        message = {'status': 200}
+        response = factories.response_for_message(message)
+        self.assert_valid_http_response_message(message, response)
+        self.assertIn(b'200 OK', response)
+        # Assert that the response is the last of the chunks.
+        # N.b. at the time of writing, Daphne did not support multiple response chunks,
+        # but still sends with Transfer-Encoding: chunked if no Content-Length header
+        # is specified (and maybe even if specified).
+        self.assertTrue(response.endswith(b'0\r\n\r\n'))
+
+    def test_status_code_required(self):
+        """
+        Asserts that passing in the 'status' key is required.
+
+        Previous versions of Daphne did not enforce this, so this test is here
+        to make sure it stays required.
+        """
+        with self.assertRaises(ValueError):
+            factories.response_for_message({})
+
+    def test_status_code_is_transmitted(self):
+        """
+        Tests that a custom status code is present in the response.
+
+        We can't really use hypothesis to test all sorts of status codes, because a lot
+        of them have meaning that is respected by Twisted. E.g. setting 204 (No Content)
+        as a status code results in Twisted discarding the body.
+        """
+        message = {'status': 201}  # 'Created'
+        response = factories.response_for_message(message)
+        self.assert_valid_http_response_message(message, response)
+        self.assertIn(b'201 Created', response)
+
+    @given(body=http_strategies.http_body())
+    def test_body_is_transmitted(self, body):
+        message = {'status': 200, 'content': body.encode('ascii')}
+        response = factories.response_for_message(message)
+        self.assert_valid_http_response_message(message, response)
+
+    @given(headers=http_strategies.headers())
+    def test_headers(self, headers):
+        # The ASGI spec requires us to lowercase our header names
+        message = {'status': 200, 'headers': [(name.lower(), value) for name, value in headers]}
+        response = factories.response_for_message(message)
+        # The assert_ method does the heavy lifting of checking that headers are
+        # as expected.
+        self.assert_valid_http_response_message(message, response)
+
+    @given(
+        headers=http_strategies.headers(),
+        body=http_strategies.http_body()
+    )
+    def test_kitchen_sink(self, headers, body):
+        """
+        This tests tries to let Hypothesis find combinations of variables that result
+        in breaking our assumptions. But responses are less exciting than responses,
+        so there's not a lot going on here.
+        """
+        message = {
+            'status': 202,  # 'Accepted'
+            'headers': [(name.lower(), value) for name, value in headers],
+            'content': body.encode('ascii')
+        }
+        response = factories.response_for_message(message)
+        self.assert_valid_http_response_message(message, response)
 
 
 class TestHTTPResponse(TestCase):
@@ -24,39 +99,6 @@ class TestHTTPResponse(TestCase):
         self.tr = proto_helpers.StringTransport()
         self.proto.makeConnection(self.tr)
 
-    def test_basic(self):
-        """
-        Tests basic HTTP parsing
-        """
-        # Send a simple request to the protocol
-        self.proto.dataReceived(
-            b"GET /te%20st-%C3%A0/?foo=+bar HTTP/1.1\r\n" +
-            b"Host: somewhere.com\r\n" +
-            b"\r\n"
-        )
-        # Get the resulting message off of the channel layer
-        _, message = self.channel_layer.receive(["http.request"])
-        self.assertEqual(message['http_version'], "1.1")
-        self.assertEqual(message['method'], "GET")
-        self.assertEqual(message['scheme'], "http")
-        self.assertEqual(message['path'], "/te st-à/")
-        self.assertEqual(message['query_string'], b"foo=+bar")
-        self.assertEqual(message['headers'], [(b"host", b"somewhere.com")])
-        self.assertFalse(message.get("body", None))
-        self.assertTrue(message['reply_channel'])
-        # Send back an example response
-        self.factory.dispatch_reply(
-            message['reply_channel'],
-            {
-                "status": 201,
-                "status_text": b"Created",
-                "content": b"OH HAI",
-                "headers": [[b"X-Test", b"Boom!"]],
-            }
-        )
-        # Make sure that comes back right on the protocol
-        self.assertEqual(self.tr.value(), b"HTTP/1.1 201 Created\r\nTransfer-Encoding: chunked\r\nX-Test: Boom!\r\n\r\n6\r\nOH HAI\r\n0\r\n\r\n")
-
     def test_http_disconnect_sets_path_key(self):
         """
         Tests http disconnect has the path key set, see https://channels.readthedocs.io/en/latest/asgi.html#disconnect

daphne/tests/testcases.py

@@ -4,12 +4,13 @@ Contains a test case class to allow verifying ASGI messages
 from __future__ import unicode_literals
 
 from collections import defaultdict
-
 import six
-import socket
 from six.moves.urllib import parse
+import socket
 import unittest
 
+from . import factories
+
 
 class ASGITestCase(unittest.TestCase):
     """
@@ -122,3 +123,27 @@ class ASGITestCase(unittest.TestCase):
             self.assertIsInstance(server_host, six.text_type)
             self.assert_is_ip_address(server_host)
             self.assertIsInstance(server_port, int)
+
+    def assert_valid_http_response_message(self, message, response):
+        self.assertTrue(message)
+        self.assertTrue(response.startswith(b'HTTP'))
+
+        status_code_bytes = six.text_type(message['status']).encode('ascii')
+        self.assertIn(status_code_bytes, response)
+
+        if 'content' in message:
+            self.assertIn(message['content'], response)
+
+        # Check that headers are in the given order.
+        # N.b. HTTP spec only enforces that the order of header values is kept, but
+        # the ASGI spec requires that order of all headers is kept. This code
+        # checks conformance with the stricter ASGI spec.
+        if 'headers' in message:
+            for name, value in message['headers']:
+                expected_header = factories.header_line(name, value)
+                # Daphne or Twisted turn our lower cased header names ('foo-bar') into title
+                # case ('Foo-Bar'). So technically we want to to match that the header name is
+                # present while ignoring casing, and want to ensure the value is present without
+                # altered casing. The approach below does this well enough.
+                self.assertIn(expected_header.lower(), response.lower())
+                self.assertIn(value.encode('ascii'), response)