-
Notifications
You must be signed in to change notification settings - Fork 89
DEP 0015 Content Type Parsing #88
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
455c94f
f8ae2b2
7ca3842
12380d9
5053ebb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,149 @@ | ||||||||||||||||||
| DEP 0015: Content type aware parsing and modernization of the HttpRequest API. | ||||||||||||||||||
|
|
||||||||||||||||||
| DEP | ||||||||||||||||||
|
|
||||||||||||||||||
| 0015 | ||||||||||||||||||
|
|
||||||||||||||||||
| Author | ||||||||||||||||||
|
|
||||||||||||||||||
| David Smith | ||||||||||||||||||
|
|
||||||||||||||||||
| Implementation Team | ||||||||||||||||||
|
|
||||||||||||||||||
| TBC | ||||||||||||||||||
|
|
||||||||||||||||||
| Shepherd | ||||||||||||||||||
|
|
||||||||||||||||||
| TBC | ||||||||||||||||||
|
|
||||||||||||||||||
| Status | ||||||||||||||||||
|
|
||||||||||||||||||
| Draft | ||||||||||||||||||
|
|
||||||||||||||||||
| Type | ||||||||||||||||||
|
|
||||||||||||||||||
| Feature | ||||||||||||||||||
|
|
||||||||||||||||||
| Created | ||||||||||||||||||
|
|
||||||||||||||||||
| 2024-03-25 | ||||||||||||||||||
|
|
||||||||||||||||||
| Last-Modified | ||||||||||||||||||
|
|
||||||||||||||||||
| 2024-05-29 | ||||||||||||||||||
|
|
||||||||||||||||||
| Table of Contents | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Abstract | ||||||||||||||||||
| ======== | ||||||||||||||||||
|
|
||||||||||||||||||
| Currently Django can parse requests for ``application/x-www-form-urlencoded`` and ``multipart/form-data`` types. Other types, such as JSON are currently returned as a string. | ||||||||||||||||||
|
|
||||||||||||||||||
| This DEP proposes to add configurable content type parsers to allow parsing of additional content types. It is proposed that Django will include a parsing of JSON, and appriate hooks to allow users to add custom parsers for other content types. | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| Parsed data from an ``HttpRequest`` is accessed via its ``POST`` attribute. It would be a breaking change if Django were to start parsing content types where currently a string is returned. To avoid introducing a breaking change it is proposed that a new ``data`` attribute is added for the new behaviour. | ||||||||||||||||||
|
|
||||||||||||||||||
| While introducing a new name for ``POST`` it is proposed that the names for the other attributes are modernized with an equivalent behaviour. | ||||||||||||||||||
|
||||||||||||||||||
| While introducing a new name for ``POST`` it is proposed that the names for the other attributes are modernized with an equivalent behaviour. | |
| While introducing a new name for ``POST`` it is proposed that the names for the other attributes are modernized with an equivalent behaviour: | |
| * ``GET`` -> ``query_params`` | |
| * ``POST`` -> ``form_data`` | |
| * ``COOKIES`` -> ``cookies` | |
| * ``META`` -> ``meta`` | |
| * ``FILES`` -> ``files`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think form_data is confusing.
In the first place, GET forms are a thing. But once you're parsing other content types from the body, there's no form even in play at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post_form_data ? I would just like another lowercase name for the existing attribute so it’s not left uppercase-only, requiring users to adopt data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you mean as well as then adding the data attribute? I'd misunderstood.
OK, yes, something like that makes sense.
Let's discuss in Vigo, where we can likely bikeshed it to death over less than a single coffee ☕️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having spoken with @adamchainz at DjangoCon, I agree with him. Adding form_data as an alias to POST, maintaining the existing behaviour is a good idea. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tim-schilling I think this is the key point:
We can’t find-and-replace request.POST with request.data without adding new functionality unsafely.
The new .data attribute isn't behaviourally neutral. We need a safe migration path for existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for that Carlton. That makes total sense.
However, I think we're trading sources of confusion by adding form_data. I think it's totally possible for a newer dev to misunderstand the differences between request.form_data and a django.forms.Form's data and cleaned_data. We'd need them to know that form_data is the data you pass into the Form class. It's not your form's actual data to be used (cleaned_data)
def view(request):
if request.method == "POST":
form = MyForm(request.form_data)
if form.is_valid():
some_value = form.cleaned_data["my_field"]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...naming this something closer to the HTTP spec...
The content-type is either application/x-www-form-urlencoded or multipart/form-data so request.form_data is pretty close, right? If we try to be sure to always refer to it as request.form_data, not just form_data, that might help. A full name could be "request body form data".
Edit: request.post_form_data is also probably fine and more clear, because in html forms you can't use method="PUT" or anything else. (or request.body_form_data??)
...differences between
request.form_dataand adjango.forms.Form'sdata...
- In normal cases
request.form_dataandform.datawill be the same thing, right?
(I still personally think that application/json should end up as request.json, like how requests' response.json() works. Same with response.json() in javascript's fetch(). I think it would be more clear than an intentionally generic request.data which could be confusing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request.json is also found in Pyramid, alongside request.body (bytes) and others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In normal cases request.form_data and form.data will be the same thing, right?
Yes that's true. I suppose the fallout of this confusion is smaller than I originally thought. Good call out.
smithdc1 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
smithdc1 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reread the DEP due to a thread on the forum. Is this actually true? While it is true that the new .data attribute is not 100% equivalent it doesn't seem true that POST would ever return a string but rather an empty QueryDict if the content type is not something that it supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's probably talking about JSON multiparts (but I'd need to look at tests to recall 100%)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In django/django#17546 (comment) it's decided not to have a setting for this. I think we should preserve that decision and rationale in this DEP too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that the DEP just assumes that configurable content type parsing is desirable. I think the reasons why we want this should be much more spelled out in this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This DEP is needed to add configurable content type parsing to Django. While this is likely to be well received the proposal also suggests adding new aliases which is more controversial. | |
| This DEP is needed to add configurable content type parsing to Django. Since Django takes a batteries included approach, supporting content type parsing for common content types such as ``application/x-www-form-urlencoded``, ``multipart/form-data`` and ``application/json`` is expected for a modern web application framework. It should be configurable so that the community can extend the content types that Django applications can support. This allows the eco-system the flexibility to grow with the rest of web development industry. | |
| While this is likely to be well received the proposal also suggests adding new aliases which is more controversial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can also add a note that django-upgrade could automate refactoring code to use the new lowercased attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial work showing this is possible: smithdc1/django-upgrade@c043761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| A setting was considered for managing the content parsers for a Django application. The setting approach was dismissed because configuring the parsers is seen as something rare. It would be similar to the `FILE_UPLOAD_HANDLERS` setting which is rarely changed from the default. The solution is to have the application configure the parsers in the view or early in the middleware. |
@LilyFoote Regarding your comment here: https://github.com/django/deps/pull/88/files#r2041206276
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, thanks!
I think the wording can be improved in one place - it talks about configuring parsers being rare, but then compares to FILE_UPLOAD_HANDLERS being frequently changed from the default. I think I get what was intended here, but rewording would make it easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Good catch. Fixed in the suggestion to say "setting which is rarely changed"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me now! 🚀
Uh oh!
There was an error while loading. Please reload this page.