Opened 13 years ago
Closed 11 years ago
#17942 closed New feature (fixed)
JSONResponse class for API responses
Reported by: | Leah Culver | Owned by: | Łukasz Balcerzak |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Normal | Keywords: | dceu13 |
Cc: | noah@…, eromijn@…, tom@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
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?
Change History (15)
comment:1 by , 13 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
comment:2 by , 13 years ago
Version: | 1.3 |
---|
comment:3 by , 13 years ago
Cc: | added |
---|
comment:4 by , 12 years ago
Component: | Uncategorized → HTTP handling |
---|---|
Triage Stage: | Unreviewed → Accepted |
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.
comment:5 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 11 years ago
Has patch: | set |
---|---|
Needs documentation: | unset |
Needs tests: | unset |
Version: | → master |
Added pull request: https://github.com/django/django/pull/1182
Implementation motiviations:
- 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.
- If extra status codes are needed they can be easily created using
status
parameter/status_code
attribute. It is also not clear ifJsonResponse
users would want to return json responses on errors (most probably but I guess not always). - Idea of allowing to serialize models/querysets is not bad and this point is actually I am most interested to get feedback on.
- I would leave XML for now. Especially as this ticket is specifically about JSON.
Please let me know your thoughts.
comment:7 by , 11 years ago
Cc: | added |
---|---|
Keywords: | dceu13 added |
Patch needs improvement: | set |
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.
comment:8 by , 11 years ago
- 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 ofJsonResponse
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.
comment:9 by , 11 years ago
Needs tests: | set |
---|
As discussed on the PR thread.
- The class should be named
JSONResponse
. - The
_default_content_type
attribute should be replaced bykwargs.setdefault('content_type', 'application/json')
. - The
JSON_RESPONSE_DEFAULT_ENCODER
andJSON_RESPONSE_ALLOW_DICTS_ONLY
settings should not be introduced and kwargs should be used instead. - The
encoder
should just default toNone
, it should be really easy to specify an encoder class without subclassing. - Why are you setting
ensure_ascii
toFalse
? I've never had any issues dealing with non-ascii escaped data?
comment:10 by , 11 years ago
- 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 toHttpResponse
- 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 (includingcharset
) - Ok, seeing settings generate a lot of comments I'm going to remove them entirely in favor of
encoder
andsafe
parameters - Still,
encoder
's default value should beDjangoJSONEncoder
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.
comment:11 by , 11 years ago
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)
comment:12 by , 11 years ago
Cc: | added |
---|
comment:13 by , 11 years ago
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.
comment:15 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Additional notes: should include
ensure_ascii=False
in the dumps call. Should use DjangoJSONEncoder by default to get datetime/decimal serialization.