fix(rds): does not print all failed validations for DatabaseCluster props #32841

iankhou · 2025-01-10T19:06:59Z

Issue #32840

Reason for this change

When initializing a new RDS DB Cluster, the current implementation fails on the first validation error, making it possible for the user to encounter another failure after fixing known validation issues.

Description of changes

Implemented a validation function that collects all validation errors and presents them to the user. Used this function in RDS Database Cluster initialization. I will implement this separately for SQS Queue initialization as a POC for usability.

There are several other places that can make use of this shared code, to show users all validation errors at once. Here's a non-exhaustive list:

Describe any new or updated permissions being added

No permissions changes.

Description of how you validated changes

Added unit tests and modified existing unit tests.

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

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.

iankhou · 2025-01-10T19:10:22Z

packages/aws-cdk-lib/aws-rds/test/cluster.test.ts

@@ -2093,30 +2172,6 @@ describe('cluster', () => {
      });
    });

-    test('throws if performanceInsightRetention is set but performance insights is disabled', () => {


These two tests did the same thing (as implemented), removed them here and moved to line 567.

iankhou · 2025-01-10T19:11:05Z

packages/aws-cdk-lib/aws-rds/test/cluster.test.ts

-        }).toThrow('Aurora Limitless Database requires I/O optimized storage type, got: aurora');
-      });
-
-      test.each([[], undefined])('throw error for invalid cloudwatch log exports', (cloudwatchLogsExports) => {


Tested at line 584.

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

codecov · 2025-01-10T19:37:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.53%. Comparing base (ac90399) to head (4d2b9d7).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32841      +/-   ##
==========================================
+ Coverage   81.52%   81.53%   +0.01%     
==========================================
  Files         225      226       +1     
  Lines       13760    13769       +9     
  Branches     2413     2414       +1     
==========================================
+ Hits        11218    11227       +9     
  Misses       2270     2270              
  Partials      272      272

Flag	Coverage Δ
suite.unit	`81.53% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.92% <ø> (ø)`
packages/aws-cdk-lib/core	`82.17% <100.00%> (+0.02%)`	⬆️

aws-cdk-automation

The pull request linter fails with the following errors:

❌ Features must contain a change to a README 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.

✅ A exemption request has been requested. Please wait for a maintainer's review.

iankhou · 2025-01-13T15:44:59Z

Exemption Request: unit test changes provide coverage for single and multiple errors returned, no README changes are needed as the effects of this change will be intuitive to users.

mrgrain

Two pieces of feedback:

Can you please change this to use ValidationError? We are currently starting a migration of all error types to the new ValidationError. You could not have known that. But would be good to get started on it.
This is great progress towards a more generic validation system. Good stuff! However I think we already should be aiming much higher and make this a generic (private) feature in core, so that all our constructs can you use this.

iankhou · 2025-01-13T16:08:04Z

@mrgrain

Two pieces of feedback:

Can you please change this to use ValidationError? We are currently starting a migration of all error types to the new ValidationError. You could not have known that. But would be good to get started on it.

This is great progress towards a more generic validation system. Good stuff! However I think we already should be aiming much higher and make this a generic (private) feature in core, so that all our constructs can you use this.

First one is an easy change and I will implement this.

For your second point, are you suggesting to generalize validateDatabaseClusterProps to validateProps, and to take in a set of validation rules? In my current PR's implementation, the rules are generated using another function createDatabaseClusterValidationRules, but I suppose this could be made into createValidationRules and take in a set of conditions + messages?

But I worry this might be too abstracted. We have different rules for standard and limitless database initializations, and offering flexibility in an abstraction might make a generalized function more difficult to use. Happy to hear your thoughts on this.

mrgrain · 2025-01-13T21:04:54Z

For your second point, are you suggesting to generalize validateDatabaseClusterProps to validateProps, and to take in a set of validation rules? In my current PR's implementation, the rules are generated using another function createDatabaseClusterValidationRules, but I suppose this could be made into createValidationRules and take in a set of conditions + messages?

If I understand the PR correctly, you are introducing a feature that allows a construct to declare a number if rules that can be validated independently from each other. If any of the rules fails, the construct will throw a validation error with all failing rules.

I think this a feature that would benefit other constructs as well and is worth generalizing.

But I worry this might be too abstracted. We have different rules for standard and limitless database initializations, and offering flexibility in an abstraction might make a generalized function more difficult to use. Happy to hear your thoughts on this.

That's a good concern, and we will have to make sure the API is easy enough to understand. I agree, this PR feels complex because you are starting from the list of all possible rules and then run some filters to only pick the applicable ones. That part I think can be kept local to the RDS DB Cluster.

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

mrgrain · 2025-01-22T16:04:54Z

packages/aws-cdk-lib/core/test/helpers-internal/validate-props.test.ts

+import { ValidationError } from '../../lib/errors';
+import { validateProps, ValidationRule } from '../../lib/helpers-internal/validate-props';
+
+class MockConstruct extends Construct {


💅🏻 nit: In the context of jest tests "mock" means a very specific thing, I usually call this Fake or Test just to not confuse a future reader of the code.

Will rename to TestConstruct.

mrgrain · 2025-01-22T16:10:11Z

packages/aws-cdk-lib/aws-rds/test/cluster.test.ts

@@ -268,7 +268,7 @@ describe('cluster new api', () => {
            storageType: DBClusterStorageType.AURORA_IOPT1,
            cloudwatchLogsExports: ['postgresql'],
          });
-        }).toThrow('Performance Insights must be enabled for Aurora Limitless Database.');
+        }).toThrow('DatabaseCluster initialization failed due to the following validation error(s):\n- Performance Insights must be enabled for Aurora Limitless Database\n- Performance Insights retention period must be set to at least 31 days for Aurora Limitless Database');


