feat(apigateway): minCompressionSize on SpecRestApi #24067

pattasai · 2023-02-08T13:57:17Z

This PR adds minCompressionSize to SpecRestApi, which allows compression on api's payload. Within SpecRestApi minComppresssionSize: Size is the additional prop added to SpecRestApiProps to support minCompressionSize which takes class Size, the value of how much the payload needs to be compressed.

This PR also adds minCompressionSize to RestApi which supports Size class e.g minComppresssionSize: Size and deprecates minimumCompressionSize which has number as a type.

For example,

const api = new apigw.RestApi(stack, 'RestApi', {
      minCompressionSize: Size.bytes(1024),
    });

compression for the payload would be 1024 bytes which is equivalent to 1 kibibyte.

Closes #22926.

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

gitpod-io · 2023-02-08T13:57:23Z

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.

TheRealAmazonKendra

PR Linter changes needed. Once updated, it will also ask for integ tests and a readme update.

rix0rrr · 2023-02-08T15:53:01Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

+ *
+ * @default - Compression is disabled.
+ */
+ readonly minimumCompressionSize?: number;


Have you considered using the Size type here?

+1 on using the Size type.

Also, just out of curiosity, if a user inputs a number higher than the maximum here, what is the user experience? If the failure message sent back from CloudFormation or the service isn't clear, we may want to add our own validation for the maximum value with a clear error message.

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

rix0rrr · 2023-02-20T14:33:55Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

- readonly minimumCompressionSize?: number;
+ readonly minCompressionSize?: Size;


I know this is morally ~the same, but I would have expected you to leave the old property in place and @deprecate it, and then add a new block of text with the new property.

In any case I think you'd like to adjust the doc comment for the new property. For example, it's no longer an "integer".

rix0rrr · 2023-02-20T14:34:36Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -216,6 +216,14 @@ export interface RestApiProps extends RestApiOptions {
 */
 readonly binaryMediaTypes?: string[];

+ /**
+ * Deprecated


Both properties need to be documented, even if they are deprecated.

rix0rrr · 2023-02-20T14:35:17Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -261,6 +269,18 @@ export interface SpecRestApiProps extends RestApiBaseProps {
 * @see https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-import-api.html
 */
 readonly apiDefinition: ApiDefinition;
+
+ /**
+ * A nullable integer that is used to enable compression (with non-negative


Not an integer.

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

rix0rrr · 2023-02-20T14:37:01Z

packages/@aws-cdk/aws-apigateway/test/restapi.test.ts

- const imported = apigw.RestApi.fromRestApiId(stack, 'imported-api', 'api-rxt4498f');
+ const stack = new Stack(app);
+ const api = new apigw.RestApi(stack, 'RestApi', {
+ minCompressionSize: Size.bytes(1024),


Please add a test to make sure that minimumCompressionSize also still works.

You might need to use testDeprecated().

Also users shouldn't be able to set both properties.

@pattasai I don't see where the second part of this was addressed. I would expect to see an error thrown if the user specifies minCompressionSize and minimumCompressionSize, and a test to assert that that behavior happens.

☝️ What Kaizen said. Now users are able to go:

new RestApi(..., { minimumCompressionSize: 666, minCompressionSize: Size.bytes(1024), });

What do you think should happen in that case?

rix0rrr · 2023-02-22T08:38:10Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -225,9 +225,22 @@ export interface RestApiProps extends RestApiOptions {
 * payload size.
 *
 * @default - Compression is disabled.
+ * @deprecated - superseded by `minCompressionSize`


👨‍🍳👌

rix0rrr · 2023-02-22T08:39:38Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -763,7 +789,7 @@ export class RestApi extends RestApiBase {
 description: props.description,
 policy: props.policy,
 failOnWarnings: props.failOnWarnings,
- minimumCompressionSize: props.minimumCompressionSize,
+ minimumCompressionSize: props.minCompressionSize?.toBytes() || props.minimumCompressionSize,


For reasons (gestures wildly), this will not what you expect it to do when you pass in Size.bytes(0).

Try it first in a new unit test

Then read about it

Then replace it with:

Suggested change

minimumCompressionSize: props.minCompressionSize?.toBytes() || props.minimumCompressionSize,

minimumCompressionSize: props.minCompressionSize?.toBytes() ?? props.minimumCompressionSize,

rix0rrr · 2023-02-22T08:41:01Z

packages/@aws-cdk/aws-apigateway/test/restapi.test.ts

- test('fromRestApiAttributes() with restApiName', () => {
- // GIVEN
- const stack = new Stack();
+ testDeprecated('RestApi minimumCompressionSize', () => {


👍 thank you!

rix0rrr · 2023-02-22T08:41:43Z

packages/@aws-cdk/aws-apigateway/test/restapi.test.ts

- // WHEN
- const api = new apigw.SpecRestApi(stack, 'api', {
- apiDefinition: apigw.ApiDefinition.fromInline({ foo: 'bar' }),
- endpointTypes: [apigw.EndpointType.EDGE, apigw.EndpointType.PRIVATE],
- });
+ test('fromRestApiAttributes() with restApiName', () => {
+ // GIVEN
+ const stack = new Stack();


Why does so much of this other code show up as changed?

rix0rrr · 2023-02-22T08:42:09Z

packages/@aws-cdk/aws-apigateway/test/restapi.test.ts

+ Template.fromStack(stack).resourceCountIs('AWS::ApiGateway::Account', 0);
+ });
+
+ test('SpecRestApi minimumCompressionSize', () => {


rix0rrr · 2023-02-22T08:44:20Z

packages/@aws-cdk/aws-apigateway/test/restapi.test.ts

- const imported = apigw.RestApi.fromRestApiId(stack, 'imported-api', 'api-rxt4498f');
+ const stack = new Stack(app);
+ const api = new apigw.RestApi(stack, 'RestApi', {
+ minCompressionSize: Size.bytes(1024),


☝️ What Kaizen said. Now users are able to go:

new RestApi(..., { minimumCompressionSize: 666, minCompressionSize: Size.bytes(1024), });

What do you think should happen in that case?

rix0rrr · 2023-02-23T10:14:10Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -758,12 +784,16 @@ export class RestApi extends RestApiBase {
 constructor(scope: Construct, id: string, props: RestApiProps = { }) {
 super(scope, id, props);

+ if (props.minCompressionSize?.toBytes()! >= 0 && props.minimumCompressionSize! >= 0) {


Suggested change

if (props.minCompressionSize?.toBytes()! >= 0 && props.minimumCompressionSize! >= 0) {

if (props.minCompressionSize !== undefined && props.minimumCompressionSize !== undefined) {

stale

mergify · 2023-03-01T15:27:47Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-03-01T16:09:06Z

AWS CodeBuild CI Report

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

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

mergify · 2023-03-01T16:09:38Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This PR adds minCompressionSize to SpecRestApi, which allows compression on api's payload. Within SpecRestApi ``` minComppresssionSize: Size``` is the additional prop added to SpecRestApiProps to support minCompressionSize which takes class Size, the value of how much the payload needs to be compressed. This PR also adds minCompressionSize to RestApi which supports Size class e.g ``` minComppresssionSize: Size``` and deprecates minimumCompressionSize which has number as a type. For example, ```ts const api = new apigw.RestApi(stack, 'RestApi', { minCompressionSize: Size.bytes(1024), }); ``` compression for the payload would be 1024 bytes which is equivalent to 1 kibibyte. Closes aws#22926. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

MinimumCompressionSize prop to SpecRestApi

eb774c1

github-actions bot added the p2 label Feb 8, 2023

aws-cdk-automation requested a review from a team February 8, 2023 13:57

mergify bot added the contribution/core This is a PR that came from AWS. label Feb 8, 2023

aws-cdk-automation previously requested changes Feb 8, 2023

View reviewed changes

TheRealAmazonKendra previously requested changes Feb 9, 2023

View reviewed changes

rix0rrr requested changes Feb 9, 2023

View reviewed changes

pattasai changed the title ~~Setting MinimumCompressionSize prop~~ Supporting Setting MinimumCompressionSize on SpecRestApi Feb 9, 2023

pattasai changed the title ~~Supporting Setting MinimumCompressionSize on SpecRestApi~~ feat(aws-apigateway): Support Setting MinimumCompressionSize on SpecRestApi Feb 10, 2023

pattasai changed the title ~~feat(aws-apigateway): Support Setting MinimumCompressionSize on SpecRestApi~~ feat(apigateway): support setting minimumCompressionSize Feb 10, 2023

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Feb 11, 2023

pattasai added 2 commits February 15, 2023 10:20

deprecated minimumCompressionSize

d2ce75d

fixed error minimumCompressionSize

2ea8ec7

pattasai force-pushed the pattasai-MinimumCompressionSize-SpecRestApi branch from 0c7097e to 2ea8ec7 Compare February 15, 2023 15:21

pattasai and others added 3 commits February 15, 2023 11:57

Merge branch 'main' into pattasai-MinimumCompressionSize-SpecRestApi

52884e9

Merge branch 'main' into pattasai-MinimumCompressionSize-SpecRestApi

54e254f

Merge branch 'main' into pattasai-MinimumCompressionSize-SpecRestApi

ac12230

rix0rrr added the pr-linter/exempt-readme The PR linter will not require README changes label Feb 16, 2023

Integ-test

6a556a9

rix0rrr requested changes Feb 20, 2023

View reviewed changes

creating testDeprecated

c5d4c58

rix0rrr requested changes Feb 22, 2023

View reviewed changes

pattasai added 2 commits February 22, 2023 07:43

additional test cases

ca26883

newtestcases

54d9345

rix0rrr changed the title ~~feat(apigateway): support setting minimumCompressionSize~~ feat(apigateway): minCompressionSize on SpecRestApi Feb 23, 2023

rix0rrr requested changes Feb 23, 2023

View reviewed changes

restapichanges

f6f1f12

rix0rrr approved these changes Mar 1, 2023

View reviewed changes

Merge branch 'main' into pattasai-MinimumCompressionSize-SpecRestApi

9b9ea6c

mergify bot merged commit 2a81f0f into main Mar 1, 2023

mergify bot deleted the pattasai-MinimumCompressionSize-SpecRestApi branch March 1, 2023 16:09

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): minCompressionSize on SpecRestApi #24067

feat(apigateway): minCompressionSize on SpecRestApi #24067

pattasai commented Feb 8, 2023 •

edited

Loading

gitpod-io bot commented Feb 8, 2023

aws-cdk-automation left a comment

TheRealAmazonKendra left a comment

rix0rrr Feb 8, 2023

TheRealAmazonKendra Feb 11, 2023

rix0rrr Feb 20, 2023

rix0rrr Feb 20, 2023

rix0rrr Feb 20, 2023

rix0rrr Feb 20, 2023

rix0rrr Feb 20, 2023

kaizencc Feb 21, 2023

rix0rrr Feb 22, 2023

rix0rrr Feb 22, 2023

rix0rrr Feb 22, 2023

rix0rrr Feb 22, 2023

rix0rrr Feb 22, 2023

rix0rrr Feb 22, 2023

rix0rrr Feb 22, 2023

rix0rrr Feb 23, 2023

mergify bot commented Mar 1, 2023

aws-cdk-automation commented Mar 1, 2023

mergify bot commented Mar 1, 2023

		readonly minimumCompressionSize?: number;
		readonly minCompressionSize?: Size;

	minimumCompressionSize: props.minCompressionSize?.toBytes() \|\| props.minimumCompressionSize,
	minimumCompressionSize: props.minCompressionSize?.toBytes() ?? props.minimumCompressionSize,

	if (props.minCompressionSize?.toBytes()! >= 0 && props.minimumCompressionSize! >= 0) {
	if (props.minCompressionSize !== undefined && props.minimumCompressionSize !== undefined) {

feat(apigateway): minCompressionSize on SpecRestApi #24067

feat(apigateway): minCompressionSize on SpecRestApi #24067

Conversation

pattasai commented Feb 8, 2023 • edited Loading

gitpod-io bot commented Feb 8, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 1, 2023

aws-cdk-automation commented Mar 1, 2023

AWS CodeBuild CI Report

mergify bot commented Mar 1, 2023

pattasai commented Feb 8, 2023 •

edited

Loading