Skip to content
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

Return 400 on invalid input #813

Merged
merged 2 commits into from Apr 14, 2017
Merged

Return 400 on invalid input #813

merged 2 commits into from Apr 14, 2017

Conversation

@EvaSDK
Copy link
Contributor

EvaSDK commented Mar 17, 2017

A simple patch to return proper information to the client side that its input data is invalid.
This is especially useful to silence errors that are not server-side when coupled with reporting tools such as sentry, Airbrake, etc.

I did not add unittests as I got lost in the structure for functional tests as library specific tests but if you tell me where to look, I'll add them.

if not isinstance(self.forwarded, dict):
raise ValueError
except ValueError:
return HttpResponseBadRequest('Invalid JSON data')

This comment has been minimized.

Copy link
@jpic

jpic Mar 29, 2017

Member

While you're at it, would you like to remove some SLOCS with:

self.forwarded = getattr(request, 'POST' if request.method == 'POST' else 'GET').get('forward', '{}')

Also, it think the code would be a better read if the try block only contains the loads() call, and then check if it's a dict and return 400 otherwise: it seems like it would be easier to read and understand with two distinct return HttpResponseBadRequest() statements rather that a raise ValueError/except ValueError.

This comment has been minimized.

Copy link
@EvaSDK

EvaSDK Apr 4, 2017

Author Contributor

I was playing with a fix according to your comment but then I figured that only GET and POST are supported here so maybe we want to return a 405 Method Not Allowed if the method is not supported and that would mean implementing OPTION as well and then this pretty much resemble the Django default View. Should I modify this view to subclass it directly ?

This comment has been minimized.

Copy link
@jpic

jpic Apr 7, 2017

Member

405 sounds useful, OPTIONS not so much, but if you're up for it go ahead, don't forget we'll need unit tests so you might not want to go through implementing OPTIONS for this reason.

@EvaSDK EvaSDK force-pushed the EvaSDK:json-error-return-400 branch 2 times, most recently from 9724217 to 0fcc834 Apr 13, 2017
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 14, 2017

Codecov Report

Merging #813 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #813      +/-   ##
==========================================
+ Coverage   96.25%   96.31%   +0.06%     
==========================================
  Files         122      123       +1     
  Lines        2057     2092      +35     
==========================================
+ Hits         1980     2015      +35     
  Misses         77       77
Impacted Files Coverage Δ
test_project/linked_data/test_views.py 100% <100%> (ø)
src/dal/views.py 92.45% <100%> (+1.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc83e28...e24c72f. Read the comment docs.

@EvaSDK EvaSDK force-pushed the EvaSDK:json-error-return-400 branch from 0fcc834 to f2285fb Apr 14, 2017
EvaSDK added 2 commits Mar 17, 2017
Make sure invalid input data triggers a client error code rather than a
server error code.

Also return a 405 if method is not supported.
e.g.: tox -- -x
@EvaSDK EvaSDK force-pushed the EvaSDK:json-error-return-400 branch from f2285fb to e24c72f Apr 14, 2017
@EvaSDK

This comment has been minimized.

Copy link
Contributor Author

EvaSDK commented Apr 14, 2017

Should be all good now. I left OPTIONS out as all cases I looked at ended up in View being in the inheritance tree so it will be handled automatically in most cases. The view now duplicates the View class check for 405 but I think there is no way around it.

@jpic jpic merged commit 16a4b2a into yourlabs:master Apr 14, 2017
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 96.25%)
Details
codecov/project 96.31% (+0.06%) compared to cc83e28
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jpic

This comment has been minimized.

Copy link
Member

jpic commented Apr 14, 2017

Released in 3.2.4, grats !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.