diff --git a/changelog.d/175.bugfix.rst b/changelog.d/175.bugfix.rst new file mode 100644 index 00000000..34bd5b19 --- /dev/null +++ b/changelog.d/175.bugfix.rst @@ -0,0 +1 @@ +Improved error handling for configuration loading. Malformed TOML files now trigger a formatted diagnostic message with line and column details instead of a Python traceback. \ No newline at end of file diff --git a/src/docbuild/cli/cmd_cli.py b/src/docbuild/cli/cmd_cli.py index ae127ec5..e8768859 100644 --- a/src/docbuild/cli/cmd_cli.py +++ b/src/docbuild/cli/cmd_cli.py @@ -4,6 +4,7 @@ import logging from pathlib import Path import sys +import tomllib from typing import Any, cast import click @@ -24,7 +25,7 @@ from ..logging import setup_logging from ..models.config.app import AppConfig from ..models.config.env import EnvConfig -from ..utils.errors import format_pydantic_error +from ..utils.errors import format_pydantic_error, format_toml_error from ..utils.pidlock import LockAcquisitionError, PidFileLock from .cmd_build import build from .cmd_c14n import c14n @@ -68,21 +69,70 @@ def handle_validation_error( :param ctx: The Click context, used to exit the CLI with an appropriate status code after handling the error. """ - config_label = "Application" if model_class == AppConfig else "Environment" + # Determine which file we were working on + config_file = str((config_files or ["unknown"])[0]) - if isinstance(e, ValidationError): - # Safely extract the first config file name for the error header - config_file = str((config_files or ["unknown"])[0]) - format_pydantic_error( - e, model_class, config_file, verbose, console=CONSOLE - ) + if isinstance(e, tomllib.TOMLDecodeError): + format_toml_error(e, config_file, console=CONSOLE) + elif isinstance(e, ValidationError): + format_pydantic_error(e, model_class, config_file, verbose, console=CONSOLE) else: - log.error("%s configuration failed validation:", config_label) + config_label = "Application" if model_class == AppConfig else "Environment" + log.error("%s configuration failed:", config_label) log.error("Error in config file(s): %s", config_files) log.error(e) ctx.exit(1) +def load_app_config( + ctx: click.Context, + app_config: Path, + max_workers: str | None +) -> None: + """Load and validate Application configuration. + + :param ctx: The Click context object. The result will be added to ``ctx.obj.appconfig``. + :param app_config: The path to the application config file provided via CLI. + :param max_workers: The max_workers value from CLI options. + """ + context = ctx.obj + result = handle_config( + app_config, + CONFIG_PATHS, + APP_CONFIG_BASENAMES + PROJECT_LEVEL_APP_CONFIG_FILENAMES, + None, + DEFAULT_APP_CONFIG, + ) + context.appconfigfiles, raw_appconfig, context.appconfig_from_defaults = cast( + tuple[tuple[Path, ...] | None, dict[str, Any], bool], result + ) + + if max_workers is not None: + raw_appconfig["max_workers"] = max_workers + + context.appconfig = AppConfig.from_dict(raw_appconfig) + + +def load_env_config(ctx: click.Context, env_config: Path) -> None: + """Load and validate Environment configuration. + + :param ctx: The Click context object. The result will be added to ``ctx.obj.envconfig``. + :param env_config: The path to the environment config file provided via CLI. + """ + context = ctx.obj + result = handle_config( + env_config, + (PROJECT_DIR,), + None, + DEFAULT_ENV_CONFIG_FILENAME, + DEFAULT_ENV_CONFIG, + ) + context.envconfigfiles, raw_envconfig, context.envconfig_from_defaults = cast( + tuple[tuple[Path, ...] | None, dict[str, Any], bool], result + ) + + context.envconfig = EnvConfig.from_dict(raw_envconfig) + @click.group( name=APP_NAME, context_settings={"show_default": True, "help_option_names": ["-h", "--help"]}, @@ -167,67 +217,41 @@ def cli( ctx.ensure_object(DocBuildContext) context = ctx.obj - context.verbose = verbose - context.dry_run = dry_run - context.debug = debug - - # --- PHASE 1: Load and Validate Application Config --- - ( - context.appconfigfiles, - raw_appconfig, - context.appconfig_from_defaults, - ) = handle_config( - app_config, - CONFIG_PATHS, - APP_CONFIG_BASENAMES + PROJECT_LEVEL_APP_CONFIG_FILENAMES, - None, - DEFAULT_APP_CONFIG, - ) - - raw_appconfig = cast(dict[str, Any], raw_appconfig) + context.verbose, context.dry_run, context.debug = verbose, dry_run, debug - if max_workers is not None: - raw_appconfig["max_workers"] = max_workers + # State tracking for centralized error handling + current_model: type[BaseModel] = AppConfig + current_files: Sequence[Path] | None = None try: - context.appconfig = AppConfig.from_dict(raw_appconfig) - except (ValueError, ValidationError) as e: - handle_validation_error(e, AppConfig, context.appconfigfiles, verbose, ctx) + # --- PHASE 1: Load Application Config --- + current_model = AppConfig + current_files = (app_config,) if app_config else None + load_app_config(ctx, app_config, max_workers) - # 3. Setup logging using the validated config object - logging_config = context.appconfig.logging.model_dump( - by_alias=True, exclude_none=True - ) - setup_logging(cliverbosity=verbose, user_config={"logging": logging_config}) - - # --- PHASE 2: Load Environment Config, Validate, and Acquire Lock --- - ( - context.envconfigfiles, - raw_envconfig, - context.envconfig_from_defaults, - ) = handle_config( - env_config, - (PROJECT_DIR,), - None, - DEFAULT_ENV_CONFIG_FILENAME, - DEFAULT_ENV_CONFIG, - ) + # Setup logging + logging_config = context.appconfig.logging.model_dump( + by_alias=True, exclude_none=True + ) + setup_logging(cliverbosity=verbose, user_config={"logging": logging_config}) - raw_envconfig = cast(dict[str, Any], raw_envconfig) + # --- PHASE 2: Load Environment Config --- + current_model = EnvConfig + current_files = (env_config,) if env_config else None + load_env_config(ctx, env_config) - try: - context.envconfig = EnvConfig.from_dict(raw_envconfig) - except (ValueError, ValidationError) as e: - handle_validation_error(e, EnvConfig, context.envconfigfiles, verbose, ctx) + except (ValueError, ValidationError, tomllib.TOMLDecodeError) as e: + handle_validation_error(e, current_model, current_files, verbose, ctx) + # --- PHASE 3: Setup Concurrency Lock --- + # (Remains outside the try block as it has its own specialized error handling) env_config_path = (context.envconfigfiles or [None])[0] - - # --- CONCURRENCY CONTROL --- if env_config_path: ctx.obj.env_lock = PidFileLock(resource_path=cast(Path, env_config_path)) try: ctx.obj.env_lock.__enter__() log.info("Acquired lock for environment config: %r", env_config_path.name) + ctx.call_on_close(lambda: ctx.obj.env_lock.__exit__(None, None, None)) except LockAcquisitionError as e: log.error(str(e)) ctx.exit(1) @@ -235,8 +259,6 @@ def cli( log.error("Failed to set up environment lock: %s", e) ctx.exit(1) - ctx.call_on_close(lambda: ctx.obj.env_lock.__exit__(None, None, None)) - # Add subcommands cli.add_command(build) diff --git a/src/docbuild/utils/errors.py b/src/docbuild/utils/errors.py index bd0ea21b..d7fb4962 100644 --- a/src/docbuild/utils/errors.py +++ b/src/docbuild/utils/errors.py @@ -1,6 +1,8 @@ """Utilities for handling and formatting application errors.""" +import tomllib + from pydantic import BaseModel, ValidationError from rich.console import Console from rich.text import Text @@ -112,3 +114,30 @@ def format_pydantic_error( f"[dim]... and {error_count - max_display} more errors. " "Use '-vv' to see all errors.[/dim]\n" ) + +def format_toml_error( + error: tomllib.TOMLDecodeError, + config_file: str, + console: Console | None = None, +) -> None: + """Format TOML syntax errors using Rich. + + :param error: The caught TOMLDecodeError object. + :param config_file: The name/path of the config file with the syntax error. + :param console: Optional Rich console object. + """ + con = console or Console(stderr=True) + + header = Text.assemble( + ("Syntax error ", "bold red"), + ("in config file ", "white"), + (f"'{config_file}'", "bold cyan"), + (":", "white") + ) + con.print(header) + + # tomllib error messages include the line and column info naturally + con.print(f" [red]{error}[/red]") + con.print() + con.print(" [dim]Please verify that the file is a valid TOML file.[/dim]") + con.print(" [dim]Note: Booleans must be lowercase (true/false) in TOML.[/dim]") diff --git a/tests/cli/test_cmd_cli.py b/tests/cli/test_cmd_cli.py index ecea9867..b464de78 100644 --- a/tests/cli/test_cmd_cli.py +++ b/tests/cli/test_cmd_cli.py @@ -1,6 +1,7 @@ """Tests for the main CLI entry point and configuration loading flow.""" from pathlib import Path +import tomllib from unittest.mock import Mock, patch import click @@ -267,3 +268,49 @@ def resolver(user_path, *args, **kwargs): assert result.exit_code == 0 assert context.verbose == 3 assert context.debug is True + + +@pytest.mark.parametrize("is_app_config_failure", [True, False]) +def test_cli_toml_syntax_error( + runner, + fake_handle_config, + mock_config_models, + is_app_config_failure, +): + """Verify that the CLI handles TOML syntax errors gracefully.""" + + def resolver_with_syntax_error(user_path, *args, **kwargs): + # If we are testing Env failure, let the App config pass first + if not is_app_config_failure and "default_app" in str(user_path): + return (Path("app.toml"),), {"logging": {"version": 1}}, False + + raise tomllib.TOMLDecodeError("Invalid value", "test.toml", 0) + + fake_handle_config(resolver_with_syntax_error) + + result = runner.invoke(cli, ["capture"]) + + assert result.exit_code == 1 + assert "Syntax error in config file" in result.output + assert "Invalid value" in result.output + assert "Booleans must be lowercase" in result.output + assert "Traceback (most recent call last)" not in result.output + + +def test_load_config_helpers_populate_context(fake_handle_config, mock_config_models): + """Verify that the refactored helper functions correctly update the context object.""" + from docbuild.cli.cmd_cli import load_app_config, load_env_config + + # Setup mock resolver + fake_handle_config(lambda *a, **k: ((Path("test.toml"),), {"key": "val"}, False)) + + mock_ctx = Mock() + mock_ctx.obj = DocBuildContext() + + # Test App Loader + load_app_config(mock_ctx, Path("app.toml"), max_workers="5") + assert mock_ctx.obj.appconfig is not None + + # Test Env Loader + load_env_config(mock_ctx, Path("env.toml")) + assert mock_ctx.obj.envconfig is not None diff --git a/tests/utils/test_errors.py b/tests/utils/test_errors.py index f7fa84ca..22118eb9 100644 --- a/tests/utils/test_errors.py +++ b/tests/utils/test_errors.py @@ -1,5 +1,6 @@ """Tests for the Pydantic error formatting utility.""" +import tomllib from typing import Any from pydantic import BaseModel, Field, IPvAnyAddress, ValidationError, create_model @@ -94,3 +95,23 @@ class UnionModel(BaseModel): assert "In 'host':" in captured.err # Verify no bracketed pydantic internals leaked in the path part assert "[" not in captured.err.split("In '")[1].split("':")[0] + + +def test_format_toml_error_smoke(capsys): + """Verify that the TOML syntax error formatter prints correctly.""" + from docbuild.utils.errors import format_toml_error + + # 1. Manually trigger a TOML syntax error + bad_toml_content = "enable_mail = True" # Invalid: Capital T + try: + tomllib.loads(bad_toml_content) + except tomllib.TOMLDecodeError as e: + # 2. Call our new formatter + format_toml_error(e, "env.devel.toml") + + captured = capsys.readouterr() + + # 3. Assertions to verify the "Rich" output content + assert "Syntax error in config file 'env.devel.toml'" in captured.err + assert "Invalid value" in captured.err + assert "Booleans must be lowercase" in captured.err