-
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 22 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
7116221
344f118
c689ff5
9beca23
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 |
---|---|---|
|
@@ -4,7 +4,6 @@ | |
*/ | ||
package software.amazon.smithy.kotlin.codegen.rendering.protocol | ||
|
||
import software.amazon.smithy.aws.traits.HttpChecksumTrait | ||
import software.amazon.smithy.codegen.core.Symbol | ||
import software.amazon.smithy.kotlin.codegen.core.* | ||
import software.amazon.smithy.kotlin.codegen.integration.SectionId | ||
|
@@ -338,21 +337,14 @@ open class HttpProtocolClientGenerator( | |
|
||
/** | ||
* Render optionally installing Md5ChecksumMiddleware. | ||
* The Md5 middleware will only be installed if the operation requires a checksum and the user has not opted-in to flexible checksums. | ||
* The Md5 middleware will only be installed if the operation requires a checksum. | ||
*/ | ||
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: Why is this installed for every operation now without consideration of flexible checksums? If we enable CRC32 by default, doesn't this also calculate the MD5 unnecessarily? |
||
private fun OperationShape.renderIsMd5ChecksumRequired(writer: KotlinWriter) { | ||
val httpChecksumTrait = getTrait<HttpChecksumTrait>() | ||
|
||
// the checksum requirement can be modeled in either HttpChecksumTrait's `requestChecksumRequired` or the HttpChecksumRequired trait | ||
if (!hasTrait<HttpChecksumRequiredTrait>() && httpChecksumTrait == null) { | ||
return | ||
} | ||
|
||
if (hasTrait<HttpChecksumRequiredTrait>() || httpChecksumTrait?.isRequestChecksumRequired == true) { | ||
if (hasTrait<HttpChecksumRequiredTrait>()) { | ||
val interceptorSymbol = RuntimeTypes.HttpClient.Interceptors.Md5ChecksumInterceptor | ||
val inputSymbol = ctx.symbolProvider.toSymbol(ctx.model.expectShape(inputShape)) | ||
writer.withBlock("op.interceptors.add(#T<#T> {", "})", interceptorSymbol, inputSymbol) { | ||
writer.write("op.context.getOrNull(#T.ChecksumAlgorithm) == null", RuntimeTypes.HttpClient.Operation.HttpOperationContext) | ||
writer.write("true") | ||
} | ||
} | ||
} | ||
|
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