From 3e82ae3d1350375628251083a5ec07edb70efcfc Mon Sep 17 00:00:00 2001
From: Noemi <45180344+unflxw@users.noreply.github.com>
Date: Tue, 5 Mar 2024 12:53:04 +0100
Subject: [PATCH] Catch unexpected errors reading config file

When reading the `__appsignal__.py` configuration file from the
demo command, handle unexpected errors in the file (e.g. syntax
errors) instead of only handling the expected error of the
`appsignal` variable being missing.

Unify the handling of errors when reading a config file. Provide
an `ExitError` that can be raised by command helpers to trigger
the command to exit.

Since loading the configuration file is only done by the diagnose
and demo commands, move that functionality out of the `Client` file
and into a helper for the CLI commands.
---
 src/appsignal/cli/base.py       |  3 +++
 src/appsignal/cli/command.py    | 12 ++++++++++
 src/appsignal/cli/config.py     | 41 +++++++++++++++++++++++++++++++++
 src/appsignal/cli/demo.py       | 11 +++------
 src/appsignal/cli/diagnose.py   | 14 +++++------
 src/appsignal/cli/exit_error.py |  8 +++++++
 src/appsignal/client.py         | 29 -----------------------
 tests/cli/test_demo.py          |  5 +---
 tests/cli/test_diagnose.py      |  5 +---
 9 files changed, 76 insertions(+), 52 deletions(-)
 create mode 100644 src/appsignal/cli/config.py
 create mode 100644 src/appsignal/cli/exit_error.py

diff --git a/src/appsignal/cli/base.py b/src/appsignal/cli/base.py
index e0e0cfcf..6c534b81 100644
--- a/src/appsignal/cli/base.py
+++ b/src/appsignal/cli/base.py
@@ -7,6 +7,7 @@
 from .command import AppsignalCLICommand
 from .demo import DemoCommand
 from .diagnose import DiagnoseCommand
+from .exit_error import ExitError
 from .install import InstallCommand
 from .version import VersionCommand
 
@@ -36,6 +37,8 @@ def main(argv: list[str]) -> int:
     cmd = cmd_class(args=args)
     try:
         return cmd.run()
+    except ExitError as error:
+        return error.exit_code
     except KeyboardInterrupt:
         return 0
 
diff --git a/src/appsignal/cli/command.py b/src/appsignal/cli/command.py
index e93a2fe6..294f06c3 100644
--- a/src/appsignal/cli/command.py
+++ b/src/appsignal/cli/command.py
@@ -6,6 +6,10 @@
 from dataclasses import dataclass
 from functools import cached_property
 
+from ..client import Client
+from .config import _client_from_config_file
+from .exit_error import ExitError
+
 
 @dataclass(frozen=True)
 class AppsignalCLICommand(ABC):
@@ -66,3 +70,11 @@ def _environment(self) -> str | None:
                 "Please enter the application environment (development/production): "
             )
         return environment
+
+    def _client_from_config_file(self) -> Client | None:
+        try:
+            return _client_from_config_file()
+        except Exception as error:
+            print(f"Error loading the __appsignal__.py configuration file:\n{error}\n")
+            print("Exiting.")
+            raise ExitError(1) from error
diff --git a/src/appsignal/cli/config.py b/src/appsignal/cli/config.py
new file mode 100644
index 00000000..85473b28
--- /dev/null
+++ b/src/appsignal/cli/config.py
@@ -0,0 +1,41 @@
+from __future__ import annotations
+
+import os
+from runpy import run_path
+
+from ..client import Client
+
+
+# Load the AppSignal client from the app specific `__appsignal__.py` client
+# file. This loads the user config, rather than our default config.
+# If no client file is found it return `None`.
+# If there's a problem with the client file it will raise an
+# `InvalidClientFileError` with a message containing more details.
+def _client_from_config_file() -> Client | None:
+    cwd = os.getcwd()
+    app_config_path = os.path.join(cwd, "__appsignal__.py")
+    if os.path.exists(app_config_path):
+        try:
+            client = run_path(app_config_path)["appsignal"]
+            if not isinstance(client, Client):
+                raise InvalidClientFileError(
+                    "The `appsignal` variable does not contain an AppSignal client. "
+                    "Please define the configuration file as described in "
+                    "our documentation: "
+                    "https://docs.appsignal.com/python/configuration.html"
+                )
+
+            return client
+        except KeyError as error:
+            raise InvalidClientFileError(
+                "No `appsignal` variable found. "
+                "Please define the configuration file as described in "
+                "our documentation: "
+                "https://docs.appsignal.com/python/configuration.html"
+            ) from error
+
+    return None
+
+
+class InvalidClientFileError(Exception):
+    pass
diff --git a/src/appsignal/cli/demo.py b/src/appsignal/cli/demo.py
index f2520254..dea4e53b 100644
--- a/src/appsignal/cli/demo.py
+++ b/src/appsignal/cli/demo.py
@@ -5,13 +5,13 @@
 
 from opentelemetry import trace
 
-from ..client import Client, InvalidClientFileError
+from ..client import Client
 from ..tracing import set_category, set_error, set_params, set_tag
 from .command import AppsignalCLICommand
 
 
 class DemoCommand(AppsignalCLICommand):