💅🏻 nit: Might be nicer to limit the test exception to what the test is trying to test. Like for this specific test, do we really care of the other error and the prelude is included?

We could keep the existing ones and then have a single test case that checks all possible parallel rules

The way that the validations are implemented now, the prelude will always be included. Since we were only testing for Performance Insights must be enabled for Aurora Limitless Database, I could change the test to include a valid retention period. However, this seems kind of contrived... since a user is unlikely to include a retention period if they did not enable performance insights.

mrgrain · 2025-01-22T16:10:48Z

packages/aws-cdk-lib/core/lib/helpers-internal/validate-props.ts

+ * Validates props against a set of rules and throws an error if any validations fail.
+ *
+ * @template T The type of the props being validated.
+ * @param {string} className - The name of the class being validated, used in the error message.


You should give the example how to use this normally. i.e. Queue.name

Will do, thanks.

mrgrain · 2025-01-22T16:12:48Z

packages/aws-cdk-lib/core/lib/helpers-internal/validate-props.ts

+ * @param {ValidationRule<T>[]} rules - An array of validation rules to apply.
+ * @throws {Error} If any validation rules fail, with a message detailing all failures.
+ */
+export function validateProps<T>(scope: Construct, className: string, props: T, rules: ValidationRule<T>[]): void {


Can we find a name that indicates the exact behavior better? I mean "fail at first error" vs "process all".

mrgrain · 2025-01-22T16:13:39Z

packages/aws-cdk-lib/aws-rds/lib/cluster.ts

@@ -829,36 +830,10 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
      });
    }

+    validateDatabaseClusterProps(scope, props);


Suggested change

validateDatabaseClusterProps(scope, props);

validateDatabaseClusterProps(this, props);

Will make this change, thanks.

mrgrain · 2025-01-22T16:14:09Z

packages/aws-cdk-lib/aws-sqs/lib/queue.ts

@@ -383,7 +383,7 @@ export class Queue extends QueueBase {
      physicalName: props.queueName,
    });

-    validateProps(props);
+    validateQueueProps(scope, props);


Suggested change

validateQueueProps(scope, props);

validateQueueProps(this, props);

Will make this change, thanks.

mrgrain · 2025-01-22T16:16:02Z

packages/aws-cdk-lib/aws-rds/lib/validate-database-cluster-props.ts

+    ? [...standardDatabaseRules, ...limitlessDatabaseRules]
+    : standardDatabaseRules;
+
+  validateProps(scope, DatabaseCluster.name, props, applicableRules);


Is using DatabaseCluster.name always the correct choice?

If someone implements a subclass of DatabaseCluster named FooBar, the error will still say "DatabaseCluster". Is that what we want? Genuine question, cause I'm not sure about it.

I did not consider that while implementing this, but I think that is the correct behavior. Ultimately we are validating the props of DatabaseCluster, not FooBar. If there are additional validations needed for FooBar, they should be implemented separately.

mrgrain

Looks great! Left some very minor comments. The other things is that we will need to have two separate PRs for this. One for e.g. RDS with the general implementation and then a follow-up for SQS. The reason being that we want two distinct entries in the changelog.

mrgrain · 2025-01-22T16:19:47Z

Side note: This feels more like a fix to me. Also the PR title should always be phrased from the perspective of the user:

fix(rds): does not print all failed validations for DatabaseCluster props

mrgrain

mergify · 2025-01-23T16:29:18Z

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 · 2025-01-23T16:58:55Z

AWS CodeBuild CI Report

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

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

mergify · 2025-01-23T16:59:24Z

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

github-actions · 2025-01-23T16:59:58Z

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.

iankhou self-assigned this Jan 10, 2025

aws-cdk-automation requested a review from a team January 10, 2025 19:07

github-actions bot added feature-request A feature should be added or improved. p2 labels Jan 10, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 10, 2025

aws-cdk-automation previously requested changes Jan 10, 2025

View reviewed changes

