From 070aa754798a4651a28a9b87808f868417f70aeb Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Tue, 21 Jan 2025 13:37:37 +0000 Subject: [PATCH 1/2] feat(s3): use `ValidationError` --- packages/aws-cdk-lib/.eslintrc.js | 9 +++ packages/aws-cdk-lib/aws-s3/lib/bucket.ts | 55 ++++++++++--------- .../notifications-resource.ts | 13 +++-- packages/aws-cdk-lib/aws-s3/lib/util.ts | 3 +- packages/aws-cdk-lib/core/lib/errors.ts | 19 ++++++- packages/aws-cdk-lib/core/test/errors.test.ts | 12 +++- 6 files changed, 74 insertions(+), 37 deletions(-) diff --git a/packages/aws-cdk-lib/.eslintrc.js b/packages/aws-cdk-lib/.eslintrc.js index 6c873338494e3..f77ca351aac5d 100644 --- a/packages/aws-cdk-lib/.eslintrc.js +++ b/packages/aws-cdk-lib/.eslintrc.js @@ -13,4 +13,13 @@ baseConfig.rules['import/no-extraneous-dependencies'] = [ } ]; + +// no-throw-default-error +const modules = ['aws-s3']; +baseConfig.overrides.push({ + files: modules.map(m => `./${m}/lib/**`), + rules: { "@cdklabs/no-throw-default-error": ['error'] }, +}); + + module.exports = baseConfig; diff --git a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts index 27cb8fc6fde2b..6a94cfaa88596 100644 --- a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts +++ b/packages/aws-cdk-lib/aws-s3/lib/bucket.ts @@ -26,6 +26,7 @@ import { Tokenization, Annotations, } from '../../core'; +import { UnscopedValidationError, ValidationError } from '../../core/lib/errors'; import { CfnReference } from '../../core/lib/private/cfn-reference'; import { AutoDeleteObjectsProvider } from '../../custom-resource-handlers/dist/aws-s3/auto-delete-objects-provider.generated'; import * as cxapi from '../../cx-api'; @@ -869,7 +870,7 @@ export abstract class BucketBase extends Resource implements IBucket { */ public grantPublicAccess(keyPrefix = '*', ...allowedActions: string[]) { if (this.disallowPublicAccess) { - throw new Error("Cannot grant public access when 'blockPublicPolicy' is enabled"); + throw new ValidationError("Cannot grant public access when 'blockPublicPolicy' is enabled", this); } allowedActions = allowedActions.length > 0 ? allowedActions : ['s3:GetObject']; @@ -982,7 +983,7 @@ export abstract class BucketBase extends Resource implements IBucket { })).statementAdded); if (accessControlTransition) { if (!account) { - throw new Error('account must be specified to override ownership access control transition'); + throw new ValidationError('account must be specified to override ownership access control transition', this); } results.push(this.addToResourcePolicy(new iam.PolicyStatement({ actions: ['s3:ObjectOwnerOverrideToBucketOwner'], @@ -1987,7 +1988,7 @@ export class Bucket extends BucketBase { const bucketName = parseBucketName(scope, attrs); if (!bucketName) { - throw new Error('Bucket name is required'); + throw new ValidationError('Bucket name is required', scope); } Bucket.validateBucketName(bucketName, true); @@ -2151,7 +2152,7 @@ export class Bucket extends BucketBase { } if (errors.length > 0) { - throw new Error(`Invalid S3 bucket name (value: ${bucketName})${EOL}${errors.join(EOL)}`); + throw new UnscopedValidationError(`Invalid S3 bucket name (value: ${bucketName})${EOL}${errors.join(EOL)}`); } } @@ -2247,7 +2248,7 @@ export class Bucket extends BucketBase { this.enforceSSLStatement(); this.minimumTLSVersionStatement(props.minimumTLSVersion); } else if (props.minimumTLSVersion) { - throw new Error('\'enforceSSL\' must be enabled for \'minimumTLSVersion\' to be applied'); + throw new ValidationError('\'enforceSSL\' must be enabled for \'minimumTLSVersion\' to be applied', this); } if (props.serverAccessLogsBucket instanceof Bucket) { @@ -2281,7 +2282,7 @@ export class Bucket extends BucketBase { if (props.publicReadAccess) { if (props.blockPublicAccess === undefined) { - throw new Error('Cannot use \'publicReadAccess\' property on a bucket without allowing bucket-level public access through \'blockPublicAccess\' property.'); + throw new ValidationError('Cannot use \'publicReadAccess\' property on a bucket without allowing bucket-level public access through \'blockPublicAccess\' property.', this); } this.grantPublicAccess(); @@ -2289,7 +2290,7 @@ export class Bucket extends BucketBase { if (props.autoDeleteObjects) { if (props.removalPolicy !== RemovalPolicy.DESTROY) { - throw new Error('Cannot use \'autoDeleteObjects\' property on a bucket without setting removal policy to \'DESTROY\'.'); + throw new ValidationError('Cannot use \'autoDeleteObjects\' property on a bucket without setting removal policy to \'DESTROY\'.', this); } this.enableAutoDeleteObjects(); @@ -2409,12 +2410,12 @@ export class Bucket extends BucketBase { // if encryption key is set, encryption must be set to KMS or DSSE. if (encryptionType !== BucketEncryption.DSSE && encryptionType !== BucketEncryption.KMS && props.encryptionKey) { - throw new Error(`encryptionKey is specified, so 'encryption' must be set to KMS or DSSE (value: ${encryptionType})`); + throw new ValidationError(`encryptionKey is specified, so 'encryption' must be set to KMS or DSSE (value: ${encryptionType})`, this); } // if bucketKeyEnabled is set, encryption can not be BucketEncryption.UNENCRYPTED if (props.bucketKeyEnabled && encryptionType === BucketEncryption.UNENCRYPTED) { - throw new Error(`bucketKeyEnabled is specified, so 'encryption' must be set to KMS, DSSE or S3 (value: ${encryptionType})`); + throw new ValidationError(`bucketKeyEnabled is specified, so 'encryption' must be set to KMS, DSSE or S3 (value: ${encryptionType})`, this); } if (encryptionType === BucketEncryption.UNENCRYPTED) { @@ -2497,7 +2498,7 @@ export class Bucket extends BucketBase { return { bucketEncryption }; } - throw new Error(`Unexpected 'encryptionType': ${encryptionType}`); + throw new ValidationError(`Unexpected 'encryptionType': ${encryptionType}`, this); } /** @@ -2521,7 +2522,7 @@ export class Bucket extends BucketBase { if ((rule.expiredObjectDeleteMarker) && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) { // ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters. - throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.'); + throw new ValidationError('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.', self); } if ( @@ -2534,7 +2535,7 @@ export class Bucket extends BucketBase { rule.noncurrentVersionTransitions === undefined && rule.transitions === undefined ) { - throw new Error('All rules for `lifecycleRules` must have at least one of the following properties: `abortIncompleteMultipartUploadAfter`, `expiration`, `expirationDate`, `expiredObjectDeleteMarker`, `noncurrentVersionExpiration`, `noncurrentVersionsToRetain`, `noncurrentVersionTransitions`, or `transitions`'); + throw new ValidationError('All rules for `lifecycleRules` must have at least one of the following properties: `abortIncompleteMultipartUploadAfter`, `expiration`, `expirationDate`, `expiredObjectDeleteMarker`, `noncurrentVersionExpiration`, `noncurrentVersionsToRetain`, `noncurrentVersionTransitions`, or `transitions`', self); } const x: CfnBucket.RuleProperty = { @@ -2580,7 +2581,7 @@ export class Bucket extends BucketBase { props.encryption && [BucketEncryption.KMS_MANAGED, BucketEncryption.DSSE_MANAGED].includes(props.encryption) ) { - throw new Error('Default bucket encryption with KMS managed or DSSE managed key is not supported for Server Access Logging target buckets'); + throw new ValidationError('Default bucket encryption with KMS managed or DSSE managed key is not supported for Server Access Logging target buckets', this); } // When there is an encryption key exists for the server access logs bucket, grant permission to the S3 logging SP. @@ -2657,7 +2658,7 @@ export class Bucket extends BucketBase { } if (accessControlRequiresObjectOwnership && this.objectOwnership === ObjectOwnership.BUCKET_OWNER_ENFORCED) { - throw new Error (`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`); + throw new ValidationError (`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`, this); } return { @@ -2703,7 +2704,7 @@ export class Bucket extends BucketBase { return undefined; } if (objectLockEnabled === false && objectLockDefaultRetention) { - throw new Error('Object Lock must be enabled to configure default retention settings'); + throw new ValidationError('Object Lock must be enabled to configure default retention settings', this); } return { @@ -2723,16 +2724,16 @@ export class Bucket extends BucketBase { } if (props.websiteErrorDocument && !props.websiteIndexDocument) { - throw new Error('"websiteIndexDocument" is required if "websiteErrorDocument" is set'); + throw new ValidationError('"websiteIndexDocument" is required if "websiteErrorDocument" is set', this); } if (props.websiteRedirect && (props.websiteErrorDocument || props.websiteIndexDocument || props.websiteRoutingRules)) { - throw new Error('"websiteIndexDocument", "websiteErrorDocument" and, "websiteRoutingRules" cannot be set if "websiteRedirect" is used'); + throw new ValidationError('"websiteIndexDocument", "websiteErrorDocument" and, "websiteRoutingRules" cannot be set if "websiteRedirect" is used', this); } const routingRules = props.websiteRoutingRules ? props.websiteRoutingRules.map((rule) => { if (rule.condition && rule.condition.httpErrorCodeReturnedEquals == null && rule.condition.keyPrefixEquals == null) { - throw new Error('The condition property cannot be an empty object'); + throw new ValidationError('The condition property cannot be an empty object', this); } return { @@ -2762,17 +2763,17 @@ export class Bucket extends BucketBase { } if (!props.versioned) { - throw new Error('Replication rules require versioning to be enabled on the bucket'); + throw new ValidationError('Replication rules require versioning to be enabled on the bucket', this); } if (props.replicationRules.length > 1 && props.replicationRules.some(rule => rule.priority === undefined)) { - throw new Error('\'priority\' must be specified for all replication rules when there are multiple rules'); + throw new ValidationError('\'priority\' must be specified for all replication rules when there are multiple rules', this); } props.replicationRules.forEach(rule => { if (rule.replicationTimeControl && !rule.metrics) { - throw new Error('\'replicationTimeControlMetrics\' must be enabled when \'replicationTimeControl\' is enabled.'); + throw new ValidationError('\'replicationTimeControlMetrics\' must be enabled when \'replicationTimeControl\' is enabled.', this); } if (rule.deleteMarkerReplication && rule.filter?.tags) { - throw new Error('tag filter cannot be specified when \'deleteMarkerReplication\' is enabled.'); + throw new ValidationError('tag filter cannot be specified when \'deleteMarkerReplication\' is enabled.', this); } }); @@ -2846,7 +2847,7 @@ export class Bucket extends BucketBase { if (isCrossAccount) { Annotations.of(this).addInfo('For Cross-account S3 replication, ensure to set up permissions on source bucket using method addReplicationPolicy() '); } else if (rule.accessControlTransition) { - throw new Error('accessControlTranslation is only supported for cross-account replication'); + throw new ValidationError('accessControlTranslation is only supported for cross-account replication', this); } return { @@ -2921,7 +2922,7 @@ export class Bucket extends BucketBase { conditions: conditions, })); } else if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) { - throw new Error("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed"); + throw new ValidationError("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed", this); } else { this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE; } @@ -2937,7 +2938,7 @@ export class Bucket extends BucketBase { const format = inventory.format ?? InventoryFormat.CSV; const frequency = inventory.frequency ?? InventoryFrequency.WEEKLY; if (inventory.inventoryId !== undefined && (inventory.inventoryId.length > 64 || inventoryIdValidationRegex.test(inventory.inventoryId))) { - throw new Error(`inventoryId should not exceed 64 characters and should not contain special characters except . and -, got ${inventory.inventoryId}`); + throw new ValidationError(`inventoryId should not exceed 64 characters and should not contain special characters except . and -, got ${inventory.inventoryId}`, this); } const id = inventory.inventoryId ?? `${this.node.id}Inventory${index}`.replace(inventoryIdValidationRegex, '').slice(-64); @@ -3523,10 +3524,10 @@ export class ObjectLockRetention { private constructor(mode: ObjectLockMode, duration: Duration) { // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-lock-managing.html#object-lock-managing-retention-limits if (duration.toDays() > 365 * 100) { - throw new Error('Object Lock retention duration must be less than 100 years'); + throw new UnscopedValidationError('Object Lock retention duration must be less than 100 years'); } if (duration.toDays() < 1) { - throw new Error('Object Lock retention duration must be at least 1 day'); + throw new UnscopedValidationError('Object Lock retention duration must be at least 1 day'); } this.mode = mode; diff --git a/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts b/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts index 1f0430ab08b50..0f77296da1d68 100644 --- a/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts +++ b/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts @@ -2,6 +2,7 @@ import { Construct, IConstruct } from 'constructs'; import { NotificationsResourceHandler } from './notifications-resource-handler'; import * as iam from '../../../aws-iam'; import * as cdk from '../../../core'; +import { ValidationError } from '../../../core/lib/errors'; import * as cxapi from '../../../cx-api'; import { Bucket, IBucket, EventType, NotificationKeyFilter } from '../bucket'; import { BucketNotificationDestinationType, IBucketNotificationDestination } from '../destination'; @@ -71,7 +72,7 @@ export class BucketNotifications extends Construct { const targetProps = target.bind(this, this.bucket); const commonConfig: CommonConfiguration = { Events: [event], - Filter: renderFilters(filters), + Filter: renderFilters(filters, this), }; // if the target specifies any dependencies, add them to the custom resource. @@ -96,7 +97,7 @@ export class BucketNotifications extends Construct { break; default: - throw new Error('Unsupported notification target type:' + BucketNotificationDestinationType[targetProps.type]); + throw new ValidationError('Unsupported notification target type:' + BucketNotificationDestinationType[targetProps.type], this); } } @@ -171,7 +172,7 @@ export class BucketNotifications extends Construct { } } -function renderFilters(filters?: NotificationKeyFilter[]): Filter | undefined { +function renderFilters(filters: NotificationKeyFilter[], scope: BucketNotifications): Filter | undefined { if (!filters || filters.length === 0) { return undefined; } @@ -182,12 +183,12 @@ function renderFilters(filters?: NotificationKeyFilter[]): Filter | undefined { for (const rule of filters) { if (!rule.suffix && !rule.prefix) { - throw new Error('NotificationKeyFilter must specify `prefix` and/or `suffix`'); + throw new ValidationError('NotificationKeyFilter must specify `prefix` and/or `suffix`', scope); } if (rule.suffix) { if (hasSuffix) { - throw new Error('Cannot specify more than one suffix rule in a filter.'); + throw new ValidationError('Cannot specify more than one suffix rule in a filter.', scope); } renderedRules.push({ Name: 'suffix', Value: rule.suffix }); hasSuffix = true; @@ -195,7 +196,7 @@ function renderFilters(filters?: NotificationKeyFilter[]): Filter | undefined { if (rule.prefix) { if (hasPrefix) { - throw new Error('Cannot specify more than one prefix rule in a filter.'); + throw new ValidationError('Cannot specify more than one prefix rule in a filter.', scope); } renderedRules.push({ Name: 'prefix', Value: rule.prefix }); hasPrefix = true; diff --git a/packages/aws-cdk-lib/aws-s3/lib/util.ts b/packages/aws-cdk-lib/aws-s3/lib/util.ts index 5110df2f76fa1..8c856fdf456d7 100644 --- a/packages/aws-cdk-lib/aws-s3/lib/util.ts +++ b/packages/aws-cdk-lib/aws-s3/lib/util.ts @@ -1,6 +1,7 @@ import { IConstruct } from 'constructs'; import { BucketAttributes } from './bucket'; import * as cdk from '../../core'; +import { ValidationError } from '../../core/lib/errors'; export function parseBucketArn(construct: IConstruct, props: BucketAttributes): string { @@ -20,7 +21,7 @@ export function parseBucketArn(construct: IConstruct, props: BucketAttributes): }); } - throw new Error('Cannot determine bucket ARN. At least `bucketArn` or `bucketName` is needed'); + throw new ValidationError('Cannot determine bucket ARN. At least `bucketArn` or `bucketName` is needed', construct); } export function parseBucketName(construct: IConstruct, props: BucketAttributes): string | undefined { diff --git a/packages/aws-cdk-lib/core/lib/errors.ts b/packages/aws-cdk-lib/core/lib/errors.ts index 727cf0228c396..a64be6850a85b 100644 --- a/packages/aws-cdk-lib/core/lib/errors.ts +++ b/packages/aws-cdk-lib/core/lib/errors.ts @@ -69,12 +69,12 @@ abstract class ConstructError extends Error { return this.#constructInfo; } - constructor(msg: string, scope?: IConstruct) { + constructor(msg: string, scope?: IConstruct, name?: string) { super(msg); Object.setPrototypeOf(this, ConstructError.prototype); Object.defineProperty(this, CONSTRUCT_ERROR_SYMBOL, { value: true }); - this.name = new.target.name; + this.name = name ?? new.target.name; this.#time = new Date().toISOString(); this.#constructPath = scope?.node.path; @@ -143,3 +143,18 @@ export class ValidationError extends ConstructError { Object.defineProperty(this, VALIDATION_ERROR_SYMBOL, { value: true }); } } + +/** + * An Error that is thrown when a class has validation errors. + */ +export class UnscopedValidationError extends ConstructError { + public get type(): 'validation' { + return 'validation'; + } + + constructor(msg: string) { + super(msg, undefined, ValidationError.name); + Object.setPrototypeOf(this, UnscopedValidationError.prototype); + Object.defineProperty(this, VALIDATION_ERROR_SYMBOL, { value: true }); + } +} diff --git a/packages/aws-cdk-lib/core/test/errors.test.ts b/packages/aws-cdk-lib/core/test/errors.test.ts index a1a7f34de051e..7a0ab7b89d97e 100644 --- a/packages/aws-cdk-lib/core/test/errors.test.ts +++ b/packages/aws-cdk-lib/core/test/errors.test.ts @@ -1,5 +1,5 @@ import { App, Stack } from '../lib'; -import { Errors, ValidationError } from '../lib/errors'; +import { Errors, UnscopedValidationError, ValidationError } from '../lib/errors'; jest .useFakeTimers() @@ -31,4 +31,14 @@ describe('ValidationError', () => { expect(error.stack).toContain('ValidationError: this is an error'); expect(error.stack).toContain('at path [MyStack] in'); }); + + test('UnscopedValidationError is ValidationError and ConstructError', () => { + const error = new UnscopedValidationError('this is an error'); + + expect(Errors.isConstructError(error)).toBe(true); + expect(Errors.isValidationError(error)).toBe(true); + expect(error.name).toBe('ValidationError'); + expect(error.stack).toContain('ValidationError: this is an error'); + }); + }); From 52815961e187809df49d0d00b729fabfe21df1d7 Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Tue, 21 Jan 2025 16:16:01 +0000 Subject: [PATCH 2/2] more docs --- packages/aws-cdk-lib/core/lib/errors.ts | 28 +++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/errors.ts b/packages/aws-cdk-lib/core/lib/errors.ts index a64be6850a85b..2d6a310e724c4 100644 --- a/packages/aws-cdk-lib/core/lib/errors.ts +++ b/packages/aws-cdk-lib/core/lib/errors.ts @@ -5,18 +5,24 @@ const CONSTRUCT_ERROR_SYMBOL = Symbol.for('@aws-cdk/core.SynthesisError'); const VALIDATION_ERROR_SYMBOL = Symbol.for('@aws-cdk/core.ValidationError'); /** - * Helper to check if an error is a SynthesisErrors + * Helper to check if an error is of a certain type. */ export class Errors { /** - * Test whether the given errors is a ConstructionError + * Test whether the given errors is a ConstructionError. + * + * A ConstructionError is a generic error that will be thrown during the App construction or synthesis. + * To check for more specific errors, use the respective methods. */ public static isConstructError(x: any): x is ConstructError { return x !== null && typeof(x) === 'object' && CONSTRUCT_ERROR_SYMBOL in x; } /** - * Test whether the given error is a ValidationError + * Test whether the given error is a ValidationError. + * + * A ValidationError is thrown when input props are failing to pass the rules of the construct. + * It usually means the underlying CloudFormation resource(s) would not deploy with a given configuration. */ public static isValidationError(x: any): x is ValidationError { return Errors.isConstructError(x) && VALIDATION_ERROR_SYMBOL in x; @@ -29,7 +35,7 @@ interface ConstructInfo { } /** - * Generic, abstract error that is thrown from the users app during app construction or synth. + * Generic, abstract error class used for errors thrown from the users app during construction or synth. */ abstract class ConstructError extends Error { #time: string; @@ -130,7 +136,12 @@ abstract class ConstructError extends Error { } /** - * An Error that is thrown when a construct has validation errors. + * A ValidationError should be used when input props fail to pass the validation rules of a construct + * or class or late binding. The error indicates that the underlying CloudFormation resource(s) would + * not deploy with a given configuration, or that some other prerequisites are not met. + * + * A ValidationError is always attached to a Construct scope. To a user, the error will present with additional + * information on the construct that caused the validation to fail. */ export class ValidationError extends ConstructError { public get type(): 'validation' { @@ -145,7 +156,12 @@ export class ValidationError extends ConstructError { } /** - * An Error that is thrown when a class has validation errors. + * An UnscopedValidationError is a ValidationError that is not attached to a specific construct. + * This can be used to report validation errors that are thrown when no construct scope is available. + * The common use case here are data classes that assert on props, but are not constructs itself. + * + * To a User, these errors still present themselves as a "ValidationError". + * However they do not contain any information about the location in the construct tree. */ export class UnscopedValidationError extends ConstructError { public get type(): 'validation' {