-
Notifications
You must be signed in to change notification settings - Fork 18
Stop using handler.abort in validators #171
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
Conversation
raise exceptions instead (catched by base.RequestHandler.handle_exception)
Let's discuss the exception type before we merge. |
api/validators.py
Outdated
_validate_json(payload, _schema, resolver_input) | ||
except jsonschema.ValidationError as e: | ||
handler.abort(400, str(e)) | ||
raise webapp2.exc.HTTPBadRequest(str(e)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having this be webapp2
-aware would actually be nicer, wouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah probably the best thing to do is to catch the ValidationError in the handler calling the method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a little more encapsulation. Your code shouldn't expose its internals, such as its use of jsonschema
. You should rather define a ValidationException
so that I can then say except validators.ValidationException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed a new commit. Added two exceptions to distinguish when the exception is critical(mongo schema validations).
d0186bb
to
ea97a70
Compare
👍 |
LGTM, merge as desired |
Stop using handler.abort in validators
raise exceptions instead (caught by
base.RequestHandler.handle_exception
)partially addresses #66