Fix #88: Implement Pydantic validation for App Configuration (#91)

* Fix #88: Implement Pydantic validation for App Configuration

Signed-off-by: sushant-suse <[email protected]>

* fix: #88: moved the model to models folder

Signed-off-by: sushant-suse <[email protected]>

* fix: #88: added more verbose in the Field call

Signed-off-by: sushant-suse <[email protected]>

* fix #88: Implemented strict Pydantic validation for the Application Configuration (AppConfig)

Signed-off-by: sushant-suse <[email protected]>

* fix: #88: updated imports

Signed-off-by: sushant-suse <[email protected]>

* Fix: #88: updated app.py and added a new test file

Signed-off-by: sushant-suse <[email protected]>

* Fix: #88: Added CROSS-REFERENCE VALIDATION TESTS

Signed-off-by: sushant-suse <[email protected]>

* Fix #88: Update failing tests

Signed-off-by: sushant-suse <[email protected]>

* Fix: #88: covered the issue in pidlock.py file

Signed-off-by: sushant-suse <[email protected]>

* Fix: #88: reverted back pidlock

Signed-off-by: sushant-suse <[email protected]>

---------

Signed-off-by: sushant-suse <[email protected]>

Author: sushant-suse

changelog.d/91.feature.rst

@@ -0,0 +1 @@
+Implemented strict Pydantic validation for the Application Configuration (AppConfig), including a precise, nested schema for all logging parameters (formatters, handlers, loggers). This prevents runtime errors due to configuration typos and ensures data integrity at startup.

src/docbuild/cli/cmd_cli.py

@@ -8,10 +8,12 @@
 import rich.logging
 from rich.pretty import install
 from rich.traceback import install as install_traceback
+from pydantic import ValidationError
 
 from ..__about__ import __version__
 from ..config.app import replace_placeholders
 from ..config.load import handle_config
