anchore / anchore-engine

A service that analyzes docker images and scans for vulnerabilities
Apache License 2.0
1.59k stars 272 forks source link

broad try/except blocks makes it hard to handle exceptions #867

Open alfredodeza opened 3 years ago

alfredodeza commented 3 years ago

There is a wide-use of a Python exception handling anti-pattern in the code base. This anti-pattern looks like this:

try:
    logger.info("some info message")
    db.query.images.get(id=1)
except Exception:
    logger.exception("error encountered when querying")

This is problematic in many different ways:

  1. The try part has usually more than one call and can have many lines. A logger.info will probably never cause an exception, and should not be caught. Catching exceptions should be done as targeted as possible.
  2. There should be a good reason to catch every single exception possible, and in this case (as it is with most cases in the code base) there isn't one. Exception catching should be as granular as possible - not generic and broad. This allows flow control by callers by catching individual exceptions.
  3. It is impossible to test this properly because the exception does not bubble up
  4. In most cases where this pattern is found, parent callers of the function are also doing the same catching which doesn't make sense for the apparent goal of producing a message when any exception is raised.
  5. It makes it extremely hard, if not impossible, to debug. It is becoming useful to remove whole try/except blocks when debugging because of this pattern.

Exceptions are there to allow behavior changes and make logical decisions on what to do next. Broad exceptions do not allow that and forces this pattern everywhere else to suppress errors.

Although this is technically a "tech debt" problem - it has been raised before, and the issue is significant, so I'm going to add this as a "bug" category/label as well. It is pressing to address this problem.

zhill commented 3 years ago

This can probably be broken down into a few different issues we can tackle as part of a milestone to cleanup.

  1. Explicit code for the "fail safe" pattern where some code needs to execute and no "reasonable" (e.g. not a RuntimeException) should cause it to exit with an exception that it doesn't explicitly throw. This would help identify these cases and distinguish from lazy handling.
  2. Create a better set of specific Exceptions for Engine. There are parts of the code that have their own but far too many cases just raise Exception types with a message. Without this change the handlers must necessarily be generic.
  3. Consistent mappers in the API layers to handle and raise AnchoreApiError exceptions rather than returning specific response payloads. This is used in some services but not others, so we have a mix of catch-all and return using 'make_error_response()' and raising the more specific exceptions that the web stack will catch and has proper handlers for marshalling responses into HTTP.

I do not think, however, that this is something that can be done randomly or as part of other feature work/bug fixes. We'll need to divide up the code into sections and work thru it systematically since these changes will certainly cause potential regressions in control flow and user-facing error responses.