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

Conversation

Projects
None yet
2 participants
@tino097
Copy link
Member

commented May 16, 2018

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

@tino097 tino097 self-assigned this May 17, 2018

@amercader

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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)

This comment has been minimized.

Copy link
@amercader

amercader Jun 13, 2018

Member

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}

This comment has been minimized.

Copy link
@amercader

amercader Jun 13, 2018

Member

In this case the message should always be Internal Server Error

@tino097 tino097 removed the WIP label Jun 13, 2018

@amercader amercader merged commit ed5645c into ckan:master Jun 14, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - requirements.txt No dependency changes
Details

@amercader amercader referenced this pull request Jun 27, 2018

Closed

Compatibility with 2.8? #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.