#2172 Error handling cleanup
Merged 4 months ago by frostyx. Opened 4 months ago by praiskup.
copr/ praiskup/copr fe-error-handling  into  main

@@ -6,15 +6,13 @@ 

  

  import flask

  from sqlalchemy.exc import SQLAlchemyError

- 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,

+ from werkzeug.exceptions import (

+     ClientDisconnected,

+     GatewayTimeout,

+     HTTPException,

+     NotFound,

  )

+ from coprs.exceptions import CoprHttpException

  

  LOG = logging.getLogger(__name__)

  
@@ -24,7 +22,8 @@ 

      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/'):

+     path = flask.request.path

+     if path.startswith('/api_3/') and "gssapi_login/web-ui" not in path:

          return APIErrorHandler()

      return UIErrorHandler()

  
@@ -34,32 +33,72 @@ 

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

+         """

+         code = self.code(error)

+         message = self.message(error)

+         headers = getattr(error, "headers", None)

+         message = self.override_message(message, error)

+         LOG.error("Response error: %s %s", code, message)

+         return self.render(message, code), code, headers

  

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

+     @staticmethod

+     def render(message, code):

+         """

+         Using the message and code, generate the appropriate error page content.

          """

          raise NotImplementedError

  

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

+     def override_message(self, message, error):

+         """

+         For particular sub-class, it may be desired to override/reformat the

+         message for particular error.  This method should be overridden then.

+         """

+         _used_in_subclass = self, error

+         return message

+ 

+     @staticmethod

+     def code(error):

          """

          Return status code for a given exception

          """

          code = getattr(error, "code", 500)

          return code if code is not None else 500

  

- 

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

+     def message(self, error):

          """

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

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

          """

+         message = "Unknown problem"

+ 

+         # Every `CoprHttpException` and `HTTPException` failure has a 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.

          if isinstance(error, HTTPException):

-             return error.description

-         return str(error)

+             message = error.description

+             if isinstance(error, ClientDisconnected):

+                 message = "Client disconnected: " + message

+             return message

+ 

+         if isinstance(error, CoprHttpException):

+             return str(error)

+ 

+         # Everything else would normally be an uncaught exception caused by

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

+         # Redis), or having a bug in the code.  For such cases we try to do

+         # our best to identify the failure reason (but we stay with

+         # error_code=500).

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

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

+         if isinstance(error, SQLAlchemyError):

+             message = "Database error, contact admin"

+         self._log_admin_only_exception()

+         return message

  

      def _log_admin_only_exception(self):

          # pylint: disable=no-self-use
@@ -73,69 +112,31 @@ 

      """

      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)

- 

-         message = "Server error, contact admin"

-         if isinstance(error, SQLAlchemyError):

-             code = 500

-             message = "Database error, contact admin"

- 

-         self._log_admin_only_exception()

-         return generic_error(message, code)

+     @staticmethod

+     def render(message, code):

+         title = {

+             400: "Bad Request",

+             409: "Conflict",

+             403: "Access Restricted",

+             404: "Page Not Found",

+             422: "Unprocessable Entity",

+         }.get(code, "Unknown error")

