refactor(core): Reorganize error hierarchy in core and workflow packages (no-changelog)
#7820
Conversation
n8n-assistant
bot
added
core
| export abstract class ExecutionBaseError extends ApplicationError { | ||
| description: string | null | undefined; | ||
|
|
||
| cause: Error | JsonObject | undefined; |
There was a problem hiding this comment.
I think we should update this to be 2 separate properties, and stop conflating the 2 distinct pieces of information.
cause should not be used to store JSONObject. We don't have to make this change in this PR, but we should make this change soon, to make errors more type-safe, and also to be able to clearly distinguish between a cause, and remote json responses in the UI.
| if (originalException instanceof ExecutionBaseError && originalException.severity === 'warning') | ||
| return null; | ||
|
|
||
| if (originalException instanceof ReportableError) { | ||
| if (originalException instanceof ApplicationError) { |
There was a problem hiding this comment.
Since ExecutionBaseError now extends ApplicationError, these checks could be unified, and we can most likely remove the ExecutionBaseError import all together here.
There was a problem hiding this comment.
Do you mean unifying like if (level === 'warning' || severity === 'warning') return null;?
If so, severity is present in ExecutionBaseError but absent in ApplicationError, so we'd still need to check for originalException instanceof ExecutionBaseError, meaning the check is moved but not unified. Might be best to clean this up once we do away with severity altogether?
if (originalException instanceof ApplicationError) {
if (
originalException instanceof ExecutionBaseError &&
originalException.severity === 'warning'
) {
return null;
}
const { level, extra } = originalException;
if (level === 'warning') return null;
event.level = level;
if (extra) event.extra = { ...event.extra, ...extra };
}
1 flaky test on run #3041 ↗︎
Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Canvas Node Manipulation and Navigation > should add merge node and test connections |
Screenshots
Video
|
|
Review all test suite changes for PR #7820 ↗︎
|
✅ All Cypress E2E specs passed |
ivov
deleted the
reorganize-error-hierarchy-in-core-and-workflow-packages
branch
…elog) (n8n-io#7839) Ensure all errors in `cli` inherit from `ApplicationError` to continue normalizing all the errors we report to Sentry Follow-up to: n8n-io#7820
…auses (no-changelog) (#7880) Store the third-party API response error separately from the error stored as `cause` Follow-up to: #7820 (comment)
|
Got released with |
Ensure all errors in
coreandworkflowinherit fromApplicationErrorso that we start normalizing all the errors we report to SentryFollow-up to: #7757 (comment)
corepackageApplicationErrorFileSystemError(abstract)FileNotFoundErrorDisallowedFilepathErrorBinaryDataError(abstract)InvalidModeErrorInvalidManagerErrorInvalidExecutionMetadataErrorworkflowpackageApplicationErrorExecutionBaseError(abstract)WorkflowActivationErrorWorkflowDeactivationErrorWebhookTakenErrorWorkflowOperationErrorSubworkflowOperationErrorCliWorkflowOperationErrorExpressionErrorExpressionExtensionErrorNodeError(abstract)NodeOperationErrorNodeApiErrorNodeSSLErrorUp next:
cliworkflow(do we really needExecutionBaseError?)ExecutionErrortypeErrorsseveritywithleveltagsextras