Skip to content

Commit 64a352b

Browse files
committed
Reject configuration fields with no matching setting
`Settings.load()` copied every non-None schema field on by name. A name that matched a property ran its setter and the validation that setter performs; a name that did not became a plain instance attribute, so the value was accepted and every validator skipped without any signal. `load()` now checks each field name against the class before assigning it and raises `UnknownSettingError` when no property answers to that name. The check runs whether or not the field carries a value, so a mismatch surfaces even when the option is unset. `recipes` is skipped by name, since `_generate_recipe_list` reads it off the schema and there is no property to receive it. Adding another such field would both raise here and fail the agreement test in tests/unit/test___main__.py, which is where the decision belongs. `Settings().recipes` no longer appears after a load. It was an accident of the old `setattr`, and nothing read it. Fixes #229
1 parent 7726f16 commit 64a352b

3 files changed

Lines changed: 120 additions & 3 deletions

File tree

src/cloud_autopkg_runner/exceptions.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,25 @@ def __init__(self, field_name: str, validation_error: str) -> None:
318318
super().__init__(f"Invalid value for '{field_name}': {validation_error}")
319319

320320

321+
class UnknownSettingError(AutoPkgRunnerError):
322+
"""Error class for handling configuration fields with no matching setting.
323+
324+
This error indicates that a configuration field was applied to `Settings`
325+
without a property of the same name to receive it.
326+
"""
327+
328+
def __init__(self, field_name: str) -> None:
329+
"""Initializes UnknownSettingError with the unmatched field name.
330+
331+
Args:
332+
field_name: The name of the unmatched configuration field.
333+
"""
334+
super().__init__(
335+
f"No Settings property named '{field_name}'. Configuration fields "
336+
"must match a Settings property."
337+
)
338+
339+
321340
# Shell Command
322341
class ShellCommandError(AutoPkgRunnerError):
323342
"""Base error class for handling issues with shell commands.

src/cloud_autopkg_runner/settings.py

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020

2121
# Keep these specific to avoid circular imports
2222
from cloud_autopkg_runner.config_schema import ConfigSchema
23-
from cloud_autopkg_runner.exceptions import SettingsValidationError
23+
from cloud_autopkg_runner.exceptions import SettingsValidationError, UnknownSettingError
2424

2525

2626
class Settings:
@@ -90,8 +90,20 @@ def load(self, schema: ConfigSchema) -> None:
9090
9191
Args:
9292
schema: A validated configuration schema.
93+
94+
Raises:
95+
UnknownSettingError: If a schema field has no property of the
96+
same name on this class.
9397
"""
9498
for field in dataclasses.fields(schema):
99+
# `recipes` selects what the runner iterates rather than how a
100+
# run behaves. `_generate_recipe_list` reads it off the schema,
101+
# so there is no property here to receive it.
102+
if field.name == "recipes":
103+
continue
104+
105+
self._validate_setting_exists(field.name)
106+
95107
schema_value = getattr(schema, field.name)
96108
if schema_value is not None:
97109
setattr(self, field.name, schema_value)
@@ -413,6 +425,24 @@ def _validate_integer_is_not_negative(field_name: str, value: int) -> None:
413425
if value < 0:
414426
raise SettingsValidationError(field_name, "Must not be negative")
415427

428+
@classmethod
429+
def _validate_setting_exists(cls, field_name: str) -> None:
430+
"""Validates that a name corresponds to a setting on this class.
431+
432+
Every setting is a property, so assigning to it runs that property's
433+
setter and the validation it performs. A name that is not a property
434+
would become a plain instance attribute instead, bypassing that
435+
validation.
436+
437+
Args:
438+
field_name: The name of the setting to look for.
439+
440+
Raises:
441+
UnknownSettingError: If this class defines no property by that name.
442+
"""
443+
if not isinstance(getattr(cls, field_name, None), property):
444+
raise UnknownSettingError(field_name)
445+
416446
# Plugin Properties
417447

418448
@property

tests/unit/test_settings.py

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
"""Tests for the settings module."""
22

3+
from dataclasses import make_dataclass
34
from pathlib import Path
5+
from typing import cast
46

57
import pytest
68

7-
from cloud_autopkg_runner import Settings
8-
from cloud_autopkg_runner.exceptions import SettingsValidationError
9+
from cloud_autopkg_runner import ConfigSchema, Settings
10+
from cloud_autopkg_runner.exceptions import SettingsValidationError, UnknownSettingError
911

1012

1113
def test_singleton_pattern() -> None:
@@ -271,3 +273,69 @@ def test_input_variables_setter(
271273
settings = Settings()
272274
settings.input_variables = input_value
273275
assert settings.input_variables == expected_output
276+
277+
278+
def test_load_applies_schema_values() -> None:
279+
"""Schema values reach the properties, running their setters."""
280+
settings = Settings()
281+
settings.load(
282+
ConfigSchema(
283+
log_format="json",
284+
max_concurrency=4,
285+
report_dir=Path("custom_reports"),
286+
pre_processors="com.example/OnlyOne",
287+
cache_plugin="s3",
288+
)
289+
)
290+
291+
assert settings.log_format == "json"
292+
assert settings.max_concurrency == 4
293+
assert settings.report_dir == Path("custom_reports")
294+
assert settings.cache_plugin == "s3"
295+
296+
# The setter wraps a lone processor in a list.
297+
assert settings.pre_processors == ["com.example/OnlyOne"]
298+
299+
300+
def test_load_leaves_defaults_for_unset_values() -> None:
301+
"""An empty schema changes nothing, since every field is None."""
302+
settings = Settings()
303+
settings.load(ConfigSchema())
304+
305+
assert settings.log_format == "text"
306+
assert settings.max_concurrency == 10
307+
assert settings.recipe_timeout == 300
308+
assert settings.report_dir == Path("recipe_reports")
309+
assert settings.verbosity_level == 0
310+
311+
312+
def test_load_skips_config_file_only_fields() -> None:
313+
"""`recipes` is read from the schema and never lands on Settings."""
314+
settings = Settings()
315+
settings.load(ConfigSchema(recipes=["Foo.recipe"]))
316+
317+
assert not hasattr(settings, "recipes")
318+
319+
320+
def test_load_raises_for_field_without_a_property() -> None:
321+
"""A schema field with no matching property is an error, not an attribute."""
322+
schema = make_dataclass("FakeSchema", [("nonexistent_setting", "str | None")])(
323+
nonexistent_setting="value"
324+
)
325+
settings = Settings()
326+
327+
with pytest.raises(UnknownSettingError, match="nonexistent_setting"):
328+
settings.load(cast("ConfigSchema", schema))
329+
330+
assert not hasattr(settings, "nonexistent_setting")
331+
332+
333+
def test_load_raises_even_when_the_value_is_unset() -> None:
334+
"""The name is checked whether or not the field carries a value."""
335+
schema = make_dataclass("FakeSchema", [("nonexistent_setting", "str | None")])(
336+
nonexistent_setting=None
337+
)
338+
settings = Settings()
339+
340+
with pytest.raises(UnknownSettingError, match="nonexistent_setting"):
341+
settings.load(cast("ConfigSchema", schema))

0 commit comments

Comments
 (0)