Added JsonRequestMixin to attempt to parse incoming requests as JSON #51

Closed
wants to merge 4 commits into
from

Projects

None yet

2 participants

@esoergel
Contributor
esoergel commented Jul 9, 2013

A mixin that attempts to parse the request as JSON. If request is properly formatted, the json is saved to self.request_json as a Python object.

I'm somewhat new to contributing and testing, so I'd welcome any feedback on this pull request. I wrote tests for this mixin and it's marked as having full coverage, so that's something, I suppose. Still, I'd appreciate it if someone could look over the tests I wrote and tell me if I'm doing something stupid.

I wanted this mixin to be as unobtrusive as possible - you can add this to a view without it affecting anything; it just provides a request_json attribute and a HTTP400 "Bad Request" response (but you'll have to call it yourself). Also, since this Mixin inherits from JSONResponseMixin, perhaps "JsonRequestMixin" is a bit of a misnomer (as it also gives access to the response methods).

If the request cannot be turned into JSON, request_json becomes None. This allows you to check for failure (request_json is None), failure or empty requests (not request_json), or ignore it altogether. I thought about putting in more API features (in particular, a method to get_or_400 from the JSON would be nice), but I thought it made sense to have a minimum functionality version first without imposing too rigid a structure.

What do you guys think about the utility of this mixin? I recently needed to parse JSON requests and had no idea how. It's pretty simple, but I think this makes it simpler. Are there more features I should add? Better ways to implement these features?

@kennethlove
Member

I like this. I wonder if it shouldn't be renamed to JSONRequestResponseMixin, though.

@esoergel
Contributor

I can change the mixin name. What do you think of error handling? I'm trying to find the balance between making stuff work out of the box and not taking too much control. Errors parsing bad JSON should register as 400 errors, not 500. Here are some things I'm thinking about putting in:

  • a require_json class attribute that when flagged will return a 400 error if the request does not contain JSON, rather than failing silently.
  • a get_or_400 method to parse the request: Use self.get_or_400('person', 'name') rather than self.request_json['person']['name'] to catch missing values and respond appropriately (client error, not server error). This method would have an optional error_dict kwarg to allow for more nuanced messages if desired.

I'm worried about feature creep though, it's just a mixin, not a full JSON API library. Are either or both of those worth putting in? What's more in line with braces' use cases?

@kennethlove
Member

get_or_400 seems like overkill. We already have KeyError exceptions that fit this perfectly.

I think the require_json attribute is a good idea, though.

@esoergel
Contributor

Okay, I'll write that up and change the name. Thanks for the input!

@kennethlove
Member

If you can get these changes in soon, and make sure it's tested, we can get this into 1.3

@esoergel
Contributor

Okay, I think that should be everything. Let me know if you see any mistakes or omissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment