Address reviews

Author: ubaskota

codegen/aws/core/src/main/java/software/amazon/smithy/python/aws/codegen/AwsConfiguration.java

@@ -60,30 +60,4 @@ private AwsConfiguration() {}
                     .build())
             .defaultValue("RetryStrategyOptions(retry_mode=\"standard\")")
             .build();
-
-    public static final ConfigProperty USER_AGENT_EXTRA = ConfigProperty.builder()
-            .name("user_agent_extra")
-            .type(Symbol.builder().name("str").build())
-            .documentation("Additional suffix to be appended to the User-Agent header.")
-            .nullable(false)
-            .useDescriptor(true)
-            .defaultValue("''")
-            .build();
-
-    public static final ConfigProperty SDK_UA_APP_ID = ConfigProperty.builder()
-            .name("sdk_ua_app_id")
-            .type(Symbol.builder().name("str").build())
-            .documentation("A unique and opaque application ID that is appended to the User-Agent header.")
-            .nullable(false)
-            .useDescriptor(true)
-            .defaultValue("''")
-            .build();
-
-    public static final ConfigProperty ENDPOINT_URL = ConfigProperty.builder()
-            .name("endpoint_url")
-            .type(Symbol.builder().name("str").build())
-            .documentation("The endpoint URL to use for requests. If not set, the standard endpoint for the service and region will be used.")
-            .nullable(true)
-            .useDescriptor(true)
-            .build();
 }

codegen/aws/core/src/main/java/software/amazon/smithy/python/aws/codegen/AwsStandardRegionalEndpointsIntegration.java

@@ -4,7 +4,6 @@
  */
 package software.amazon.smithy.python.aws.codegen;
 
-import static software.amazon.smithy.python.aws.codegen.AwsConfiguration.ENDPOINT_URL;
 import static software.amazon.smithy.python.aws.codegen.AwsConfiguration.REGION;
 
 import java.util.List;
