Remove all instances of `SystemExit` and start implementing our own custom exceptions where needed

[ThomasLaPiana]

Is your feature request related to a specific problem?

Right now we use a lot of SystemExit, but to maintain best practices for software development we should instead be creating custom exceptions everywhere that we would like to raise an exception.

Describe the solution you'd like

A single exceptions.py file per module (fideslang, fidesctl.core, fidesctl.api, fidesctl.connectors, etc...) that contains the custom exceptions.

Describe alternatives you've considered, if any

Not using custom exceptions. Having the exceptions defined in the same place where they are used.

Additional context

N/A

[earmenda]

@ThomasLaPiana Can you clarify what the functional difference is with this change? From the user's perspective does this really change anything? Just want to make sure I understand what we're gaining so it's actually achieved by the implementation

[ThomasLaPiana]

The original thinking around this was that system exits such as https://github.com/ethyca/fides/blob/f780ee8529648525f1d5d3c943e6ff4aa950cdae/src/fidesctl/cli/utils.py#L30 feel really weird. But also, I'm not sure how useful a custom exit or exception would be in that case.

I remember @PSalant726 having some specific examples in mind I believe?

[PSalant726]

@earmenda The goal here is to reduce the leakage of implementation details when our CLI commands fail. There's no need to print a stack trace to stdout if we instead 1) accurately identify our failure scenarios and 2) handle exceptions with clear and uniform feedback to the user. So, when a command fails or an exception is raised, rather than print a wall of text that reveals things like Python internals to the user, we should instead print only a short/clear error message (and optionally log exception information, if #596 includes changes to add CLI logging).

[ThomasLaPiana]

@PSalant726 can you give some concrete examples so we can pattern match and figure out where to start implementing?

[ThomasLaPiana]

de-prioritizing this as it isn't critical for the 1.7 release

[PSalant726]

There are several places in the codebase where the below concept is seen, but the inspiration for this issue came from the check_server function, introduced in #433:

https://github.com/ethyca/fides/blob/ef0f636cf588b3edf2a6ba3ac5913fce198f83a5/src/fidesctl/cli/utils.py#L31-L53

It has four possible exit scenarios...

1. The fidesctl API server is unreachable, so print some red error text to stdout and raise SystemExit(1) with a stack trace.
2. The CLI + API versions don't match, so print some [different] red error text to stdout.
3. Quiet mode is enabled, and the CLI + API versions match, so don't print anything to stdout.
4. Quiet mode is disabled, and the CLI + API versions match, so print some green success text to stdout.

---

The check_server function is currently referenced in two places:

https://github.com/ethyca/fides/blob/ef0f636cf588b3edf2a6ba3ac5913fce198f83a5/src/fidesctl/cli/init.py#L91-L92

https://github.com/ethyca/fides/blob/ef0f636cf588b3edf2a6ba3ac5913fce198f83a5/src/fidesctl/cli/commands/util.py#L93-L104

In both of the above contexts, the code calling check_server is part of the "main" codepath.

Uh... "main codepath"...? What?

Executing any CLI command directly calls the cli() function, and so check_server is called prior to any command handler being executed (except for the status command, which of course calls check_server itself).

---

As a best practice, all logging and user feedback should occur within the main codepath directly, not within helper functions. To resolve this issue with respect to the above example only, I think an acceptable refactor could look like:

# src/fidesctl/cli/utils.py

def check_server(cli_version: str, server_url: str) -> None:
    """Runs a health check and a version check against the server."""

    healthcheck_url = server_url + API_PREFIX + "/health"
    try:
        health_response = check_response(_api.ping(healthcheck_url))
    except requests.exceptions.ConnectionError:
        raise APIServerUnavailableError(url=healthcheck_url)
    except json.JSONDecodeError as err:
        raise InvalidAPIResponseError(err)

    server_version = health_response.json()["version"]
    if clean_version(server_version) != clean_version(cli_version):
        raise APICLIVersionMismatchError(cli_version, server_version)


def clean_version(raw_version: str) -> str:
    """Removes any cruft on version number strings due to local changes."""

    return raw_version.replace(".dirty", "", 1)

# src/fidesctl/cli/__init__.py

if ctx.invoked_subcommand in SERVER_CHECK_COMMAND_NAMES:
    try:
        check_server(VERSION, config.cli.server_url)
    except Exception as err:
        echo_red(err)

# src/fidesctl/cli/commands/util.py

def status(ctx: click.Context) -> None:
    """
    Sends a request to the Fidesctl API healthcheck endpoint and verifies
    that it responds, and that the CLI and API version numbers match.
    """

    click.echo("Checking server status...")

    config = ctx.obj["CONFIG"]
    try:
        check_server(cli_version=fidesctl.__version__, server_url=config.cli.server_url)
    except Exception as err:
        echo_red(err)
    else:
        echo_green(
            "Server is reachable and the client/server application versions match."
        )

...and APIServerUnavailableError, InvalidAPIResponseError, and APICLIVersionMismatchError should be implemented as custom exceptions with __str__() methods that return useful error messages.

The above code is incomplete, but with such a refactor check_server has exactly two exit scenarios: do nothing, or raise an exception. Then, each "main" codepath handles the possible exceptions while producing log output and/or user feedback itself.

---

The results are twofold:

1. For CLI users, implementation details are never leaked in error scenarios
   • CLI users shouldn't and don't need to know that fidesctl is implemented with Python. As long as our error messaging is clear and actionable, no value is lost by hiding stack traces, and the user experience improves.
2. For CLI developers, error messaging and user feedback is easily discoverable, controlled, and modified

This pattern should be repeated throughout the CLI codebase wherever applicable, as our default pattern for exception handling and providing user feedback. Separately, a resolution to #596 can replace echo_red and echo_green with a library that helps maintain output consistency and enables true CLI logging to a local file, for debugging purposes.