#1494 frontend: improve APIv3 exception handling for better messages
Merged 3 years ago by praiskup. Opened 3 years ago by frostyx.
copr/ frostyx/copr apiv3-exception-handler  into  master

file modified
+13 -9
@@ -155,6 +155,17 @@ 

              owner = self.config["username"]

          return owner, name

  

+     def parse_chroot_path(self, path):

+         """

+         Take a `path` in an `owner/project/chroot` format and return a tuple of

+         the corresponding values, i.e. `(owner, project, chroot)`

+         """

+         m = re.match(r"(([^/]+)/)?([^/]+)/(.*)", path)

+         if m:

+             owner = m.group(2) or self.config["username"]

+             return owner, m.group(3), m.group(4)

+         raise CoprException("Unexpected chroot path format")

+ 

      def _watch_builds(self, build_ids):

          """

          :param build_ids: list of build IDs
@@ -566,7 +577,7 @@ 

  

          :param args: argparse arguments provided by the user

          """

-         owner, copr, chroot = parse_chroot_path(args.coprchroot)

+         owner, copr, chroot = self.parse_chroot_path(args.coprchroot)

          project_chroot = self.client.project_chroot_proxy.edit(

              ownername=owner, projectname=copr, chrootname=chroot,

              comps=args.upload_comps, delete_comps=args.delete_comps,
@@ -580,7 +591,7 @@ 

  

          :param args: argparse arguments provided by the user

          """

-         owner, copr, chroot = parse_chroot_path(args.coprchroot)

+         owner, copr, chroot = self.parse_chroot_path(args.coprchroot)

          project_chroot = self.client.project_chroot_proxy.get(

              ownername=owner, projectname=copr, chrootname=chroot

          )
@@ -1374,13 +1385,6 @@ 

      return parser

  

  

- def parse_chroot_path(path):

-     m = re.match(r"(([^/]+)/)?([^/]+)/(.*)", path)

-     if m:

-         return m.group(2), m.group(3), m.group(4)

-     return None

- 

- 

  def enable_debug():

      logging.basicConfig(

          level=logging.DEBUG,

@@ -114,6 +114,7 @@ 

      NonAdminCannotCreatePersistentProject,

      NonAdminCannotDisableAutoPrunning,

  )

+ from coprs.error_handlers import get_error_handler

  from .context_processors import include_banner, inject_fedmenu, counter_processor

  

  setup_log()
@@ -136,27 +137,7 @@ 

  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(400)

- @app.errorhandler(BadRequest)

- @app.errorhandler(MalformedArgumentException)

- @app.errorhandler(403)

- @app.errorhandler(AccessRestricted)

- @app.errorhandler(NonAdminCannotCreatePersistentProject)

- @app.errorhandler(NonAdminCannotDisableAutoPrunning)

- @app.errorhandler(404)

- @app.errorhandler(ObjectNotFound)

- @app.errorhandler(409)

- @app.errorhandler(ConflictingRequest)

- @app.errorhandler(500)

- @app.errorhandler(CoprHttpException)

- @app.errorhandler(504)

+ @app.errorhandler(Exception)

  def handle_exceptions(error):

      error_handler = get_error_handler()

      return error_handler.handle_error(error)

@@ -0,0 +1,119 @@ 

+ """

+ A place for exception-handling logic

+ """

+ 

+ import flask

+ from werkzeug.exceptions import HTTPException, NotFound, GatewayTimeout

+ from coprs.exceptions import CoprHttpException

+ from coprs.views.misc import (

+     generic_error,

+     access_restricted,

+     bad_request_handler,

+     conflict_request_handler,

+     page_not_found,

+ )

+ 

+ 

+ def get_error_handler():

+     """

+     Determine what error handler should be used for this request

+     See http://flask.pocoo.org/docs/1.0/blueprints/#error-handlers

+     """

+     if flask.request.path.startswith('/api_3/'):

+         return APIErrorHandler()

+     return UIErrorHandler()

+ 

+ 

+ class BaseErrorHandler:

+     """

+     Do not use this class for handling errors. It is only a parent class for

+     the actual error-handler classes.

+     """

+ 

+     def handle_error(self, error):

+         """

+         Return a flask response suitable for the current situation (e.g. reder

+         HTML page for UI failures, send JSON back to API client, etc).

+ 

+         This method is expected to be implemented in descendants of this class.

+         """

+         raise NotImplementedError

+ 

+     def code(self, error):  # pylint: disable=no-self-use

+         """

+         Return status code for a given exception

+         """

+         return getattr(error, "code", 500)

+ 

+     def message(self, error):  # pylint: disable=no-self-use

+         """

+         Return an error message for a given exception. We want to obtain messages

+         differently for `CoprHttpException`, `HTTPException`, or others.

+         """

+         if isinstance(error, HTTPException):

+             return error.description

+         return str(error)

+ 

+ 

+ class UIErrorHandler(BaseErrorHandler):

+     """

+     Handle exceptions raised from the web user interface

+     """

+ 

+     def handle_error(self, error):

+         code = self.code(error)

+         message = self.message(error)

+ 

+         # The most common error has their own custom error pages. When catching

+         # a new exception, try to keep it simple and just the the generic one.

+         # Create it's own view only if necessary.

+         error_views = {

+             400: bad_request_handler,

+             403: access_restricted,

+             404: page_not_found,

+             409: conflict_request_handler,

+         }

+         if code in error_views:

+             return error_views[code](message)

+         return generic_error(message, code)

+ 

+ 

+ class APIErrorHandler(BaseErrorHandler):

+     """

+     Handle exceptions raised from API (v3)

+     """

+ 

+     def handle_error(self, error):

+         code = self.code(error)

+         message = self.message(error)

+ 

+         # In the majority of cases, we want to return the message that was

+         # passed through an exception, but occasionally we want to redefine the

+         # message to some API-related one. Please try to keep it simple and

+         # do this only if necessary.

+         errors = {

+             NotFound: "Such API endpoint doesn't exist",

+             GatewayTimeout: "The API request timeouted",

+         }

+         if error.__class__ in errors:

+             message = errors[error.__class__]

+ 

+         # Every `CoprHttpException` and `HTTPException` failure has valuable

+         # message for the end user. It holds information that e.g. some value is

+         # missing or incorrect, something cannot be done, something doesn't

+         # exist. Eveything else should really be an uncaught exception caused by

+         # either not properly running all frontend requirements (PostgreSQL,

+         # Redis), or having a bug in the code.

+         if not any([isinstance(error, CoprHttpException),

+                     isinstance(error, HTTPException)]):

+             message = ("Request wasn't successful, "

+                        "there is probably a bug in the API code.")

+         return self.respond(message, code)

+ 

+     def respond(self, message, code):  # pylint: disable=no-self-use

+         """

+         Return JSON response suitable for API clients

+         """

+         response = flask.jsonify(error=message)

+         response.status_code = code

+         return response

@@ -5,7 +5,7 @@ 

  import inspect

  from functools import wraps

  from werkzeug.datastructures import ImmutableMultiDict

- from werkzeug.exceptions import NotFound, GatewayTimeout

+ from werkzeug.exceptions import HTTPException, NotFound, GatewayTimeout

  from coprs import app

  from coprs.exceptions import (

      AccessRestricted,
@@ -27,41 +27,6 @@ 

  DELETE = ["POST", "DELETE"]

  

  

- class APIErrorHandler(object):

-     def handle_error(self, error):

-         # In the majority of cases, we want to return the message that was

-         # passed through an exception, but occasionally we want to redefine the

-         # message to some API-related one. Please try to keep it simple and

-         # do this only if necessary.

-         errors = {

-             NotFound: "Such API endpoint doesn't exist",

-             CoprHttpException: "Request wasn't successful, "

-                                "there is probably a bug in the API code.",

-             GatewayTimeout: "The API request timeouted",

-         }

-         message = self.message(error)

-         if error.__class__ in errors:

-             message = errors[error.__class__]

-         return self.respond(message, error.code)

- 

-     def respond(self, message, code):

-         response = flask.jsonify(error=message)

-         response.status_code = code

-         return response

- 

-     def message(self, error):

-         if isinstance(error, CoprHttpException):

-             # Ideally we want to return a custom message that was passed to the

-             # exception when initializing its object. In case there is no custom

-             # message, all descendants of `CoprHttpException` define some

-             # reasonable default, such as "You don't have required permission",

-             # "Requested object was not found" or "Generic copr exception"

-             return error.message or error._default

-         if hasattr(error, "description"):

-             return error.description

-         return str(error)

- 

- 

  def query_params():

      def query_params_decorator(f):

          @wraps(f)

@@ -2,39 +2,5 @@ 

  

  import flask

  

- from coprs.views.misc import (

-     generic_error,

-     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_error(self, error):

-         # The most common error has their own custom error pages. When catching

-         # a new exception, try to keep it simple and just the the generic one.

-         # Create it's own view only if necessary.

-         error_views = {

-             400: bad_request_handler,

-             403: access_restricted,

-             404: page_not_found,

-             409: conflict_request_handler,

-         }

-         message = self.message(error)

-         if error.code in error_views:

-             return error_views[error.code](message)

-         return generic_error(self.message(error), error.code)

- 

-     def message(self, error):

-         if isinstance(error, CoprHttpException):

-             return error.message or error._default

-         if hasattr(error, "description"):

-             return error.description

-         return str(error)

@@ -79,7 +79,7 @@ 

          def raise_exception():

              raise GatewayTimeout()

          app.view_functions["apiv3_ns.home"] = raise_exception

-         r1 = self.tc.get("/api_3", follow_redirects=True)

+         r1 = self.tc.get("/api_3/", follow_redirects=True)

          assert r1.status_code == 504

          data = json.loads(r1.data)

          assert "The API request timeouted" in data["error"]
@@ -89,7 +89,27 @@ 

          def raise_exception():

              raise CoprHttpException("Whatever unspecified error")

          app.view_functions["apiv3_ns.home"] = raise_exception

-         r1 = self.tc.get("/api_3", follow_redirects=True)

+         r1 = self.tc.get("/api_3/", follow_redirects=True)

+         assert r1.status_code == 500

+         data = json.loads(r1.data)

+         assert "Whatever unspecified error" in data["error"]

+ 

+     @new_app_context

+     def test_api_500_default_message(self):

+         def raise_exception():

+             raise CoprHttpException

+         app.view_functions["apiv3_ns.home"] = raise_exception

+         r1 = self.tc.get("/api_3/", follow_redirects=True)

+         assert r1.status_code == 500

+         data = json.loads(r1.data)

+         assert "Generic copr exception" in data["error"]

+ 

+     @new_app_context

+     def test_api_500_runtime_error(self):

+         def raise_exception():

+             raise RuntimeError("Whatever unspecified error")

+         app.view_functions["apiv3_ns.home"] = raise_exception

+         r1 = self.tc.get("/api_3/", follow_redirects=True)

          assert r1.status_code == 500

          data = json.loads(r1.data)

          assert ("Request wasn't successful, there is probably "
@@ -100,7 +120,7 @@ 

          def raise_exception():

              raise InsufficientStorage

          app.view_functions["apiv3_ns.home"] = raise_exception

-         r1 = self.tc.get("/api_3", follow_redirects=True)

+         r1 = self.tc.get("/api_3/", follow_redirects=True)

          assert r1.status_code == 500

          data = json.loads(r1.data)

          assert "Not enough space left" in data["error"]
@@ -110,7 +130,7 @@ 

          def raise_exception():

              raise ActionInProgressException("Hey! Action in progress", None)

          app.view_functions["apiv3_ns.home"] = raise_exception

-         r1 = self.tc.get("/api_3", follow_redirects=True)

+         r1 = self.tc.get("/api_3/", follow_redirects=True)

          assert r1.status_code == 500

          data = json.loads(r1.data)

          assert "Hey! Action in progress" in data["error"]

Fix #1491

The previous implementation had a one simple flaw. Even though
CoprHttpException is a base class and holds a general, unspecified
error, it still has valuable information for the end user. Therefore
we want to return its message, not the well-known

Request wasn't successful, there is probably a bug in the API code.

This error message should be returned only when the caught exception
is not an instance of CoprHttpException, HTTPException or their
descendants because only other exceptions are really bugs in the code
with a message that we don't want to expose to the user.

While digging into the exception handling code, I also dropped all the
specific @app.errorhandler decorators and replaced them with just
one, so catching a new exceptions should be a less of a hassle.

1 new commit added

  • cli: access chroots only through owner/project/chroot argument
3 years ago

This affects UIErrorHandler, too. So you probably need to do the filtering there as well.

Actually fixing the command to accept the "project/chroot" variant would be better IMO. Do you want me to take a look at it?

Actually, I think UIErrorHandler is fine since bccee8da.

Per the commit message

The project/chroot would be unnecessarily confusing because for other
commands we have owner/project. I think it is better to make the distinction
clear and support only full owner/project/chroot format here.

But if you think implementing project/chroot is a good idea, I can do that. The implementation should be easy enough.

But if you think implementing project/chroot is a good idea

I omit the leading <owner> anytime I can. So I think it's IMO a reasonable feature in this case, too.

Actually, I think UIErrorHandler is fine since bccee8d.

+    def handle_error(self, error):
+        # The most common error has their own custom error pages. When catching
+        # a new exception, try to keep it simple and just the the generic one.
+        # Create it's own view only if necessary.
+        error_views = {
+            400: bad_request_handler,
+            403: access_restricted,
+            404: page_not_found,
+            409: conflict_request_handler,
+        }
+        message = self.message(error)
+        if error.code in error_views:
+            return error_views[error.code](message)
+        return generic_error(self.message(error), error.code)

The error.code isn't always defined (for all Exception types)?

rebased onto 89f8e3200b89cc7ce13577f25424ed9fbdbf8d75

3 years ago

I omit the leading <owner> anytime I can. So I think it's IMO a reasonable feature in this case, too.

Fixed

The error.code isn't always defined (for all Exception types)?

Ah, true. I did the same thing as for APIErrorHandler, PTAL. I was also thinking about moving both APIErrorHandler and UIErrorHandler to a same file and inheriting them from something like BaseErrorHandler because they now define code and message methods in an identical way. Do we want to do that or not?

Minor thing is the pylint rant about docstring and @staticmethod; but the frontend build seems failing on F31... Otherwise LGTM.

I was also thinking about moving both APIErrorHandler and UIErrorHandler to a same file and inheriting them from something like BaseErrorHandler because they now define code and message methods in an identical way. Do we want to do that or not?

I don't think it has to be done now, but it doesn't sound like a bad idea. It's up to you.

1 new commit added

  • frontend: move error handlers to the same file
3 years ago

3 new commits added

  • frontend: move error handlers to the same file
  • cli: support project/chroot format for getting/editting chroots
  • frontend: improve APIv3 exception handling for better messages
3 years ago

4 new commits added

  • cli: support project/chroot format for getting/editting chroots
  • frontend: move error handlers to the same file
  • frontend: fix exception tests for F31
  • frontend: improve APIv3 exception handling for better messages
3 years ago

I don't think it has to be done now, but it doesn't sound like a bad idea. It's up to you.

Ah, since I was already in that code, I did the change

Minor thing is the pylint rant about docstring and @staticmethod; but the frontend build seems failing on F31... Otherwise LGTM.

Both fixed.

rebased onto e1c3d72

3 years ago

Pull-Request has been merged by praiskup

3 years ago
Metadata