-    """Run demo application."""
+    """Send demonstration data to AppSignal."""
 
     @staticmethod
     def init_parser(parser: ArgumentParser) -> None:
@@ -20,12 +20,7 @@ def init_parser(parser: ArgumentParser) -> None:
         AppsignalCLICommand._push_api_key_argument(parser)
 
     def run(self) -> int:
-        try:
-            client = Client._load__appsignal__file()
-        except InvalidClientFileError as error:
-            print(f"Error: {error}")
-            print("Exiting.")
-            return 1
+        client = self._client_from_config_file()
 
         if client:
             # For demo CLI purposes, AppSignal is always active
diff --git a/src/appsignal/cli/diagnose.py b/src/appsignal/cli/diagnose.py
index db43a8f3..15b91ae2 100644
--- a/src/appsignal/cli/diagnose.py
+++ b/src/appsignal/cli/diagnose.py
@@ -14,7 +14,6 @@
 
 from ..__about__ import __version__
 from ..agent import Agent
-from ..client import Client, InvalidClientFileError
 from ..config import Config
 from ..push_api_key_validator import PushApiKeyValidator
 from .command import AppsignalCLICommand
@@ -172,18 +171,19 @@ def run(self) -> int:
             print("Error: Cannot use --send-report and --no-send-report together.")
             return 1
 
-        try:
-            client = Client._load__appsignal__file()
-        except InvalidClientFileError as error:
-            print(f"Error: {error}")
-            print("Exiting.")
-            return 1
+        client = self._client_from_config_file()
 
         if client:
             self.config = client._config
         else:
+            print(
+                "Could not load the configuration from the `__appsignal__.py` "
+                "configuration file. Some configuration options may be missing."
+            )
             self.config = Config()
 
+        print()
+
         agent = Agent()
         agent_json = json.loads(agent.diagnose(self.config))
         self.agent_report = AgentReport(agent_json)
diff --git a/src/appsignal/cli/exit_error.py b/src/appsignal/cli/exit_error.py
new file mode 100644
index 00000000..4bcede05
--- /dev/null
+++ b/src/appsignal/cli/exit_error.py
@@ -0,0 +1,8 @@
+from __future__ import annotations
+
+
+class ExitError(Exception):
+    exit_code: int
+
+    def __init__(self, exit_code: int) -> None:
+        self.exit_code = exit_code
diff --git a/src/appsignal/client.py b/src/appsignal/client.py
index 490a393c..8ef96def 100644
--- a/src/appsignal/client.py
+++ b/src/appsignal/client.py
@@ -1,10 +1,8 @@
 from __future__ import annotations
 
 import logging
-import os
 import sys
 from logging import DEBUG, ERROR, INFO, WARNING, Logger
-from runpy import run_path
 from typing import TYPE_CHECKING, ClassVar
 
 from .agent import agent
@@ -28,29 +26,6 @@ class Client:
         "trace": DEBUG,
     }
 
-    # Load the AppSignal client from the app specific `__appsignal__.py` client
-    # file. This loads the user config, rather than our default config.
-    # If no client file is found it return `None`.
-    # If there's a problem with the client file it will raise an
-    # `InvalidClientFileError` with a message containing more details.
-    @staticmethod
-    def _load__appsignal__file() -> Client | None:
-        cwd = os.getcwd()
-        app_config_path = os.path.join(cwd, "__appsignal__.py")
-        if os.path.exists(app_config_path):
-            try:
-                return run_path(app_config_path)["appsignal"]
-            except KeyError as error:
-                raise InvalidClientFileError(
-                    "No `appsignal` variable was exported by the "
-                    "__appsignal__.py config file. "
-                    "Please update the __appsignal__.py file as described in "
-                    "our documentation: "
-                    "https://docs.appsignal.com/python/configuration.html"
-                ) from error
-
-        return None
-
     def __init__(self, **options: Unpack[Options]) -> None:
         self._config = Config(options)
         self.start_logger()
@@ -95,7 +70,3 @@ def _start_stdout_logger(self) -> None:
             )
         )
         self._logger.addHandler(handler)
-
-
-class InvalidClientFileError(Exception):
-    pass
diff --git a/tests/cli/test_demo.py b/tests/cli/test_demo.py
index c49ec05e..ba070dda 100644
--- a/tests/cli/test_demo.py
+++ b/tests/cli/test_demo.py
@@ -142,7 +142,4 @@ def test_demo_with_invalid_config_file(request, mocker, capfd):
     os.chdir(request.config.invocation_params.dir)
 
     out, err = capfd.readouterr()
-    assert (
-        "No `appsignal` variable was exported by the __appsignal__.py config file."
-        in out
-    )
+    assert "No `appsignal` variable found" in out
diff --git a/tests/cli/test_diagnose.py b/tests/cli/test_diagnose.py
index 1b0b4326..2ce1061d 100644
--- a/tests/cli/test_diagnose.py
+++ b/tests/cli/test_diagnose.py
@@ -115,10 +115,7 @@ def test_diagnose_with_invalid_config_file(request, mocker, capfd):
     os.chdir(request.config.invocation_params.dir)
 
     out, err = capfd.readouterr()
-    assert (
-        "No `appsignal` variable was exported by the __appsignal__.py config file."
-        in out
-    )
+    assert "No `appsignal` variable found" in out
 
 
 def test_diagnose_with_missing_paths(mocker, capfd):