From 52f29fae0d88a2999e99ddf8ca6f5644cd23c35f Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 2 Mar 2021 16:59:22 +0100 Subject: [PATCH] chore(guidelines): update the section on error messages (#13345) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- DESIGN_GUIDELINES.md | 127 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 107 insertions(+), 20 deletions(-) diff --git a/DESIGN_GUIDELINES.md b/DESIGN_GUIDELINES.md index d00f63e453515..7fc635960da86 100644 --- a/DESIGN_GUIDELINES.md +++ b/DESIGN_GUIDELINES.md @@ -1261,19 +1261,6 @@ for (const az of availabilityZones) { ### Errors -#### Input Validation - -Prefer to validate input as early as it is passed into your code (ctor, methods, -etc) and bail out by throwing an **Error** (no need to create subclasses of -Error since all errors in the CDK are unrecoverable): - -* All lowercase sentences (usually they are printed after “Error: \”) -* Include a descriptive message -* Include the value provided -* Include the expected/allowed values -* No need to include information that can be obtained from the stack trace -* No need to add a period at the end of error messages - #### Avoid Errors If Possible Always prefer to do the right thing for the user instead of raising an @@ -1282,18 +1269,79 @@ example, VPC has **enableDnsHostnames** and **enableDnsSupport**. DNS hostnames *require* DNS support, so only fail if the user enabled DNS hostnames but explicitly disabled DNS support. Otherwise, auto-enable DNS support for them. +#### Error reporting mechanism + +There are three mechanism you can use to report errors: + +* Eagerly throw an exception (fails synthesis) +* Attach a (lazy) validator to a construct (fails synthesis) +* Attach errors to a construct (succeeds synthesis, fails deployment) + +Between these, the first two fail synthesis, while the latter doesn't. Failing synthesis +means that no Cloud Assembly will be produced. + +The distinction becomes apparent when you consider multiple stacks in the same Cloud +Assembly: + +* If synthesis fails due to an error in *one* stack (either by throwing an exception + or by failing validation), the other stack can also not be deployed. +* In contrast, if you attach an error to a construct in one stack, that stack cannot + be deployed but the other one still can. + +Choose one of the first two methods if the failure is caused by a misuse of the API, +which the user should be alerted to and fix as quickly as possible. Choose attaching +an error to a construct if the failure is due to environmental factors outside the +direct use of the API surface (for example, lack of context provider lookup values). + +#### Throwing exceptions + +This should be the preferred error reporting method. + +Validate input as early as it is passed into your code (ctor, methods, +etc) and bail out by throwing an `Error`. No need to create subclasses of +Error since all errors in the CDK are unrecoverable. + +When validating inputs, don't forget to account for the fact that these +values may be `Token`s and not available for inspection at synthesis time. + +Example: + +```ts +if (!Token.isUnresolved(props.minCapacity) && props.minCapacity < 1) { + throw new Error(`'minCapacity' should be at least 1, got '${props.minCapacity}'`); +} +``` + #### Never Catch Exceptions -All CDK errors are unrecoverable. If a method wishes to signal a recoverable +All CDK errors are unrecoverable. If a method wishes to signal a recoverable error, this should be modeled in a return value and not through exceptions. -#### Post Validation +#### Attaching (lazy) Validators -In the rare case where the integrity of your construct can only be checked right -before synthesis, override the **Construct.validate()** method and return -meaningful errors. Always prefer early input validation over post-validation. +In the rare case where the integrity of your construct can only be checked +after the app has completed its initialization, call the +**this.node.addValidation()** method to add a validation object. This will +generally only be necessary if you want to produce an error when a certain +interaction with your construct did *not* happen (for example, a property +that should have been configured over the lifetime of the construct, wasn't): -#### Attached Errors/Warnings +Always prefer early input validation over post-validation, as the necessity +of these should be rare. + +Example: + +```ts +this.node.addValidation({ + // 'validate' should return a string[] list of errors + validate: () => this.rules.length === 0 + ? ['At least one Rule must be added. Call \'addRule()\' to add Rules.'] + : [] + } +}); +``` + +#### Attaching Errors/Warnings You can also “attach” an error or a warning to a construct via the **Annotations** class. These methods (e.g., `Annotations.of(construct).addWarning`) @@ -1301,7 +1349,46 @@ will attach CDK metadata to your construct, which will be displayed to the user by the toolchain when the stack is deployed. Errors will not allow deployment and warnings will only be displayed in -highlight (unless **--strict** mode is used). +highlight (unless `--strict` mode is used). + +```ts +if (!Token.isUnresolved(subnetIds) && subnetIds.length < 2) { + Annotations.of(this).addError(`Need at least 2 subnet ids, got: ${JSON.stringify(subnetIds)}`); +} +``` + +#### Error messages + +Think about error messages from the point of view of the end user of the CDK. +This is not necessarily someone who knows about the internals of your +construct library, so try to phrase the message in a way that would make +sense to them. + +For example, if a value the user supplied gets handed off between a number of +functions before finally being validated, phrase the message in terms of the +API the user interacted with, not in terms of the internal APIs. + +A good error message should include the following components: + +* What went wrong, in a way that makes sense to a top-level user +* An example of the incorrect value provided (if applicable) +* An example of the expected/allowed values (if applicable) +* The message should explain the (most likely) cause and change the user can + make to rectify the situation + +The message should be all lowercase and not end in a period, or contain +information that can be obtained from the stack trace. + +```ts +// ✅ DO - show the value you got and be specific about what the user should do +`supply at least one of minCapacity or maxCapacity, got ${JSON.stringify(action)}` + +// ❌ DO NOT - this tells the user nothing about what's wrong or what they should do +`required values are missing` + +// ❌ DO NOT - this error only makes sense if you know the implementation +`'undefined' is not a number` +``` ### Tokens