From bdccfa34e87d7890682bf7fbc8507063bf62d661 Mon Sep 17 00:00:00 2001 From: ubaskota <19787410+ubaskota@users.noreply.github.com> Date: Tue, 24 Feb 2026 00:11:32 -0500 Subject: [PATCH 1/2] Add a config property descriptor along with a custom resolver and validators --- .../config/custom_resolvers.py | 52 ++++ .../src/smithy_aws_core/config/validators.py | 144 +++++++++ .../tests/unit/config/test_custom_resolver.py | 109 +++++++ .../tests/unit/config/test_validators.py | 72 +++++ .../src/smithy_core/config/property.py | 99 +++++++ .../tests/unit/config/test_property.py | 277 ++++++++++++++++++ 6 files changed, 753 insertions(+) create mode 100644 packages/smithy-aws-core/src/smithy_aws_core/config/custom_resolvers.py create mode 100644 packages/smithy-aws-core/src/smithy_aws_core/config/validators.py create mode 100644 packages/smithy-aws-core/tests/unit/config/test_custom_resolver.py create mode 100644 packages/smithy-aws-core/tests/unit/config/test_validators.py create mode 100644 packages/smithy-core/src/smithy_core/config/property.py create mode 100644 packages/smithy-core/tests/unit/config/test_property.py diff --git a/packages/smithy-aws-core/src/smithy_aws_core/config/custom_resolvers.py b/packages/smithy-aws-core/src/smithy_aws_core/config/custom_resolvers.py new file mode 100644 index 00000000..13e702b1 --- /dev/null +++ b/packages/smithy-aws-core/src/smithy_aws_core/config/custom_resolvers.py @@ -0,0 +1,52 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +from typing import cast + +from smithy_core.config.resolver import ConfigResolver +from smithy_core.retries import RetryStrategyOptions, RetryStrategyType + +from smithy_aws_core.config.validators import validate_retry_mode + + +def resolve_retry_strategy( + resolver: ConfigResolver, +) -> tuple[RetryStrategyOptions | None, str | None]: + """Resolve retry strategy from multiple config keys. + + Resolves both retry_mode and max_attempts from sources and constructs + a RetryStrategyOptions object. This allows the retry strategy to be + configured from multiple sources. Example: retry_mode from config file and + max_attempts from environment variables. + + :param resolver: The config resolver to use for resolution + + :returns: Tuple of (RetryStrategyOptions or None, source_name or None). + Returns (None, None) if neither retry_mode nor max_attempts is set. + + For mixed sources, the source string includes both component sources: + "retry_mode=environment, max_attempts=config_file" + """ + # Get retry_mode + retry_mode, mode_source = resolver.get("retry_mode") + + # Get max_attempts + max_attempts, attempts_source = resolver.get("max_attempts") + + # If neither is set, return None + if retry_mode is None and max_attempts is None: + return (None, None) + + if retry_mode is not None: + retry_mode = validate_retry_mode(retry_mode, mode_source) + retry_mode = cast(RetryStrategyType, retry_mode) + + # Construct options with defaults + options = RetryStrategyOptions( + retry_mode=retry_mode or "standard", + max_attempts=int(max_attempts) if max_attempts else None, + ) + + # Construct mixed source string showing where each component came from + source = f"retry_mode={mode_source or 'unresolved'}, max_attempts={attempts_source or 'unresolved'}" + + return (options, source) diff --git a/packages/smithy-aws-core/src/smithy_aws_core/config/validators.py b/packages/smithy-aws-core/src/smithy_aws_core/config/validators.py new file mode 100644 index 00000000..8971543d --- /dev/null +++ b/packages/smithy-aws-core/src/smithy_aws_core/config/validators.py @@ -0,0 +1,144 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +import re +from collections.abc import Callable +from functools import wraps +from typing import Any, TypeVar + +from smithy_core.interfaces.retries import RetryStrategy +from smithy_core.retries import RetryStrategyOptions + +T = TypeVar("T") + + +def allow_none(validator: Callable[[Any, str | None], T]) -> Callable[..., T | None]: + """Decorator that allows None values to pass through validators. + + If the value is None, returns None without calling the validator. + Otherwise, calls the validator with the value. + + :param validator: The validation function to wrap + + :returns: Wrapped validator that allows None values + """ + + @wraps(validator) + def wrapper(value: Any, source: str | None = None) -> T | None: + if value is None: + return None + return validator(value, source) + + return wrapper + + +class ConfigValidationError(ValueError): + """Raised when a configuration value fails validation.""" + + def __init__(self, key: str, value: Any, reason: str, source: str | None = None): + self.key = key + self.value = value + self.reason = reason + self.source = source + + msg = f"Invalid value for '{key}': {value!r}. {reason}" + if source: + msg += f" (from source: {source})" + super().__init__(msg) + + +@allow_none +def validate_region(value: Any, source: str | None = None) -> str | None: + """Validate AWS region format. + + Valid formats: + - us-east-1, us-west-2, eu-west-1, etc. + - Pattern: {partition}-{region}-{number} + + :param value: The region value to validate + :param source: The config source that provided this value + + :returns: The validated region string, or None if value is None + + :raises ConfigValidationError: If the region format is invalid + """ + if not isinstance(value, str): + raise ConfigValidationError( + "region", + value, + f"Region must be a string, got {type(value).__name__}", + source, + ) + + # AWS region pattern: partition-region-number + pattern = r"^[a-z]{2}-[a-z]+-\d+$" + + if not re.match(pattern, value): + raise ConfigValidationError( + "region", + value, + "Region must match pattern '{partition}-{region}-{number}' (e.g., 'us-west-2', 'eu-central-1')", + source, + ) + return value + + +@allow_none +def validate_retry_mode(value: Any, source: str | None = None) -> str | None: + """Validate retry mode. + + Valid values: 'standard', 'simple' + + :param value: The retry mode value to validate + :param source: The source that provided this value + + :returns: The validated retry mode string, or None if value is None + + :raises: ConfigValidationError: If the retry mode is invalid + """ + if not isinstance(value, str): + raise ConfigValidationError( + "retry_mode", + value, + f"Retry mode must be a string, got {type(value).__name__}", + source, + ) + + valid_modes = {"standard", "simple"} + + if value not in valid_modes: + raise ConfigValidationError( + "retry_mode", + value, + f"Retry mode must be one of {valid_modes}, got {value!r}", + source, + ) + + return value + + +@allow_none +def validate_retry_strategy(value: Any, source: str | None = None) -> Any: + """Validate retry strategy configuration. + + :param value: The retry strategy value to validate (None is allowed and returns None) + :param source: The source that provided this value (for error messages) + + :returns: The validated retry strategy (RetryStrategy or RetryStrategyOptions) + + :raises: ConfigValidationError: If the value is not a valid retry strategy type + """ + # Allow RetryStrategy instances + if isinstance(value, RetryStrategy): + return value + + # Allow RetryStrategyOptions instances + if isinstance(value, RetryStrategyOptions): + return value + + raise ConfigValidationError( + "retry_strategy", + value, + f"Retry strategy must be a RetryStrategy or RetryStrategyOptions got {type(value).__name__}", + source, + ) diff --git a/packages/smithy-aws-core/tests/unit/config/test_custom_resolver.py b/packages/smithy-aws-core/tests/unit/config/test_custom_resolver.py new file mode 100644 index 00000000..639420cf --- /dev/null +++ b/packages/smithy-aws-core/tests/unit/config/test_custom_resolver.py @@ -0,0 +1,109 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for custom resolver functions.""" + +from typing import Any + +from smithy_aws_core.config.custom_resolvers import resolve_retry_strategy +from smithy_core.config.resolver import ConfigResolver +from smithy_core.retries import RetryStrategyOptions + + +class StubSource: + """A simple ConfigSource implementation for testing.""" + + def __init__(self, source_name: str, data: dict[str, Any] | None = None) -> None: + self._name = source_name + self._data = data or {} + + @property + def name(self) -> str: + return self._name + + def get(self, key: str) -> Any | None: + return self._data.get(key) + + +class TestResolveCustomResolverRetryStrategy: + """Test suite for complex configuration resolution""" + + def test_resolves_when_only_retry_mode_set(self) -> None: + source = StubSource("environment", {"retry_mode": "standard"}) + resolver = ConfigResolver(sources=[source]) + + result, source_name = resolve_retry_strategy(resolver) + + assert isinstance(result, RetryStrategyOptions) + assert result.retry_mode == "standard" + assert result.max_attempts is None + assert source_name == "retry_mode=environment, max_attempts=unresolved" + + def test_resolves_when_only_max_attempts_set(self) -> None: + source = StubSource("environment", {"max_attempts": "5"}) + resolver = ConfigResolver(sources=[source]) + + result, source_name = resolve_retry_strategy(resolver) + + assert isinstance(result, RetryStrategyOptions) + assert result.retry_mode == "standard" + assert result.max_attempts == 5 + assert source_name == "retry_mode=unresolved, max_attempts=environment" + + def test_resolves_from_both_values_when_set(self) -> None: + # When both retry mode and max attempts are set + # It should use source names for both values + source = StubSource( + "environment", {"retry_mode": "standard", "max_attempts": "3"} + ) + resolver = ConfigResolver(sources=[source]) + + result, source_name = resolve_retry_strategy(resolver) + + assert isinstance(result, RetryStrategyOptions) + assert result.retry_mode == "standard" + assert result.max_attempts == 3 + assert source_name == "retry_mode=environment, max_attempts=environment" + + def test_returns_none_when_neither_value_set(self) -> None: + source = StubSource("environment", {}) + resolver = ConfigResolver(sources=[source]) + + result, source_name = resolve_retry_strategy(resolver) + # It should return (None, None) when values not set + assert result is None + assert source_name is None + + def test_tracks_different_sources_for_each_component(self) -> None: + source1 = StubSource("environment", {"retry_mode": "standard"}) + source2 = StubSource("config_file", {"max_attempts": "5"}) + resolver = ConfigResolver(sources=[source1, source2]) + + result, source_name = resolve_retry_strategy(resolver) + + assert isinstance(result, RetryStrategyOptions) + assert result.retry_mode == "standard" + assert result.max_attempts == 5 + assert source_name == "retry_mode=environment, max_attempts=config_file" + + def test_tracks_source_when_only_max_attempts_set(self) -> None: + source1 = StubSource("environment", {}) + source2 = StubSource("config_file", {"max_attempts": "5"}) + resolver = ConfigResolver(sources=[source1, source2]) + + result, source_name = resolve_retry_strategy(resolver) + + assert isinstance(result, RetryStrategyOptions) + assert result.retry_mode == "standard" # Default + assert result.max_attempts == 5 + assert source_name == "retry_mode=unresolved, max_attempts=config_file" + + def test_converts_max_attempts_string_to_int(self) -> None: + source = StubSource("environment", {"max_attempts": "10"}) + resolver = ConfigResolver(sources=[source]) + + result, _ = resolve_retry_strategy(resolver) + + assert isinstance(result, RetryStrategyOptions) + assert result.max_attempts == 10 + assert isinstance(result.max_attempts, int) diff --git a/packages/smithy-aws-core/tests/unit/config/test_validators.py b/packages/smithy-aws-core/tests/unit/config/test_validators.py new file mode 100644 index 00000000..293647e4 --- /dev/null +++ b/packages/smithy-aws-core/tests/unit/config/test_validators.py @@ -0,0 +1,72 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Unit tests for AWS configuration validators.""" + +from typing import Any + +import pytest +from smithy_aws_core.config.validators import ( + ConfigValidationError, + allow_none, + validate_region, + validate_retry_mode, +) + + +class TestAllowNoneDecorator: + def test_allows_none_to_pass_through(self) -> None: + call_count = 0 + + @allow_none + def mock_validator(value: Any, source: str | None = None) -> Any: + nonlocal call_count + call_count += 1 + return value + + result: Any = mock_validator(None) + + assert result is None + assert call_count == 0 # Validator should not be called + + def test_calls_validator_for_non_none_values(self) -> None: + call_count = 0 + + @allow_none + def mock_validator(value: Any, source: str | None = None) -> Any: + nonlocal call_count + call_count += 1 + return value + + _ = mock_validator("test") + + assert call_count == 1 + + def test_preserves_validator_exceptions(self) -> None: + @allow_none + def failing_validator(value: Any, source: str | None = None) -> str: + raise ValueError("Validation failed") + + with pytest.raises(ValueError, match="Validation failed"): + failing_validator("test") + + +class TestValidators: + @pytest.mark.parametrize("region", ["us-east-1", "eu-west-1", "ap-south-1"]) + def test_validate_region_accepts_valid_values(self, region: str) -> None: + assert validate_region(region) == region + + @pytest.mark.parametrize("invalid", ["invalid-east-2", "us-east", "", "US-EAST-1"]) + def test_validate_region_rejects_invalid_values(self, invalid: str) -> None: + with pytest.raises(ConfigValidationError): + validate_region(invalid) + + @pytest.mark.parametrize("mode", ["standard", "simple"]) + def test_validate_retry_mode_accepts_valid_values(self, mode: str) -> None: + assert validate_retry_mode(mode) == mode + + @pytest.mark.parametrize("invalid_mode", ["some_retry", "some_retry_one", ""]) + def test_validate_retry_mode_rejects_invalid_values( + self, invalid_mode: str + ) -> None: + with pytest.raises(ConfigValidationError): + validate_retry_mode(invalid_mode) diff --git a/packages/smithy-core/src/smithy_core/config/property.py b/packages/smithy-core/src/smithy_core/config/property.py new file mode 100644 index 00000000..497d46c4 --- /dev/null +++ b/packages/smithy-core/src/smithy_core/config/property.py @@ -0,0 +1,99 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +from collections.abc import Callable +from typing import Any + +from smithy_core.config.resolver import ConfigResolver + + +class ConfigProperty: + """Descriptor for config properties with resolution, caching, and validation. + + This descriptor handles: + - Lazy resolution from sources (only on first access) + - Custom resolution for variables requiring complex resolution + - Caching of resolved values + - Source tracking for provenance + - Validation of values + + Example: + class Config: + region = ConfigProperty('region', validator=validate_region) + + def __init__(self): + self._resolver = ConfigResolver(sources=[...]) + """ + + def __init__( + self, + key: str, + validator: Callable[[Any, str | None], Any] | None = None, + resolver_func: Callable[[ConfigResolver], tuple[Any, str | None]] | None = None, + ): + """Initialize config property descriptor. + + :param key: The configuration key (e.g., 'region') + :param validator: Optional validation function that takes (value, source) + and returns validated value or raises an exception + :param resolver_func: Optional custom resolver function for complex resolution. + Takes a ConfigResolver and returns (value, source) tuple. + """ + self.key = key + self.validator = validator + self.resolver_func = resolver_func + # Cache attribute name in instance __dict__ (e.g., "_cache_region") + self.cache_attr = f"_cache_{key}" + + def __get__(self, obj: Any, objtype: type | None = None) -> Any: + """Get the config value with lazy resolution and caching. + + On first access, the property checks if the value is already cached. If not, it resolves + the value from sources using resolver via either a resolver function or obj._resolver. + When a validator is provided, the resolved value is validated before use. Finally, the + property caches the (value, source) tuple in obj.__dict__. On subsequent accesses, it + returns cached value from obj.__dict__. + + :param obj: The Config instance + :param objtype: The Config class + + :returns: The resolved and validated config value + """ + + cached = obj.__dict__.get(self.cache_attr) + if cached is not None: + return cached[ + 0 + ] # Return value from tuple (value, source) if already cached + + # If not cached, use a resolver to go through the sources to get (value, source) + # For complex config resolutions, use a custom resolver function to resolve values + if self.resolver_func: + value, source = self.resolver_func(obj._resolver) + else: + value, source = obj._resolver.get(self.key) + + if self.validator: + value = self.validator(value, source) + + obj.__dict__[self.cache_attr] = (value, source) + return value + + def __set__(self, obj: Any, value: Any) -> None: + """Set the config value (called during __init__ or after). + + When a config value is set, the property validates the new value if a validator is provided, then + updates the cached (value, source) tuple. The source is marked as 'instance' if the value + is set during __init__, or 'plugin' if set later. + + :param obj: The Config instance + :param value: The new value to set + """ + # Determine source based on when the value was set + # If cache already exists, it means it was not set during initialization + # In that case source will be set to plugin + source = "plugin" if self.cache_attr in obj.__dict__ else "instance" + + if self.validator: + value = self.validator(value, source) + + obj.__dict__[self.cache_attr] = (value, source) diff --git a/packages/smithy-core/tests/unit/config/test_property.py b/packages/smithy-core/tests/unit/config/test_property.py new file mode 100644 index 00000000..d268c89b --- /dev/null +++ b/packages/smithy-core/tests/unit/config/test_property.py @@ -0,0 +1,277 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from collections.abc import Callable +from typing import Any, NoReturn + +import pytest +from smithy_core.config.property import ConfigProperty +from smithy_core.config.resolver import ConfigResolver + + +class StubSource: + """A simple ConfigSource implementation for testing.""" + + def __init__(self, source_name: str, data: dict[str, Any] | None = None) -> None: + self._name = source_name + self._data = data or {} + + @property + def name(self) -> str: + return self._name + + def get(self, key: str) -> Any | None: + return self._data.get(key) + + +class StubConfig: + """A minimal Config class for testing ConfigProperty descriptor.""" + + region = ConfigProperty("region") + retry_mode = ConfigProperty("retry_mode") + + def __init__(self, resolver: ConfigResolver) -> None: + self._resolver = resolver + + +class TestConfigPropertyDescriptor: + def test_resolves_value_from_resolver_on_first_access(self) -> None: + source = StubSource("environment", {"region": "us-west-2"}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + result = config.region + + assert result == "us-west-2" + + def test_caches_resolved_value(self) -> None: + source = StubSource("environment", {"region": "us-west-2"}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + # First access + result1 = config.region + # Second access + result2 = config.region + + assert result1 == result2 == "us-west-2" + # Verify it's cached in __dict__ of config object + assert "_cache_region" in config.__dict__ + + def test_returns_none_when_value_unresolved(self) -> None: + source = StubSource("environment", {}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + result = config.region + + assert result is None + + def test_caches_none_for_unresolved_values(self) -> None: + source = StubSource("environment", {}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + # First access + result1 = config.region + # Second access + result2 = config.region + + assert result1 is None + assert result2 is None + # Verify it's cached + assert "_cache_region" in config.__dict__ + assert config.__dict__["_cache_region"] == (None, None) + + def test_different_properties_resolve_independently(self) -> None: + source = StubSource( + "environment", {"region": "us-west-2", "retry_mode": "adaptive"} + ) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + region = config.region + retry_mode = config.retry_mode + + assert region == "us-west-2" + assert retry_mode == "adaptive" + + +class TestConfigPropertyValidation: + """Test suite for ConfigProperty validation behavior.""" + + def _create_config_with_validator( + self, validator: Callable[[Any, str | None], Any] + ) -> type[Any]: + """Helper to create a config class with a specific validator.""" + + class ConfigWithValidator: + region = ConfigProperty("region", validator=validator) + + def __init__(self, resolver: ConfigResolver) -> None: + self._resolver = resolver + + return ConfigWithValidator + + def test_calls_validator_on_resolution(self) -> None: + call_log: list[tuple[Any, str | None]] = [] + + def mock_validator(value: Any, source: str | None) -> Any: + call_log.append((value, source)) + return value + + ConfigWithValidator = self._create_config_with_validator(mock_validator) + source = StubSource("environment", {"region": "us-west-2"}) + resolver = ConfigResolver(sources=[source]) + config = ConfigWithValidator(resolver) + + result = config.region + + assert result == "us-west-2" + assert len(call_log) == 1 + assert call_log[0] == ("us-west-2", "environment") + + def test_validator_exception_propagates(self) -> None: + def failing_validator(value: Any, source: str | None) -> NoReturn: + raise ValueError("Invalid value") + + ConfigWithValidator = self._create_config_with_validator(failing_validator) + source = StubSource("environment", {"region": "invalid-region-123"}) + resolver = ConfigResolver(sources=[source]) + config = ConfigWithValidator(resolver) + + with pytest.raises(ValueError, match="Invalid value"): + _ = config.region + + def test_validator_not_called_on_cached_access(self) -> None: + call_count = 0 + + def counting_validator(value: Any, source: str | None) -> Any: + nonlocal call_count + call_count += 1 + return value + + ConfigWithValidator = self._create_config_with_validator(counting_validator) + source = StubSource("environment", {"region": "us-west-2"}) + resolver = ConfigResolver(sources=[source]) + config = ConfigWithValidator(resolver) + + # Multiple accesses + _ = config.region + _ = config.region + _ = config.region + + # Only the first call accessed the validator + assert call_count == 1 # Validator called only once + + +class TestConfigPropertySetter: + """Test suite for ConfigProperty setter behavior.""" + + def test_set_value_marks_source_as_instance(self) -> None: + source = StubSource("environment", {}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + config.region = "eu-west-1" + + # Check the cached tuple + assert config.__dict__["_cache_region"] == ("eu-west-1", "instance") + + def test_value_set_after_resolution_marks_source_as_plugin(self) -> None: + source = StubSource("environment", {"region": "us-west-2"}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + # First access triggers resolution from environment source + _ = config.region + + # Modify after resolution + config.region = "eu-west-1" + + # Verify the new value is returned + assert config.region == "eu-west-1" + # Verify source is marked as 'plugin' + # Any config value modified after initialization will have 'plugin' for source + assert config.__dict__["_cache_region"] == ("eu-west-1", "plugin") + + def test_validator_is_called_when_setting_values(self) -> None: + call_log: list[tuple[Any, str | None]] = [] + + def mock_validator(value: Any, source: str | None) -> Any: + call_log.append((value, source)) + return value + + class ConfigWithValidator: + region = ConfigProperty("region", validator=mock_validator) + + def __init__(self, resolver: ConfigResolver) -> None: + self._resolver = resolver + + source = StubSource("environment", {}) + resolver = ConfigResolver(sources=[source]) + config = ConfigWithValidator(resolver) + + config.region = "us-west-2" + + assert config.region == "us-west-2" + assert len(call_log) == 1 + assert call_log[0] == ("us-west-2", "instance") + + def test_validator_throws_exception_when_setting_invalid_value(self) -> None: + def mock_failing_validation(value: Any, source: str | None) -> NoReturn: + raise ValueError("Invalid value") + + class ConfigWithValidator: + region = ConfigProperty("region", validator=mock_failing_validation) + + def __init__(self, resolver: ConfigResolver) -> None: + self._resolver = resolver + + source = StubSource("environment", {}) + resolver = ConfigResolver(sources=[source]) + config = ConfigWithValidator(resolver) + + with pytest.raises(ValueError, match="Invalid value"): + config.region = "some-invalid-2" + + def test_set_overrides_resolved_value(self) -> None: + source = StubSource("environment", {"region": "us-west-2"}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + # First access resolves from environment + assert config.region == "us-west-2" + + # Setting overrides + config.region = "eu-west-1" + + assert config.region == "eu-west-1" + + +class TestConfigPropertyCaching: + """Test suite for ConfigProperty caching implementation details.""" + + def test_cache_stores_value_and_source_as_tuple(self) -> None: + source = StubSource("environment", {"region": "us-west-2"}) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + _ = config.region + + cached: Any = config.__dict__["_cache_region"] + assert cached == ("us-west-2", "environment") + + def test_cache_key_is_unique_per_property(self) -> None: + source = StubSource( + "environment", {"region": "us-west-2", "retry_mode": "adaptive"} + ) + resolver = ConfigResolver(sources=[source]) + config = StubConfig(resolver) + + _ = config.region + _ = config.retry_mode + + assert "_cache_region" in config.__dict__ + assert "_cache_retry_mode" in config.__dict__ + assert config.__dict__["_cache_region"] != config.__dict__["_cache_retry_mode"] From 4eb1b762df6c267c5eaa252ae06407a48ece8479 Mon Sep 17 00:00:00 2001 From: ubaskota <19787410+ubaskota@users.noreply.github.com> Date: Tue, 24 Feb 2026 12:48:34 -0500 Subject: [PATCH 2/2] Remove a decorator that handles None value and update the tests --- .../src/smithy_aws_core/config/validators.py | 72 ++++++------------- .../tests/unit/config/test_validators.py | 42 +---------- .../src/smithy_core/config/property.py | 2 +- 3 files changed, 24 insertions(+), 92 deletions(-) diff --git a/packages/smithy-aws-core/src/smithy_aws_core/config/validators.py b/packages/smithy-aws-core/src/smithy_aws_core/config/validators.py index 8971543d..2150a86c 100644 --- a/packages/smithy-aws-core/src/smithy_aws_core/config/validators.py +++ b/packages/smithy-aws-core/src/smithy_aws_core/config/validators.py @@ -2,34 +2,10 @@ # SPDX-License-Identifier: Apache-2.0 import re -from collections.abc import Callable -from functools import wraps -from typing import Any, TypeVar +from typing import Any, get_args from smithy_core.interfaces.retries import RetryStrategy -from smithy_core.retries import RetryStrategyOptions - -T = TypeVar("T") - - -def allow_none(validator: Callable[[Any, str | None], T]) -> Callable[..., T | None]: - """Decorator that allows None values to pass through validators. - - If the value is None, returns None without calling the validator. - Otherwise, calls the validator with the value. - - :param validator: The validation function to wrap - - :returns: Wrapped validator that allows None values - """ - - @wraps(validator) - def wrapper(value: Any, source: str | None = None) -> T | None: - if value is None: - return None - return validator(value, source) - - return wrapper +from smithy_core.retries import RetryStrategyOptions, RetryStrategyType class ConfigValidationError(ValueError): @@ -47,77 +23,73 @@ def __init__(self, key: str, value: Any, reason: str, source: str | None = None) super().__init__(msg) -@allow_none -def validate_region(value: Any, source: str | None = None) -> str | None: +def validate_region(region_name: Any, source: str | None = None) -> str | None: """Validate AWS region format. Valid formats: - us-east-1, us-west-2, eu-west-1, etc. - Pattern: {partition}-{region}-{number} - :param value: The region value to validate + :param region_name: The region value to validate :param source: The config source that provided this value :returns: The validated region string, or None if value is None :raises ConfigValidationError: If the region format is invalid """ - if not isinstance(value, str): + if not isinstance(region_name, str): raise ConfigValidationError( "region", - value, - f"Region must be a string, got {type(value).__name__}", + region_name, + f"Region must be a string, got {type(region_name).__name__}", source, ) - # AWS region pattern: partition-region-number - pattern = r"^[a-z]{2}-[a-z]+-\d+$" + pattern = r"^(?![0-9]+$)(?!-)[a-zA-Z0-9-]{,63}(? str | None: +def validate_retry_mode(retry_mode: Any, source: str | None = None) -> str | None: """Validate retry mode. Valid values: 'standard', 'simple' - :param value: The retry mode value to validate + :param retry_mode: The retry mode value to validate :param source: The source that provided this value :returns: The validated retry mode string, or None if value is None :raises: ConfigValidationError: If the retry mode is invalid """ - if not isinstance(value, str): + if not isinstance(retry_mode, str): raise ConfigValidationError( "retry_mode", - value, - f"Retry mode must be a string, got {type(value).__name__}", + retry_mode, + f"Retry mode must be a string, got {type(retry_mode).__name__}", source, ) - valid_modes = {"standard", "simple"} + valid_modes = set(get_args(RetryStrategyType)) - if value not in valid_modes: + if retry_mode not in valid_modes: raise ConfigValidationError( "retry_mode", - value, - f"Retry mode must be one of {valid_modes}, got {value!r}", + retry_mode, + f"Retry mode must be one of {RetryStrategyType}, got {retry_mode}", source, ) - return value + return retry_mode -@allow_none def validate_retry_strategy(value: Any, source: str | None = None) -> Any: """Validate retry strategy configuration. diff --git a/packages/smithy-aws-core/tests/unit/config/test_validators.py b/packages/smithy-aws-core/tests/unit/config/test_validators.py index 293647e4..18b6d7fa 100644 --- a/packages/smithy-aws-core/tests/unit/config/test_validators.py +++ b/packages/smithy-aws-core/tests/unit/config/test_validators.py @@ -2,60 +2,20 @@ # SPDX-License-Identifier: Apache-2.0 """Unit tests for AWS configuration validators.""" -from typing import Any - import pytest from smithy_aws_core.config.validators import ( ConfigValidationError, - allow_none, validate_region, validate_retry_mode, ) -class TestAllowNoneDecorator: - def test_allows_none_to_pass_through(self) -> None: - call_count = 0 - - @allow_none - def mock_validator(value: Any, source: str | None = None) -> Any: - nonlocal call_count - call_count += 1 - return value - - result: Any = mock_validator(None) - - assert result is None - assert call_count == 0 # Validator should not be called - - def test_calls_validator_for_non_none_values(self) -> None: - call_count = 0 - - @allow_none - def mock_validator(value: Any, source: str | None = None) -> Any: - nonlocal call_count - call_count += 1 - return value - - _ = mock_validator("test") - - assert call_count == 1 - - def test_preserves_validator_exceptions(self) -> None: - @allow_none - def failing_validator(value: Any, source: str | None = None) -> str: - raise ValueError("Validation failed") - - with pytest.raises(ValueError, match="Validation failed"): - failing_validator("test") - - class TestValidators: @pytest.mark.parametrize("region", ["us-east-1", "eu-west-1", "ap-south-1"]) def test_validate_region_accepts_valid_values(self, region: str) -> None: assert validate_region(region) == region - @pytest.mark.parametrize("invalid", ["invalid-east-2", "us-east", "", "US-EAST-1"]) + @pytest.mark.parametrize("invalid", ["-invalid", "-east", "12345", 1234]) def test_validate_region_rejects_invalid_values(self, invalid: str) -> None: with pytest.raises(ConfigValidationError): validate_region(invalid) diff --git a/packages/smithy-core/src/smithy_core/config/property.py b/packages/smithy-core/src/smithy_core/config/property.py index 497d46c4..bb5bd57f 100644 --- a/packages/smithy-core/src/smithy_core/config/property.py +++ b/packages/smithy-core/src/smithy_core/config/property.py @@ -72,7 +72,7 @@ def __get__(self, obj: Any, objtype: type | None = None) -> Any: else: value, source = obj._resolver.get(self.key) - if self.validator: + if self.validator and value is not None: value = self.validator(value, source) obj.__dict__[self.cache_attr] = (value, source)