Move default unknown=EXCLUDE to BaseConfigSchema (#1149)

This simplifies the code as we don't need to manipulate the `load()`
arguments in the wrapper functions. It also uses `BaseConfigSchema` as
the default base schema in `load_config()`.

Author: llucax

RELEASE_NOTES.md

@@ -30,8 +30,9 @@ This release includes a new `ConfigManager` class to simplify managing the confi
 
     * `load_config()`:
 
-         + The `base_schema` argument is now keyword-only.
+         + The `base_schema` argument is now keyword-only and defaults to `BaseConfigSchema` (and because of this, it uses `unknown=EXCLUDE` by default).
          + The arguments forwarded to `marshmallow.Schema.load()` now must be passed explicitly via the `marshmallow_load_kwargs` argument, as a `dict`, to improve the type-checking.
+         + Will now raise a `ValueError` if `unknown` is set to `INCLUDE` in `marshmallow_load_kwargs`.
 
     * `ConfigManagingActor`: Raise a `ValueError` if the `config_files` argument an empty sequence.
 

src/frequenz/sdk/config/__init__.py

@@ -93,7 +93,8 @@ class AppConfig:
 
 Customization can also be done via a `base_schema`. By default
 [`BaseConfigSchema`][frequenz.sdk.config.BaseConfigSchema] is used to provide support
-for some extra commonly used fields (like [quantities][frequenz.quantities]).
+for some extra commonly used fields (like [quantities][frequenz.quantities]) and to
+exclude unknown fields by default.
 
 ```python
 import marshmallow.validate
@@ -109,11 +110,7 @@ class Config:
 Additional arguments can be passed to [`marshmallow.Schema.load`][] using
 the `marshmallow_load_kwargs` keyword arguments.
 
-If unspecified, the `marshmallow_load_kwargs` will have the `unknown` key set to
-[`marshmallow.EXCLUDE`][] (instead of the normal [`marshmallow.RAISE`][]
-default).
-
-But when [`marshmallow.EXCLUDE`][] is used, a warning will be logged if there are extra
+When [`marshmallow.EXCLUDE`][] is used, a warning will be logged if there are extra
 fields in the configuration that are excluded. This is useful, for example, to catch
 typos in the configuration file.
 

src/frequenz/sdk/config/_base_schema.py

@@ -3,8 +3,18 @@
 
 """Base schema for configuration classes."""
 
+import marshmallow
 from frequenz.quantities.experimental.marshmallow import QuantitySchema
 
 
 class BaseConfigSchema(QuantitySchema):
-    """A base schema for configuration classes."""
+    """A base schema for configuration classes.
+
+    This schema provides validation for quantities and ignores unknown fields by
+    default.
+    """
+
+    class Meta:
+        """Meta options for the schema."""
+
+        unknown = marshmallow.EXCLUDE

src/frequenz/sdk/config/_manager.py

@@ -20,7 +20,7 @@
 from ..actor._background_service import BackgroundService
 from ._base_schema import BaseConfigSchema
 from ._managing_actor import ConfigManagingActor
-from ._util import DataclassT, load_config
+from ._util import DataclassT, _validate_load_kwargs, load_config
 
 _logger = logging.getLogger(__name__)
 
@@ -224,21 +224,8 @@ def new_receiver(  # pylint: disable=too-many-arguments
 
         Returns:
             The receiver for the configuration.
-
-        Raises:
-            ValueError: If the `unknown` option in `marshmallow_load_kwargs` is set to
-                [`marshmallow.INCLUDE`][].
         """
-        marshmallow_load_kwargs = (
-            {} if marshmallow_load_kwargs is None else marshmallow_load_kwargs.copy()
-        )
-
-        if "unknown" not in marshmallow_load_kwargs:
-            marshmallow_load_kwargs["unknown"] = marshmallow.EXCLUDE
-        elif marshmallow_load_kwargs["unknown"] == marshmallow.INCLUDE:
-            raise ValueError(
-                "The 'unknown' option can't be 'INCLUDE' when loading to a dataclass"
-            )
+        _validate_load_kwargs(marshmallow_load_kwargs)
 
         receiver = self.config_channel.new_receiver(name=f"{self}:{key}", limit=1).map(
             lambda config: _load_config_with_logging_and_errors(
@@ -493,23 +480,22 @@ def _load_config(
         {} if marshmallow_load_kwargs is None else marshmallow_load_kwargs.copy()
     )
 
-    unknown = marshmallow_load_kwargs.get("unknown")
-    if unknown == marshmallow.EXCLUDE:
-        # When excluding unknown fields we still want to notify the user, as
-        # this could mean there is a typo in the configuration and some value is
-        # not being loaded as desired.
-        marshmallow_load_kwargs["unknown"] = marshmallow.RAISE
-        try:
-            load_config(
-                config_class,
-                config,
-                base_schema=base_schema,
-                marshmallow_load_kwargs=marshmallow_load_kwargs,
-            )
-        except ValidationError as err:
-            _logger.warning(
-                "The configuration for key %r has extra fields that will be ignored: %s",
-                key,
-                err,
-            )
+    # When excluding unknown fields we still want to notify the user, as
+    # this could mean there is a typo in the configuration and some value is
+    # not being loaded as desired.
+    marshmallow_load_kwargs["unknown"] = marshmallow.RAISE
+    try:
+        load_config(
+            config_class,
+            config,
+            base_schema=base_schema,
+            marshmallow_load_kwargs=marshmallow_load_kwargs,
+        )
+    except ValidationError as err:
+        _logger.warning(
+            "The configuration for key %r has extra fields that will be ignored: %s",
+            key,
+            err,
+        )
+
     return loaded_config

src/frequenz/sdk/config/_util.py

@@ -6,9 +6,12 @@
 from collections.abc import Mapping
 from typing import Any, ClassVar, Protocol, TypeVar, cast
 
+import marshmallow
 from marshmallow import Schema
 from marshmallow_dataclass import class_schema
 
+from ._base_schema import BaseConfigSchema
+
 
 # This is a hack that relies on identifying dataclasses by looking into an undocumented
 # property of dataclasses[1], so it might break in the future. Nevertheless, it seems to
@@ -33,7 +36,7 @@ def load_config(
     config: Mapping[str, Any],
     /,
     *,
-    base_schema: type[Schema] | None = None,
+    base_schema: type[Schema] | None = BaseConfigSchema,
     marshmallow_load_kwargs: dict[str, Any] | None = None,
 ) -> DataclassT:
     """Load a configuration from a dictionary into an instance of a configuration class.
@@ -69,9 +72,32 @@ def load_config(
     Returns:
         The loaded configuration as an instance of the configuration class.
     """
+    _validate_load_kwargs(marshmallow_load_kwargs)
+
     instance = class_schema(cls, base_schema)().load(
         config, **(marshmallow_load_kwargs or {})
     )
     # We need to cast because `.load()` comes from marshmallow and doesn't know which
     # type is returned.
     return cast(DataclassT, instance)
+
+
+def _validate_load_kwargs(marshmallow_load_kwargs: dict[str, Any] | None) -> None:
+    """Validate the marshmallow load kwargs.
+
+    This function validates the `unknown` option of the marshmallow load kwargs to
+    prevent loading unknown fields when loading to a dataclass.
+
+    Args:
+        marshmallow_load_kwargs: The dictionary to get the marshmallow load kwargs from.
+
+    Raises:
+        ValueError: If the `unknown` option is set to [`marshmallow.INCLUDE`][].
+    """
+    if (
+        marshmallow_load_kwargs
+        and marshmallow_load_kwargs.get("unknown") == marshmallow.INCLUDE
+    ):
+        raise ValueError(
+            "The 'unknown' option can't be 'INCLUDE' when loading to a dataclass"
+        )

tests/config/test_logging_actor.py

@@ -48,8 +48,7 @@ def test_logging_config() -> None:
         load_config(LoggingConfig, config_raw)
 
     config_raw = {"unknown": {"frequenz.sdk.actor": {"level": "DEBUG"}}}
-    with pytest.raises(ValidationError):
-        load_config(LoggingConfig, config_raw)
+    assert load_config(LoggingConfig, config_raw) == config
 
 
 @pytest.fixture

tests/config/test_util.py

@@ -79,15 +79,15 @@ class _MyBaseSchema(marshmallow.Schema):
         class Meta:
             """Meta options for the schema."""
 
-            unknown = marshmallow.EXCLUDE
+            unknown = marshmallow.RAISE
 
     config: dict[str, Any] = {"name": "test", "value": 42, "extra": "extra"}
 
-    loaded_config = load_config(config_class, config, base_schema=_MyBaseSchema)
+    loaded_config = load_config(config_class, config)
     assert loaded_config == config_class(name="test", value=42)
 
     with pytest.raises(marshmallow.ValidationError):
-        _ = load_config(config_class, config)
+        _ = load_config(config_class, config, base_schema=_MyBaseSchema)
 
 
 def test_load_config_type_hints(mocker: MockerFixture) -> None: