diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 46fd449ae..8af471ead 100644 --- a/RELEASE_NOTES.md +++ b/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. diff --git a/src/frequenz/sdk/config/__init__.py b/src/frequenz/sdk/config/__init__.py index bf564fa26..461bc7e8a 100644 --- a/src/frequenz/sdk/config/__init__.py +++ b/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. diff --git a/src/frequenz/sdk/config/_base_schema.py b/src/frequenz/sdk/config/_base_schema.py index d25611958..98b355750 100644 --- a/src/frequenz/sdk/config/_base_schema.py +++ b/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 diff --git a/src/frequenz/sdk/config/_manager.py b/src/frequenz/sdk/config/_manager.py index 61d969915..7cc05d462 100644 --- a/src/frequenz/sdk/config/_manager.py +++ b/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 diff --git a/src/frequenz/sdk/config/_util.py b/src/frequenz/sdk/config/_util.py index 3f49f0c77..6d9bd0c96 100644 --- a/src/frequenz/sdk/config/_util.py +++ b/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" + ) diff --git a/tests/config/test_logging_actor.py b/tests/config/test_logging_actor.py index 71b020b41..c06aa07dc 100644 --- a/tests/config/test_logging_actor.py +++ b/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 diff --git a/tests/config/test_util.py b/tests/config/test_util.py index f6743a7b4..95fcba31a 100644 --- a/tests/config/test_util.py +++ b/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: