Opened 12 years ago

Closed 10 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:

Related to this wiki page: https://code.djangoproject.com/wiki/AJAX

This patch should include:

  • mimetype "application/json"

Optional ideas:

Change History (15)

comment:1 by Leah Culver, 12 years ago

Needs documentation: set
Needs tests: set

comment:2 by Leah Culver, 12 years ago

Version: 1.3

comment:3 by Noah Kantrowitz, 12 years ago

Cc: noah@… added

Additional notes: should include ensure_ascii=False in the dumps call. Should use DjangoJSONEncoder by default to get datetime/decimal serialization.

comment:4 by Jannis Leidel, 12 years ago

Component: UncategorizedHTTP handling
Triage Stage: UnreviewedAccepted

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 Łukasz Balcerzak, 11 years ago

Owner: changed from nobody to Łukasz Balcerzak
Status: newassigned

comment:6 by Łukasz Balcerzak, 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:

  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.

comment:7 by Sasha Romijn, 11 years ago

Cc: eromijn@… 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 Łukasz Balcerzak, 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 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.

comment:9 by Simon Charette, 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 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?

comment:10 by Łukasz Balcerzak, 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 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.

comment:11 by Łukasz Balcerzak, 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 Tom Christie, 11 years ago

Cc: tom@… added

comment:13 by Łukasz Balcerzak, 10 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:14 by Hiroki Kiyohara, 10 years ago

+1 for this PR.

comment:15 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 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.

Note: See TracTickets for help on using tickets.
Back to Top