[Django] #17942: JSONResponse class for API responses
Django
-------------------------------+--------------------
Reporter: leahculver | Owner: nobody
Type: New feature | Status: new
Component: Uncategorized | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
To make APIs, many developers have created their own JSONResponse classes
which are subclasses of Response but returning JSON and with the proper
mimetype ("application/json").
Some examples:
* https://gist.github.com/1265346
*
https://github.com/coderanger/statusboard/blob/master/statusboard/utils/json.py
* http://chronosbox.org/blog/jsonresponse-in-django
* http://djangosnippets.org/search/?q=jsonresponse
Related to this wiki page: https://code.djangoproject.com/wiki/AJAX
This patch should include:
* mimetype "application/json"
Optional ideas:
* support for a callback parameter for JSON-P style responses (see
https://gist.github.com/1265346)
* more status codes (see https://gist.github.com/1265346)
* option to pass a model with a "to_dict", "to_list", or "to_json"
function (see
https://github.com/coderanger/statusboard/blob/master/statusboard/utils/json.py)
* support for XML, if anyone likes to make XML APIs anymore?
--
Ticket URL: <https://code.djangoproject.com/ticket/17942>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: New feature | Status: new
Component: Uncategorized | Version: 1.3
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Changes (by leahculver):
* needs_docs: => 1
* needs_tests: => 1
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:1>
Django
Type: New feature | Status: new
Component: Uncategorized | Version:
Keywords: | Triage Stage: Unreviewed
Changes (by leahculver):
* version: 1.3 =>
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:2>
Django
Type: New feature | Status: new
Component: Uncategorized | Version:
Keywords: | Triage Stage: Unreviewed
Changes (by coderanger):
* cc: noah@… (added)
Comment:
Additional notes: should include {{{ensure_ascii=False}}} in the dumps
call. Should use DjangoJSONEncoder by default to get datetime/decimal
serialization.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:3>
Django
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Changes (by jezdez):
* component: Uncategorized => HTTP handling
* stage: Unreviewed => Accepted
Comment:
Accepting in general as it seems to be a commonly used feature, indeed.
About the XML response class I'm a bit reluctant though, as this smells
like trouble.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:4>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by LukaszBalcerzak):
* owner: nobody => LukaszBalcerzak
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:5>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by LukaszBalcerzak):
* needs_docs: 1 => 0
* has_patch: 0 => 1
* version: => master
* needs_tests: 1 => 0
Comment:
Added pull request: https://github.com/django/django/pull/1182
Implementation motiviations:
1. Trying to support JSONP callbacks would require to somehow allow Cross-
origin resource sharing. I would say it's not needed for most use cases.
2. If extra status codes are needed they can be easily created using
`status` parameter/`status_code` attribute. It is also not clear if
`JsonResponse` users would want to return json responses on errors (most
probably but I guess not always).
3. Idea of allowing to serialize models/querysets is not bad and this
point is actually I am most interested to get feedback on.
4. I would leave XML for now. Especially as this ticket is specifically
about JSON.
Please let me know your thoughts.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:6>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* cc: eromijn@… (added)
* keywords: => dceu13
* needs_better_patch: 0 => 1
Comment:
I haven't done a very thorough review right now, but I do notice a few
issues:
* Is the adding of JSON_RESPONSE_DEFAULT_ENCODER really essential? In
general, we try not to add new settings unless it is really needed. For
this configuration, which can also be done when creating each
JSONResponse, I would not add a global setting at this time. We can always
add it later, but if we add it now it will be very difficult to remove it
at a later time.
* I don't entirely understand JSON_RESPONSE_ALLOW_DICTS_ONLY. For who is
there an attack vector against who when Array() is poisoned? In other
words, what risk/breakage is introduced by setting this to False?
* I'm not entirely convinced that in `_default_content_type`, the
underscore is appropriate. Although I see the point from Python
conventions, I rarely see the single underscore prefix in Django, so
perhaps there is some intention not to use this in Django.
* The documentation text doesn't seem to flow very well. It's difficult
for me to point out exactly what my issue with it is - I'll happily modify
it for you, but I'm not able to do that right now.
* As far as I know, we have just adopted the plan to make all code samples
in Django include their import path. I am not 100% sure of this. If this
is the case, it would be good to add them to this patch too.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:7>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by LukaszBalcerzak):
- The reason I added `JSON_RESPONSE_DEFAULT_ENCODER` is that if there is
no possibility to set it globally, user would most probably create own
subclass of `JsonResponse` if need to use another encode in most cases
(and we are adding this class so user won't have to create their own). I
can remove it if someone feels like it's not necessary (or we can mark the
ticket for design decision request). This was in fact a setting I was not
really sure if is 100% needed but would be nice for users that in example
would create own encoder that can consume model or queryset.
- As for `JSON_RESPONSE_ALLOW_DICTS_ONLY`: security flaw would be created
if i.e. some view is CSRF vulnerable and returns top-level Array object,
and user uses pre-EcmaScript5 compliant browser. Attacker could prepare
malicious page with a request to that page. Normally, attacker would not
be able to retrieve data from such request but with patched Array it is
possible. See http://flask.pocoo.org/docs/security/#json-security to get
more information and quite precise example.
- single prefixed names are used through whole `HttpResponse` constructor.
I can make it public but then it becomes part of the interface and
changing it would be more difficult in future
- yep, please update documentation if you can!
- am not aware of that. Can you point me to the related
mails/discussions/ticket or tell that with 100% certainty? In addition -
if I would change newly added code snippet now styles would be mixed. I
believe this is responsibility of the one who would need to merge changes
and find them conflicting with master branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:8>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_tests: 0 => 1
Comment:
As discussed on the PR thread.
- The class should be named `JSONResponse`.
- The `_default_content_type` attribute should be replaced by
`kwargs.setdefault('content_type', 'application/json')`.
- The `JSON_RESPONSE_DEFAULT_ENCODER` and `JSON_RESPONSE_ALLOW_DICTS_ONLY`
settings should not be introduced and kwargs should be used instead.
- The `encoder` should just default to `None`, it should be really easy to
specify an encoder class without subclassing.
- Why are you setting `ensure_ascii` to `False`? I've never had any issues
dealing with non-ascii escaped data?
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:9>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by LukaszBalcerzak):
* Well, name of the class should correspond to the nearest classes in my
view. As already noticed at https://github.com/django/django/pull/1182
those Xml/Html/Http names are used inconsistently however it would look
odd seeing `JSONResponse` near to `HttpResponse`
* I can move content type default value to the class implementation (as
opposed to `HttpResponseBase` attribute) but then we have 2 places where
we need to compute full header's value (including `charset`)
* Ok, seeing settings generate a lot of comments I'm going to remove them
entirely in favor of `encoder` and `safe` parameters
* Still, `encoder`'s default value should be `DjangoJSONEncoder`
* `ensure_ascii=False` is there to allow non ascii chars to be pushed into
response's content. On second thought, though, I actually believe it
wasn't good idea. Am going to remove that.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:10>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by LukaszBalcerzak):
I've pushed changes. One particular thing I don't really like is encoder
import inside `JsonResponse.__init__` method. Should we put it at the top
of the module instead? (serializers package imports `django.db` so
importing `django.http` would not be allowed unless Django context is
configured - this was not needed before)
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:11>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by tomchristie):
* cc: tom@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:12>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by LukaszBalcerzak):
I've just updated PR (https://github.com/django/django/pull/1182).
Let's give it another shot and maybe include in 1.7.
Let me know if you believe there are any issues - I'd be happy to fix
them.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:13>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by hirokiky):
+1 for this PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:14>
Django
Reporter: leahculver | Owner: LukaszBalcerzak
Component: HTTP handling | Version: master
Keywords: dceu13 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0242134d32aa99a54442211ed05576b7061866d1"]:
{{{
#!CommitTicketReference repository=""
revision="0242134d32aa99a54442211ed05576b7061866d1"
Fixed #17942 -- Added a JsonResponse class to more easily create JSON
encoded responses.
Thanks leahculver for the suggestion and Erik Romijn,
Simon Charette, and Marc Tamlyn for the reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17942#comment:15>