+         return flask.render_template("html-error.html",

+                                      message=message,

+                                      error_code=code,

+                                      error_title=title)

  

  

  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 = {

+     def override_message(self, message, error):

+         return {

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

-         self._log_admin_only_exception()

-         return self.respond(message, code)

+         }.get(error.__class__, message)

  

-     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

+     @staticmethod

+     def render(message, code):

+         return flask.jsonify(error=message)

@@ -3,9 +3,10 @@ 

      _default = "Generic copr exception"

      _code = 500

  

-     def __init__(self, message=None, code=None, **kwargs):

+     def __init__(self, message=None, code=None, headers=None, **kwargs):

          self.message = str(message) if message else None

          self.code = code or self._code

+         self.headers = headers

          self.kwargs = kwargs

  

      def __unicode__(self):

@@ -1,11 +0,0 @@ 

- {% extends "layout.html" %}

- {% block title %}Permission denied{% endblock %}

- {% block header %}Permission denied!{% endblock %}

- {% block body %}

-   <h1> Error 403: Access Denied/Forbidden </h1>

-   {% if message %}

-     {{ message }}

-   {% else %}

-     You are not allowed to see this page.

-   {% endif %}

- {% endblock %}

@@ -1,13 +0,0 @@ 

- {% extends "layout.html" %}

- {% block title %}Not Found{% endblock %}

- {% block header %}Not Found!{% endblock %}

- {% block body %}

-   <h1> Error 404: Not Found </h1>

-   {% if message %}

-     {% for line in message.split("\n") %}

-       {{ line }} {% if not loop.last %}<br />{% endif %}

-     {% endfor %}

-   {% else %}

-     The thing you're looking for does not exist.

-   {% endif %}

- {% endblock %}

frontend/coprs_frontend/coprs/templates/html-error.html frontend/coprs_frontend/coprs/templates/_error.html
file renamed
+17 -21
@@ -1,24 +1,20 @@ 

  {% extends "layout.html" %}

- {% block title %}Not Found{% endblock %}

- {% block header %}Not Found!{% endblock %}

+ {% set error_title = error_title or "Unknown error" %}

+ {% block title %}Copr Build System – {{ error_title }}{% endblock %}

  {% block body %}

-   <h1> Error {{ error_code }}

-   {% if error_title %}

-     : {{ error_title }}

-   {% endif %}

- 

-   </h1>

-   {% if message %}

-     {{ message }}

-   {% else %}

-     Something went wrong

-   {% endif %}

- 

-   {% if config.debug or config.env != "production" %}

-     <p>

-       Administrators should take a look at

-       <code>/var/log/copr-frontend/frontend.log</code>

-       to analyze the full error output.

-     </p>

-   {% endif %}

+ <h1> Error {{ error_code }}: {{ error_title }}</h1>

+ {% if message %}

+ {% for line in message.split("\n") %}

+   {{ line }} {% if not loop.last %}<br />{% endif %}

+ {% endfor %}

+ {% else %}

+    <p>Something went wrong.  Contact admins.</p>

+    {% if config.debug or config.env != "production" %}

+      <p>

+        Administrators should take a look at

+        <code>/var/log/copr-frontend/frontend.log</code>

+        to analyze the full error output.

+      </p>

+    {% endif %}

+ {% endif %}

  {% endblock %}

@@ -61,8 +61,6 @@ 

      Return appropriately formatted GSSAPI 403 error for both web-ui and API

      """

      message = "Can't log-in using GSSAPI: " + message

-     if "web-ui" in flask.request.full_path:

-         return flask.render_template("403.html", message=message), 403

      raise AccessRestricted(message)

  

  

@@ -27,7 +27,7 @@ 

  from coprs import forms

  from coprs import helpers

  from coprs import models

- from coprs.exceptions import ObjectNotFound

+ from coprs.exceptions import ObjectNotFound, BadRequest, CoprHttpException

  from coprs.logic.coprs_logic import CoprsLogic, PinnedCoprsLogic, MockChrootsLogic

  from coprs.logic.stat_logic import CounterStatLogic

  from coprs.logic.modules_logic import ModulesLogic, ModulemdGenerator, ModuleBuildFacade
@@ -37,9 +37,7 @@ 

  from coprs.logic.outdated_chroots_logic import OutdatedChrootsLogic

  

  from coprs.views.misc import (

-     generic_error,

      login_required,

-     page_not_found,

      req_with_copr,

      req_with_copr_dir,

      req_with_pagination,
@@ -114,8 +112,7 @@ 

  def coprs_by_user(username=None, page=1):

      user = users_logic.UsersLogic.get(username).first()

      if not user:

-         return page_not_found(

-             "User {0} does not exist.".format(username))

+         raise ObjectNotFound("User {0} does not exist.".format(username))

  

      pinned = [pin.copr for pin in PinnedCoprsLogic.get_by_user_id(user.id)] if page == 1 else []

      query = CoprsLogic.get_multiple_owned_by_username(username)
@@ -947,9 +944,12 @@ 

                  available_chroots.append(available_chroot.name)

                  html_error_msg += available_chroot.name + "\n"

  

-         cmd_error_msg = {"available chroots": available_chroots}

-         return flask.render_template("404.html", message=html_error_msg), 404, {

-             'Copr-Error-Data': base64.b64encode(json.dumps(cmd_error_msg).encode("utf-8"))}

+         dnf_copr_error = {"available chroots": available_chroots}

+         dnf_copr_error_data = json.dumps(dnf_copr_error).encode("utf-8")

+         dnf_copr_plugin_headers = {

+             "Copr-Error-Data": base64.b64encode(dnf_copr_error_data),

+         }

+         raise ObjectNotFound(html_error_msg, headers=dnf_copr_plugin_headers)

  

      # append multilib counterpart repo only upon explicit request (ach != None),

      # and only if the chroot actually is multilib capable
@@ -1037,20 +1037,6 @@ 

  

  #########################################################

  

- @coprs_ns.route("/<username>/<coprname>/rpm/<name_release>/<rpmfile>")

- def copr_repo_rpm_file(username, coprname, name_release, rpmfile):

-     try:

-         packages_dir = os.path.join(app.config["DATA_DIR"], "repo-rpm-packages")

-         with open(os.path.join(packages_dir, rpmfile), "rb") as rpm:

-             response = flask.make_response(rpm.read())

-             response.mimetype = "application/x-rpm"

-             response.headers["Content-Disposition"] = \

-                 "filename={0}".format(rpmfile)

-             return response

-     except IOError:

-         return flask.render_template("404.html")

- 

- 

  @coprs_ns.route("/<username>/<coprname>/monitor/")

  @coprs_ns.route("/<username>/<coprname>/monitor/<detailed>")

  @coprs_ns.route("/g/<group_name>/<coprname>/monitor/")
@@ -1106,7 +1092,7 @@ 

      if form.validate_on_submit():

          dstgroup = ([g for g in flask.g.user.user_groups if g.at_name == form.owner.data] or [None])[0]

          if flask.g.user.name != form.owner.data and not dstgroup:

-             return generic_error("There is no such group: {}".format(form.owner.data))

+             raise BadRequest("There is no such group: {}".format(form.owner.data))

  

          dst_copr = CoprsLogic.get(flask.g.user.name, form.name.data).all()

          if dst_copr and not form.confirm.data:
@@ -1262,7 +1248,7 @@ 

          msg = ("Unable to parse module YAML. The most probable reason is that "

                 "the module YAML is in an old modulemd version and/or has some "

                 "mistake in it. Hint: {}".format(ex))

-         return generic_error(msg, code=422, title="Unable to parse module YAML")

+         raise CoprHttpException(msg, code=422) from ex

  

      return flask.render_template("coprs/detail/module.html", copr=copr, module=module,

                                   yaml=pretty_yaml, unique_chroots=unique_chroots)

@@ -2,17 +2,4 @@ 

  

  import flask

  

- from coprs.views.misc import page_not_found, access_restricted

- from coprs.exceptions import ObjectNotFound, AccessRestricted

- 

  groups_ns = flask.Blueprint("groups_ns", __name__, url_prefix="/groups")

- 

- 

- @groups_ns.errorhandler(ObjectNotFound)

- def handle_404(error):

-     return page_not_found(error.message)

- 

- 

- @groups_ns.errorhandler(AccessRestricted)

- def handle_403(error):

-     return access_restricted(error.message)

@@ -2,7 +2,7 @@ 

  

  import flask

  from flask import render_template, url_for

- from coprs.exceptions import InsufficientRightsException

+ from coprs.exceptions import InsufficientRightsException, ObjectNotFound

  from coprs.forms import ActivateFasGroupForm

  from coprs.helpers import Paginator

  from coprs.logic import builds_logic
@@ -86,7 +86,7 @@ 

  @login_required

  def list_user_groups():

      if not app.config['FAS_LOGIN']:

-         return flask.render_template("404.html"), 404

+         raise ObjectNotFound("Logging via Fedora Accounts not enabled")

  

      teams = flask.g.user.user_teams

      active_map = {

@@ -1,7 +1,7 @@ 

  import base64

  import datetime

  import functools

- from functools import wraps, partial

+ from functools import wraps

  from urllib.parse import urlparse

  import flask

  
@@ -52,29 +52,6 @@ 

              models.User.username == username).first()

  

  

- def page_not_found(message):

-     return flask.render_template("404.html", message=message), 404

- 

- 

- def access_restricted(message):

-     return flask.render_template("403.html", message=message), 403

- 

- 

- def generic_error(message, code=500, title=None):

-     """

-     :type message: str

-     :type err: CoprHttpException

-     """

-     return flask.render_template("_error.html",

-                                  message=message,

-                                  error_code=code,

-                                  error_title=title), code

- 

- 

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

  

  

@@ -7,10 +7,9 @@ 

  from coprs.logic.complex_logic import ComplexLogic

  from coprs.logic.packages_logic import PackagesLogic

  

- from coprs.exceptions import ObjectNotFound

+ from coprs.exceptions import ObjectNotFound, AccessRestricted

  

  from coprs.views.webhooks_ns import webhooks_ns

- from coprs.views.misc import page_not_found, access_restricted

  

  import logging

  import os
@@ -77,13 +76,9 @@ 

  def webhooks_bitbucket_push(copr_id, uuid):

      # For the documentation of the data we receive see:

      # https://confluence.atlassian.com/bitbucket/event-payloads-740262817.html

-     try:

-         copr = ComplexLogic.get_copr_by_id_safe(copr_id)

-     except ObjectNotFound:

-         return page_not_found("Project does not exist")

- 

+     copr = ComplexLogic.get_copr_by_id_safe(copr_id)

      if copr.webhook_secret != uuid:

-         return access_restricted("This webhook is not valid")

+         raise AccessRestricted("This webhook is not valid")

  

      try:

          payload = flask.request.json
@@ -123,13 +118,9 @@ 

          return "OK", 200

      # For the documentation of the data we receive see:

      # https://developer.github.com/v3/activity/events/types/#pushevent

-     try:

-         copr = ComplexLogic.get_copr_by_id_safe(copr_id)

-     except ObjectNotFound:

-         return page_not_found("Project does not exist")

- 

+     copr = ComplexLogic.get_copr_by_id_safe(copr_id)

      if copr.webhook_secret != uuid:

-         return access_restricted("This webhook is not valid")

+         raise AccessRestricted("This webhook is not valid")

  

      try:

          payload = flask.request.json
@@ -171,13 +162,9 @@ 

  def webhooks_gitlab_push(copr_id, uuid):

      # For the documentation of the data we receive see:

      # https://gitlab.com/help/user/project/integrations/webhooks#events

-     try:

-         copr = ComplexLogic.get_copr_by_id_safe(copr_id)

-     except ObjectNotFound:

-         return page_not_found("Project does not exist")

- 

+     copr = ComplexLogic.get_copr_by_id_safe(copr_id)

      if copr.webhook_secret != uuid:

-         return access_restricted("This webhook is not valid")

+         raise AccessRestricted("This webhook is not valid")

  

      try:

          payload = flask.request.json

@@ -25,7 +25,7 @@ 

      def test_both_nonexisting_page_and_object(self):

          r1 = self.tc.get("/nonexisting/endpoint/")

          assert r1.status_code == 404

-         assert "<h1> Error 404: Not Found </h1>" in str(r1.data)

+         assert "<h1> Error 404: Page Not Found</h1>" in str(r1.data)

  

          r2 = self.tc.get("/coprs/nonexisting/nonexisting/")

          assert r2.status_code == 404
@@ -113,7 +113,7 @@ 

          assert r1.status_code == 500

          data = json.loads(r1.data)

          assert ("Request wasn't successful, there is probably "

-                 "a bug in the API code.") in data["error"]

+                 "a bug in the Copr code.") in data["error"]

  

      @new_app_context

      def test_api_500_storage(self):

Remove several duplications, and make sure we generate the error html pages
on one place - while the rest of the code just raises the exceptions.

Metadata Update from @praiskup:
- Pull-request tagged with: wip

4 months ago

4 new commits added

  • frontend: concentrate exception handling on one place
  • frontend: no need to special-case @group_ns
  • frontend: de-duplicate the html error templates
  • frontend: drop one unused route
4 months ago

Build succeeded.

6 new commits added

  • frontend: detect ClientDisconnected errors
  • frontend: unify the UI and API error handling
  • frontend: concentrate exception handling in one file
  • frontend: no need to special-case @group_ns
  • frontend: de-duplicate the html error templates
  • frontend: drop one unused route
4 months ago

Metadata Update from @praiskup:
- Pull-request untagged with: wip

4 months ago

Build succeeded.

6 new commits added

  • frontend: detect ClientDisconnected errors
  • frontend: unify the UI and API error handling
  • frontend: concentrate exception handling in one file
  • frontend: no need to special-case @group_ns
  • frontend: de-duplicate the html error templates
  • frontend: drop one unused route
4 months ago

Build succeeded.

rebased onto 7597d338200662b4b0be399dad39dea7b7678bf5

4 months ago

Build succeeded.

The "This method is expected to be implemented in descendants of this class." in the docstring is no longer true.

I think you can return the message straight away. Same for the elif branch below. It would allow not having the else branch and it would IMHO simplify the code a little. Because now we need to read the function to the end to be sure nothing more is done with the message and that it is just simply returned.

I don't like that much that we are returning 3 values.
Looking at both UIErrorHandler and APIErrorHandler, they are returning the code and headers unchanged. They both differ only in creating the first parameter. Do you think we could drop the other two from here and deal with them in BaseErrorHandler?

Thank you for the review! This comment is the only one I'm not 100% is the best idea. I originally thought that we could be, in further sub-classes, interested in changing at least the headers dict. But sure we can do this later once we really need it ... lemme fix this, too.

6 new commits added

  • frontend: detect ClientDisconnected errors
  • frontend: unify the UI and API error handling
  • frontend: concentrate exception handling in one file
  • frontend: no need to special-case @group_ns
  • frontend: de-duplicate the html error templates
  • frontend: drop one unused route
4 months ago

Build succeeded.

+1, thank you for the changes.

rebased onto 4c2e243

4 months ago

Build succeeded.

Pull-Request has been merged by frostyx

4 months ago