PR#542: [frontend] rework error handlers to fix #531 - copr/copr

copr / copr

#542 [frontend] rework error handlers to fix #531

Merged 5 years ago by msuchy. Opened 5 years ago by frostyx.

copr/ frostyx/copr fix-error-handlers into master

[frontend] rework error handlers to fix #531

Jakub Kadlčík • 5 years ago

dddc4ad

frontend/coprs_frontend/coprs/__init__.py

file modified

+37

		`@@ -74,6 +74,7 @@`
		`from coprs.views.webhooks_ns import webhooks_general`


		`+ from coprs.exceptions import ObjectNotFound, AccessRestricted, BadRequest, CoprHttpException`
		`from .context_processors import include_banner, inject_fedmenu, counter_processor`

		`setup_log()`
		`@@ -94,6 +95,42 @@`

		`app.add_url_rule("/", "coprs_ns.coprs_show", coprs_general.coprs_show)`

		`+`
		`+ def get_error_handler():`
		`+ # http://flask.pocoo.org/docs/1.0/blueprints/#error-handlers`
		`+ if flask.request.path.startswith('/api_3/'):`
		`+ return apiv3_ns.APIErrorHandler()`
		`+ return coprs_ns.UIErrorHandler()`
		`+`
		`+`
		`+ @app.errorhandler(404)`
		`+ @app.errorhandler(ObjectNotFound)`
		`+ def handle_404(error):`
		`+ error_handler = get_error_handler()`
		`+ return error_handler.handle_404(error)`
		`+`
		`+`
		`+ @app.errorhandler(403)`
		`+ @app.errorhandler(AccessRestricted)`
		`+ def handle_403(error):`
		`+ error_handler = get_error_handler()`
		`+ return error_handler.handle_403(error)`
		`+`
		`+`
		`+ @app.errorhandler(400)`
		`+ @app.errorhandler(BadRequest)`
		`+ def handle_400(error):`
		`+ error_handler = get_error_handler()`
		`+ return error_handler.handle_400(error)`
		`+`
		`+`
		`+ @app.errorhandler(500)`
		`+ @app.errorhandler(CoprHttpException)`
		`+ def handle_500(error):`
		`+ error_handler = get_error_handler()`
		`+ return error_handler.handle_500(error)`
		`+`
		`+`
		`app.jinja_env.trim_blocks = True`
		`app.jinja_env.lstrip_blocks = True`

frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py

file modified

+24 -14

		`@@ -6,7 +6,7 @@`
		`from functools import wraps`
		`from werkzeug.datastructures import ImmutableMultiDict`
		`from coprs import app`
		`- from coprs.exceptions import CoprHttpException`
		`+ from coprs.exceptions import CoprHttpException, ObjectNotFound`
		`from coprs.logic.complex_logic import ComplexLogic`


		`@@ -20,25 +20,35 @@`
		`DELETE = ["POST", "DELETE"]`


		`- @app.errorhandler(CoprHttpException)`
		`- def handle_copr_exception(error):`
		`- return handle_api_error(error.message, error.code)`
		`+ class APIErrorHandler(object):`
		`+ def handle_404(self, error):`
		`+ if isinstance(error, ObjectNotFound):`
		`+ return self.respond(str(error), 404)`
		`+ return self.respond("Such API endpoint doesn't exist", 404)`

		`+ def handle_403(self, error):`
		`+ return self.handle_xxx(error)`

		`- @app.errorhandler(404)`
		`- def page_not_found(error):`
		`- return handle_api_error("Such API endpoint doesn't exist", 404)`
		`+ def handle_400(self, error):`
		`+ return self.handle_xxx(error)`

		`+ def handle_500(self, error):`
		`+ return self.respond("Request wasn't successful, there is probably a bug in the API code.", 500)`

		`- @app.errorhandler(500)`
		`- def page_not_found(error):`
		`- return handle_api_error("Request wasn't successful, there is probably a bug in the API code.", 500)`
		`+ def handle_xxx(self, error):`
		`+ return self.respond(self.message(error), error.code)`

		`+ def respond(self, message, code):`
		`+ response = flask.jsonify(error=message)`
		`+ response.status_code = code`
		`+ return response`

		`- def handle_api_error(message, code):`
		`- response = flask.jsonify(error=message)`
		`- response.status_code = code`
		`- return response`
		`+ def message(self, error):`
		`+ if isinstance(error, CoprHttpException):`
		`+ return error.message`
		`+ if hasattr(error, "description"):`
		`+ return error.description`
		`+ return str(error)`


		`def query_params():`

frontend/coprs_frontend/coprs/views/coprs_ns/__init__.py

file modified

+16 -15

		`@@ -3,26 +3,27 @@`
		`import flask`

		`from coprs.views.misc import page_not_found, access_restricted, bad_request_handler, server_error_handler`
		`- from coprs.exceptions import ObjectNotFound, AccessRestricted, BadRequest, CoprHttpException`
		`+ from coprs.exceptions import CoprHttpException`

		`coprs_ns = flask.Blueprint("coprs_ns", __name__, url_prefix="/coprs")`


		`- @coprs_ns.errorhandler(ObjectNotFound)`
		`- def handle_404(error):`
		`- return page_not_found(error.message)`
		`+ class UIErrorHandler(object):`
		`+ def handle_404(self, error):`
		`+ return page_not_found(self.message(error))`

		`+ def handle_403(self, error):`
		`+ return access_restricted(self.message(error))`

		`- @coprs_ns.errorhandler(AccessRestricted)`
		`- def handle_403(error):`
		`- return access_restricted(error.message)`
		`+ def handle_400(self, error):`
		`+ return bad_request_handler(self.message(error))`

		`+ def handle_500(self, error):`
		`+ return server_error_handler(self.message(error))`

		`- @coprs_ns.errorhandler(BadRequest)`
		`- def handle_400(error):`
		`- return bad_request_handler(error.message)`
		`-`
		`-`
		`- @coprs_ns.errorhandler(CoprHttpException)`
		`- def handle_500(error):`
		`- return server_error_handler(error.message)`
		`+ def message(self, error):`
		`+ if isinstance(error, CoprHttpException):`
		`+ return error.message`
		`+ if hasattr(error, "description"):`
		`+ return error.description`
		`+ return str(error)`

