Skip to content
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

refactor(core): Reorganize error hierarchy in core and workflow packages (no-changelog) #7820

Merged
merged 7 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/cli/src/ActiveWebhooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
WorkflowActivateMode,
WorkflowExecuteMode,
} from 'n8n-workflow';
import { WebhookPathAlreadyTakenError } from 'n8n-workflow';
import { WebhookTakenError } from 'n8n-workflow';
netroy marked this conversation as resolved.
Show resolved Hide resolved
import * as NodeExecuteFunctions from 'n8n-core';

@Service()
Expand Down Expand Up @@ -46,7 +46,7 @@ export class ActiveWebhooks {

// check that there is not a webhook already registered with that path/method
if (this.webhookUrls[webhookKey] && !webhookData.webhookId) {
throw new WebhookPathAlreadyTakenError(webhookData.node);
throw new WebhookTakenError(webhookData.node);
}

if (this.workflowWebhooks[webhookData.workflowId] === undefined) {
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/ActiveWorkflowRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
Workflow,
WorkflowActivationError,
ErrorReporterProxy as ErrorReporter,
WebhookPathAlreadyTakenError,
WebhookTakenError,
} from 'n8n-workflow';

import type express from 'express';
Expand Down Expand Up @@ -403,7 +403,7 @@ export class ActiveWorkflowRunner implements IWebhookManager {
// TODO check if there is standard error code for duplicate key violation that works
// with all databases
if (error instanceof Error && error.name === 'QueryFailedError') {
error = new WebhookPathAlreadyTakenError(webhook.node, error);
error = new WebhookTakenError(webhook.node, error);
} else if (error.detail) {
// it's a error running the webhook methods (checkExists, create)
error.message = error.detail;
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/ErrorReporting.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createHash } from 'crypto';
import config from '@/config';
import { ErrorReporterProxy, ExecutionBaseError, ReportableError } from 'n8n-workflow';
import { ErrorReporterProxy, ApplicationError, ExecutionBaseError } from 'n8n-workflow';

let initialized = false;

Expand Down Expand Up @@ -42,7 +42,7 @@ export const initErrorHandling = async () => {
if (originalException instanceof ExecutionBaseError && originalException.severity === 'warning')
return null;

if (originalException instanceof ReportableError) {
if (originalException instanceof ApplicationError) {
Comment on lines 42 to +45
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ExecutionBaseError now extends ApplicationError, these checks could be unified, and we can most likely remove the ExecutionBaseError import all together here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 };
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this in another PR.

const { level, extra } = originalException;
if (level === 'warning') return null;
event.level = level;
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/BinaryData/BinaryData.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import { readFile, stat } from 'node:fs/promises';
import prettyBytes from 'pretty-bytes';
import Container, { Service } from 'typedi';
import { BINARY_ENCODING } from 'n8n-workflow';
import { UnknownManagerError, InvalidModeError } from './errors';
import { InvalidModeError } from '../errors/invalid-mode.error';
import { areConfigModes, toBuffer } from './utils';

import type { Readable } from 'stream';
import type { BinaryData } from './types';
import type { INodeExecutionData, IBinaryData } from 'n8n-workflow';
import { InvalidManagerError } from '../errors/invalid-manager.error';

@Service()
export class BinaryDataService {
Expand Down Expand Up @@ -241,6 +242,6 @@ export class BinaryDataService {

if (manager) return manager;

throw new UnknownManagerError(mode);
throw new InvalidManagerError(mode);
}
}
11 changes: 6 additions & 5 deletions packages/core/src/BinaryData/FileSystem.manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ import path from 'node:path';
import { v4 as uuid } from 'uuid';
import { jsonParse } from 'n8n-workflow';
import { assertDir, doesNotExist } from './utils';
import { BinaryFileNotFoundError, InvalidPathError } from '../errors';
import { DisallowedFilepathError } from '../errors/disallowed-filepath.error';

import type { Readable } from 'stream';
import type { BinaryData } from './types';
import { FileNotFoundError } from '../errors/file-not-found.error';

const EXECUTION_ID_EXTRACTOR =
/^(\w+)(?:[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/;
Expand Down Expand Up @@ -47,7 +48,7 @@ export class FileSystemManager implements BinaryData.Manager {
const filePath = this.resolvePath(fileId);

if (await doesNotExist(filePath)) {
throw new BinaryFileNotFoundError(filePath);
throw new FileNotFoundError(filePath);
}

return createReadStream(filePath, { highWaterMark: chunkSize });
Expand All @@ -57,7 +58,7 @@ export class FileSystemManager implements BinaryData.Manager {
const filePath = this.resolvePath(fileId);

if (await doesNotExist(filePath)) {
throw new BinaryFileNotFoundError(filePath);
throw new FileNotFoundError(filePath);
}

return fs.readFile(filePath);
Expand Down Expand Up @@ -171,7 +172,7 @@ export class FileSystemManager implements BinaryData.Manager {
const returnPath = path.join(this.storagePath, ...args);

if (path.relative(this.storagePath, returnPath).startsWith('..')) {
throw new InvalidPathError(returnPath);
throw new DisallowedFilepathError(returnPath);
}

return returnPath;
Expand All @@ -190,7 +191,7 @@ export class FileSystemManager implements BinaryData.Manager {
const stats = await fs.stat(filePath);
return stats.size;
} catch (error) {
throw new BinaryFileNotFoundError(filePath);
throw new FileNotFoundError(filePath);
}
}
}
11 changes: 0 additions & 11 deletions packages/core/src/BinaryData/errors.ts

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,20 +1,9 @@
import type { IRunExecutionData } from 'n8n-workflow';
import { LoggerProxy as Logger } from 'n8n-workflow';
import { InvalidExecutionMetadata } from './errors/invalid-execution-metadata.error';

export const KV_LIMIT = 10;

export class ExecutionMetadataValidationError extends Error {
constructor(
public type: 'key' | 'value',
key: unknown,
message?: string,
options?: ErrorOptions,
) {
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
super(message ?? `Custom data ${type}s must be a string (key "${key}")`, options);
}
}

export function setWorkflowExecutionMetadata(
executionData: IRunExecutionData,
key: string,
Expand All @@ -31,17 +20,17 @@ export function setWorkflowExecutionMetadata(
return;
}
if (typeof key !== 'string') {
throw new ExecutionMetadataValidationError('key', key);
throw new InvalidExecutionMetadata('key', key);
}
if (key.replace(/[A-Za-z0-9_]/g, '').length !== 0) {
throw new ExecutionMetadataValidationError(
throw new InvalidExecutionMetadata(
'key',
key,
`Custom date key can only contain characters "A-Za-z0-9_" (key "${key}")`,
);
}
if (typeof value !== 'string' && typeof value !== 'number' && typeof value !== 'bigint') {
throw new ExecutionMetadataValidationError('value', key);
throw new InvalidExecutionMetadata('value', key);
}
const val = String(value);
if (key.length > 50) {
Expand Down
11 changes: 5 additions & 6 deletions packages/core/src/NodeExecuteFunctions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import type {
BinaryHelperFunctions,
ConnectionTypes,
ContextType,
ExecutionError,
FieldType,
FileSystemHelperFunctions,
FunctionsBase,
Expand Down Expand Up @@ -101,7 +100,7 @@ import {
NodeApiError,
NodeHelpers,
NodeOperationError,
NodeSSLError,
NodeSslError,
OAuth2GrantType,
WorkflowDataProxy,
createDeferredPromise,
Expand Down Expand Up @@ -143,7 +142,7 @@ import {
getWorkflowExecutionMetadata,
setAllWorkflowExecutionMetadata,
setWorkflowExecutionMetadata,
} from './WorkflowExecutionMetadata';
} from './ExecutionMetadata';
import { getSecretsProxy } from './Secrets';
import Container from 'typedi';
import type { BinaryData } from './BinaryData/types';
Expand Down Expand Up @@ -808,7 +807,7 @@ export async function proxyRequestToAxios(
response: pick(response, ['headers', 'status', 'statusText']),
});
} else if ('rejectUnauthorized' in configObject && error.code?.includes('CERT')) {
throw new NodeSSLError(error);
throw new NodeSslError(error);
}
}

Expand Down Expand Up @@ -3442,7 +3441,7 @@ export function getExecuteFunctions(

addInputData(
connectionType: ConnectionTypes,
data: INodeExecutionData[][] | ExecutionError,
data: INodeExecutionData[][] | ExecutionBaseError,
): { index: number } {
const nodeName = this.getNode().name;
let currentNodeRunIndex = 0;
Expand Down Expand Up @@ -3473,7 +3472,7 @@ export function getExecuteFunctions(
addOutputData(
connectionType: ConnectionTypes,
currentNodeRunIndex: number,
data: INodeExecutionData[][] | ExecutionError,
data: INodeExecutionData[][] | ExecutionBaseError,
): void {
addExecutionDataFunctions(
'output',
Expand Down
30 changes: 20 additions & 10 deletions packages/core/src/WorkflowExecute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { setMaxListeners } from 'events';
import PCancelable from 'p-cancelable';

import type {
ExecutionError,
ExecutionBaseError,
ExecutionStatus,
GenericValue,
IConnection,
Expand Down Expand Up @@ -797,7 +797,7 @@ export class WorkflowExecute {

// Variables which hold temporary data for each node-execution
let executionData: IExecuteData;
let executionError: ExecutionError | undefined;
let executionError: ExecutionBaseError | undefined;
let executionNode: INode;
let nodeSuccessData: INodeExecutionData[][] | null | undefined;
let runIndex: number;
Expand Down Expand Up @@ -838,11 +838,13 @@ export class WorkflowExecute {
await this.executeHook('workflowExecuteBefore', [workflow]);
}
} catch (error) {
const e = error as unknown as ExecutionBaseError;

// Set the error that it can be saved correctly
executionError = {
...(error as NodeOperationError | NodeApiError),
message: (error as NodeOperationError | NodeApiError).message,
stack: (error as NodeOperationError | NodeApiError).stack,
...e,
message: e.message,
stack: e.stack,
};

// Set the incoming data of the node that it can be saved correctly
Expand Down Expand Up @@ -1253,10 +1255,12 @@ export class WorkflowExecute {
} catch (error) {
this.runExecutionData.resultData.lastNodeExecuted = executionData.node.name;

const e = error as unknown as ExecutionBaseError;

executionError = {
...(error as NodeOperationError | NodeApiError),
message: (error as NodeOperationError | NodeApiError).message,
stack: (error as NodeOperationError | NodeApiError).stack,
...e,
message: e.message,
stack: e.stack,
};

Logger.debug(`Running node "${executionNode.name}" finished with error`, {
Expand Down Expand Up @@ -1325,11 +1329,17 @@ export class WorkflowExecute {
lineResult.json.$error !== undefined &&
lineResult.json.$json !== undefined
) {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
lineResult.error = lineResult.json.$error as NodeApiError | NodeOperationError;
lineResult.json = {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
error: (lineResult.json.$error as NodeApiError | NodeOperationError).message,
};
} else if (lineResult.error !== undefined) {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
lineResult.json = { error: lineResult.error.message };
}
}
Expand Down Expand Up @@ -1697,7 +1707,7 @@ export class WorkflowExecute {
async processSuccessExecution(
startedAt: Date,
workflow: Workflow,
executionError?: ExecutionError,
executionError?: ExecutionBaseError,
closeFunction?: Promise<void>,
): Promise<IRun> {
const fullRunData = this.getFullRunData(startedAt);
Expand All @@ -1711,7 +1721,7 @@ export class WorkflowExecute {
...executionError,
message: executionError.message,
stack: executionError.stack,
} as ExecutionError;
} as ExecutionBaseError;
if (executionError.message?.includes('canceled')) {
fullRunData.status = 'canceled';
}
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/errors/abstract/binary-data.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { ApplicationError } from 'n8n-workflow';

export abstract class BinaryDataError extends ApplicationError {}
7 changes: 7 additions & 0 deletions packages/core/src/errors/abstract/filesystem.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { ApplicationError } from 'n8n-workflow';

export abstract class FileSystemError extends ApplicationError {
constructor(message: string, filePath: string) {
super(message, { extra: { filePath } });
}
}
7 changes: 7 additions & 0 deletions packages/core/src/errors/disallowed-filepath.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { FileSystemError } from './abstract/filesystem.error';

export class DisallowedFilepathError extends FileSystemError {
constructor(filePath: string) {
super('Disallowed path detected', filePath);
}
}
7 changes: 7 additions & 0 deletions packages/core/src/errors/file-not-found.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { FileSystemError } from './abstract/filesystem.error';

export class FileNotFoundError extends FileSystemError {
constructor(filePath: string) {
super('File not found', filePath);
}
}
21 changes: 0 additions & 21 deletions packages/core/src/errors/filesystem.errors.ts

This file was deleted.

6 changes: 5 additions & 1 deletion packages/core/src/errors/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
export { BinaryFileNotFoundError, FileNotFoundError, InvalidPathError } from './filesystem.errors';
export { FileNotFoundError } from './file-not-found.error';
export { DisallowedFilepathError } from './disallowed-filepath.error';
export { InvalidModeError } from './invalid-mode.error';
export { InvalidManagerError } from './invalid-manager.error';
export { InvalidExecutionMetadata } from './invalid-execution-metadata.error';
13 changes: 13 additions & 0 deletions packages/core/src/errors/invalid-execution-metadata.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { ApplicationError } from 'n8n-workflow';

export class InvalidExecutionMetadata extends ApplicationError {
constructor(
public type: 'key' | 'value',
key: unknown,
message?: string,
options?: ErrorOptions,
) {
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
super(message ?? `Custom data ${type}s must be a string (key "${key}")`, options);
}
}
7 changes: 7 additions & 0 deletions packages/core/src/errors/invalid-manager.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { BinaryDataError } from './abstract/binary-data.error';

export class InvalidManagerError extends BinaryDataError {
constructor(mode: string) {
super(`No binary data manager found for: ${mode}`);
}
}
Loading
Loading