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.

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://github.com/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.

…0622) ### Issue # (if applicable) None ### Reason for this change Unlike the other integrations, `WebSocketMockIntegration` did not have a props interface, and was missing a few properties. This PR does not include integration responses, they are already being implemented in #29661 ### Description of changes * Added `requestTemplates` and `templateSelectionExpression` to the newly created `WebSocketMockIntegrationProps` ### Description of how you validated changes Unit and integration tests were updated ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2025-01-16T00:10:35Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2025-02-02T00:11:24Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2025-02-02T00:11:43Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

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

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

Merged

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

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Feb 2, 2025

aws-cdk-automation closed this Feb 2, 2025

github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2025

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

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 commented Jun 21, 2024

nmussy commented Jun 22, 2024

aws-cdk-automation commented Jul 6, 2024

pahud commented Sep 25, 2024

aws-cdk-automation commented Jan 16, 2025

aws-cdk-automation commented Feb 2, 2025

github-actions bot commented Feb 2, 2025

		// 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

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

aws-cdk-automation commented Jan 16, 2025

aws-cdk-automation commented Feb 2, 2025

github-actions bot commented Feb 2, 2025

feat(apigateway): `WebSocketIntegrationResponse` implementation #29661

feat(apigateway): `WebSocketIntegrationResponse` implementation #29661

nmussy commented Mar 31, 2024 •

edited

Loading