feat(apigateway): `WebSocketIntegrationResponse` implementation #29661

nmussy · 2024-03-31T14:13:28Z

Issue # (if applicable)

None as far as I could tell, related to #29562.

Reason for this change

While it is possible to use the L1 CfnIntegrationResponse construct, it's not trivial given the WebSocketRouteIntegration are currently bound to the WebSocketIntegration on the fly.

Description of changes

Added a basic L2 WebSocketIntegrationResponse construct
- Gave two-way WebSocketIntegrations (capable of setting IntegrationResponse) a responses config prop, as well as a addResponse method. This allows me to check that there are no repeat responseKeys, and that returnResponse is active if there are responses set
- An intermediate CustomResponseWebSocketRoute abstract class was created to isolate WebSocketLambdaIntegration, which does not support response customization
Added a WebSocketIntegrationResponseKey helper class to access common and to generate custom responseKeys

Description of how you validated changes

Unit tests were added/modified, and existing integration files were extended to include responses

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…tion

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

nmussy · 2024-03-31T14:16:27Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration.ts

+ // FIXME change to a warning?
+ throw new Error('Setting up integration responses without setting up returnResponse to true will have no effect, and is likely a mistake.');


Not sure which is best here, as far as I can tell the web console (and thus I assume Cfn) allows the user to create responses, with a simple notice:

It does feel like quite a big pitfall though, I don't think an exception is unwarranted

I feel like a warning would be more suitable here if it's something that's possible to do from the web console, is there a very strong reason you think we should throw an exception instead?

I can see a stronger reason to allow this in the web console, say if you want to add responses first, before enabling returnResponse. With IaC, you can just block comment both the integration responses and the property. I don't mind just showing a warning, an exception just seemed like it would save people time debugging something that could could be easily missed at deploy time

Rather than doing this validation after the fact, we should structure these inputs so that they are tightly coupled and so that users can't do this.

Ultimately, what I think should be done here is to deprecate the returnResponse field and update it to response that takes in a enum like class with two options: DEFAULT, which is what it's doing now, and customResponse() that is a function that creates all the stuff you're creating in this PR. What do you think?

nmussy · 2024-03-31T14:17:18Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/route.ts

- /**
- * Integration response ID
- */
- public readonly integrationResponseId?: string;
-


I assume this was added in anticipation of an implementation of integration responses, but serves no purpose and cannot store multiple responses

Technically, we need to deprecate this, not delete it. While it doesn't actually do anything, the removal is a breaking change. This is a stable module so we can't allow it.

Also, can we get access to this id in this scope? If so, why not just set it?

My main issue is that there can be multiple integrationResponseId, it would need to be a string[] to be usable. I don't think setting it to the first value is worth the confusion. I can leave it up and just @deprecrate it, but again, I think it's needlessly confusing given it was never implemented

nmussy · 2024-03-31T14:18:05Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration.ts

+ // FIXME any better way to generate a unique id?
+ Names.nodeUniqueId(this.integration.node) + slugify(responseProps.responseKey.key) + 'IntegrationResponse',
+ { ...responseProps, integration: this.integration },
+ );


Unsure this is the best way to generate this ID, feedback would be appreciated

nmussy · 2024-04-01T14:25:55Z

AWS_PROXY, implemented as WebSocketLambdaIntegration, is the only currently available integration type that does not support IntegrationResponse:

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Co-authored-by: paulhcsun <[email protected]>

Pull request has been modified.

Co-authored-by: paulhcsun <[email protected]>

…ebsocket-integration-response

TheRealAmazonKendra

Thank you for your contribution! This is a great start! I have several comments inline where I think we can make this experience a little more strongly typed and a bit cleaner for the user, especially if the user is working in one of our other supported target languages.

packages/aws-cdk-lib/aws-apigatewayv2-integrations/README.md

TheRealAmazonKendra · 2024-06-21T18:11:58Z

packages/aws-cdk-lib/aws-apigatewayv2-integrations/lib/websocket/mock.ts

+ * @default - No request template provided to the integration.
+ * @see https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api-mapping-template-reference.html
+ */
+ readonly requestTemplates?: { [contentType: string]: string };


I think we could make this more strongly typed. From the link you added, we know all of the valid parameters a user can add here and some of the valid values for those

I'm not sure what you mean by improving the type strength, given the key can be any MIME type and the value can be any string, which may contain one of the linked parameters.

We could create a builder/helper class to generate those values but that seems like a bit of overkill. An parameter enum would also be insufficient given some of them are functions, e.g. $input.json('$.pets')

