Skip to content
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.

Sign up for GitHub

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(aws-apigateway-sqs): update construct to allow custom path params #1079

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(aws-apigateway-sqs): update construct to allow custom path params #1079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion source/patterns/@aws-solutions-constructs/aws-apigateway-sqs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.|
Copy link
Contributor

@biffgaut biffgaut Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

|createRequestTemplate?|`string`|API Gateway Request Template for the create method for the default `application/json` content-type. This property is required if the `allowCreateOperation` property is set to true.|
|additionalCreateRequestTemplates?|`{ [contentType: string]: string; }`|Optional Create Request Templates for content-types other than `application/json`. Use the `createRequestTemplate` property to set the request template for the `application/json` content-type. This property can only be specified if the `allowCreateOperation` property is set to true.|
|createIntegrationResponses?|[`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 create method. This property can only be specified if the `allowCreateOperation` property is set to true.|
|allowReadOperation?|`boolean`|Whether to deploy an API Gateway Method for GET HTTP operations on the queue (i.e. sqs:ReceiveMessage).|
|readRequestPath?|`string`| Optional, user provided request path for GET HTTP operations.|
|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).|
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapped DELETE AND HTTP to be consistent with the GET and POST documentation

|deleteRequestPath?|`string`| Optional, user provided request path for DELETE HTTP operations.|
|deleteRequestTemplate?|`string`|API Gateway Request Template for THE delete method for the default `application/json` content-type. This property can only be specified if the `allowDeleteOperation` property is set to true.|
|additionalDeleteRequestTemplates?|`{ [contentType: string]: string; }`|Optional Delete request templates for content-types other than `application/json`. Use the `deleteRequestTemplate` property to set the request template for the `application/json` content-type. This property can only be specified if the `allowDeleteOperation` property is set to true.|
|deleteIntegrationResponses?|[`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 delete method. This property can only be specified if the `allowDeleteOperation` property is set to true.|
Expand Down
43 changes: 33 additions & 10 deletions source/patterns/@aws-solutions-constructs/aws-apigateway-sqs/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,27 @@ export interface ApiGatewayToSqsProps {
* @default - None
*/
readonly encryptionKeyProps?: kms.KeyProps;
/**
* Optional, custom API Gateway path for the GET method.
* This property can only be specified if the `allowReadOperation` property is not set to false.
*
* @default - ""
*/
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

*
* @default - ""
*/
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure you don't mean 'specif' OR 'ied' here and that this is a typo?

*
* @default - "message"
*/
readonly deleteRequestPath?: string;
}

/**
Expand Down Expand Up @@ -200,15 +221,15 @@ export class ApiGatewayToSqs extends Construct {

if (this.CheckCreateRequestProps(props)) {
throw new Error(`The 'allowCreateOperation' property must be set to true when setting any of the following: ` +
`'createRequestTemplate', 'additionalCreateRequestTemplates', 'createIntegrationResponses'`);
`'createRequestTemplate', 'additionalCreateRequestTemplates', 'createIntegrationResponses', 'createRequestPath'`);
}
if (this.CheckReadRequestProps(props)) {
throw new Error(`The 'allowReadOperation' property must be set to true or undefined when setting any of the following: ` +
`'readRequestTemplate', 'additionalReadRequestTemplates', 'readIntegrationResponses'`);
`'readRequestTemplate', 'additionalReadRequestTemplates', 'readIntegrationResponses', 'readRequestPath'`);
}
if (this.CheckDeleteRequestProps(props)) {
throw new Error(`The 'allowDeleteOperation' property must be set to true when setting any of the following: ` +
`'deleteRequestTemplate', 'additionalDeleteRequestTemplates', 'deleteIntegrationResponses'`);
`'deleteRequestTemplate', 'additionalDeleteRequestTemplates', 'deleteIntegrationResponses', 'deleteRequestPath'`);
}

// Setup the dead letter queue, if applicable
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

this.addActionToPolicy("sqs:SendMessage");
defaults.addProxyMethodToApiResource({
service: "sqs",
path: `${cdk.Aws.ACCOUNT_ID}/${this.sqsQueue.queueName}`,
apiGatewayRole: this.apiGatewayRole,
apiMethod: "POST",
apiResource: this.apiGateway.root,
apiResource: apiCreateRequestResource,
requestTemplate: createRequestTemplate,
additionalRequestTemplates: props.additionalCreateRequestTemplates,
contentType: "'application/x-www-form-urlencoded'",
Expand All @@ -261,13 +283,14 @@ export class ApiGatewayToSqs extends Construct {
// Read
const readRequestTemplate = props.readRequestTemplate ?? this.defaultReadRequestTemplate;
if (props.allowReadOperation === undefined || props.allowReadOperation === true) {
const apiReadRequestResource = props.readRequestPath ? this.apiGateway.root.addResource(props.readRequestPath) : this.apiGateway.root;
this.addActionToPolicy("sqs:ReceiveMessage");
defaults.addProxyMethodToApiResource({
service: "sqs",
path: `${cdk.Aws.ACCOUNT_ID}/${this.sqsQueue.queueName}`,
apiGatewayRole: this.apiGatewayRole,
apiMethod: "GET",
apiResource: this.apiGateway.root,
apiResource: apiReadRequestResource,
requestTemplate: readRequestTemplate,
additionalRequestTemplates: props.additionalReadRequestTemplates,
contentType: "'application/x-www-form-urlencoded'",
Expand All @@ -278,14 +301,14 @@ export class ApiGatewayToSqs extends Construct {
// Delete
const deleteRequestTemplate = props.deleteRequestTemplate ?? this.defaultDeleteRequestTemplate;
if (props.allowDeleteOperation && props.allowDeleteOperation === true) {
const apiGatewayResource = this.apiGateway.root.addResource('message');
const apiDeleteRequestResource = this.apiGateway.root.addResource(props.deleteRequestPath ?? 'message');
this.addActionToPolicy("sqs:DeleteMessage");
defaults.addProxyMethodToApiResource({
service: "sqs",
path: `${cdk.Aws.ACCOUNT_ID}/${this.sqsQueue.queueName}`,
apiGatewayRole: this.apiGatewayRole,
apiMethod: "DELETE",
apiResource: apiGatewayResource,
apiResource: apiDeleteRequestResource,
requestTemplate: deleteRequestTemplate,
additionalRequestTemplates: props.additionalDeleteRequestTemplates,
contentType: "'application/x-www-form-urlencoded'",
Expand All @@ -294,22 +317,22 @@ export class ApiGatewayToSqs extends Construct {
}
}
private CheckReadRequestProps(props: ApiGatewayToSqsProps): boolean {
if ((props.readRequestTemplate || props.additionalReadRequestTemplates || props.readIntegrationResponses)
if ((props.readRequestTemplate || props.additionalReadRequestTemplates || props.readIntegrationResponses || props.readRequestPath)
&& props.allowReadOperation === false) {
return true;
}
return false;
}
private CheckDeleteRequestProps(props: ApiGatewayToSqsProps): boolean {
if ((props.deleteRequestTemplate || props.additionalDeleteRequestTemplates || props.deleteIntegrationResponses)
if ((props.deleteRequestTemplate || props.additionalDeleteRequestTemplates || props.deleteIntegrationResponses || props.deleteRequestPath)
&& props.allowDeleteOperation !== true) {
return true;
}
return false;
}

private CheckCreateRequestProps(props: ApiGatewayToSqsProps): boolean {
if ((props.createRequestTemplate || props.additionalCreateRequestTemplates || props.createIntegrationResponses)
if ((props.createRequestTemplate || props.additionalCreateRequestTemplates || props.createIntegrationResponses || props.createRequestPath)
&& props.allowCreateOperation !== true) {
return true;
}
Expand Down
41 changes: 40 additions & 1 deletion source/patterns/@aws-solutions-constructs/aws-apigateway-sqs/test/apigateway-sqs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ test('Test properties', () => {
deployDeadLetterQueue: true,
maxReceiveCount: 3
});
// Assertion 1
// Assertion 1
expect(pattern.apiGateway).toBeDefined();
// Assertion 2
expect(pattern.sqsQueue).toBeDefined();
Expand Down Expand Up @@ -163,6 +163,45 @@ test('Test deployment for override ApiGateway deleteRequestTemplate', () => {
});
});

test('Test deployment for override ApiGateway createRequestPath', () => {
const stack = new Stack();

new ApiGatewayToSqs(stack, 'api-gateway-sqs', {
createRequestPath: "testPath",
allowCreateOperation: true
});
const template = Template.fromStack(stack);
template.hasResourceProperties('AWS::ApiGateway::Resource', {
PathPart: "testPath"
});
});

test('Test deployment for override ApiGateway getRequestPath', () => {
const stack = new Stack();

new ApiGatewayToSqs(stack, 'api-gateway-sqs', {
readRequestPath: "testPath",
allowReadOperation: true
});
const template = Template.fromStack(stack);
template.hasResourceProperties('AWS::ApiGateway::Resource', {
PathPart: "testPath"
});
});

test('Test deployment for for override ApiGateway deleteRequestTemplate', () => {
const stack = new Stack();

new ApiGatewayToSqs(stack, 'api-gateway-sqs', {
allowDeleteOperation: true,
deleteRequestPath: "testPath"
});
const template = Template.fromStack(stack);
template.hasResourceProperties('AWS::ApiGateway::Resource', {
PathPart: "testPath"
});
});

test('Test deployment for disallow delete operation', () => {
const stack = new Stack();

Expand Down