feat(apigateway): throw ValidationError instead of untyped errors
#33075
Conversation
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Features must contain a change to a README file.
❌ Features must contain a change to a test file.
❌ Features must contain a change to an integration test file and the resulting snapshot.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33075 +/- ##
=======================================
Coverage 81.52% 81.52%
=======================================
Files 224 224
Lines 13762 13762
Branches 2414 2414
=======================================
Hits 11220 11220
Misses 2270 2270
Partials 272 272
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
i think the idea is for us to use UnscopedValidationError sparingly, and there are a few places where I'm not sure if you exhausted options for adding scope before defaulting to Unscoped. Some of these could be wrong, but it's worrisome that there's so many UnscopedValidationErrors in this PR
| @@ -168,7 +169,7 @@ class LatestDeploymentResource extends CfnDeployment { | |||
| // if the construct is locked, it means we are already synthesizing and then | |||
| // we can't modify the hash because we might have already calculated it. | |||
| if (this.node.locked) { | |||
| throw new Error('Cannot modify the logical ID when the construct is locked'); | |||
| throw new UnscopedValidationError('Cannot modify the logical ID when the construct is locked'); | |||
There was a problem hiding this comment.
why can't this be scoped to this?
| @@ -235,12 +236,12 @@ export class Integration { | |||
| if (vpcLink instanceof VpcLink) { | |||
| const targets = vpcLink._targetDnsNames; | |||
| if (targets.length > 1) { | |||
| throw new Error("'uri' is required when there are more than one NLBs in the VPC Link"); | |||
| throw new UnscopedValidationError("'uri' is required when there are more than one NLBs in the VPC Link"); | |||
There was a problem hiding this comment.
this should be scoped to method, does that not work for some reason?
| } else { | ||
| return `http://${targets[0]}`; | ||
| } | ||
| } else { | ||
| throw new Error("'uri' is required when the 'connectionType' is VPC_LINK"); | ||
| throw new UnscopedValidationError("'uri' is required when the 'connectionType' is VPC_LINK"); |
| @@ -150,7 +151,7 @@ class StepFunctionsExecutionIntegration extends AwsIntegration { | |||
| if (this.stateMachine instanceof sfn.StateMachine) { | |||
| const stateMachineType = (this.stateMachine as sfn.StateMachine).stateMachineType; | |||
| if (stateMachineType !== sfn.StateMachineType.EXPRESS) { | |||
| throw new Error('State Machine must be of type "EXPRESS". Please use StateMachineType.EXPRESS as the stateMachineType'); | |||
| throw new UnscopedValidationError('State Machine must be of type "EXPRESS". Please use StateMachineType.EXPRESS as the stateMachineType'); | |||
| @@ -86,13 +87,13 @@ export class LambdaRestApi extends RestApi { | |||
| } | |||
|
|
|||
| function addResourceThrows(): Resource { | |||
| throw new Error('Cannot call \'addResource\' on a proxying LambdaRestApi; set \'proxy\' to false'); | |||
| throw new UnscopedValidationError('Cannot call \'addResource\' on a proxying LambdaRestApi; set \'proxy\' to false'); | |||
There was a problem hiding this comment.
these 3 methods are not public so we can change them. i think we should just add scope: Construct to the function parameters so these don't have to be unscoped
| @@ -471,7 +472,7 @@ export class Stage extends StageBase { | |||
| if (self.enableCacheCluster === undefined) { | |||
| self.enableCacheCluster = true; | |||
| } else if (self.enableCacheCluster === false) { | |||
| throw new Error(`Cannot enable caching for method ${path} since cache cluster is disabled on stage`); | |||
| throw new UnscopedValidationError(`Cannot enable caching for method ${path} since cache cluster is disabled on stage`); | |||
There was a problem hiding this comment.
we can probably add scope to this method also. are you trying to do this before defaulting to Unscoped?
Issue
aws-apigatewayfor #32569Description of changes
ValidationErrors everywhere
Describe any new or updated permissions being added
n/a
Description of how you validated changes
Existing tests. Exemptions granted as this is basically a refactor of existing code.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license