PR#1405: frontend: don't traceback on invalid cancel requests - copr/copr

copr / copr

#1405 frontend: don't traceback on invalid cancel requests

Merged 3 years ago by praiskup. Opened 3 years ago by praiskup.

Unknown source better-errors-for-cancel-failure into master

frontend: don't traceback on invalid cancel requests

Pavel Raiskup • 3 years ago

d0ee783

frontend/coprs_frontend/coprs/__init__.py

file modified

+17 -2

		`@@ -1,3 +1,6 @@`
		`+ # This is very complicated module. TODO: drop the ignores`
		`+ # pylint: disable=wrong-import-order,wrong-import-position`
		`+`
		`from __future__ import with_statement`

		`import os`
		`@@ -101,8 +104,14 @@`
		`from coprs.views.webhooks_ns import webhooks_general`
		`from coprs.views.rss_ns import rss_ns`
		`from coprs.views.rss_ns import rss_general`
		`-`
		`- from coprs.exceptions import ObjectNotFound, AccessRestricted, BadRequest, CoprHttpException, MalformedArgumentException`
		`+ from coprs.exceptions import (`
		`+ AccessRestricted,`
		`+ BadRequest,`
		`+ CoprHttpException,`
		`+ ConflictingRequest,`
		`+ MalformedArgumentException,`
		`+ ObjectNotFound,`
		`+ )`
		`from .context_processors import include_banner, inject_fedmenu, counter_processor`

		`setup_log()`
		`@@ -153,6 +162,12 @@`
		`error_handler = get_error_handler()`
		`return error_handler.handle_400(error)`

		`+ @app.errorhandler(409)`
		`+ @app.errorhandler(ConflictingRequest)`
		`+ def handle_409(error):`
		`+ """ Handle the 409 nicely """`
		`+ error_handler = get_error_handler()`
		`+ return error_handler.handle_409(error)`

		`@app.errorhandler(500)`
		`@app.errorhandler(CoprHttpException)`

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

file modified

		`@@ -21,6 +21,9 @@`


		`class APIErrorHandler(object):`
		`+ def handle_409(self, error):`
		`+ return self.handle_xxx(error)`
		`+`
		`def handle_404(self, error):`
		`if isinstance(error, ObjectNotFound):`
		`return self.respond(str(error), 404)`

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

file modified

+11 -1

		`@@ -2,13 +2,23 @@`

		`import flask`

		`- from coprs.views.misc import page_not_found, access_restricted, bad_request_handler, server_error_handler`
		`+ from coprs.views.misc import (`
		`+ access_restricted,`
		`+ bad_request_handler,`
		`+ conflict_request_handler,`
		`+ page_not_found,`
		`+ server_error_handler,`
		`+ )`
		`+`
		`from coprs.exceptions import CoprHttpException`

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


		`class UIErrorHandler(object):`
		`+ def handle_409(self, error):`
		`+ return conflict_request_handler(self.message(error))`
		`+`
		`def handle_404(self, error):`
		`return page_not_found(self.message(error))`

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

file modified

+8 -5

		`@@ -15,10 +15,13 @@`
		`req_with_copr, send_build_icon)`
		`from coprs.views.coprs_ns import coprs_ns`

		`- from coprs.exceptions import (ActionInProgressException,`
		`- InsufficientRightsException,`
		`- UnrepeatableBuildException,`
		`- BadRequest)`
		`+ from coprs.exceptions import (`
		`+ ActionInProgressException,`
		`+ BadRequest,`
		`+ ConflictingRequest,`
		`+ InsufficientRightsException,`
		`+ UnrepeatableBuildException,`
		`+ )`


		`@coprs_ns.route("/build/<int:build_id>/")`
		`@@ -433,7 +436,7 @@`
		`def process_cancel_build(build):`
		`try:`
		`builds_logic.BuildsLogic.cancel_build(flask.g.user, build)`
		`- except InsufficientRightsException as e:`
		`+ except (InsufficientRightsException, ConflictingRequest) as e:`
		`flask.flash(str(e), "error")`
		`else:`
		`db.session.commit()`

frontend/coprs_frontend/coprs/views/misc.py

file modified

		`@@ -100,6 +100,7 @@`

		`server_error_handler = partial(generic_error, code=500, title="Internal Server Error")`
		`bad_request_handler = partial(generic_error, code=400, title="Bad Request")`
		`+ conflict_request_handler = partial(generic_error, code=409, title="Conflict")`

		`misc = flask.Blueprint("misc", __name__)`

praiskup commented 3 years ago

Flash in web-UI for ConflictingRequest raised by cancel request, but
also avoid another cli failure:

$ copr cancel 51501
Something went wrong:
Error: Request wasn't successful, there is probably a bug in the API code.

Newly we write:

$ copr cancel 51501
Something went wrong:
Error: Cannot cancel build 51501

Fixes: #1325

frostyx commented on line 34 of frontend/coprs_frontend/coprs/__init__.py 3 years ago

I am thinking if we should rename the handle_400 method to handle_4xx if we are going to use it for all client errors.

frostyx commented 3 years ago

but otherwise LGTM

rebased onto 26ba857ebb65b7ef97ca13bb42cca0e2300cc74f

3 years ago

praiskup commented 3 years ago

I am thinking if we should rename the handle_400 method to handle_4xx if we are going to use it for all client errors.

I'd have to do some better implementation and testing for the web-ui handlers. So
I rather only added handle_409 methods for both api and UI for now. PTAL.

frostyx commented 3 years ago