fix: idempotency tokens being code-generated for nested structures #1248
Conversation
This comment has been minimized.
This comment has been minimized.
| val shape = ctx.model.expectShape(op.input.get()) | ||
| writer.write("val serializer = #T()", RuntimeTypes.Serde.SerdeFormUrl.FormUrlSerializer) | ||
| renderSerializerBody(ctx, shape, documentMembers, writer) | ||
| renderSerializerBody(ctx, shape, documentMembers, writer, true) |
There was a problem hiding this comment.
Style: Suggest adding the parameter name to boolean args when their meaning is unclear at the call site:
renderSerializerBody(ctx, shape, documentMembers, writer, idempotencyTokenEligible = true)| private val model = """ | ||
| ${"$"}version: "2" | ||
| namespace com.test | ||
| use aws.protocols#restJson1 | ||
| use aws.api#service | ||
| @restJson1 | ||
| @service(sdkId: "Example") | ||
| service Example { | ||
| version: "1.0.0", | ||
| operations: [GetBarUnNested, GetBarNested] | ||
| } | ||
| @http(method: "POST", uri: "/get-bar-un-nested") | ||
| operation GetBarUnNested { | ||
| input: BarUnNested | ||
| } | ||
| structure BarUnNested { | ||
| @idempotencyToken | ||
| bar: String | ||
| } | ||
| @http(method: "POST", uri: "/get-bar-nested") | ||
| operation GetBarNested { | ||
| input: BarNested | ||
| } | ||
| structure BarNested { | ||
| bar: Nest | ||
| } | ||
| structure Nest { | ||
| @idempotencyToken | ||
| baz: String | ||
| } | ||
| """.toSmithyModel() |
There was a problem hiding this comment.
Nit: Incorrect indentation. Applies to other multiline strings in this PR.
| private fun idempotencyTokenPostfix(memberShape: MemberShape): String = | ||
| if (memberShape.hasTrait<IdempotencyTokenTrait>()) { | ||
| if (memberShape.hasTrait<IdempotencyTokenTrait>() && idempotencyTokenEligible) { |
There was a problem hiding this comment.
Question: I can see that this works but the connection between idempotency member eligibility and being a top-level input structure member is not at all clear in the implementation. Did you investigate trying to derive whether the shape is a top-level input field directly rather than passing around a flag?
I'm wondering about something like a new knowledge index:
class TopLevelIndex(model: Model, service: ServiceShape) {
private val operations = TopDownIndex(ctx.model).getContainedOperations(ctx.service)
private val inputStructs = operations.mapNotNull { it.input.getOrNull() }.map { ctx.model.expectShape(it) }
private val inputMembers = inputStructs.flatMap { it.members() }.toSet()
fun isTopLevelInputMember(member: Shape): Boolean = member in inputMembers
}The question would be where to store the instance such that we're not recalculating it over and over again...
| if (memberShape.hasTrait<IdempotencyTokenTrait>()) { | ||
| if (memberShape.hasTrait<IdempotencyTokenTrait>() && idempotencyTokenEligible) { |
There was a problem hiding this comment.
Suggestion: I suggest logging an INFO or WARN about shapes with the @idempotencyToken trait which are not top-level input members and are therefore being ignored.
Affected ArtifactsNo artifacts changed size |
| private fun idempotencyTokenPostfix(memberShape: MemberShape): String = | ||
| if (memberShape.hasTrait<IdempotencyTokenTrait>()) { | ||
| // https://github.com/smithy-lang/smithy-kotlin/issues/1128 | ||
| if (topLevelIndex.isTopLevelInputMember(memberShape)) { |
3 participants
Issue #
closes #1128
Description of changes
SerializeStructGeneratornow knows when a structure is nested or not so it can process the@idempotencyTokentrait correctlyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.