-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Error handler update #4257
Error handler update #4257
Conversation
Here's how errors should be handled from my point of view:
Are all these cases covered? Would be good to have some tests mocking some errors to make sure we do |
we need to add For example for |
Currently im testing few extensions against 2.8 and there are few scenarios where error handler is not working as it should. First example is by enabling of Second example is by enabling @amercader ^^ |
OK, I had a closer look at this, here are some points to help clarify the situation:
I think this is fine until we get finish #4291 and switch to run the local development server with Werkzeug, thus enabling the debugger for Flask requests.
diff --git a/ckan/views/api.py b/ckan/views/api.py
index 8873f74..e3f776c 100644
--- a/ckan/views/api.py
+++ b/ckan/views/api.py
@@ -336,6 +336,13 @@ def action(logic_function, ver=API_DEFAULT_VERSION):
str(e)}
return_dict[u'success'] = False
return _finish(500, return_dict, content_type=u'json')
+ except Exception as e:
+ return_dict[u'error'] = {
+ u'__type': u'Internal Server Error',
+ u'message': u'Internal Server Error'}
+ return_dict[u'success'] = False
+ log.exception(e)
+ return _finish(500, return_dict, content_type=u'json')
return _finish_ok(return_dict) |
ckan/config/middleware/flask_app.py
Outdated
|
||
for code in default_exceptions: | ||
app.register_error_handler(code, error_handler) | ||
app.register_error_handler(Exception, error_handler) |
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.
We should only catch unhandled exceptions on production mode (debug=False
) so add an if not app.debug:
here. (see main comment for context).
ckan/config/middleware/flask_app.py
Outdated
if isinstance(e, HTTPException): | ||
extra_vars = {u'code': [e.code], u'content': e.description} | ||
return base.render(u'error_document_template.html', extra_vars), e.code | ||
extra_vars = {u'code': 500, u'content': e.message} |
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.
In this case the message should always be Internal Server Error
We have issues with non HTTPExceptions and exceptions from extensions. Also small change for reading the
debug
configuration value