FormParser and MultiPartParser are never invoked

[paulmelnikow]

While writing my own parser subclass to override handling of application/x-www-form-urlencoded, I discovered a startling bug: FormParser and MultiPartParser are never invoked, nor is it possible to use my own parser when the media type is application/x-www-form-urlencoded or multipart/form-data.

I tracked down the code responsible for this, which is in Request._perform_form_overloading:

# At this point we're committed to parsing the request as form data.
self._data = self._request.POST
self._files = self._request.FILES

Then _load_data_and_files sees _data is there, and doesn't call _parse.

Here are some test cases which illustrate the issue. The first two fail, and the third passes.

from rest_framework import parsers

class TestFormParser(TestCase):

    def setUp(self):
        class TestView(APIView):
            class TestParser(parsers.FormParser):
                def parse(self, stream, media_type=None, parser_context=None):
                    return {'replacing_all_the_content': 'yes'}
            parser_classes = (TestParser,)
            def post(self, request, *args, **kwargs):
                return Response(request.DATA)
        self.view = TestView.as_view()

    def test_form_parser_called(self):
        request = factory.post('/', 'foo=bar', content_type='application/x-www-form-urlencoded')
        response = self.view(request)
        self.assertEquals(response.data, {'replacing_all_the_content': 'yes'})

class TestMultipartParser(TestCase):

    def setUp(self):
        class TestView(APIView):
            class TestParser(parsers.MultiPartParser):
                def parse(self, stream, media_type=None, parser_context=None):
                    return {'replacing_all_the_content': 'yes'}
            parser_classes = (TestParser,)
            def post(self, request, *args, **kwargs):
                return Response(request.DATA)
        self.view = TestView.as_view()

    def test_form_parser_called(self):
        request = factory.post('/', {'foo': 'bar'})
        response = self.view(request)
        self.assertEquals(response.data, {'replacing_all_the_content': 'yes'})

class TestJsonParsers(TestCase):

    def setUp(self):
        class TestView(APIView):
            class TestParser(parsers.JSONParser):
                def parse(self, stream, media_type=None, parser_context=None):
                    return {'replacing_all_the_content': 'yes'}
            parser_classes = (TestParser,)
            def post(self, request, *args, **kwargs):
                return Response(request.DATA)
        self.view = TestView.as_view()

    def test_form_parser_called(self):
        request = factory.post('/', {'foo':'bar'}, format='json')
        response = self.view(request)
        self.assertEquals(response.data, {'replacing_all_the_content': 'yes'})

I tried for a while but couldn't come up with a working fix.

[tomchristie]

Okay - this needs some serious sanity checking!

Related: #1174.

[paulmelnikow]

I've had some more time to work with this issue.

The simple workaround for my case and for #1174 is these settings:

REST_FRAMEWORK = {
    'FORM_METHOD_OVERRIDE': None,
    'FORM_CONTENT_OVERRIDE': None,
}

That entirely disables form overloading and forces the parsers to run.

This begs the question: should form overloading be the default behavior? If so, the view documentation could just show that parser_class doesn't apply to form-encoded requests. (If not, it's an even easier fix.)

If there is some way to support method overriding and a custom parser simultaneously, that would be an even better fix.

Finally, changing those settings in my app caused some unrelated tests to fail. These were using the Django test client with the default multipart serialization, on views which did not have parser_classes defined. I assume MultipartFormParser ideally should behave identically to Request._request.POST, but these results suggest that they behave slightly differently. I got the tests passing by using APIClient and format=json, but when I have a moment I'll look into what was happening and try to capture the discrepancies.

[tomchristie]

Thanks @paulmelnikow I'll try to catch up on this soon.

[paulmelnikow]

Hey Tom, here are some thoughts on this feature. I'm not that familiar with APIs that require content or method overriding. It seems like an unusual use case, but one which some people must use, and that is included in DRF because it's impossible to add in a modular way.

Am I characterizing it right?

If so, what would you think about disabling it by default? That way integrators who need it can turn it on. That way the settings docs can mention the limitation, which is that it interferes with parser overriding.

With the defaults changed, the tests I wrote above should pass.

Or, do you think another solution is more appropriate?

I'd be happy to work on updating the docs or writing some code for this – it makes a module I wrote (drf-to-s3) easier to integrate.

[tomchristie]

Hiya!

Am I characterizing it right?

Not quite, no.

The method and content overriding are part of what enables the browsable API to work the way it does, by allowing you to make PUT/DELETE etc requests with non-form content directly from the browser.

The alternative is for the browsable API to use javascript to make those requests. What's problematic with that is that approach is that it doesn't give you quite what we're looking for...

• When making the request directly, we simply allow the browser to handle the response.
• If making the request via AJAX, we'd be returned some content that we then need to inject into our existing page. This means we're not modifying the URL bar appropriately, and we're required to make assumptions about what media types the server might respond with in order to determine how to inject that content into our existing page.

I think the right solution will be to make sure that we do properly run the custom form parsers in the case of overriding. I think we can sort it out with a bit of tweaking but not had the time to deal with it yet.

[tomchristie]

Note that this is properly resolved in Flask API, where we impose the additional constraint that only URLEncoded form data submissions support method/content type overloading.

Need to take the same approach here.

Ref: https://github.com/tomchristie/flask-api/blob/master/flask_api/request.py#L146

[adamJLev]

+1, running into same issue.

@paulmelnikow I'm curious if we may be trying to override FormParser for same reason :)

I need to override FormParser to add proper handling of jQuery style $.param submissions. (e.g. a form submitting data like ids[]=26&ids[]=78 right now is a pain to deal with in DRF.

[tomchristie]

s/add proper handling/add improper handling/ ;)

But yes, this issue is a PITA, needs some fundamental rewiring to fix.

[paulmelnikow]

Hey, sorry I dropped off there. Yeah, I definitely didn't realize the purpose of the method and content overrides were to support the browsable API. Because of the issue you mention – where we get data back from a JavaScript HTTP request and need to inject it into the page, and deal with the address bar – it's complicated to solve the problem with a more JavaScript / dynamic client.

@adamJLev I'm dealing with nested keys. I use querystring-parser but I don't think it supports the syntax you're looking for. Here's my custom parser.

[adamJLev]

@tomchristie ha yes. proper broken-by-design handling. all PHP's fault.

@paulmelnikow that's almost what I need, thanks!

[tomchristie]

Closing in favor of #1769.