frontend/coprs_frontend/coprs/views/misc.py

file modified

-5

		`@@ -79,12 +79,10 @@`
		`models.User.username == username).first()`


		`- @app.errorhandler(404)`
		`def page_not_found(message):`
		`return flask.render_template("404.html", message=message), 404`


		`- @app.errorhandler(403)`
		`def access_restricted(message):`
		`return flask.render_template("403.html", message=message), 403`

		`@@ -103,9 +101,6 @@`
		`server_error_handler = partial(generic_error, code=500, title="Internal Server Error")`
		`bad_request_handler = partial(generic_error, code=400, title="Bad Request")`

		`- app.errorhandler(500)(server_error_handler)`
		`- app.errorhandler(400)(bad_request_handler)`
		`-`
		`misc = flask.Blueprint("misc", __name__)`

frontend/coprs_frontend/tests/coprs_test_case.py

file modified

		`@@ -479,6 +479,11 @@`
		`}`
		`)`

		`+ def post_api3_with_auth(self, url, content, user):`
		`+ headers = {"Authorization": self._get_auth_string(user.api_login, user.api_token),`
		`+ "Content-Type": "application/json"}`
		`+ return self.tc.post(url, data=json.dumps(content), headers=headers)`
		`+`

		`class TransactionDecorator(object):`

frontend/coprs_frontend/tests/test_exceptions.py

file added

+48

		`@@ -0,0 +1,48 @@`
		`+ import json`
		`+ import pytest`
		`+ from tests.coprs_test_case import CoprsTestCase`
		`+`
		`+`
		`+ class TestExceptionHandling(CoprsTestCase):`
		`+ def test_json_only_for_api(self):`
		`+ r1 = self.tc.get("/nonexisting/endpoint/")`
		`+ assert r1.status_code == 404`
		`+`
		`+ with pytest.raises(json.JSONDecodeError):`
		`+ json.loads(r1.data)`
		`+`
		`+ r2 = self.tc.get("/api_3/nonexisting/endpoint/")`
		`+ assert r2.status_code == 404`
		`+ data = json.loads(r2.data)`
		`+ assert "error" in data`
		`+`
		`+ def test_both_nonexisting_page_and_object(self):`
		`+ r1 = self.tc.get("/nonexisting/endpoint/")`
		`+ assert r1.status_code == 404`
		`+`
		`+ r2 = self.tc.get("/coprs/nonexisting/nonexisting/")`
		`+ assert r2.status_code == 404`
		`+`
		`+ def test_both_nonexisting_page_and_object_api(self):`
		`+ r1 = self.tc.get("/api_3/nonexisting/endpoint/")`
		`+ assert r1.status_code == 404`
		`+ d1 = json.loads(r1.data)`
		`+ assert "API endpoint" in d1["error"]`
		`+`
		`+ r2 = self.tc.get("/api_3/project?ownername=nonexisting&projectname=nonexisting")`
		`+ assert r2.status_code == 404`
		`+ d2 = json.loads(r2.data)`
		`+ assert "Project nonexisting/nonexisting does not exist" in d2["error"]`
		`+`
		`+ def test_api_401(self):`
		`+ r1 = self.tc.post("api_3/project/add/someone")`
		`+ assert r1.status_code == 401`
		`+ data = json.loads(r1.data)`
		`+ assert "Login invalid/expired" in data["error"]`
		`+`
		`+ def test_api_403(self, f_users, f_coprs, f_mock_chroots, f_users_api, f_db):`
		`+ request_data = {"chroots": None, "description": "Changed description"}`
		`+ r1 = self.post_api3_with_auth("api_3/project/edit/user2/foocopr", request_data, self.u3)`
		`+ assert r1.status_code == 403`
		`+ data = json.loads(r1.data)`
		`+ assert "Only owners and admins may update their projects" in data["error"]`

frostyx commented 5 years ago

There are multiple issues with our current implementation
of error handling.

First, apiv3_ns defines error handlers on application level,
not blueprint level. This causes the #531 issue and results in
showing API errors even for exceptions, that happened in UI.

Also, coprs_ns defines error handler for 404, but only for the case,
that some object is not found. It doesn't cover the case when
a whole API endpoint is not found. We can't add it there, though.
According to the Flask documentation
http://flask.pocoo.org/docs/1.0/blueprints/#error-handlers
it is not possible to add handlers for 404 and 405 on blueprint level.
That's why it is currently implemented in misc.py.

That basically means, that our error handlers are inconsistently spread
within the application and we need to have some of them for apiv3_ns
and coprs_ns blueprints and also a conditioned handler for 404 on
application level. I have had multiple moments of clarity, thinking
that I finally understand how the code works, but I didn't. And then
again and gain, so I decided to rather have all handlers implemented
the same way.