Moving this discussion to https:/aws/aws-cdk/pull/30622/files#r1649600512

TheRealAmazonKendra · 2024-06-21T18:16:42Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration-response.ts

+ *
+ * @default - No response parameters
+ */
+ readonly responseParameters?: { [key: string]: string };


There's a lot of description above of how the user must format these and which values they can be. Can we provide any further constraints here or do some of the formatting for them so that they aren't on their own here?

TheRealAmazonKendra · 2024-06-21T18:19:05Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration.ts

+ // FIXME change to a warning?
+ throw new Error('Setting up integration responses without setting up returnResponse to true will have no effect, and is likely a mistake.');


Rather than doing this validation after the fact, we should structure these inputs so that they are tightly coupled and so that users can't do this.

TheRealAmazonKendra · 2024-06-21T18:21:28Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration.ts

+ new WebSocketIntegrationResponse(
+ options.scope,
+ // FIXME any better way to generate a unique id?
+ Names.nodeUniqueId(this.integration.node) + slugify(responseProps.responseKey.key) + 'IntegrationResponse',


I would use Names.uniqueResourceName instead. It's less likely to cause collisions and to ensure the name has the proper length. This naming could result in names being too long and failing at deployment time.

TheRealAmazonKendra · 2024-06-21T20:45:38Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/route.ts

- /**
- * Integration response ID
- */
- public readonly integrationResponseId?: string;
-


Also, can we get access to this id in this scope? If so, why not just set it?

TheRealAmazonKendra · 2024-06-21T20:49:19Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration.ts

+ * The abstract class that two-way communication route integration classes
+ * with customized responses will implement.
+ */
+export abstract class CustomResponseWebSocketRoute extends WebSocketRouteIntegration {


Is this necessary? Are there WebSocketRouteIntegration types that can't take in a custom response?

Yes, WebSocketLambdaIntegration is the only integration that does not support custom responses, see #29661 (comment)

TheRealAmazonKendra · 2024-06-21T20:52:17Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration-response.ts

+ *
+ * @default - No response templates
+ */
+ readonly responseTemplates?: { [contentType: string]: string };


What are the valid content types? Rather than this being a key value pair, can we make this more strongly typed? Like maybe a class that is ResponseTemplate.applicationJson(...)and then the parameter is responseTemplates: ResponseTemplate[]?

TheRealAmazonKendra · 2024-06-21T20:58:19Z

packages/aws-cdk-lib/aws-apigatewayv2-integrations/README.md

+
+integration.addResponse({ 
+ // Success response key, will match all 2xx response HTTP status codes
+ responseKey: apigwv2.WebSocketIntegrationResponseKey.success,


Instead of having this here, why not make the first input responseKey and then make the rest of them props? it makes it a little cleaner for the user.

I'd used the same pattern and interface as the responses prop in the Config, but you're right, this is much cleaner, fixed

TheRealAmazonKendra · 2024-06-21T21:00:03Z

packages/aws-cdk-lib/aws-apigatewayv2/lib/websocket/integration-response.ts

+ *
+ * @see https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api-integration-responses.html
+ */
+export interface InternalWebSocketIntegrationResponseProps {


Instead of making this a props interface, can you make this a class with the first input in the constructor being the responseKey and then the rest props? I think that will be a bit cleaner than the current experience.

TheRealAmazonKendra · 2024-06-21T21:04:52Z

I forgot to add, you added some changes that were actually unrelated to this implementation. Can you please separate those out into a separate PR?

Co-authored-by: Kendra Neil <[email protected]>

Pull request has been modified.

…ebsocket-integration-response

nmussy · 2024-06-22T08:21:42Z

I forgot to add, you added some changes that were actually unrelated to this implementation. Can you please separate those out into a separate PR?

WebSocketMockIntegrationProps.requestTemplates and .templateSelectionExpression were moved to #30622

…ebsocket-integration-response

aws-cdk-automation · 2024-07-06T15:01:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: ddbf57b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

pahud · 2024-09-25T11:30:20Z

This PR is pending for community review now.
Please check https:/aws/aws-cdk/wiki/CDK-Community-PR-Reviews for more details.
We need at least one community approving review before it could bump to pending maintainer review.

nmussy added 10 commits March 29, 2024 17:16

feat: response class (wip)

8e020be

feat: response mapping, lambda unit test

c045d71

chore: throws JSDoc

c5e81cd

chore: remove unused import

aa40b0f

chore: unit tests

25c9485

chore: README wip

7dc2af0

chore: add returnResponse check, refactor WebSocketTwoWayRouteIntegra…

664e1b2

…tion

chore: fix unit tests

45e5cd3

chore: remove unused imports

0b83d03

chore: build fixes

9b34229

github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p2 labels Mar 31, 2024

aws-cdk-automation requested a review from a team March 31, 2024 14:13

aws-cdk-automation previously requested changes Mar 31, 2024

View reviewed changes

nmussy commented Mar 31, 2024

View reviewed changes

nmussy added 4 commits April 1, 2024 16:32

fix: IntegrationResponse type compatibility

8c5432d

chore: add jsdoc

b0bfe57

chore: update AWS integ

38a0d73

chore: add breaking change

1bf091d

nmussy added 7 commits April 1, 2024 22:28

chore: update integ

3a00e50

feat: add missing mock properties

7590d96

chore: update mock integ

70ce981

chore: README update

fa2e68e

fix: WebSocketIntegrationResponse constructor

6655588

chore: docs fixes

5bdc242

chore: refactor CustomResponseWebSocketRoute

0287dc2

docs: punctuation

0774e88

Co-authored-by: paulhcsun <[email protected]>

nmussy and others added 4 commits June 10, 2024 16:41

docs: punctuation

7e0b76d

Co-authored-by: paulhcsun <[email protected]>

chore: missing new line

cdc120d

Merge remote-tracking branch 'upstream/main' into feat-apigatewayv2-w…

fc37366

…ebsocket-integration-response

docs: fix fromStatusRegExp example

99cdd68

nmussy requested a review from paulhcsun June 10, 2024 14:51

TheRealAmazonKendra previously requested changes Jun 21, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 21, 2024

docs: grammar

b64002f

Co-authored-by: Kendra Neil <[email protected]>

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 21, 2024

nmussy added 2 commits June 22, 2024 09:11

Merge remote-tracking branch 'upstream/main' into feat-apigatewayv2-w…

31302b8

…ebsocket-integration-response

revert: extra Mock props

279bac0

nmussy mentioned this pull request Jun 22, 2024

feat(apigatewayv2-integrations): WebSocketMockIntegration props #30622

Open

1 task

nmussy added 11 commits June 25, 2024 15:47

Merge remote-tracking branch 'upstream/main' into feat-apigatewayv2-w…

51ce60a

…ebsocket-integration-response

fix: restore breaking change, deprecate field

32a2aaf

chore: remove nl

747cc08

feat: add common status response keys

f35c28c

chore: refactor addResponse params

62ebc9c

Merge remote-tracking branch 'upstream/main' into feat-apigatewayv2-w…

45066bb

…ebsocket-integration-response

fix: jsii build

4e9a901

fix: use uniqueResourceName to generate integ response id

5cfbb34

docs: fix status description

0d57e25

feat: WebSocketIntegrationResponseKey.fromKeys method

a1b0928

docs: fix rosetta example

ddbf57b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): `WebSocketIntegrationResponse` implementation #29661

feat(apigateway): `WebSocketIntegrationResponse` implementation #29661

nmussy commented Mar 31, 2024 •

edited

Loading

aws-cdk-automation left a comment

nmussy Mar 31, 2024

paulhcsun Jun 6, 2024

nmussy Jun 7, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra Jun 21, 2024

nmussy Mar 31, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra Jun 21, 2024

nmussy Jun 21, 2024

nmussy Mar 31, 2024

nmussy commented Apr 1, 2024

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jun 21, 2024

nmussy Jun 22, 2024

nmussy Jun 22, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra Jun 21, 2024

nmussy Jul 6, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra Jun 21, 2024

nmussy Jun 25, 2024

TheRealAmazonKendra Jun 21, 2024

TheRealAmazonKendra commented Jun 21, 2024

nmussy commented Jun 22, 2024

aws-cdk-automation commented Jul 6, 2024

pahud commented Sep 25, 2024

		// FIXME change to a warning?
		throw new Error('Setting up integration responses without setting up returnResponse to true will have no effect, and is likely a mistake.');

feat(apigateway): WebSocketIntegrationResponse implementation #29661

Are you sure you want to change the base?

feat(apigateway): WebSocketIntegrationResponse implementation #29661

Conversation

nmussy commented Mar 31, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmussy commented Apr 1, 2024

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jun 21, 2024

nmussy commented Jun 22, 2024

aws-cdk-automation commented Jul 6, 2024

AWS CodeBuild CI Report

pahud commented Sep 25, 2024

feat(apigateway): `WebSocketIntegrationResponse` implementation #29661

feat(apigateway): `WebSocketIntegrationResponse` implementation #29661

nmussy commented Mar 31, 2024 •

edited

Loading