-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix: Route 53 TrimHostedZone customization is incomplete #1445
Conversation
|
||
import ClientRuntime | ||
|
||
public struct Route53TrimHostedZoneMiddleware<Input, Output>: ClientRuntime.Middleware { |
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.
This is the same as the previously code-generated middleware, except:
- The input & output are generics that get substituted for each operation at compile time.
- The hosted zone ID on the input model is accessed via a key path that is passed in at initialization.
@@ -38,8 +39,7 @@ class Route53TrimHostedZone : SwiftIntegration { | |||
val inputShape = MiddlewareShapeUtils.inputShape(ctx.model, operationShape) | |||
val hostedZoneMember = inputShape.members().find { it.hasTrait<TrimHostedZone>() } | |||
if (hostedZoneMember != null) { | |||
StripHostedZoneURLPathMiddleware.renderMiddleware(ctx, operationShape) | |||
operationMiddleware.prependMiddleware(operationShape, StripHostedZoneUrlPathMiddlewareRenderable(ctx.model, ctx.symbolProvider)) | |||
operationMiddleware.prependMiddleware(operationShape, StripHostedZoneURLPathMiddlewareRenderable(ctx.model, ctx.symbolProvider)) |
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.
The middleware is no longer rendered for each operation that needs it since a reusable one is used instead.
import software.amazon.smithy.swift.codegen.middleware.MiddlewarePosition | ||
import software.amazon.smithy.swift.codegen.middleware.MiddlewareRenderable | ||
import software.amazon.smithy.swift.codegen.middleware.MiddlewareStep | ||
|
||
class StripHostedZoneUrlPathMiddlewareRenderable( | ||
class StripHostedZoneURLPathMiddlewareRenderable( |
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.
Type renamed
@@ -21,7 +20,6 @@ class StripHostedZoneUrlPathMiddlewareRenderable( | |||
override val position = MiddlewarePosition.AFTER | |||
|
|||
override fun render(ctx: ProtocolGenerator.GenerationContext, writer: SwiftWriter, op: OperationShape, operationStackName: String) { | |||
val inputShapeName = MiddlewareShapeUtils.inputSymbol(symbolProvider, model, op).name | |||
writer.write("$operationStackName.${middlewareStep.stringValue()}.intercept(position: ${position.stringValue()}, middleware: ${inputShapeName}StripHostedZoneMiddleware())") | |||
writer.write("$operationStackName.${middlewareStep.stringValue()}.intercept(position: ${position.stringValue()}, middleware: Route53TrimHostedZoneMiddleware(\\.hostedZoneId))") |
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.
The reusable middleware, initialized with a key path, is inserted into the operation stack instead of a code-generated middleware.
import software.amazon.smithy.swift.codegen.integration.middlewares.handlers.MiddlewareShapeUtils | ||
import software.amazon.smithy.swift.codegen.integration.steps.OperationInitializeStep | ||
|
||
class StripHostedZoneURLPathMiddleware( |
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.
This middleware is no longer rendered since a reusable one in AWSClientRuntime
is used instead.
@@ -96,31 +96,6 @@ extension Route53ClientTypes { | |||
} | |||
} | |||
|
|||
public struct ActivateKeySigningKeyInputStripHostedZoneMiddleware: ClientRuntime.Middleware { |
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.
14 generated middlewares are removed from Models, because the reusable one is used instead.
.withIdentityResolver(value: config.awsCredentialIdentityResolver, schemeID: "aws.auth#sigv4") | ||
.withIdentityResolver(value: config.awsCredentialIdentityResolver, schemeID: "aws.auth#sigv4a") | ||
.withRegion(value: config.region) | ||
.withSigningName(value: "route53") | ||
.withSigningRegion(value: config.signingRegion) | ||
.build() | ||
var operation = ClientRuntime.OperationStack<ActivateKeySigningKeyInput, ActivateKeySigningKeyOutput>(id: "activateKeySigningKey") | ||
operation.initializeStep.intercept(position: .after, middleware: ActivateKeySigningKeyInputStripHostedZoneMiddleware()) | ||
operation.initializeStep.intercept(position: .after, middleware: Route53TrimHostedZoneMiddleware(\.hostedZoneId)) |
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.
Middleware insertion is updated to use the new, reusable middleware and to pass in the key path to the field holding the hosted zone ID.
Six additional operations get the customization as well; these are ones that use a hosted zone ID but it is not exactly named "HostedZoneId".
|
||
class StripHostedZoneUrlPathMiddlewareRenderable( | ||
class TrimHostedZoneURLPathMiddlewareRenderable( |
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.
- Types are renamed
- Code cleanup
- The reusable middleware is inserted instead of the custom one
// | ||
|
||
import XCTest | ||
import AWSRoute53 |
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.
A simple integration test for Route 53:
- creates a hosted zone,
- creates an A record in that zone,
- deletes the A record,
- deletes the hosted zone.
All operations used after creating the zone require the TrimHostedZone customization in order to succeed.
} | ||
} | ||
|
||
private fun isHostId(shape: Shape): Boolean { | ||
return (shape is MemberShape && shape.target == ShapeId.from("com.amazonaws.route53#ResourceId")) && shape.hasTrait<HttpLabelTrait>() && shape.memberName == "HostedZoneId" | ||
return (shape is MemberShape && shape.target == ShapeId.from("com.amazonaws.route53#ResourceId")) && shape.hasTrait<HttpLabelTrait>() |
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.
The overly-restrictive requirement that the member be named HostedZoneId
is removed.
Issue #
#1444 (reusable middleware refactor)
#1446 (apply customization to all operations)
Description of changes
Fix more Route 53 operations
Currently, the "trim hosted zone" customization fixes fourteen AWS Route 53 operations where a string-typed input member named "HostedZoneId" is bound to a URL component.
Integration testing shows that there are six other Route 53 operations that require the same customization, but do not have it applied because they carry a hosted zone ID in a field not named "HostedZoneId".
This PR performs the "trim hosted zone" customization for six additional Route 53 operations where a HostedZone is used in the URL but the name of the property is something other than "HostedZoneId".
Reusable middleware refactor
Currently, a custom middleware is rendered for each of the 14 operations that require the "trim hosted zone" customization.
Instead, a single reusable middleware is provided, and it is inserted into the operation stack for the operations that need it. A key path is passed into the middleware to account for the varying names of the field containing hosted zone ID.
This saves 500 lines of generated code by not generating 20 custom middlewares. A regenerated Route 53 service client is included in this PR for reference.
Integration Test
An integration test is added which uses the
createHostedZone
,changeResourceRecordSets
, anddeleteHostedZone
operations to manipulate live Route 53. This test fails if the fix to the customization above is not applied becausedeleteHostedZone
takes the hosted zone ID in a field not named "HostedZoneId". With the fix applied, the test passes.New/existing dependencies impact assessment, if applicable
No new dependencies were added to this change.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.