-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: support default checksums #1475
base: main
Are you sure you want to change the base?
Changes from 49 commits
4729584
7b73088
dbfb973
b15358b
dacc345
bbafc3f
a79aa1e
e61efbb
285c08b
fbb3dd5
136c9db
cb4ab12
41ad1b4
1a2d989
11c733f
4d24a24
c514cba
2796317
6d53243
40cfb17
410c085
472fae9
e9bf8e5
5133a60
4ce5da7
da31deb
6037580
2f6e10b
d085dec
0bafdcc
fc84b61
0f6fa65
c76aefe
3476242
b21ec41
dd2792f
779cd3a
6674dc7
a3a2e09
c80b55c
5e2855c
0405b20
eb338f4
a2fb7fb
093ed72
d498e30
b534927
2761bf9
5328641
ad30f70
4a162de
d665873
1aefe66
55bdcb8
44894e6
ad8d1ad
d398801
3c259ef
0029768
427daf3
41ad742
5c6c0bf
4d915c6
609226c
80ba070
94dfc93
77f6ea5
4b61347
756f66d
3ed4d91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package aws.sdk.kotlin.runtime.config.checksums | ||
|
||
import aws.sdk.kotlin.runtime.ConfigurationException | ||
import aws.sdk.kotlin.runtime.InternalSdkApi | ||
import aws.sdk.kotlin.runtime.config.AwsSdkSetting | ||
import aws.sdk.kotlin.runtime.config.profile.AwsProfile | ||
import aws.sdk.kotlin.runtime.config.profile.requestChecksumCalculation | ||
import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption | ||
import aws.smithy.kotlin.runtime.config.resolve | ||
import aws.smithy.kotlin.runtime.util.LazyAsyncValue | ||
import aws.smithy.kotlin.runtime.util.PlatformProvider | ||
|
||
/** | ||
* Attempts to resolve requestChecksumCalculation from the specified sources. | ||
* @return requestChecksumCalculation setting if found, the default value if not. | ||
*/ | ||
@InternalSdkApi | ||
public suspend fun resolveRequestChecksumCalculation(platform: PlatformProvider = PlatformProvider.System, profile: LazyAsyncValue<AwsProfile>): HttpChecksumConfigOption { | ||
val unparsedString = AwsSdkSetting.AwsRequestChecksumCalculation.resolve(platform) ?: profile.get().requestChecksumCalculation | ||
return unparsedString?.let { | ||
when (unparsedString.uppercase()) { | ||
"WHEN_SUPPORTED" -> HttpChecksumConfigOption.WHEN_SUPPORTED | ||
"WHEN_REQUIRED" -> HttpChecksumConfigOption.WHEN_REQUIRED | ||
else -> throw ConfigurationException( | ||
"'$it' is not a valid value for request checksum calculation. Valid values are: ${HttpChecksumConfigOption.entries}", | ||
) | ||
} | ||
} ?: HttpChecksumConfigOption.WHEN_SUPPORTED | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Could simplify: return when (unparsedString?.uppercase()) {
null -> HttpChecksumConfigOption.WHEN_SUPPORTED
"WHEN_SUPPORTED" -> HttpChecksumConfigOption.WHEN_SUPPORTED
"WHEN_REQUIRED" -> HttpChecksumConfigOption.WHEN_REQUIRED
else -> throw ConfigurationException(
"'$unparsedString' is not a valid value for request checksum calculation. Valid values are: ${HttpChecksumConfigOption.entries}",
)
} |
||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This file seems very similar to ResolveRequestChecksumCalculation.kt. Seems like they could be combined into a single file and maybe even reuse some logic. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package aws.sdk.kotlin.runtime.config.checksums | ||
|
||
import aws.sdk.kotlin.runtime.ConfigurationException | ||
import aws.sdk.kotlin.runtime.InternalSdkApi | ||
import aws.sdk.kotlin.runtime.config.AwsSdkSetting | ||
import aws.sdk.kotlin.runtime.config.profile.AwsProfile | ||
import aws.sdk.kotlin.runtime.config.profile.responseChecksumValidation | ||
import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption | ||
import aws.smithy.kotlin.runtime.config.resolve | ||
import aws.smithy.kotlin.runtime.util.LazyAsyncValue | ||
import aws.smithy.kotlin.runtime.util.PlatformProvider | ||
|
||
/** | ||
* Attempts to resolve responseChecksumValidation from the specified sources. | ||
* @return responseChecksumValidation setting if found, the default value if not. | ||
*/ | ||
@InternalSdkApi | ||
public suspend fun resolveResponseChecksumValidation(platform: PlatformProvider = PlatformProvider.System, profile: LazyAsyncValue<AwsProfile>): HttpChecksumConfigOption { | ||
val unparsedString = AwsSdkSetting.AwsResponseChecksumValidation.resolve(platform) ?: profile.get().responseChecksumValidation | ||
return unparsedString?.let { | ||
when (unparsedString.uppercase()) { | ||
"WHEN_SUPPORTED" -> HttpChecksumConfigOption.WHEN_SUPPORTED | ||
"WHEN_REQUIRED" -> HttpChecksumConfigOption.WHEN_REQUIRED | ||
else -> throw ConfigurationException( | ||
"'$it' is not a valid value for request checksum calculation. Valid values are: ${HttpChecksumConfigOption.entries}", | ||
) | ||
} | ||
} ?: HttpChecksumConfigOption.WHEN_SUPPORTED | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to exist? buildSrc is treated as an included build which means every subproject will have these config options applied. It seems odd to need this just for codegen tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the best way I could find to share a data class between subprojects, do you know of alternatives that would work better? I might've missed them There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can make a new project that both of those subprojects depend on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still a work in progress, I don't want to block the review because of this. I think worst case scenario we can come back and refactor this without it being a breaking change |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
plugins { | ||
alias(libs.plugins.kotlin.jvm) | ||
} | ||
|
||
repositories { | ||
mavenCentral() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
dependencyResolutionManagement { | ||
versionCatalogs { | ||
create("libs") { | ||
from(files("../gradle/libs.versions.toml")) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
package aws.sdk.kotlin.shared | ||
|
||
/** | ||
* An AWS SDK for Kotlin codegen test | ||
*/ | ||
data class CodegenTest( | ||
val name: String, | ||
val model: Model, | ||
val serviceShapeId: String, | ||
val protocolName: String? = null, | ||
) | ||
|
||
/** | ||
* A smithy model file | ||
*/ | ||
data class Model( | ||
val fileName: String, | ||
val path: String = "src/commonTest/resources/", | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,16 @@ package aws.sdk.kotlin.codegen.customization.flexiblechecksums | |
|
||
import software.amazon.smithy.aws.traits.HttpChecksumTrait | ||
import software.amazon.smithy.kotlin.codegen.KotlinSettings | ||
import software.amazon.smithy.kotlin.codegen.core.CodegenContext | ||
import software.amazon.smithy.kotlin.codegen.core.KotlinWriter | ||
import software.amazon.smithy.kotlin.codegen.core.RuntimeTypes | ||
import software.amazon.smithy.kotlin.codegen.core.withBlock | ||
import software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration | ||
import software.amazon.smithy.kotlin.codegen.model.* | ||
import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolGenerator | ||
import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolMiddleware | ||
import software.amazon.smithy.kotlin.codegen.rendering.util.ConfigProperty | ||
import software.amazon.smithy.kotlin.codegen.rendering.util.ConfigPropertyType | ||
import software.amazon.smithy.kotlin.codegen.utils.getOrNull | ||
import software.amazon.smithy.model.Model | ||
import software.amazon.smithy.model.shapes.OperationShape | ||
|
@@ -26,8 +29,44 @@ class FlexibleChecksumsRequest : KotlinIntegration { | |
.shapes<OperationShape>() | ||
.any { it.hasTrait<HttpChecksumTrait>() } | ||
|
||
override fun additionalServiceConfigProps(ctx: CodegenContext): List<ConfigProperty> = | ||
listOf( | ||
ConfigProperty { | ||
name = "requestChecksumCalculation" | ||
symbol = RuntimeTypes.SmithyClient.Config.HttpChecksumConfigOption | ||
baseClass = RuntimeTypes.SmithyClient.Config.HttpChecksumClientConfig | ||
useNestedBuilderBaseClass() | ||
documentation = "Configures request checksum calculation" | ||
propertyType = ConfigPropertyType.RequiredWithDefault("HttpChecksumConfigOption.WHEN_SUPPORTED") | ||
}, | ||
) | ||
|
||
override fun customizeMiddleware(ctx: ProtocolGenerator.GenerationContext, resolved: List<ProtocolMiddleware>) = | ||
resolved + flexibleChecksumsRequestMiddleware | ||
resolved + flexibleChecksumsRequestMiddleware + configBusinessMetrics | ||
|
||
private val configBusinessMetrics = object : ProtocolMiddleware { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: middleware name and value's name should match |
||
override val name: String = "requestChecksumCalculationBusinessMetric" | ||
|
||
override fun isEnabledFor(ctx: ProtocolGenerator.GenerationContext, op: OperationShape): Boolean = | ||
op.hasTrait<HttpChecksumTrait>() | ||
|
||
override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) { | ||
writer.withBlock("when(config.requestChecksumCalculation) {", "}") { | ||
writer.write( | ||
"#T.WHEN_SUPPORTED -> op.context.#T(#T.FLEXIBLE_CHECKSUMS_REQ_WHEN_SUPPORTED)", | ||
RuntimeTypes.SmithyClient.Config.HttpChecksumConfigOption, | ||
RuntimeTypes.Core.BusinessMetrics.emitBusinessMetric, | ||
RuntimeTypes.Core.BusinessMetrics.SmithyBusinessMetric, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: refactor duplicate calls to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are two different runtime types, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was meaning your two separate calls to |
||
) | ||
writer.write( | ||
"#T.WHEN_REQUIRED -> op.context.#T(#T.FLEXIBLE_CHECKSUMS_REQ_WHEN_REQUIRED)", | ||
RuntimeTypes.SmithyClient.Config.HttpChecksumConfigOption, | ||
RuntimeTypes.Core.BusinessMetrics.emitBusinessMetric, | ||
RuntimeTypes.Core.BusinessMetrics.SmithyBusinessMetric, | ||
) | ||
} | ||
} | ||
} | ||
|
||
private val flexibleChecksumsRequestMiddleware = object : ProtocolMiddleware { | ||
override val name: String = "FlexibleChecksumsRequest" | ||
|
@@ -42,7 +81,6 @@ class FlexibleChecksumsRequest : KotlinIntegration { | |
} | ||
|
||
override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) { | ||
val interceptorSymbol = RuntimeTypes.HttpClient.Interceptors.FlexibleChecksumsRequestInterceptor | ||
val inputSymbol = ctx.symbolProvider.toSymbol(ctx.model.expectShape(op.inputShape)) | ||
|
||
val httpChecksumTrait = op.getTrait<HttpChecksumTrait>()!! | ||
|
@@ -52,12 +90,17 @@ class FlexibleChecksumsRequest : KotlinIntegration { | |
.first { it.memberName == httpChecksumTrait.requestAlgorithmMember.get() } | ||
|
||
val requestAlgorithmMemberName = ctx.symbolProvider.toMemberName(requestAlgorithmMember) | ||
val requestChecksumRequired = httpChecksumTrait.isRequestChecksumRequired | ||
|
||
writer.withBlock("op.interceptors.add(#T<#T>() {", "})", interceptorSymbol, inputSymbol) { | ||
writer.write("input.#L?.value", requestAlgorithmMemberName) | ||
} | ||
writer.withBlock("input.#L?.let {", "}", requestAlgorithmMemberName) { | ||
writer.write("op.context[#T.ChecksumAlgorithm] = it.value", RuntimeTypes.HttpClient.Operation.HttpOperationContext) | ||
writer.withBlock( | ||
"op.interceptors.add(#T<#T>(", | ||
"))", | ||
RuntimeTypes.HttpClient.Interceptors.FlexibleChecksumsRequestInterceptor, | ||
inputSymbol, | ||
) { | ||
writer.write("#L,", requestChecksumRequired) | ||
writer.write("config.requestChecksumCalculation,") | ||
writer.write("input.#L?.value,", requestAlgorithmMemberName) | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these test reports used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure why but sometimes the logs from failed CI tests are not very useful. In those cases you can just get a test report from the build and see which test cases failed and exception messages. Other CI checks have this as well