Repository: falconry/falcon
Pull request #748 (2016-04-02 07:12:19)

fix(resource): prevent req.stream.read() from throwing assertionerror during tests
Alright, please forgive the rather wordy PR but this was, in my opinion, a really obscure bug.

## Problem

The following TestCase:

```python
class TestLoginResource(testing.TestCase):
    def test_bad_credentials_get_401(self):
        result = self.simulate_post(
            path='/login',
            body=json.dumps({'username': 'foo', 'password': 'bar'})
        )

        self.assertEqual(result.status, falcon.HTTP_401)
```

Produces an `AssertionError` with the following stack trace:

	  File "/wherever/falconproject/middleware.py", line 30, in process_request
	    body = req.stream.read()
	  File "/usr/lib/python3.4/wsgiref/validate.py", line 197, in read
	    assert_(len(args) == 1)
	  File "/usr/lib/python3.4/wsgiref/validate.py", line 128, in assert_
	    raise AssertionError(*args)
	nose.proxy.AssertionError: 

## Cause

Under normal conditions, `req.stream` is an instance of `<class gunicorn.http.body.Body>`. However, during testing it is a `<class wsgiref.validate.InputWrapper>`. Both have `read()` methods, but they differ as such:

`gunicorn.http.body.Body` ([source](https://github.com/benoitc/gunicorn/blob/master/gunicorn/http/body.py#L199)):

```python
def read(self, size=None):
    size = self.getsize(size)
    if size == 0:
        return b""
    # ...snip...
```

`wsgiref.validate.InputWrapper` ([source](https://github.com/python/cpython/blob/1fe0fd9feb6a4472a9a1b186502eb9c0b2366326/Lib/wsgiref/validate.py#L196)):

```python
def read(self, *args):
    assert_(len(args) == 1)
    v = self.input.read(*args)
    assert_(type(v) is bytes)
    return v
```

`InputWrapper` throws an AssertionError because **the call to `req.stream.read()` in my middleware doesn't declare a value for `size`!** Normally it's not needed because gunicorn sets the size automatically, but `InputWrapper` only asserts that it needs to be set.

When `size` is None (as in case of a typical call to `req.stream.read()`), `gunicorn.http.body.Body`'s [`getsize()` method](https://github.com/benoitc/gunicorn/blob/master/gunicorn/http/body.py#L190) returns a value called `six.MAXSIZE`. [Going into `gunicorn.six`](https://github.com/benoitc/gunicorn/blob/master/gunicorn/six.py#L38), we see that, in a Python 3 environment, `MAXSIZE` is simply set to `sys.maxsize` (in Python 2, gunicorn runs several checks to help determine what `MAXSIZE` should be but I wasn't sure if I could just copy-paste it or not. `sys.maxsize` returned the same value locally in Python 2 and Python 3 so I left it as-is in my code).

To get my test to run, I just needed to update my middleware from this:

```python
class JSONTranslator(object):
	def process_request(self, req, resp):
	    if req.content_length in (None, 0):
	        return

	    body = req.stream.read()
```

To this:

```python
class JSONTranslator(object):
	def process_request(self, req, resp):
	    if req.content_length in (None, 0):
	        return
		
		# You'll need to add 'import sys' above
	    body = req.stream.read(sys.maxsize)
```

And calls to `TestCase.simulate_request()` in my tests started to pass. Even better, this appears to have no impact on my server when I run it as a gunicorn instance with `gunicorn app:api`.

Unfortunately this isn't much of a fix. Falcon users would need to pass in `sys.maxsize` every time they call `req.stream.read()` for the sole purpose of allowing tests to pass. That'd get pretty annoying pretty quickly.

## Solution

Fortunately it appears possible to fix this issue by subclassing `wsgiref.validate.InputWrapper` and overriding `read()` to include a size. I created the following `WsgiRefInputWrapperWithSize` class in `falcon.request.request_helpers`:

```python
class WsgiRefInputWrapperWithSize(InputWrapper):
    def read(self, *args):
        return super(WsgiRefInputWrapperWithSize, self).read(sys.maxsize)
```

Then, wrap `self.stream` in `falcon.request.Request.__init__()` if it's an instance of `InputWrapper`:

```python
def __init__(self, env, options=None):
    # ...snip...
    if type(self.stream) is wsgiref.validate.InputWrapper:
        self.stream = helpers.WsgiRefInputWrapperWithSize(self.stream)
    # ...snip...
```

This addresses issue #747 

MasterKale: Whoa, this PR went pretty poorly...I just installed 2.6 locally, I'll keep tweaking things until tox returns green across the board. Should I close this until I get everything working?
kgriffs: Hi @MasterKale, nice catch! Thanks for the comprehensive writeup; makes sense.
MasterKale: @kgriffs I just confirmed, an instance of `Request` isn't created during testing until some time after `wsgiref.validate.validator()` is called ([L327](https://github.com/falconry/falcon/blob/master/falcon/testing/test_case.py#L327) in `testing/test_case.py`). Since L327 is the one that errors out during a `simulate_request()` there'd be no chance for that wrap check in L306 to work its magic.
MasterKale: Okay, as requested I've squashed down to that single-line change to L306 and accompanying tests, and recommitted.
kgriffs: OK, this is almost ready to merge. Just a few final nitpicks to look at. Thanks!
kgriffs: LGTM. Nice work!

Patch:
diff --git a/falcon/request.py b/falcon/request.py
index b2ce9bd..5b4634a 100644
--- a/falcon/request.py
+++ b/falcon/request.py
@@ -26,6 +26,7 @@ except AttributeError:
 
 import mimeparse
 import six
+from wsgiref.validate import InputWrapper
 
 from falcon.errors import *  # NOQA
 from falcon import util
@@ -303,7 +304,7 @@ class Request(object):
         # NOTE(kgriffs): Wrap wsgi.input if needed to make read() more robust,
         # normalizing semantics between, e.g., gunicorn and wsgiref.
         if _maybe_wrap_wsgi_stream:
-            if isinstance(self.stream, NativeStream):
+            if isinstance(self.stream, (NativeStream, InputWrapper,)):
                 self._wrap_stream()
             else:
                 # PERF(kgriffs): If self.stream does not need to be wrapped


Test patch:
diff --git a/tests/test_wsgiref_inputwrapper_with_size.py b/tests/test_wsgiref_inputwrapper_with_size.py
new file mode 100644
index 0000000..96f5b47
--- /dev/null
+++ b/tests/test_wsgiref_inputwrapper_with_size.py
@@ -0,0 +1,36 @@
+import json
+
+import falcon
+from falcon import testing
+
+
+class TypeResource(testing.SimpleTestResource):
+    """A simple resource to return the posted request body."""
+    @falcon.before(testing.capture_responder_args)
+    def on_post(self, req, resp, **kwargs):
+        resp.status = falcon.HTTP_200
+        # NOTE(masterkale): No size needs to be specified here because we're
+        # emulating a stream read in production. The request should be wrapped
+        # well enough to automatically specify a size when calling `read()`
+        # during either production or when running tests
+        resp.body = json.dumps({'data': req.stream.read().decode('utf-8')})
+
+
+class TestWsgiRefInputWrapper(testing.TestCase):
+    def setUp(self):
+        super(TestWsgiRefInputWrapper, self).setUp()
+
+        # Set up a route to our TypeResoure
+        self.type_route = '/type'
+        self.api.add_route(self.type_route, TypeResource())
+
+    def test_resources_can_read_request_stream_during_tests(self):
+        """Make sure we can perform a simple request during testing.
+
+        Originally, testing would fail after performing a request because no
+        size was specified when calling `wsgiref.validate.InputWrapper.read()`
+        via `req.stream.read()`"""
+        result = self.simulate_post(path=self.type_route, body='hello')
+
+        self.assertEqual(result.status, falcon.HTTP_200)
+        self.assertEqual(result.json, {'data': 'hello'})