-
Notifications
You must be signed in to change notification settings - Fork 28
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 #1191
base: main
Are you sure you want to change the base?
Changes from all commits
2a9d104
205839c
7233b71
e1dc616
e436482
abdba02
3e4c891
9760ee1
f8b39b0
f676b7b
6e9b206
fb3a52a
40bb298
828adaa
e2068e7
08b4a37
91355d1
1fcd4b2
4edabfb
db65d74
b2e6f61
c63cf83
b68a9f5
0dd7886
d74c949
1157f26
d9f0659
dc3ce8b
3ef1c20
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 |
---|---|---|
|
@@ -37,7 +37,11 @@ operation SayHello { output: TestOutputDocument, errors: [Error] } | |
@http(method: "POST", uri: "/") | ||
operation SayHelloXml { output: TestOutput, errors: [Error] } | ||
|
||
structure TestOutputDocument with [TestStruct] { innerField: Nested, @required document: Document } | ||
structure TestOutputDocument with [TestStruct] { | ||
innerField: Nested, | ||
// @required | ||
document: Document | ||
} | ||
structure TestOutput with [TestStruct] { innerField: Nested } | ||
|
||
@mixin | ||
|
@@ -60,7 +64,7 @@ structure TestStruct { | |
@required | ||
nestedListValue: NestedList | ||
|
||
@required | ||
// @required | ||
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. Were these 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. The required trait isn't supposed to be there, a smithy validator will throw an error if it is, even if I disable the protocol test |
||
nested: Nested | ||
|
||
@required | ||
|
@@ -91,7 +95,7 @@ union MyUnion { | |
} | ||
|
||
structure Nested { | ||
@required | ||
// @required | ||
a: String | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import software.amazon.smithy.rulesengine.language.EndpointRuleSet | |
import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait | ||
import software.amazon.smithy.rulesengine.traits.EndpointTestCase | ||
import software.amazon.smithy.rulesengine.traits.EndpointTestsTrait | ||
import kotlin.streams.toList as kotlinToList // Gave this import a unique name because the Java one was being preferred | ||
|
||
/** | ||
* Get all shapes of a particular type from the model. | ||
|
@@ -32,7 +33,7 @@ import software.amazon.smithy.rulesengine.traits.EndpointTestsTrait | |
* shape's closure for example) | ||
*/ | ||
@Suppress("EXTENSION_SHADOWED_BY_MEMBER") | ||
inline fun <reified T : Shape> Model.shapes(): List<T> = shapes(T::class.java).toList() | ||
inline fun <reified T : Shape> Model.shapes(): List<T> = shapes(T::class.java).kotlinToList() | ||
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 has never been a problem before, what changed? 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. There was a refactor in AWS SDK Kotlin to make the codegen tests KMP, which led to the JVM CI checks failing because we've been using JVM 17. I set the JVM version in the tests to 1.8 and the 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. Ok that makes sense. You should be able to use |
||
|
||
/** | ||
* Extension function to return a shape of expected type. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
package software.amazon.smithy.kotlin.codegen.rendering.checksums | ||
|
||
import software.amazon.smithy.aws.traits.HttpChecksumTrait | ||
import software.amazon.smithy.kotlin.codegen.KotlinSettings | ||
import software.amazon.smithy.kotlin.codegen.core.KotlinWriter | ||
import software.amazon.smithy.kotlin.codegen.core.RuntimeTypes | ||
import software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration | ||
import software.amazon.smithy.kotlin.codegen.model.hasTrait | ||
import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolGenerator | ||
import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolMiddleware | ||
import software.amazon.smithy.model.Model | ||
import software.amazon.smithy.model.shapes.OperationShape | ||
import software.amazon.smithy.model.traits.HttpChecksumRequiredTrait | ||
|
||
/** | ||
* Handles the `httpChecksumRequired` trait. | ||
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/docs: Explain more about how it "handles" the trait 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. The "meat" of the integration is the middleware and both of them have their own Kdocs. It feels like docs overkill to also add a summary of what each middleware is doing here |
||
* See: https://smithy.io/2.0/spec/http-bindings.html#httpchecksumrequired-trait | ||
*/ | ||
class HttpChecksumRequiredIntegration : KotlinIntegration { | ||
override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = | ||
model.isTraitApplied(HttpChecksumRequiredTrait::class.java) | ||
|
||
override fun customizeMiddleware( | ||
ctx: ProtocolGenerator.GenerationContext, | ||
resolved: List<ProtocolMiddleware>, | ||
): List<ProtocolMiddleware> = resolved + httpChecksumRequiredDefaultAlgorithmMiddleware + httpChecksumRequiredMiddleware | ||
} | ||
|
||
/** | ||
* Adds default checksum algorithm to the execution context | ||
*/ | ||
private val httpChecksumRequiredDefaultAlgorithmMiddleware = object : ProtocolMiddleware { | ||
override val name: String = "httpChecksumRequiredDefaultAlgorithmMiddleware" | ||
override val order: Byte = -2 // Before S3 Express (possibly) changes the default (-1) and before calculating checksum (0) | ||
|
||
override fun isEnabledFor(ctx: ProtocolGenerator.GenerationContext, op: OperationShape): Boolean = | ||
op.hasTrait<HttpChecksumRequiredTrait>() && !op.hasTrait<HttpChecksumTrait>() | ||
|
||
override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) { | ||
writer.write( | ||
"op.context[#T.DefaultChecksumAlgorithm] = #S", | ||
RuntimeTypes.HttpClient.Operation.HttpOperationContext, | ||
"MD5", | ||
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. question/correctness: Can/should we store this as a 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. We could, I think we would have to initialize the |
||
) | ||
} | ||
} | ||
|
||
/** | ||
* Adds interceptor to calculate request checksums. | ||
* The `httpChecksum` trait supersedes the `httpChecksumRequired` trait. If both are applied to an operation use `httpChecksum`. | ||
* | ||
* See: https://smithy.io/2.0/aws/aws-core.html#behavior-with-httpchecksumrequired | ||
*/ | ||
private val httpChecksumRequiredMiddleware = object : ProtocolMiddleware { | ||
override val name: String = "httpChecksumRequiredMiddleware" | ||
|
||
override fun isEnabledFor(ctx: ProtocolGenerator.GenerationContext, op: OperationShape): Boolean = | ||
op.hasTrait<HttpChecksumRequiredTrait>() && !op.hasTrait<HttpChecksumTrait>() | ||
|
||
override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) { | ||
writer.write( | ||
"op.interceptors.add(#T())", | ||
RuntimeTypes.HttpClient.Interceptors.HttpChecksumRequiredInterceptor, | ||
) | ||
} | ||
} |
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.
FYI I know we talked about disabling some tests that are failing in Smithy 1.53.0, but these are disabling the entire test suite, which we don't want to do