feat(aws-apigateway-sqs): update construct to allow custom path params #1079
Conversation
dcustic
requested review from
biffgaut,
hayesry,
mobri2a,
emcfins,
mickychetta and
georgebearden
as code owners
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…s for CRD operations on the SQS
| |readRequestTemplate?|`string`|API Gateway Request Template for the read method for the default `application/json` content-type.| | ||
| |additionalReadRequestTemplates?|`{ [contentType: string]: string; }`|Optional Read Request Templates for content-types other than `application/json`. Use the `readRequestTemplate` property to set the request template for the `application/json` content-type.| | ||
| |readIntegrationResponses?|[`api.IntegrationResponses[]`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_apigateway.IntegrationResponse.html)|Optional, custom API Gateway Integration Response for the read method.| | ||
| |allowDeleteOperation?|`boolean`|Whether to deploy an API Gateway Method for HTTP DELETE operations on the queue (i.e. sqs:DeleteMessage).| | ||
| |allowDeleteOperation?|`boolean`|Whether to deploy an API Gateway Method for DELETE HTTP operations on the queue (i.e. sqs:DeleteMessage).| |
There was a problem hiding this comment.
Swapped DELETE AND HTTP to be consistent with the GET and POST documentation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thanks for the contribution - we're excited to review it, but won't be able to until Thursday morning. |
Thanks for the heads up. Cheers :) |
| readonly readRequestPath?: string; | ||
| /** | ||
| * Optional, custom API Gateway path for the POST method. | ||
| * This property can only be specified if the `allowCreateOperation` property is not set to false. |
There was a problem hiding this comment.
Since the allowDeleteOperation and allowCreateOperation attributes default to false, it seems the wording for these would be better as "This property can only be specified if the `allowCreateOperation' is set to true."
| readonly createRequestPath?: string; | ||
| /** | ||
| * Optional, custom API Gateway path for the DELETE method. | ||
| * This property can only be specif||ied if the `allowDeleteOperation` property is not set to false. |
There was a problem hiding this comment.
Pretty sure you don't mean 'specif' OR 'ied' here and that this is a typo?
| @@ -244,13 +265,14 @@ export class ApiGatewayToSqs extends Construct { | |||
| // Create | |||
| const createRequestTemplate = props.createRequestTemplate ?? this.defaultCreateRequestTemplate; | |||
| if (props.allowCreateOperation && props.allowCreateOperation === true) { | |||
| const apiCreateRequestResource = props.createRequestPath ? this.apiGateway.root.addResource(props.createRequestPath) : this.apiGateway.root; | |||
There was a problem hiding this comment.
The first thing I tried was creating a new integration test with the following:
new ApiGatewayToSqs(stack, 'test-api-gateway-sqs-default', {
allowCreateOperation: true,
allowDeleteOperation: true,
createRequestPath: 'my-resource',
deleteRequestPath: 'my-resource',
I think think a single resource for a workload, using the HTTP method to distinguish between operations, is a reasonable expectation of a client. While it's going to be non-trivial, it seems worthwhile to sort out the required resources prior to line 266. addResource will return the resulting resource and we would then call addMethod for additional operations.
(I did not write this test anticipating this issue, I was lazy and reused the resource name :-)
An alternative would be specifying a single, optional resource name that would be used across all implemented HTTP methods. The more I think about this, the more it appeals to me. Using different resource names for different HTTP methods on the same queue does not seem like a design we would want to encourage (and shouldn't have implemented ourselves with the 'message' resource).
What do you think?
| @@ -63,14 +63,17 @@ new ApiGatewayToSqs(this, "ApiGatewayToSqsPattern", new ApiGatewayToSqsProps.Bui | |||
| |deployDeadLetterQueue?|`boolean`|Whether to deploy a secondary queue to be used as a dead letter queue. Defaults to `true`.| | |||
| |maxReceiveCount|`number`|The number of times a message can be unsuccessfully dequeued before being moved to the dead-letter queue.| | |||
| |allowCreateOperation?|`boolean`|Whether to deploy an API Gateway Method for POST HTTP operations on the queue (i.e. sqs:SendMessage).| | |||
| |createRequestPath?|`string`| Optional, user provided request path for POST HTTP operations.| | |||
There was a problem hiding this comment.
I think we should call these attributes resources rather than paths (createRequestResource, readRequestResource, etc.). We only allow one level deep - if someone tries to specify a valid "path" name of "resource/subresource" the stack will fail to launch because this is an invalid resource name.
You did just fine. Some issues did pop up in review that you will read about, but the issues were surprises to us as well. |
…for GET, POST, DELETE operations on the SQS
Add 3 new optional parameters to ApiGatewayToSqsProps
With those the user can change the endpoints for GET, POST, DELETE message of the SQS
I've read that one concern was backwards compatibility
This solution should backwards compatible, because if the properties are not set, the fallback will be the previous behaviour.
Small PSA: this is my first Open Source PR, so bear with me please. I'm open to any suggestions/requests to learn how to do a propper OpenSource PR.