+from ..models.config_model.app import AppConfig
 from ..constants import (
     APP_CONFIG_BASENAMES,
     APP_NAME,
@@ -124,10 +126,13 @@ def cli(
     context.verbose = verbose
     context.dry_run = dry_run
     context.debug = debug
-    # Set the defaults. Will be overridden if config files are provided.
+    
+    # --- PHASE 1: Load and Validate Application Config ---
+    
+    # 1. Load the raw application config dictionary
     (
         context.appconfigfiles,
-        context.appconfig,
+        raw_appconfig, # Store config as raw dictionary
         context.appconfig_from_defaults,
     ) = handle_config(
         app_config,
@@ -136,12 +141,25 @@ def cli(
         None,
         DEFAULT_APP_CONFIG,
     )
-    context.appconfig = replace_placeholders(context.appconfig)
 
-    # Phase 2: Advanced logging setup with user configuration.
-    logging_config = context.appconfig.get('logging', {})
+    # 2. Validate the raw config dictionary using Pydantic
+    try:
+        # Pydantic validation also handles placeholder replacement via @model_validator
+        context.appconfig = AppConfig.from_dict(raw_appconfig)
+    except (ValueError, ValidationError) as e:
+        log.error("Application configuration failed validation:")
+        log.error("Error in config file(s): %s", context.appconfigfiles)
+        log.error(e)
+        ctx.exit(1)
+
+    # 3. Setup logging using the validated config object
+    # Use model_dump(by_alias=True) to ensure the 'class' alias is used.
+    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 and Acquire Lock ---
+    
+    # Load Environment Config (still returns raw dict)
     (
         context.envconfigfiles,
         context.envconfig,
@@ -153,8 +171,6 @@ def cli(
         DEFAULT_ENV_CONFIG_FILENAME,
         DEFAULT_ENV_CONFIG,
     )
-    # The environment config file path is the resource ID for the lock.
-    # The path is in context.envconfigfiles, which is a tuple of paths.
 
     env_config_path = context.envconfigfiles[0] if context.envconfigfiles else None
 
@@ -177,6 +193,7 @@ def cli(
             ctx.exit(1)
 
     # Final config processing must happen outside the lock acquisition check
+    # Note: Placeholder replacement is still necessary here because EnvConfig is not validated yet.
     context.envconfig = replace_placeholders(
         context.envconfig,
     )

src/docbuild/config/load.py

@@ -1,4 +1,4 @@
-"""Load and process configuration files.""" ''
+"""Load and process configuration files."""
 
 from collections.abc import Iterable, Sequence
 from itertools import product
@@ -7,15 +7,18 @@
 from typing import Any
 
 from ..constants import DEFAULT_ENV_CONFIG_FILENAME
-from .app import replace_placeholders
 from .merge import deep_merge
 
 
 def process_envconfig(envconfigfile: str | Path | None) -> tuple[Path, dict[str, Any]]:
     """Process the env config.
 
+    Note: This function now returns the raw dictionary. Validation and
+    placeholder replacement should be done by the caller using a Pydantic model
+    (e.g., EnvConfig).
+
     :param envconfigfile: Path to the env TOML config file.
-    :return: Tuple of the env config file path and the config object.
+    :return: Tuple of the env config file path and the config object (raw dict).
     :raise ValueError: If neither envconfigfile nor role is provided.
     """
     if envconfigfile:
@@ -32,7 +35,7 @@ def process_envconfig(envconfigfile: str | Path | None) -> tuple[Path, dict[str,
         )
 
     rawconfig = load_single_config(envconfigfile)
-    return envconfigfile, replace_placeholders(rawconfig)
+    return envconfigfile, rawconfig
 
 
 def load_single_config(configfile: str | Path) -> dict[str, Any]:
@@ -62,7 +65,7 @@ def load_and_merge_configs(
     :param defaults: a sequence of base filenames (without path!) to look for
                      in the paths
     :param paths: the paths to look for config files (without the filename!)
-    :return: the found config files and the merged dictionary
+    :return: the found config files and the merged dictionary (raw dict)
     """
     configs: Sequence[dict[str, Any]] = []
     configfiles: Sequence[Path] = []
@@ -95,6 +98,9 @@ def handle_config(
 ) -> tuple[tuple[Path, ...] | None, object | dict, bool]:
     """Return (config_files, config, from_defaults) for config file handling.
 
+    Note: The returned configuration is the **raw loaded dictionary**. No
+    placeholder replacement or validation has been performed on it.
+
     :param user_path: Path to the user-defined config file, if any.
     :param search_dirs: Iterable of directories to search for config files.
     :param basenames: Iterable of base filenames to search for.
@@ -122,4 +128,4 @@ def handle_config(
             if candidate.exists():
                 return (candidate,), load_single_config(candidate), False
     # Not found, use defaults
-    return None, default_config, True
+    return None, default_config, True

src/docbuild/logging.py

@@ -9,13 +9,12 @@
 import queue
 import time
 from pathlib import Path
-from typing import Any
+from typing import Any, Sequence
 
 from .constants import APP_NAME, BASE_LOG_DIR, GITLOGGER_NAME
 
 # --- Default Logging Configuration ---
-# This dictionary provides a flexible, default setup that can be easily
-# overridden by a user's configuration file.
+# This dictionary provides the minimal required configuration structure.
 DEFAULT_LOGGING_CONFIG = {
     "version": 1,
     "disable_existing_loggers": False,
@@ -48,7 +47,7 @@
             "level": "DEBUG",
             "propagate": False,
         },
-        GITLOGGER_NAME: { # Use the constant here
+        GITLOGGER_NAME: {
             "handlers": ["console", "file"],
             "level": "DEBUG",
             "propagate": False,
@@ -68,10 +67,9 @@
 
 def create_base_log_dir(base_log_dir: str | Path = BASE_LOG_DIR) -> Path:
     """Create the base log directory if it doesn't exist.
-
-    This directory is typically located at :file:`~/.local/state/docbuild/logs`
+    
+    This directory is typically located at :file:`~/.local/state/docbuild/logs`		
     as per the XDG Base Directory Specification.
-
     :param base_log_dir: The base directory where logs should be stored.
         Considers the `XDG_STATE_HOME` environment variable if set.
     :return: The path to the base log directory.
@@ -92,14 +90,14 @@ def setup_logging(
 ) -> None:
     """Sets up a non-blocking, configurable logging system.
     
-    :param cliverbosity: ...
-    :param user_config: ...
+    This function merges the default logging configuration with the validated
+    user configuration and sets up the asynchronous handlers.
     """
     config = copy.deepcopy(DEFAULT_LOGGING_CONFIG)
 
     if user_config and "logging" in user_config:
         # Use a more robust deep merge approach
-        def deep_merge(target, source):
+        def deep_merge(target: dict, source: dict) -> None:
             for k, v in source.items():
                 if k in target and isinstance(target[k], dict) and isinstance(v, dict):
                     deep_merge(target[k], v)
@@ -109,31 +107,63 @@ def deep_merge(target, source):
         deep_merge(config, user_config.get("logging", {}))
 
     # --- Verbosity & Log File Path Setup ---
-    # The handler's level determines what gets printed/written.
     verbosity_level = LOGLEVELS.get(min(cliverbosity, 2), logging.WARNING)
     config["handlers"]["console"]["level"] = logging.getLevelName(verbosity_level)
 
     log_dir = create_base_log_dir()
-    # Use a timestamp to generate a unique filename for each run.
     timestamp = time.strftime("%Y-%m-%d_%H-%M-%S")
     log_filename = f"{APP_NAME}_{timestamp}.log"
     log_path = log_dir / log_filename
     config["handlers"]["file"]["filename"] = str(log_path)
 
+    built_handlers = []
+    
     # --- Handler and Listener Initialization ---
     built_handlers = []
+    
+    # Required keys to ignore when instantiating a handler (they are for dictConfig lookup)
+    HANDLER_INTERNAL_KEYS = ["class", "formatter", "level", "class_name"]
+    FORMATTER_INTERNAL_KEYS = ["class", "formatter", "level", "class_name", "validate"] # Added 'validate'
+
     for hname, hconf in config["handlers"].items():
         cls = _resolve_class(hconf["class"])
+        
         handler_args = {
-            k: v for k, v in hconf.items() if k not in ["class", "formatter", "level"]
+            k: v for k, v in hconf.items() if k not in HANDLER_INTERNAL_KEYS
         }
+        
         handler = cls(**handler_args)
 
         handler.setLevel(hconf.get("level", "NOTSET"))
-        fmt_conf = config["formatters"][hconf["formatter"]]
-        handler.setFormatter(logging.Formatter(fmt_conf["format"], fmt_conf.get("datefmt")))
+        
+        formatter_name = hconf.get("formatter")
+        if formatter_name and formatter_name in config["formatters"]:
+            fmt_conf = config["formatters"][formatter_name]
+            
+            # --- FIX APPLIED HERE ---
+            # Filter out keys that are Pydantic metadata/aliases (like 'class_name')
+            # We specifically filter out 'format' because the Formatter.__init__
+            # expects 'fmt' or uses 'style', not 'format' directly as a kwarg.
+            formatter_kwargs = {
+                k: v for k, v in fmt_conf.items() 
+                if k not in FORMATTER_INTERNAL_KEYS and k not in ['format']
+            }
+            
+            # Pass 'fmt' instead of 'format' to the standard Formatter constructor
+            formatter_kwargs['fmt'] = fmt_conf.get("format")
+            formatter_kwargs['datefmt'] = fmt_conf.get("datefmt")
+            formatter_kwargs['style'] = fmt_conf.get("style") # Should pass style if present
+            
+            # Remove keys that are None
+            formatter_kwargs = {k: v for k, v in formatter_kwargs.items() if v is not None}
+            
+            fmt_cls = _resolve_class(fmt_conf.get("class", "logging.Formatter"))
+            
+            handler.setFormatter(fmt_cls(**formatter_kwargs))
+        
         built_handlers.append(handler)
 
+    # --- Asynchronous Queue Setup ---
     log_queue = queue.Queue(-1)
     queue_handler = logging.handlers.QueueHandler(log_queue)
     listener = logging.handlers.QueueListener(
@@ -143,7 +173,6 @@ def deep_merge(target, source):
     atexit.register(listener.stop)
 
     # --- Logger Initialization ---
-    # Attach the QueueHandler to all loggers and set their levels.
     for lname, lconf in config["loggers"].items():
         logger = logging.getLogger(lname)
         logger.setLevel(lconf["level"])