Conversation
| fun render(serviceSymbol: Symbol) { | ||
| writer.openBlock( | ||
| "${ctx.settings.visibility} class \$L: \$N {", | ||
| "${ctx.settings.visibility} final class \$L: \$N, Sendable {", |
There was a problem hiding this comment.
See my note on the other PR, rather than mark Sendable here (and on every generated client) I think we can just specify that ClientRuntime.Client conforms to Sendable
|
|
||
| writer.openBlock( | ||
| "public class \$LConfiguration: \$L {", | ||
| "public final class \$LConfiguration: \$L, @unchecked Sendable {", |
There was a problem hiding this comment.
Also see my note on the other PR, even after applying final this type is not Sendable (checked or unchecked)
| override fun getMethods(ctx: ProtocolGenerator.GenerationContext): Set<Function> = | ||
| setOf( | ||
| Function( | ||
| name = "addInterceptorProvider", | ||
| renderBody = { writer -> writer.write("self.httpInterceptorProviders.append(provider)") }, | ||
| parameters = | ||
| listOf( | ||
| FunctionParameter.NoLabel("provider", ClientRuntimeTypes.Core.HttpInterceptorProvider), | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The reason for this and similar changes is because the newly updated structs now have only immutable properties so we can't append interceptors after initialization. This would have be provided at initialization. Will need to double check how this fits in with SRA
| throw ClientBuilderError.incompatibleConfigurationType( | ||
| expected: String(describing: ClientType.Config.self), | ||
| received: String(describing: type(of: configuredClient)) | ||
| ) |
There was a problem hiding this comment.
Can change this to be more similar to our current errors
| parameters = | ||
| listOf( | ||
| FunctionParameter.NoLabel("provider", ClientRuntimeTypes.Core.InterceptorProvider), | ||
| software.amazon.smithy.swift.codegen.lang.FunctionParameter.NoLabel( |
There was a problem hiding this comment.
better to import this type above than use the full identifier here
| parameters = | ||
| listOf( | ||
| FunctionParameter.NoLabel("provider", ClientRuntimeTypes.Core.HttpInterceptorProvider), | ||
| software.amazon.smithy.swift.codegen.lang.FunctionParameter.NoLabel( |
There was a problem hiding this comment.
better to import this type above than use the full identifier here
| visibility: String, | ||
| ) { | ||
| writer.openBlock("$visibility protocol \$N {", "}", EndpointTypes.EndpointResolver) { | ||
| writer.openBlock("$visibility protocol \$N: Sendable {", "}", EndpointTypes.EndpointResolver) { |
There was a problem hiding this comment.
nit: use the Sendable symbol in SwiftTypes rather than a literal string
|
|
||
| writer.openBlock( | ||
| "public class \$LConfiguration: \$L {", | ||
| "public struct \$LConfiguration: \$L, Sendable {", |
There was a problem hiding this comment.
same as above use SwiftTypes.Sendable
| properties: List<ConfigProperty>, | ||
| ) { | ||
| // Only render if there are async properties | ||
| if (properties.none { it.default?.isAsync == true }) return |
There was a problem hiding this comment.
Are we potentially removing an initializer that customers are using by doing this?
There was a problem hiding this comment.
That init was customer facing so potentially -- I'll revert this change back so we dont break anyone just in case
| interceptorProviders: [ClientRuntime.InterceptorProvider]? = nil, | ||
| httpInterceptorProviders: [ClientRuntime.HttpInterceptorProvider]? = nil | ||
| httpInterceptorProviders: [ClientRuntime.HttpInterceptorProvider]? = nil, | ||
| bearerTokenIdentityResolver: (any SmithyIdentity.BearerTokenIdentityResolver)? = nil |
There was a problem hiding this comment.
Changes to order of init parameters is breaking; we need to preserve the current order.
| /// | ||
| /// Default options are used if none are set. | ||
| var retryStrategyOptions: RetryStrategyOptions { get set } | ||
| var retryStrategyOptions: RetryStrategyOptions { get } |
There was a problem hiding this comment.
Per discussion, revert these to { get set }
| self.authSchemes = authSchemes | ||
| } | ||
|
|
||
| public func configureClient(clientConfiguration: ClientConfiguration) { |
There was a problem hiding this comment.
Per discussion, param should be clientConfiguration: inout ClientConfiguration
| val OrchestratorBuilder = runtimeSymbol("OrchestratorBuilder", SwiftDeclaration.CLASS) | ||
| val OrchestratorTelemetry = runtimeSymbol("OrchestratorTelemetry", SwiftDeclaration.CLASS) | ||
| val OrchestratorBuilder = runtimeSymbol("OrchestratorBuilder", null) | ||
| val OrchestratorTelemetry = runtimeSymbol("OrchestratorTelemetry", null) |
There was a problem hiding this comment.
Why do all of these no longer have types assigned?
| val ClientError = runtimeSymbol("ClientError", SwiftDeclaration.ENUM) | ||
| val Context = runtimeSymbol("Context", SwiftDeclaration.CLASS) | ||
| val ContextBuilder = runtimeSymbol("ContextBuilder", SwiftDeclaration.CLASS) | ||
| val ContextBuilder = runtimeSymbol("ContextBuilder", null) |
There was a problem hiding this comment.
Same as in ClientRuntimeTypes
| /// Note: Uses `@unchecked Sendable` because the wrapper is designed to safely encapsulate | ||
| /// non-Sendable providers for use in concurrent contexts. The safety is ensured by the | ||
| /// immutability of the stored provider reference. | ||
| public struct SendableInterceptorProviderBox: InterceptorProvider, @unchecked Sendable { |
There was a problem hiding this comment.
We will be able to remove these wrapper types once we deprecate the class based config. The purpose of these wrappers is to allow the new struct based config to fully support being marked as sendable.
| (token is String && !(token as! String).isEmpty) || | ||
| (token is [String: Any] && !(token as! [String: Any]).isEmpty) { | ||
| self.input = input.usingPaginationToken(token) | ||
| if let token = token { |
There was a problem hiding this comment.
changed for readability when solving a bug, allows us to remove swiftlint disable
| /// Note: Uses `@unchecked Sendable` because the wrapper is designed to safely encapsulate | ||
| /// non-Sendable providers for use in concurrent contexts. The safety is ensured by the | ||
| /// immutability of the stored provider reference. | ||
| public struct SendableHttpInterceptorProviderBox: HttpInterceptorProvider, @unchecked Sendable { |
There was a problem hiding this comment.
Same as below, see comment on SendableInterceptorProviderBox
2 participants
Issue #
Description of changes
Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.