-
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
chore(guidelines): update the section on error messages #13345
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: \<message\>”) | ||
* 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,26 +1269,126 @@ 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. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example |
||
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`) | ||
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 | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Example?