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

Content Negotiation Improvement Proposal #1

Merged
merged 1 commit into from
Sep 12, 2014

Conversation

mlavin
Copy link

@mlavin mlavin commented Apr 15, 2014

This is a rough draft of a proposal for improving Django's support for content negotiation based on previous discussions on django-developers and work by @tomchristie. Currently it's fairly light on technical details but this process is very new and I'm not sure how much as needed at this point. Mostly I just want to revive this discussion and iterate on the details if it is agreed that this is something that would be an improvement to Django.

@pstch
Copy link

pstch commented Apr 15, 2014

This is a great idea (and your DEP seems well written), and I would like to help (if possible). I looked at the tomchristie/ticket_21442 ticket, and it's really great to see his dynamic django.http.request.body function and the parsers module behind it.

Is there any coding work (other than Tom Christie's branch) being done at this time ? If I could find some more example implementations, I'd be very pleased to test them.

Sorry if this isn't the right place to ask those questions (I know that this issue is more about the DEP in itself).

Also, maybe you should provide clear definitions (what goes in, what goes out, side effects if any) of what is a parser and a definition ?

@mlavin
Copy link
Author

mlavin commented Apr 15, 2014

I think this is the right place for your comments. I couldn't find any other examples of currently on going work in this space. Of course the community has a number of solutions in:

With Tastypie and Rest Framework handle serializers which handle both incoming and outgoing types, the Piston Mimer is only meant for incoming requests and outgoing requests are handled by emitters. The serializers at times mix the parsing of incoming content types and the validation which I'm leaving out of the scope of this DEP. I'm going to continue to work on clarifying what exactly these APIs will be. I like Tom's general direction but I'd like to see a solution which doesn't include more settings. For most of the current solutions in the community these parsers are defined at the resource level. I wonder if that is largely due to the fact that they can't easily modify the parsing at global level or whether people believe it should be done on a per view/resource basis.

@foresmac
Copy link

foresmac commented May 2, 2014

I could be wrong, but REST Framework has parsers that are separate from serializers. The serializers basically function a little more like forms; parsers themselves handle raw input and convert into standard Python datatypes.

@mlavin
Copy link
Author

mlavin commented May 2, 2014

Yes that's the same approach which is being described here.

@mlavin
Copy link
Author

mlavin commented May 2, 2014

Actually to be clear the input-side validations are out of the scope of this proposal.

@foresmac
Copy link

foresmac commented May 2, 2014

If I understand correctly, you're merely talking about sucking in the body of a request and converting it into some useful dictionary of Python datatypes/objects. Obviously JSON is the most common in current usage, but it could be YAML, XML, JUnit, CSV, plain text, whatever. Validation would still be handled by forms. Correct?

Would it also be worthwhile to make the API work in both directions? In other words, could a JSONParser be used to output properly formatted JSON on the response?

@mlavin
Copy link
Author

mlavin commented May 2, 2014

Correct. A JSON parser would be included with Django as part of this work. Additional parsers (YAML, XML, CSV) could be written and maintained as external projects. A description of how additional parsers would be registered is included in the proposal. Validations would be handled by forms or extensions of forms such as django-rest-frameworks serializers.

This proposal does the response formatting as well. See the section on Reponse Types/Emitters https://github.com/django/deps/pull/1/files?short_path=76d7d47#response-types I don't see it being handled by the same parsing API functions/classes but that's more of an implementation detail.

@tomchristie
Copy link
Member

Hi @mlavin really great to see you getting this moving. The proposal looks good and all seems well-thought out.

I'm a little unsure about the 'no settings' approach at the moment, as described it essentially just moves the parser configuration out of the settings.py module and into the wsgi.py module. If that's part of a broader plan for how Django configuration works then that might be okay, but if not I'm not sure I'd see the benefit in moving away from the standard approach. Agreed, adding new settings is something we should tend away from, but in this case I think it's justified.

My personal suggestion is that the way parsers are configured and set on the request should closely follow the request.upload_handlers and FILE_UPLOAD_HANDLERS API, as the file parsing this provides is pretty much an exact parallel to the request parsing we'd be supporting. An equivalent request.parsers and REQUEST_PARSERS API would seem to be adequate and require less of a mindset change for developers.

You might also consider having the media_type be a property on the parser itself, rather than a direct mapping from media type strings to parsers. Having the parsers set as a list rather than a mapping would allow for server preference to be taken into account during content negotiation as well as client preference.

My main priority on any of this would be trying to ensure that REST framework (or a future release of it) nicely dovetails with any Django APIs - I think it'd be hugely beneficial for the community if we can make sure that the existing work that's been done on third party parsing and rendering can transition easily to being using directly with Django.

Some quick points on this that I can follow up in more detail if needed.

  • I'm now strongly leaning towards using a lowercase request.data instead of the caps casing that's really just a hangover from PHP and HTTP request method names being uppercased.
  • After a lot of thought I'm also leaning towards the property exposing all the parsed data (included any files that were part of the request body) - I won't go into the reasons for this in depth just now, but I think it presents a cleaner parsing interface (as parser classes don't need to differentiate between returning either a single data item, or a pair of data and files items.) and also ends up allowing for cleaner serializer API design.

I'm sure I'll have more to add as the proposal progresses, but looking really good! Thanks again.

Tom

@mlavin
Copy link
Author

mlavin commented May 3, 2014

Thanks for the feedback! I would say the motivations for not introducing another setting is two fold. One that feels like the general direction of Django in recent releases: trying to break dependencies on the settings as global state. Two there aren't good patterns for extending settings while keeping the defaults. There are a few in the community but none that I can find documented by Django itself. This gives users a foot-gun to do things like remove the form-encoded parsers with something like

REQUEST_PARSERS = (
    'my.custom.parser',
)

This comes up often in the case of TEMPLATE_CONTEXT_PROCESSORS when users try to add django.core.context_processors.request. I don't know if you see that frequently for Django Rest Framework. The common case is to add additional parsers and maybe extend existing ones but I think removing the limited default parsers (form-encoded, multi-part, json) would be a rare case. I'd like the API to reflect that and I'm open to other solutions.

To me the most ideal solution is to deprecate FILE_UPLOAD_HANDLERS and allow for passing a custom subclass of WSGIRequest to get_wsgi_application which could declare its additional parsers and upload handlers. This is the approach of Werkzeug: http://werkzeug.pocoo.org/docs/request_data/#how-to-extend-parsing

@tomchristie
Copy link
Member

I would say the motivations for not introducing another setting is two fold.

Those points are reasonable enough. I don't feel particularly strongly about the settings.py configuration vs. wsgi.py/middleware configuration, so I'd rather leave that judgement to others.

To me the most ideal solution is to deprecate FILE_UPLOAD_HANDLERS

Indeed, if you do go with the no-settings approach it'd make sense to take the same style with the file parsing configuration. Though I'm not sure it follows that subclassing WSGIRequest would necessarily be the right API to deal with that - rather use an API on the application object in the same way you've suggested for the parsers, no?

@mlavin
Copy link
Author

mlavin commented May 4, 2014

Though I'm not sure it follows that subclassing WSGIRequest would necessarily be the right API to deal with that - rather use an API on the application object in the same way you've suggested for the parsers, no?

Yes it's more a matter of taste. That is should the application returned by get_wsgi_application be a black box callable or should it expose addition APIs beyond the WSGI specification. The current proposal is written as the latter but I go back and forth about it myself.

@akaariai
Copy link
Member

We are trying to get the DEP process started again.

Could you squash the commits into one? After that we can merge the proposal to drafts.

@mlavin
Copy link
Author

mlavin commented Sep 11, 2014

Yes I'll incorporate some of the above feedback that I haven't yet addressed and squash my commits.

Rename content-negotiation.txt to content-negotiation.rst

Update copyright.

More detail on the request parsing API.

More detail on the response handling API.

Minor tweaks to the DEP.
@mlavin
Copy link
Author

mlavin commented Sep 12, 2014

@akaariai the commits have been squashed.

akaariai added a commit that referenced this pull request Sep 12, 2014
Content Negotiation Improvement draft DEP
@akaariai akaariai merged commit b6684b7 into django:master Sep 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants