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

Error handler update #4257

Merged
merged 6 commits into from
Jun 14, 2018
Merged

Conversation

tino097
Copy link
Member

@tino097 tino097 commented May 16, 2018

We have issues with non HTTPExceptions and exceptions from extensions. Also small change for reading the debug configuration value

@amercader
Copy link
Member

amercader commented May 18, 2018

Here's how errors should be handled from my point of view:

Error type debug=true debug=false
HTTP error (eg 400, 404, etc) in a Web request Return error status code + Render error template with code and message Return error status code + Render error template with code and message
HTTP error (eg 400, 404, etc) in an API Request Return error status code + Return JSON error object Return error status code + Return JSON error object
Uncaught exception in a Web request Werkzeug / Pylons error page with stacktrace Return 500 status code + Render error template showing "Internal Server Error" (no error details)
Uncaught exception in an API request Return 500 status code + Return JSON error object showing "Internal Server Error" (no error details) Return 500 status code + Render JSON error object showing "Internal Server Error" (no error details)

Are all these cases covered? Would be good to have some tests mocking some errors to make sure we do

@tino097
Copy link
Member Author

tino097 commented May 22, 2018

we need to add try catch blocks in some of the helpers.py functions

For example for check_access as we have situation where c.user is throwing an AttributeError coming from extension

@tino097
Copy link
Member Author

tino097 commented May 29, 2018

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 ckanext-pages extension and when the instance is starting, the following error is occurring
This error is result of the home blueprint endpoint and plugin is looking for pylons c.action attribute instead

Second example is by enabling ckanext-ldap extension and the error stack is here
In this case, while trying to display error page, check_access in helpers.py is trying to get c.user but because the extension had failed, user is not set

@amercader ^^

@amercader
Copy link
Member

amercader commented Jun 12, 2018

OK, I had a closer look at this, here are some points to help clarify the situation:

  • When running with debug=True, general uncaught exceptions should not be handled by Flask/Pylons. They should be allowed to raise so they can be picked up by the interactive debugger. By interactive debugger I mean this:

    • Pylons
      screenshot-2018-6-12 server error

    • Flask
      screenshot-2018-6-11 valueerror norr yo werkzeug debugger

  • For Flask requests, I wrongly assumed that FlaskDebugToolbar was the responsible for showing the interactive debbuger, but it isn't. This is generated by Werkzeug, but only if running on Werkzeug of course! When running under paster and serving a Flask request the exception won't get handled and we will see a plain server error page like the cases @tino097 mentions in his comment:

    screenshot-2018-6-12 http 127 0 0 1

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.

  • To cover the Return 500 status code + Return JSON error object showing "Internal Server Error" (no error details) on API calls you will need something like this:
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)


for code in default_exceptions:
app.register_error_handler(code, error_handler)
app.register_error_handler(Exception, error_handler)
Copy link
Member

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

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}
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

2 participants