Skip to content

feat: Support auth scheme preference list #934

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

Merged
merged 10 commits into from
Jun 12, 2025
Merged

Conversation

dayaffe
Copy link
Contributor

@dayaffe dayaffe commented Jun 9, 2025

Issue #

Description of changes

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dayaffe dayaffe requested review from sichanyoo and jbelkins June 10, 2025 15:58
Copy link
Contributor

@jbelkins jbelkins left a comment

Choose a reason for hiding this comment

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

See inline questions/comments

/// - authSchemePreference: List of preferred auth scheme IDs
/// - authOptions: List of auth options to prioritize
/// - Returns: A new list of auth options with preferred options first, followed by non-preferred options
public extension AuthSchemeResolver {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for using normalized (i.e. without the namespace) names for comparison here? Will there ever be an auth scheme defined in two different namespaces, and if so, would we necessarily want to treat them as the same?

Copy link
Contributor Author

@dayaffe dayaffe Jun 10, 2025

Choose a reason for hiding this comment

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

Its part of the SEP update -- allows users to specify sigv4 / sigv4a / etc. without needing to know which namespace its from since when loading from environment its a String whereas in code they could say [Sigv4ATrait.ID] and not worry about it

valid auth options have the fully qualified name still

/// Sets the auth scheme priority from a preference list of IDs.
@discardableResult
public func withAuthSchemePreference(value: [String]?) -> Self {
if (value ?? []).isEmpty {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: value?.isEmpty ?? true skips the array literal

@@ -149,6 +151,19 @@ class AuthSchemeResolverGenerator {
}
}

private fun renderInit(writer: SwiftWriter) {
writer.apply {
write("public let authSchemePreference: [String]?")
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to permit both nil and [] as allowed values?

@@ -47,6 +47,7 @@ extension RestJsonProtocolClient {
public var httpClientEngine: SmithyHTTPAPI.HTTPClient
public var httpClientConfiguration: ClientRuntime.HttpClientConfiguration
public var authSchemes: SmithyHTTPAuthAPI.AuthSchemes?
public var authSchemePreference: [String]?
Copy link
Contributor

Choose a reason for hiding this comment

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

again, should nil and [] both be allowed here? Do they have different meanings?

@dayaffe dayaffe requested a review from jbelkins June 10, 2025 22:11
@dayaffe dayaffe merged commit 1455887 into main Jun 12, 2025
33 checks passed
@dayaffe dayaffe deleted the day/auth-scheme-pref branch June 12, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants