-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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(apigateway): minCompressionSize on SpecRestApi #24067
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Linter changes needed. Once updated, it will also ask for integ tests and a readme update.
* | ||
* @default - Compression is disabled. | ||
*/ | ||
readonly minimumCompressionSize?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using the Size type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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.
0c7097e
to
2ea8ec7
Compare
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
readonly minimumCompressionSize?: number; | ||
readonly minCompressionSize?: Size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
@@ -216,6 +216,14 @@ export interface RestApiProps extends RestApiOptions { | |||
*/ | |||
readonly binaryMediaTypes?: string[]; | |||
|
|||
/** | |||
* Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both properties need to be documented, even if they are deprecated.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an integer.
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to make sure that minimumCompressionSize
also still works.
You might need to use testDeprecated()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also users shouldn't be able to set both properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ 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?
@@ -225,9 +225,22 @@ export interface RestApiProps extends RestApiOptions { | |||
* payload size. | |||
* | |||
* @default - Compression is disabled. | |||
* @deprecated - superseded by `minCompressionSize` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👨🍳👌
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
minimumCompressionSize: props.minCompressionSize?.toBytes() || props.minimumCompressionSize, | |
minimumCompressionSize: props.minCompressionSize?.toBytes() ?? props.minimumCompressionSize, |
test('fromRestApiAttributes() with restApiName', () => { | ||
// GIVEN | ||
const stack = new Stack(); | ||
testDeprecated('RestApi minimumCompressionSize', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thank you!
// 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does so much of this other code show up as changed?
Template.fromStack(stack).resourceCountIs('AWS::ApiGateway::Account', 0); | ||
}); | ||
|
||
test('SpecRestApi minimumCompressionSize', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ 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?
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (props.minCompressionSize?.toBytes()! >= 0 && props.minimumCompressionSize! >= 0) { | |
if (props.minCompressionSize !== undefined && props.minimumCompressionSize !== undefined) { |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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*
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,
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