Skip to content

Fix User-Agent duplication and support case-sensitive constant classes for feature IDs #3763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

muhammad-othman
Copy link
Member

@muhammad-othman muhammad-othman commented Apr 16, 2025

Description

Resolved an issue where the User-Agent header would grow on each request when reusing the same request object, this was caused by reusing the same UserAgentDetails instance attached to the request.
Fixed this by creating a copy of the request’s UserAgentDetails in the RequestContext instead of reusing the request instance.

Also found an issue where ConstantClass defaulted to case-insensitive comparisons, which caused conflicts with similarly named feature IDs.
For example when copying PAGINATOR ("C") feature id from the request to the RequestContext the FLEXIBLE_CHECKSUMS_RES_WHEN_REQUIRED ("c") feature id gets added instead.

Added ConstantClassComparerAttribute that allows derived ConstantClass types to specify the desired comparer type (OrdinalIgnoreCase, Ordinal.), and default behavior remains OrdinalIgnoreCase to preserve existing behavior.
Updated the feature ID constant class to use Ordinal to support case-sensitive identifiers.

Motivation and Context

#3759

Testing

  • DRY_RUN-20271a23-b168-4004-b640-ec5a77fb847b
  • Tested invoking multiple operations using the same request object and validated that the user agent stays constant.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

Copy link
Contributor

@peterrsongg peterrsongg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the System.Reflection namespace was added to ConstantClass and we've had issues with AOT compatibility in that class before, I tested that this change was AOT safe by creating a console application and instantiating one of the enums

var x = UserAgentFeatureId.ACCOUNT_ID_MODE_DISABLED;

which would trigger that code path

The console app was able to publish via native AOT successfully

C:\IssuesBugs\TestAOT> dotnet publish  TestAOT.csproj --framework net8.0 --configuration Release
  AWSSDK.Core.NetStandard net8.0 succeeded (8.5s) → C:\Dev\Repos\aws-sdk-net-staging\sdk\src\Core\bin\Release\net8.0\AWSSDK.Core.dll
  AWSSDK.S3.NetStandard net8.0 succeeded (9.0s) → C:\Dev\Repos\aws-sdk-net-staging\sdk\src\Services\S3\bin\Release\net8.0\AWSSDK.S3.dll
  AWSSDK.SimpleSystemsManagement.NetStandard net8.0 succeeded (14.1s) → C:\Dev\Repos\aws-sdk-net-staging\sdk\src\Services\SimpleSystemsManagement\bin\Release\net8.0\AWSSDK.SimpleSystemsManagement.dll
  TestAOT net8.0 succeeded with 4 warning(s) (9.2s) → bin\Release\net8.0\win-x64\publish\

Build succeeded with 4 warning(s) in 32.7s


Assert.AreEqual(Regex.Matches(userAgentHeader, "md/ClientSync").Count, 1);
Assert.AreEqual(Regex.Matches(userAgentHeader, "cfg/init-coll#").Count, 1);
Assert.AreEqual(Regex.Matches(userAgentHeader, "aws-sdk-dotnet-framework/").Count, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This would fail if the test project didn't run on Windows or targeted .NET 8 for example. I know that's not the case today, but still good not to write Windows specific asserts (for new tests at least) if we can.

@dscpinheiro dscpinheiro merged commit aec1752 into v4-development Apr 21, 2025
2 checks passed
@dscpinheiro dscpinheiro deleted the muhamoth/DOTNET-8072-bug-User-Agent-header-is-growing-when-reusing-the-request-object branch April 21, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants