From 2ab59d775b20b697c7677463363226e200b5993b Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Tue, 18 Feb 2025 17:47:11 +0200 Subject: [PATCH] refactor(core): Extend error hierarchy (#12267) --- .../errors/__tests__/error-reporter.test.ts | 39 ++++++++++++- packages/core/src/errors/error-reporter.ts | 35 ++++++----- .../errors/abstract/execution-base.error.ts | 3 +- .../workflow/src/errors/application.error.ts | 12 ++-- .../workflow/src/errors/base/base.error.ts | 58 +++++++++++++++++++ .../src/errors/base/operational.error.ts | 21 +++++++ .../src/errors/base/unexpected.error.ts | 21 +++++++ .../workflow/src/errors/base/user.error.ts | 24 ++++++++ packages/workflow/src/errors/error.types.ts | 14 +++++ packages/workflow/src/errors/index.ts | 7 ++- .../workflow/src/errors/node-api.error.ts | 4 +- .../src/errors/trigger-close.error.ts | 5 +- .../errors/base/operational.error.test.ts | 26 +++++++++ .../test/errors/base/unexpected.error.test.ts | 26 +++++++++ .../test/errors/base/user.error.test.ts | 26 +++++++++ 15 files changed, 293 insertions(+), 28 deletions(-) create mode 100644 packages/workflow/src/errors/base/base.error.ts create mode 100644 packages/workflow/src/errors/base/operational.error.ts create mode 100644 packages/workflow/src/errors/base/unexpected.error.ts create mode 100644 packages/workflow/src/errors/base/user.error.ts create mode 100644 packages/workflow/src/errors/error.types.ts create mode 100644 packages/workflow/test/errors/base/operational.error.test.ts create mode 100644 packages/workflow/test/errors/base/unexpected.error.test.ts create mode 100644 packages/workflow/test/errors/base/user.error.test.ts diff --git a/packages/core/src/errors/__tests__/error-reporter.test.ts b/packages/core/src/errors/__tests__/error-reporter.test.ts index ff260ea1f7..33ff6ea04d 100644 --- a/packages/core/src/errors/__tests__/error-reporter.test.ts +++ b/packages/core/src/errors/__tests__/error-reporter.test.ts @@ -2,7 +2,7 @@ import { QueryFailedError } from '@n8n/typeorm'; import type { ErrorEvent } from '@sentry/types'; import { AxiosError } from 'axios'; import { mock } from 'jest-mock-extended'; -import { ApplicationError } from 'n8n-workflow'; +import { ApplicationError, BaseError } from 'n8n-workflow'; import type { Logger } from '@/logging/logger'; @@ -133,6 +133,43 @@ describe('ErrorReporter', () => { expect(beforeSendFilter).toHaveBeenCalledWith(event, hint); }); }); + + describe('BaseError', () => { + class TestError extends BaseError {} + + it('should drop errors with shouldReport false', async () => { + const originalException = new TestError('test', { shouldReport: false }); + + expect(await errorReporter.beforeSend(event, { originalException })).toEqual(null); + }); + + it('should keep events with shouldReport true', async () => { + const originalException = new TestError('test', { shouldReport: true }); + + expect(await errorReporter.beforeSend(event, { originalException })).toEqual(event); + }); + + it('should set level, extra, and tags from BaseError', async () => { + const originalException = new TestError('Test error', { + level: 'error', + extra: { foo: 'bar' }, + tags: { tag1: 'value1' }, + }); + + const testEvent = {} as ErrorEvent; + + const result = await errorReporter.beforeSend(testEvent, { originalException }); + + expect(result).toEqual({ + level: 'error', + extra: { foo: 'bar' }, + tags: { + packageName: 'core', + tag1: 'value1', + }, + }); + }); + }); }); describe('error', () => { diff --git a/packages/core/src/errors/error-reporter.ts b/packages/core/src/errors/error-reporter.ts index 3c0bffcd4b..2f5b285787 100644 --- a/packages/core/src/errors/error-reporter.ts +++ b/packages/core/src/errors/error-reporter.ts @@ -3,7 +3,8 @@ import type { NodeOptions } from '@sentry/node'; import { close } from '@sentry/node'; import type { ErrorEvent, EventHint } from '@sentry/types'; import { AxiosError } from 'axios'; -import { ApplicationError, ExecutionCancelledError, type ReportingOptions } from 'n8n-workflow'; +import type { ReportingOptions } from 'n8n-workflow'; +import { ApplicationError, ExecutionCancelledError, BaseError } from 'n8n-workflow'; import { createHash } from 'node:crypto'; import type { InstanceType } from '@/instance-settings'; @@ -44,11 +45,15 @@ export class ErrorReporter { const context = executionId ? ` (execution ${executionId})` : ''; do { - const msg = [ - e.message + context, - e instanceof ApplicationError && e.level === 'error' && e.stack ? `\n${e.stack}\n` : '', - ].join(''); - const meta = e instanceof ApplicationError ? e.extra : undefined; + let stack = ''; + let meta = undefined; + if (e instanceof ApplicationError || e instanceof BaseError) { + if (e.level === 'error' && e.stack) { + stack = `\n${e.stack}\n`; + } + meta = e.extra; + } + const msg = [e.message + context, stack].join(''); this.logger.error(msg, meta); e = e.cause as Error; } while (e); @@ -137,11 +142,17 @@ export class ErrorReporter { if (originalException instanceof AxiosError) return null; + if (originalException instanceof BaseError) { + if (!originalException.shouldReport) return null; + + this.extractEventDetailsFromN8nError(event, originalException); + } + if (this.isIgnoredSqliteError(originalException)) return null; - if (this.isApplicationError(originalException)) { + if (originalException instanceof ApplicationError) { if (this.isIgnoredApplicationError(originalException)) return null; - this.extractEventDetailsFromApplicationError(event, originalException); + this.extractEventDetailsFromN8nError(event, originalException); } if ( @@ -193,17 +204,13 @@ export class ErrorReporter { ); } - private isApplicationError(error: unknown): error is ApplicationError { - return error instanceof ApplicationError; - } - private isIgnoredApplicationError(error: ApplicationError) { return error.level === 'warning'; } - private extractEventDetailsFromApplicationError( + private extractEventDetailsFromN8nError( event: ErrorEvent, - originalException: ApplicationError, + originalException: ApplicationError | BaseError, ) { const { level, extra, tags } = originalException; event.level = level; diff --git a/packages/workflow/src/errors/abstract/execution-base.error.ts b/packages/workflow/src/errors/abstract/execution-base.error.ts index 819b17353b..0fbbdde6d8 100644 --- a/packages/workflow/src/errors/abstract/execution-base.error.ts +++ b/packages/workflow/src/errors/abstract/execution-base.error.ts @@ -1,5 +1,6 @@ import type { Functionality, IDataObject, JsonObject } from '../../Interfaces'; -import { ApplicationError, type ReportingOptions } from '../application.error'; +import { ApplicationError } from '../application.error'; +import type { ReportingOptions } from '../error.types'; interface ExecutionBaseErrorOptions extends ReportingOptions { cause?: Error; diff --git a/packages/workflow/src/errors/application.error.ts b/packages/workflow/src/errors/application.error.ts index b8f54cf8b4..1bacf5a7c8 100644 --- a/packages/workflow/src/errors/application.error.ts +++ b/packages/workflow/src/errors/application.error.ts @@ -1,15 +1,13 @@ import type { Event } from '@sentry/node'; import callsites from 'callsites'; -export type Level = 'warning' | 'error' | 'fatal' | 'info'; - -export type ReportingOptions = { - level?: Level; - executionId?: string; -} & Pick; +import type { ErrorLevel, ReportingOptions } from '@/errors/error.types'; +/** + * @deprecated Use `UserError`, `OperationalError` or `UnexpectedError` instead. + */ export class ApplicationError extends Error { - level: Level; + level: ErrorLevel; readonly tags: NonNullable; diff --git a/packages/workflow/src/errors/base/base.error.ts b/packages/workflow/src/errors/base/base.error.ts new file mode 100644 index 0000000000..15d1d4dacb --- /dev/null +++ b/packages/workflow/src/errors/base/base.error.ts @@ -0,0 +1,58 @@ +import type { Event } from '@sentry/node'; +import callsites from 'callsites'; + +import type { ErrorTags, ErrorLevel, ReportingOptions } from '../error.types'; + +export type BaseErrorOptions = { description?: undefined | null } & ErrorOptions & ReportingOptions; + +/** + * Base class for all errors + */ +export abstract class BaseError extends Error { + /** + * Error level. Defines which level the error should be logged/reported + * @default 'error' + */ + readonly level: ErrorLevel; + + /** + * Whether the error should be reported to Sentry. + * @default true + */ + readonly shouldReport: boolean; + + readonly description: string | null | undefined; + + readonly tags: ErrorTags; + + readonly extra?: Event['extra']; + + readonly packageName?: string; + + constructor( + message: string, + { + level = 'error', + description, + shouldReport, + tags = {}, + extra, + ...rest + }: BaseErrorOptions = {}, + ) { + super(message, rest); + + this.level = level; + this.shouldReport = shouldReport ?? (level === 'error' || level === 'fatal'); + this.description = description; + this.tags = tags; + this.extra = extra; + + try { + const filePath = callsites()[2].getFileName() ?? ''; + const match = /packages\/([^\/]+)\//.exec(filePath)?.[1]; + + if (match) this.tags.packageName = match; + } catch {} + } +} diff --git a/packages/workflow/src/errors/base/operational.error.ts b/packages/workflow/src/errors/base/operational.error.ts new file mode 100644 index 0000000000..c67848dca8 --- /dev/null +++ b/packages/workflow/src/errors/base/operational.error.ts @@ -0,0 +1,21 @@ +import type { BaseErrorOptions } from './base.error'; +import { BaseError } from './base.error'; + +export type OperationalErrorOptions = Omit & { + level?: 'info' | 'warning' | 'error'; +}; + +/** + * Error that indicates a transient issue, like a network request failing, + * a database query timing out, etc. These are expected to happen, are + * transient by nature and should be handled gracefully. + * + * Default level: warning + */ +export class OperationalError extends BaseError { + constructor(message: string, opts: OperationalErrorOptions = {}) { + opts.level = opts.level ?? 'warning'; + + super(message, opts); + } +} diff --git a/packages/workflow/src/errors/base/unexpected.error.ts b/packages/workflow/src/errors/base/unexpected.error.ts new file mode 100644 index 0000000000..a204569052 --- /dev/null +++ b/packages/workflow/src/errors/base/unexpected.error.ts @@ -0,0 +1,21 @@ +import type { BaseErrorOptions } from './base.error'; +import { BaseError } from './base.error'; + +export type UnexpectedErrorOptions = Omit & { + level?: 'error' | 'fatal'; +}; + +/** + * Error that indicates something is wrong in the code: logic mistakes, + * unhandled cases, assertions that fail. These are not recoverable and + * should be brought to developers' attention. + * + * Default level: error + */ +export class UnexpectedError extends BaseError { + constructor(message: string, opts: UnexpectedErrorOptions = {}) { + opts.level = opts.level ?? 'error'; + + super(message, opts); + } +} diff --git a/packages/workflow/src/errors/base/user.error.ts b/packages/workflow/src/errors/base/user.error.ts new file mode 100644 index 0000000000..787bcf1cc8 --- /dev/null +++ b/packages/workflow/src/errors/base/user.error.ts @@ -0,0 +1,24 @@ +import type { BaseErrorOptions } from './base.error'; +import { BaseError } from './base.error'; + +export type UserErrorOptions = Omit & { + level?: 'info' | 'warning'; + description?: string | null | undefined; +}; + +/** + * Error that indicates the user performed an action that caused an error. + * E.g. provided invalid input, tried to access a resource they’re not + * authorized to, or violates a business rule. + * + * Default level: info + */ +export class UserError extends BaseError { + readonly description: string | null | undefined; + + constructor(message: string, opts: UserErrorOptions = {}) { + opts.level = opts.level ?? 'info'; + + super(message, opts); + } +} diff --git a/packages/workflow/src/errors/error.types.ts b/packages/workflow/src/errors/error.types.ts new file mode 100644 index 0000000000..54435298ec --- /dev/null +++ b/packages/workflow/src/errors/error.types.ts @@ -0,0 +1,14 @@ +import type { Event } from '@sentry/node'; + +export type ErrorLevel = 'fatal' | 'error' | 'warning' | 'info'; + +export type ErrorTags = NonNullable; + +export type ReportingOptions = { + /** Whether the error should be reported to Sentry */ + shouldReport?: boolean; + level?: ErrorLevel; + tags?: ErrorTags; + extra?: Event['extra']; + executionId?: string; +}; diff --git a/packages/workflow/src/errors/index.ts b/packages/workflow/src/errors/index.ts index 653f4d2f00..fd501dc566 100644 --- a/packages/workflow/src/errors/index.ts +++ b/packages/workflow/src/errors/index.ts @@ -1,4 +1,9 @@ -export { ApplicationError, type ReportingOptions } from './application.error'; +export type * from './error.types'; +export { BaseError, type BaseErrorOptions } from './base/base.error'; +export { OperationalError, type OperationalErrorOptions } from './base/operational.error'; +export { UnexpectedError, type UnexpectedErrorOptions } from './base/unexpected.error'; +export { UserError, type UserErrorOptions } from './base/user.error'; +export { ApplicationError } from './application.error'; export { ExpressionError } from './expression.error'; export { CredentialAccessError } from './credential-access-error'; export { ExecutionCancelledError } from './execution-cancelled.error'; diff --git a/packages/workflow/src/errors/node-api.error.ts b/packages/workflow/src/errors/node-api.error.ts index b631fe933b..8c3750ee92 100644 --- a/packages/workflow/src/errors/node-api.error.ts +++ b/packages/workflow/src/errors/node-api.error.ts @@ -5,7 +5,7 @@ import { AxiosError } from 'axios'; import { parseString } from 'xml2js'; import { NodeError } from './abstract/node.error'; -import type { ReportingOptions } from './application.error'; +import type { ErrorLevel } from './error.types'; import { NO_OP_NODE_TYPE, UNKNOWN_ERROR_DESCRIPTION, @@ -27,7 +27,7 @@ export interface NodeOperationErrorOptions { description?: string; runIndex?: number; itemIndex?: number; - level?: ReportingOptions['level']; + level?: ErrorLevel; messageMapping?: { [key: string]: string }; // allows to pass custom mapping for error messages scoped to a node functionality?: Functionality; type?: string; diff --git a/packages/workflow/src/errors/trigger-close.error.ts b/packages/workflow/src/errors/trigger-close.error.ts index 25b5e186b8..49a05a0897 100644 --- a/packages/workflow/src/errors/trigger-close.error.ts +++ b/packages/workflow/src/errors/trigger-close.error.ts @@ -1,8 +1,9 @@ -import { ApplicationError, type Level } from './application.error'; +import { ApplicationError } from './application.error'; +import type { ErrorLevel } from './error.types'; import type { INode } from '../Interfaces'; interface TriggerCloseErrorOptions extends ErrorOptions { - level: Level; + level: ErrorLevel; } export class TriggerCloseError extends ApplicationError { diff --git a/packages/workflow/test/errors/base/operational.error.test.ts b/packages/workflow/test/errors/base/operational.error.test.ts new file mode 100644 index 0000000000..510acb9fb2 --- /dev/null +++ b/packages/workflow/test/errors/base/operational.error.test.ts @@ -0,0 +1,26 @@ +import { BaseError } from '@/errors/base/base.error'; +import { OperationalError } from '@/errors/base/operational.error'; + +describe('OperationalError', () => { + it('should be an instance of OperationalError', () => { + const error = new OperationalError('test'); + expect(error).toBeInstanceOf(OperationalError); + }); + + it('should be an instance of BaseError', () => { + const error = new OperationalError('test'); + expect(error).toBeInstanceOf(BaseError); + }); + + it('should have correct defaults', () => { + const error = new OperationalError('test'); + expect(error.level).toBe('warning'); + expect(error.shouldReport).toBe(false); + }); + + it('should allow overriding the default level and shouldReport', () => { + const error = new OperationalError('test', { level: 'error', shouldReport: true }); + expect(error.level).toBe('error'); + expect(error.shouldReport).toBe(true); + }); +}); diff --git a/packages/workflow/test/errors/base/unexpected.error.test.ts b/packages/workflow/test/errors/base/unexpected.error.test.ts new file mode 100644 index 0000000000..b78da22895 --- /dev/null +++ b/packages/workflow/test/errors/base/unexpected.error.test.ts @@ -0,0 +1,26 @@ +import { BaseError } from '@/errors/base/base.error'; +import { UnexpectedError } from '@/errors/base/unexpected.error'; + +describe('UnexpectedError', () => { + it('should be an instance of UnexpectedError', () => { + const error = new UnexpectedError('test'); + expect(error).toBeInstanceOf(UnexpectedError); + }); + + it('should be an instance of BaseError', () => { + const error = new UnexpectedError('test'); + expect(error).toBeInstanceOf(BaseError); + }); + + it('should have correct defaults', () => { + const error = new UnexpectedError('test'); + expect(error.level).toBe('error'); + expect(error.shouldReport).toBe(true); + }); + + it('should allow overriding the default level and shouldReport', () => { + const error = new UnexpectedError('test', { level: 'fatal', shouldReport: false }); + expect(error.level).toBe('fatal'); + expect(error.shouldReport).toBe(false); + }); +}); diff --git a/packages/workflow/test/errors/base/user.error.test.ts b/packages/workflow/test/errors/base/user.error.test.ts new file mode 100644 index 0000000000..83c681eb11 --- /dev/null +++ b/packages/workflow/test/errors/base/user.error.test.ts @@ -0,0 +1,26 @@ +import { BaseError } from '@/errors/base/base.error'; +import { UserError } from '@/errors/base/user.error'; + +describe('UserError', () => { + it('should be an instance of UserError', () => { + const error = new UserError('test'); + expect(error).toBeInstanceOf(UserError); + }); + + it('should be an instance of BaseError', () => { + const error = new UserError('test'); + expect(error).toBeInstanceOf(BaseError); + }); + + it('should have correct defaults', () => { + const error = new UserError('test'); + expect(error.level).toBe('info'); + expect(error.shouldReport).toBe(false); + }); + + it('should allow overriding the default level and shouldReport', () => { + const error = new UserError('test', { level: 'warning', shouldReport: true }); + expect(error.level).toBe('warning'); + expect(error.shouldReport).toBe(true); + }); +});