iankhou commented Jan 10, 2025

View reviewed changes

iankhou changed the title ~~test refactor to notify user of all validation issues when initializi…~~ feature(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 10, 2025

iankhou changed the title ~~feature(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ test(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 10, 2025

iankhou marked this pull request as ready for review January 11, 2025 01:37

github-actions bot added the effort/small Small work item – less than a day of effort label Jan 11, 2025

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 11, 2025

iankhou changed the title ~~test(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ test(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 11, 2025

iankhou changed the title ~~test(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ feat(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 13, 2025

aws-cdk-automation previously requested changes Jan 13, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 13, 2025

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Jan 13, 2025

mrgrain requested changes Jan 13, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 13, 2025

iankhou force-pushed the iankhou-rds-test-refactor branch from 8aef9e4 to 16787f0 Compare January 16, 2025 19:13

iankhou requested a review from a team as a code owner January 16, 2025 19:13

iankhou changed the title ~~feat(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ feat(rds + sqs): refactor to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue Jan 16, 2025

iankhou changed the title ~~feat(rds + sqs): refactor to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue~~ feat(rds,sqs): refactor to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue Jan 16, 2025

iankhou added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Jan 17, 2025

mrgrain reviewed Jan 22, 2025

View reviewed changes

mrgrain requested changes Jan 22, 2025

View reviewed changes

iankhou changed the title ~~feat(validation): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue~~ feat(rds): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue Jan 22, 2025

iankhou changed the title ~~feat(rds): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue~~ feat(rds): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster Jan 22, 2025

iankhou changed the title ~~feat(rds): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster~~ fix(rds): does not print all failed validations for DatabaseCluster props Jan 22, 2025

iankhou force-pushed the iankhou-rds-test-refactor branch from fb36dec to bef0496 Compare January 22, 2025 17:20

iankhou added 2 commits January 22, 2025 13:55

Merge conflict

64a1125

WIP

9450eb8

iankhou force-pushed the iankhou-rds-test-refactor branch from bef0496 to 9450eb8 Compare January 22, 2025 18:55

iankhou requested a review from mrgrain January 22, 2025 19:29

mrgrain approved these changes Jan 23, 2025

View reviewed changes

Merge branch 'main' into iankhou-rds-test-refactor

4d2b9d7

mergify bot merged commit 344d916 into main Jan 23, 2025
20 checks passed

mergify bot deleted the iankhou-rds-test-refactor branch January 23, 2025 16:59

github-actions bot locked as resolved and limited conversation to collaborators Jan 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rds): does not print all failed validations for DatabaseCluster props #32841

fix(rds): does not print all failed validations for DatabaseCluster props #32841

iankhou commented Jan 10, 2025 •

edited

Loading

aws-cdk-automation left a comment

iankhou Jan 10, 2025 •

edited

Loading

iankhou Jan 10, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

iankhou commented Jan 13, 2025

mrgrain left a comment

iankhou commented Jan 13, 2025

mrgrain commented Jan 13, 2025

mrgrain Jan 22, 2025

iankhou Jan 22, 2025

mrgrain Jan 22, 2025

iankhou Jan 22, 2025 •

edited

Loading

mrgrain Jan 22, 2025

iankhou Jan 22, 2025

mrgrain Jan 22, 2025

mrgrain Jan 22, 2025

iankhou Jan 22, 2025

mrgrain Jan 22, 2025

iankhou Jan 22, 2025

mrgrain Jan 22, 2025

iankhou Jan 22, 2025

mrgrain left a comment

mrgrain commented Jan 22, 2025

mrgrain left a comment

mergify bot commented Jan 23, 2025

aws-cdk-automation commented Jan 23, 2025

mergify bot commented Jan 23, 2025

github-actions bot commented Jan 23, 2025

	validateDatabaseClusterProps(scope, props);
	validateDatabaseClusterProps(this, props);

	validateQueueProps(scope, props);
	validateQueueProps(this, props);

fix(rds): does not print all failed validations for DatabaseCluster props #32841

fix(rds): does not print all failed validations for DatabaseCluster props #32841

Conversation

iankhou commented Jan 10, 2025 • edited Loading

Issue #32840

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

iankhou Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

iankhou commented Jan 13, 2025

mrgrain left a comment

Choose a reason for hiding this comment

iankhou commented Jan 13, 2025

mrgrain commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iankhou Jan 22, 2025 • edited Loading

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

mrgrain left a comment

Choose a reason for hiding this comment

mrgrain commented Jan 22, 2025

mrgrain left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 23, 2025

aws-cdk-automation commented Jan 23, 2025

AWS CodeBuild CI Report

mergify bot commented Jan 23, 2025

github-actions bot commented Jan 23, 2025

iankhou commented Jan 10, 2025 •

edited

Loading

iankhou Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

iankhou Jan 22, 2025 •

edited

Loading