@@ -24,7 +23,6 @@ public List<RuntimeClientPlugin> getClientPlugins(GenerationContext context) {
             return List.of(
                     RuntimeClientPlugin.builder()
                             .addConfigProperty(REGION)
-                            .addConfigProperty(ENDPOINT_URL)
                             .build());
         } else {
             return List.of();

codegen/aws/core/src/main/java/software/amazon/smithy/python/aws/codegen/AwsUserAgentIntegration.java

@@ -9,14 +9,12 @@
 import software.amazon.smithy.codegen.core.Symbol;
 import software.amazon.smithy.codegen.core.SymbolReference;
 import software.amazon.smithy.python.codegen.CodegenUtils;
+import software.amazon.smithy.python.codegen.ConfigProperty;
 import software.amazon.smithy.python.codegen.GenerationContext;
 import software.amazon.smithy.python.codegen.integrations.PythonIntegration;
 import software.amazon.smithy.python.codegen.integrations.RuntimeClientPlugin;
 import software.amazon.smithy.utils.SmithyInternalApi;
 
-import static software.amazon.smithy.python.aws.codegen.AwsConfiguration.SDK_UA_APP_ID;
-import static software.amazon.smithy.python.aws.codegen.AwsConfiguration.USER_AGENT_EXTRA;
-
 /**
  * Adds a runtime plugin to set user agent.
  */
@@ -38,6 +36,27 @@ def aws_user_agent_plugin(config: $1T):
     @Override
     public List<RuntimeClientPlugin> getClientPlugins(GenerationContext context) {
         if (context.applicationProtocol().isHttpProtocol()) {
+            final ConfigProperty userAgentExtra = ConfigProperty.builder()
+                    .name("user_agent_extra")
+                    .documentation("Additional suffix to be added to the User-Agent header.")
+                    .type(Symbol.builder().name("str").build())
+                    .nullable(true)
+                    .build();
+
+            final ConfigProperty uaAppId = ConfigProperty.builder()
+                    .name("sdk_ua_app_id")
+                    .documentation(
+                            "A unique and opaque application ID that is appended to the User-Agent header.")
+                    .type(Symbol.builder().name("str").build())
+                    .nullable(true)
+                    .useDescriptor(true)
+                    .validator(Symbol.builder()
+                            .name("validate_and_sanitize_ua_string")
+                            .namespace("smithy_aws_core.config.validators", ".")
+                            .addDependency(AwsPythonDependency.SMITHY_AWS_CORE)
+                            .build())
+                    .build();
+
             final String user_agent_plugin_file = "user_agent";
 
             final String moduleName = context.settings().moduleName();
@@ -73,8 +92,8 @@ public List<RuntimeClientPlugin> getClientPlugins(GenerationContext context) {
 
             return List.of(
                     RuntimeClientPlugin.builder()
-                            .addConfigProperty(USER_AGENT_EXTRA)
-                            .addConfigProperty(SDK_UA_APP_ID)
+                            .addConfigProperty(userAgentExtra)
+                            .addConfigProperty(uaAppId)
                             .pythonPlugin(userAgentPlugin)
                             .writeAdditionalFiles((c) -> {
                                 String filename = "src/%s/%s.py".formatted(moduleName, user_agent_plugin_file);

codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ConfigGenerator.java

@@ -97,22 +97,6 @@ public final class ConfigGenerator implements Runnable {
                                     .build())
                             .build())
                     .documentation("The retry strategy or options for configuring retry behavior. Can be either a configured RetryStrategy or RetryStrategyOptions to create one.")
-                    .build(),
-            ConfigProperty.builder()
-                    .name("user_agent_extra")
-                    .type(Symbol.builder().name("str").build())
-                    .documentation("Additional suffix to be appended to the User-Agent header.")
-                    .build(),
-            ConfigProperty.builder()
-                    .name("sdk_ua_app_id")
-                    .type(Symbol.builder().name("str").build())
-                    .documentation("A unique and opaque application ID that is appended to the User-Agent header.")
-                    .build(),
-            ConfigProperty.builder()
-                    .name("endpoint_url")
-                    .type(Symbol.builder().name("str").build())
-                    .documentation("The endpoint URL to use for requests. If not set, the standard endpoint for the service and region will be used.")
-                    .nullable(true)
                     .build());
 
     // This list contains any properties that must be added to any http-based

packages/smithy-aws-core/src/smithy_aws_core/config/validators.py

@@ -2,13 +2,16 @@
 # SPDX-License-Identifier: Apache-2.0
 
 import re
+from string import ascii_letters, digits
 from typing import Any, get_args
 
 from smithy_core.interfaces.retries import RetryStrategy
 from smithy_core.retries import RetryStrategyOptions, RetryStrategyType
 
 from smithy_aws_core.config.source_info import SourceInfo
 
+_USERAGENT_ALLOWED_CHARACTERS = ascii_letters + digits + "!#$%&'*+-.^_`|~/"
+
 
 class ConfigValidationError(ValueError):
     """Raised when a configuration value fails validation."""
@@ -144,3 +147,30 @@ def validate_retry_strategy(
         f"retry_strategy must be RetryStrategy or RetryStrategyOptions, got {type(value).__name__}",
         source,
     )
+
+
+def validate_and_sanitize_ua_string(
+    value: Any, source: SourceInfo | None = None
+) -> str | None:
+    """Validate and sanitize a User-Agent string component.
+
+    Replaces all disallowed characters with a hyphen. Allowed characters are
+    ASCII alphanumerics and "!#$%&'*+-.^_`|~/".
+
+    :param value: The UA string value to sanitize
+    :param source: The source that provided this value
+
+    :returns: The sanitized UA string or None if value is None
+
+    :raises ConfigValidationError: If the value is not a string
+    """
+    if value is None:
+        return None
+    if not isinstance(value, str):
+        raise ConfigValidationError(
+            "sdk_ua_app_id",
+            value,
+            f"UA string must be a string, got {type(value).__name__}",
+            source,
+        )
+    return "".join(c if c in _USERAGENT_ALLOWED_CHARACTERS else "-" for c in value)

packages/smithy-aws-core/tests/unit/config/test_property.py

@@ -96,31 +96,9 @@ def test_different_properties_resolve_independently(self) -> None:
         assert region == "us-west-2"
         assert retry_mode == "adaptive"
 
-    def test_unresolved_ua_extra_defaults_to_empty_string(self) -> None:
-        class ConfigWithEmptyDefault:
-            user_agent_extra = ConfigProperty(
-                "user_agent_extra",
-                default_value="",
-            )
-
-            def __init__(self, resolver: ConfigResolver) -> None:
-                self._resolver = resolver
-
-        source = StubSource("environment", {})
-        resolver = ConfigResolver(sources=[source])
-        config = ConfigWithEmptyDefault(resolver)
-
-        result = config.user_agent_extra
-
-        assert result == ""
-        assert getattr(config, "_cache_user_agent_extra") == (
-            "",
-            SimpleSource("default"),
-        )
-
-    def test_endpoint_url_defaults_to_none(self) -> None:
+    def test_unresolved_ua_app_id_defaults_to_none(self) -> None:
         class ConfigWithNoDefault:
-            endpoint_url = ConfigProperty("endpoint_url")
+            sdk_ua_app_id = ConfigProperty("sdk_ua_app_id")
 
             def __init__(self, resolver: ConfigResolver) -> None:
                 self._resolver = resolver
@@ -129,10 +107,10 @@ def __init__(self, resolver: ConfigResolver) -> None:
         resolver = ConfigResolver(sources=[source])
         config = ConfigWithNoDefault(resolver)
 
-        result = config.endpoint_url
+        result = config.sdk_ua_app_id
 
         assert result is None
-        assert getattr(config, "_cache_endpoint_url") == (
+        assert getattr(config, "_cache_sdk_ua_app_id") == (
             None,
             SimpleSource("default"),
         )
@@ -148,7 +126,6 @@ def _create_config_with_validator(
 
         class ConfigWithValidator:
             region = ConfigProperty("region", validator=validator)
-            retry_strategy = ConfigProperty("retry_strategy", validator=validator)
 
             def __init__(self, resolver: ConfigResolver) -> None:
                 self._resolver = resolver
@@ -194,9 +171,7 @@ class ConfigWithComplexResolver:
             retry_strategy = ConfigProperty(
                 "retry_strategy",
                 resolver_func=mock_resolver,
-                default_value=RetryStrategyOptions(
-                    retry_mode="standard", max_attempts=3
-                ),
+                default_value=RetryStrategyOptions(retry_mode="standard"),
             )
 
             def __init__(self, resolver: ConfigResolver) -> None:
@@ -212,7 +187,7 @@ def __init__(self, resolver: ConfigResolver) -> None:
 
         assert isinstance(result, RetryStrategyOptions)
         assert result.retry_mode == "standard"
-        assert result.max_attempts == 3
+        assert result.max_attempts is None
         assert source_info == SimpleSource("default")
 
     def test_validator_not_called_on_cached_access(self) -> None:

packages/smithy-aws-core/tests/unit/config/test_validators.py

@@ -5,6 +5,7 @@
 import pytest
 from smithy_aws_core.config.validators import (
     ConfigValidationError,
+    validate_and_sanitize_ua_string,
     validate_max_attempts,
     validate_region,
     validate_retry_mode,
@@ -49,3 +50,27 @@ def test_invalid_retry_mode_error_message(self) -> None:
             "Invalid value for 'retry_mode': 'random_mode'. retry_mode must be one "
             "of ('standard',), got random_mode" in str(exc_info.value)
         )
+
+
+class TestValidateUaString:
+    def test_allows_alphanumeric(self) -> None:
+        assert validate_and_sanitize_ua_string("abc123") == "abc123"
+
+    def test_none_returns_none(self) -> None:
+        assert validate_and_sanitize_ua_string(None) is None
+
+    def test_allows_spec_special_chars(self) -> None:
+        assert validate_and_sanitize_ua_string("!#$%&'*+-.^_`|~/") == "!#$%&'*+-.^_`|~/"
+
+    def test_sanitizes_parentheses(self) -> None:
+        result = validate_and_sanitize_ua_string("Java_HotSpot_(TM)_64-Bit_Server_VM")
+        assert result == "Java_HotSpot_-TM-_64-Bit_Server_VM"
+
+    def test_empty_string_passthrough(self) -> None:
+        assert validate_and_sanitize_ua_string("") == ""
+
+    def test_rejects_non_string(self) -> None:
+        with pytest.raises(ConfigValidationError) as exc_info:
+            validate_and_sanitize_ua_string(123)
+
+        assert exc_info.value.key == "sdk_ua_app_id"