Falcon: Processing empty req.media results in 400

[alysivji]

The following code snippet results in a 400 when making a POST request with an empty body.

import falcon
import logging

logger = logging.getLogger()


class QuoteResource:
    def on_post(self, req, resp):
        """Handles GET requests"""
        quote = {
            'quote': (
                "I've always been more interested in "
                "the future than in the past."
            ),
            'author': 'Grace Hopper'
        }

        logger.info(req.media)

        resp.media = quote

api = falcon.API()
api.add_route('/quote', QuoteResource())

I would expect req.media to return an empty dictionary if the body is empty.

[vytas7]

An empty body is, strictly speaking, not a valid JSON representation.

I do not think an empty dict is fitting in this scenario. However, one could maybe consider making req.media reduce to None in this case, as test.Request.json will start behaving in 2.0 (see the related discussion in, for instance, https://github.com/falconry/falcon/issues/1205 )

On the other hand, as said above, the correct way to serialize None in JSON is null, not an empty string:

>>> json.loads('')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 382, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded

An unrelated note: although YAML is almost a superset of JSON, it does treat empty payload differently:

>>> yaml.load('') is None
True

[foresmac]

I think there are a few ways to maybe change some behavior and make the framework a little more flexible in dealing with incoming request data. A solution here might use one or more of the following:

1. Don't allow/rely on the handler to raise BadRequest
2. set req.media to some consistent value (e.g. None) when the media handler can't return valid results.
3. Let the resource handler explicitly raise Bad Request based on the value of req.media.
4. I know that accessing the request body via req.stream.read() is there for performance reasons, but there should be some kind of API that caches the result of that read on the Request object so it can be accessed more than once per request _if needed_. As it is, req.stream is readable but not seekable, so once it's been consumed, that's it; you can't get to its data ever again. If req.media results in an error, you can't do anything useful with whatever the request body was, because now you can't see it.

For the last one, I'd suggest something like req.body as the recommended way to get the contents of the request. If it hasn't been accessed, read the stream and return it. If it has, return the cached value. Maybe sort of like:

class Request:
    self._body = EMPTY_VALUE

    @property
    def body(self):
         if self._body == EMPTY_VALUE:
            self._body = self.stream.read()
        return self._body

[foresmac]

I just wanted to add that where this is a performance bottleneck, req.stream.read() could still be used when desired.

[kgriffs]

I agree that it would be useful to be able to examine the request body when it fails to decode. But I'm not convinced that an error shouldn't be raised as well, since it _is_ an exceptional circumstance, assuming user-agents usually submit well-formed bodies (this hearkens back to the old return-code vs. exceptions debate). One possible solution would be to modify JSONHandler such that it raises a subclass of HTTPBadRequest which has an additional attribute containing the original request body. The caller could then catch said exception and examine the raw body if so desired.

Re adding a Request.body attribute, I'm not categorically opposed to the idea (although it may be more appropriate to call it Request.data to mirror Response.data), but as you say, it must be opt-in. That becomes a bit non-obvious when it comes to something like Request.media that might or might not use the buffered value; presumably that would be controlled via RequestOptions.

[foresmac]

I think ultimately if calling request.media raises an exception when trying to deserialize, it should set the value to None so that middleware can check to see if it's None or not and do something appropriate if that's the case. I'm happy to opt-in to req.media first storing the result of req.stream.read() as req.data before attempting to deserialize, so that—for example—if I want to log all bad request payloads for later analysis, I could do something like:

payload = req.media if req.media is not None else req.data
logger.error(payload)

[foresmac]

Just so we're clear, end users can opt into the body caching by calling req.data instead of reading the stream; but you're talking about how to opt in/out of it when using things like req.media that have a side-effect. It seems like the trade off is fairly small in terms of time, but more so in memory use. Probably worth profiling to decide which would be the default; my thinking is for media to use data as the default for developer convenience, and opt out for performance tuning if needed, but that might not necessarily fit in with falcon's "bare-minimum" approach.

[foresmac]

@kgriffs This is what I came up with to pass along to the API constructor as request_type; this enforces reading bounded_stream to a _data attribute and reading it with the data property. I can adapt this to use an opt-in, but I wasn't sure what a good API for that would be.

from falcon.request import Request


class AnalyteRequest(Request):
    """
    This subclass of falcon's `Request` adds a `data` property, which is the
    raw bytes of the request body. This will retain the value of the request
    body in the even that calling `media` fails to decode that data.
    """

    def __init__(self, env, options=None):
        self._data = None
        super().__init__(env, options)

    @property
    def data(self):
        if self._data is not None:
            return self._data

        self._data = self.bounded_stream.read()
        return self._data

    @property
    def media(self):
        if self._media:
            return self._media

        handler = self.options.media_handlers.find_by_media_type(
            self.content_type,
            self.options.default_media_type
        )

        self._media = handler.deserialize(self.data)
        return self._media

[foresmac]

To @vytas7's point, maybe it should be:

try:
    self._media = handler.deserialize(self.data)
except:
    self._media = None

Or something along those lines.

[tribals]

The parsing of request body is not so simple as it looks at first glance. For example, HTTP spec says that GET request _can_ have body, but it _shouldn't have any meaning_ (to the server).

Of what I'm definitely sure is that _accessing req.media shouldn't result in any exception raised_, be it empty or not. [1]

(I placed this statement on separate line intentionally)

Why so? Think of middlewares. When you are writing resource, you are crystal-clear assured which HTTP verb exactly you are processing - you implement different methods for each one - on_get, on_post and so on. You may write a lot of simple resources which responds to only GET method and not encounter this kind of problem. But you will be very surprised when you are going to implement your first middleware that deals with request body in some way, because middleware processes _every request_, be it GET, POST or any other HTTP verb. We are, in our company, using _a lot_ of middlewares.

Personally I prefer to return None for empty request body. It is not comply to all demands, but think of it again: when request body is empty, you should return _something_ (if we agree with statement [1]), or, in other words, you should return _nothing_. And, in Python, _nothing_ is, simlpy, None.

The root cause of problem, I think, is that we need a way to distinguish whether the body was _actually empty_ or media handler just failed to parse it. By now we can't do that because of of missing interface of media handlers and ugly interfaces of parsers. For example, json doesn't distinguish between empty and malformed input:

>>> import json
>>> json.loads('')
Traceback (most recent call last):
< ...omitted... >
ValueError: No JSON object could be decoded
>>> json.loads('/')
Traceback (most recent call last):
< ...omitted... >
ValueError: No JSON object could be decoded

But this can be achieved by wrapping parsing errors into specialized exceptions. Also, I think that raising HTTPError (which causes the immediate response) is not responsibility of media handler. This leads to hard-to-debug problems, if you don't know how req.media works under the hood.

Here is the example:

class MediaHandlerError(Exception): pass
class EmptyBodyError(MediaHandlerError): pass
class RequestBodyParsingError(MediaHandlerError): pass


class JsonMediaHandler(BaseHandler):
    def deserialize(self, raw):
        if raw == '':
            raise EmptyBodyError
        try:
            return json.loads(raw)
        except ValueError as e:
            raise RequestBodyParsingError(raw) from e

And Request:

_unserialized = object()  # sentinel


class Request(falcon.Request):
    def __init__(self, env, options=None):
        super().__init__(env, options)

        self._media = _unserialized

    @property
    def media(self):
        if self._media is _unserialized:
            handler = self.options.media_handlers.find_by_media_type(
                self.content_type, self.options.default_media_type
            )
            try:
                self._media = handler.deserialize(self.bounded_stream.read())
            except EmptyBodyError:
                self._media = None

        return self._media

And somewhere in default error handler:

    try:
        # calling responder
    except RequestBodyParsingError as e:
        raise falcon.HTTPBadRequest(
            'Invalid JSON', 'Could not parse JSON body - {0}'.format(e)
        )

This way we give a developer full control of how things goes: if he/she want to catch errors of parsing request body, he can do so. If not - well, default handler will return 400 Bad Request. If we catch parsing errors in, for example, Request.media property, we divest a programmer of such ability - he should catch HTTErrors, which is ugly. What is it to HTTPError in request body parsing? Nothing. What if a developer want to respond with different HTTP code? Catch HTTPError. Ugly.