Add support for sdk_ua_app_id. by ubaskota · Pull Request #653 · smithy-lang/smithy-python

ubaskota · 2026-03-11T02:18:21Z

Issue #, if available:

Description of changes:
Add codegen support for a new config property: sdk_ua_app_id, along with a validator. For AWS SDKs, these are generated as descriptor-based properties with config resolver and default values.

Testing

Added a unit tests to verify default value behavior: sdk_ua_app_id
All existing tests pass.
Manually tested the generated config from the client side and verified that all tests pass.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jonathan343

Had a few comments. I also noticed there are no validators for any of these options. I know at least the user agent app ID will need it:

Valid values: String with maximum length of 50. Letters, numbers and the following special characters are allowed: !,#,$,%,&,',*,+,-,.,^,_,`,|,~.

jonathan343 · 2026-03-11T20:33:16Z

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

            .build();
+
+    public static final ConfigProperty USER_AGENT_EXTRA = ConfigProperty.builder()
+            .name("user_agent_extra")


Why was this config option added? This config option isn't standardized across all SDKs (source). By adding it here we're now reading AWS_USER_AGENT_EXTRA from the environment which we shouldn't be doing.

jonathan343 · 2026-03-11T20:35:42Z

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

+            .documentation("A unique and opaque application ID that is appended to the User-Agent header.")
+            .nullable(false)
+            .useDescriptor(true)
+            .defaultValue("''")


Is there any limitation with keeping the default to None? This feels weird to me.

I thought users would be able to add it without first checking if the value is None if the default is an empty string. But I see why returning None would be a better choice. I will update it.

jonathan343 · 2026-03-11T21:10:56Z

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

+            .defaultValue("''")
+            .build();
+
+    public static final ConfigProperty ENDPOINT_URL = ConfigProperty.builder()


I haven't tested but it looks like we now have endpoint_uri and endpoint_url. Since there are not other changes, if someone sets export AWS_ENDPOINT_URL="...", then config.endpoint_url will resolve correctly, but it won't actually be used by the SDK since we're using endpoint_uri still.

This one is a bit more complicated since smithy python stuff uses endpoint_uri. Can we leave the existing endpoint_uri but map to endpoint_url config options?

SamRemis · 2026-03-11T21:19:41Z

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

                            .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()


This will add all of these to generic clients; can we keep them as AWS specific for now?

SamRemis · 2026-03-11T21:20:49Z

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

+                    .type(Symbol.builder().name("str").build())
+                    .documentation("A unique and opaque application ID that is appended to the User-Agent header.")
+                    .build(),
+            ConfigProperty.builder()


endpoint_url and endpoint_uri are effectively the same. We should address this in the long run since the SDKs have standardized on endpoint_url, but we currently only accept endpoint_uri.

For now, let's just remove the duplicate endpoint_url.

SamRemis · 2026-03-13T14:02:06Z

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

-                default_value=RetryStrategyOptions(
-                    retry_mode="standard", max_attempts=3
-                ),
+                default_value=RetryStrategyOptions(retry_mode="standard"),


Can you help me understand why we are making these changes?

The default value for retry_strategy should only specify retry_mode, since max_attempts will be determined later based on the selected retry mode. It would have worked either way though.

SamRemis · 2026-03-13T17:04:12Z

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

    )
+
+
+def validate_and_sanitize_ua_string(


I think this is already done here?

So just to record this here for posterity - we did specifically ask for this to be added; at the time, we weren't aware that it would be duplicate logic.

This kind of points to a minor issue with smithy-python that I started a discussion on, but we won't be acting on it for now. We are putting an HTTP-specific concept (user-agent) in a non-HTTP-specific place (client config). While we could theoretically reuse the user agent string for another protocol, the same concepts may or may not apply.

As of today, it happens as a plugin during the modify-before-signing event. While a user shouldn't be able to set an invalid user agent that we know will fail on the config, we could theoretically pass user agent components to the user agent part of the HTTP code from somewhere other than the config in the future. On the other hand, we could wind up using another plugin in the future that loses this behavior and miss it that way; neither is a perfect place for this logic to be.

Right now I'd lean towards removing validations beyond typing for this input. If these requirements somehow change in the future based on who is consuming the user agent, it's better to keep these downstream where the header actually gets set.

SamRemis · 2026-03-13T20:42:38Z

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

+
+
+class TestValidateUaString:
+    def test_allows_string(self) -> None:


Nit: all of these tests except the error test could easily be one test with pytest.mark.parametrize; everything is only validating that it's passed through, so the test body could just be
assert validate_ua_string(value) == value

SamRemis · 2026-03-13T20:45:26Z

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

        assert region == "us-west-2"
        assert retry_mode == "adaptive"

+    def test_unresolved_ua_app_id_defaults_to_none(self) -> None:


Can you help me understand what value this test provides? Are we just testing that the default works?

I don't think this test has anything to do with the other code changes- again, unless you changed ConfigProperty and broke it, this test wouldn't fail? There's nothing about this PR that is being tested here, so I'd hope we already had proper tests on TestConfigProperty

ubaskota requested a review from a team as a code owner March 11, 2026 02:18

jonathan343 reviewed Mar 11, 2026

View reviewed changes

SamRemis reviewed Mar 11, 2026

View reviewed changes

ubaskota added 2 commits March 12, 2026 23:24

Add support for endpoint_url, user_agent_extra, and sdk_ua_app_id

1c0febf

Address reviews

7146d45

ubaskota force-pushed the new_config_vars branch from 36f41f8 to 7146d45 Compare March 13, 2026 05:25

ubaskota changed the title ~~Add support for endpoint_url, user_agent_extra, and sdk_ua_app_id.~~ Add support for sdk_ua_app_id. Mar 13, 2026

SamRemis reviewed Mar 13, 2026

View reviewed changes

Address reviews

f55cdbf

SamRemis reviewed Mar 13, 2026

View reviewed changes

Address reviews

1092672

SamRemis approved these changes Mar 13, 2026

View reviewed changes

ubaskota merged commit fd6d81a into smithy-lang:config_resolution_main Mar 13, 2026
4 checks passed



		class TestValidateUaString:
		def test_allows_string(self) -> None:

Conversation

ubaskota commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathan343 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ubaskota commented Mar 11, 2026 •

edited

Loading