From 67702c2485d32ba21960868a36c14d585d733ea5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 30 Nov 2023 12:46:45 +0100 Subject: [PATCH] refactor(core): Switch plain errors in `workflow` to `ApplicationError` (no-changelog) (#7877) Ensure all errors in `workflow` are `ApplicationError` or children of it and contain no variables in the message, to continue normalizing all the errors we report to Sentry Follow-up to: https://github.com/n8n-io/n8n/pull/7873 --- packages/workflow/src/ErrorReporterProxy.ts | 2 +- packages/workflow/src/Expression.ts | 13 +++--- packages/workflow/src/NodeHelpers.ts | 46 ++++++++++++------- packages/workflow/src/TelemetryHelpers.ts | 3 +- packages/workflow/src/Workflow.ts | 50 ++++++++++++++------- packages/workflow/src/WorkflowDataProxy.ts | 11 +++-- packages/workflow/src/utils.ts | 3 +- packages/workflow/test/Helpers.ts | 36 +++++++++------ packages/workflow/test/utils.test.ts | 7 ++- 9 files changed, 110 insertions(+), 61 deletions(-) diff --git a/packages/workflow/src/ErrorReporterProxy.ts b/packages/workflow/src/ErrorReporterProxy.ts index 460e2def85076..75c1b9fef313c 100644 --- a/packages/workflow/src/ErrorReporterProxy.ts +++ b/packages/workflow/src/ErrorReporterProxy.ts @@ -24,7 +24,7 @@ export function init(errorReporter: ErrorReporter) { const wrap = (e: unknown) => { if (e instanceof Error) return e; - if (typeof e === 'string') return new Error(e); + if (typeof e === 'string') return new ApplicationError(e); return; }; diff --git a/packages/workflow/src/Expression.ts b/packages/workflow/src/Expression.ts index 2511085b77c5b..c5e65f926b20c 100644 --- a/packages/workflow/src/Expression.ts +++ b/packages/workflow/src/Expression.ts @@ -25,6 +25,7 @@ import { extendedFunctions } from './Extensions/ExtendedFunctions'; import { extendSyntax } from './Extensions/ExpressionExtension'; import { evaluateExpression, setErrorHandler } from './ExpressionEvaluatorProxy'; import { getGlobalState } from './GlobalState'; +import { ApplicationError } from './errors/application.error'; const IS_FRONTEND_IN_DEV_MODE = typeof process === 'object' && @@ -75,7 +76,7 @@ export class Expression { const typeName = Array.isArray(value) ? 'Array' : 'Object'; if (value instanceof DateTime && value.invalidReason !== null) { - throw new Error('invalid DateTime'); + throw new ApplicationError('invalid DateTime'); } let result = ''; @@ -310,12 +311,12 @@ export class Expression { const extendedExpression = extendSyntax(parameterValue); const returnValue = this.renderExpression(extendedExpression, data); if (typeof returnValue === 'function') { - if (returnValue.name === '$') throw new Error('invalid syntax'); + if (returnValue.name === '$') throw new ApplicationError('invalid syntax'); if (returnValue.name === 'DateTime') - throw new Error('this is a DateTime, please access its methods'); + throw new ApplicationError('this is a DateTime, please access its methods'); - throw new Error('this is a function, please add ()'); + throw new ApplicationError('this is a function, please add ()'); } else if (typeof returnValue === 'string') { return returnValue; } else if (returnValue !== null && typeof returnValue === 'object') { @@ -339,14 +340,14 @@ export class Expression { } catch (error) { if (isExpressionError(error)) throw error; - if (isSyntaxError(error)) throw new Error('invalid syntax'); + if (isSyntaxError(error)) throw new ApplicationError('invalid syntax'); if (isTypeError(error) && IS_FRONTEND && error.message.endsWith('is not a function')) { const match = error.message.match(/(?[^.]+is not a function)/); if (!match?.groups?.msg) return null; - throw new Error(match.groups.msg); + throw new ApplicationError(match.groups.msg); } } finally { Object.defineProperty(Function.prototype, 'constructor', { value: fnConstructors.sync }); diff --git a/packages/workflow/src/NodeHelpers.ts b/packages/workflow/src/NodeHelpers.ts index ae46dc2fdb247..32c6e3ce35a20 100644 --- a/packages/workflow/src/NodeHelpers.ts +++ b/packages/workflow/src/NodeHelpers.ts @@ -48,6 +48,7 @@ import { deepCopy } from './utils'; import { DateTime } from 'luxon'; import type { Workflow } from './Workflow'; +import { ApplicationError } from './errors/application.error'; export const cronNodeOptions: INodePropertyCollection[] = [ { @@ -416,7 +417,7 @@ export function getContext( ): IContextObject { if (runExecutionData.executionData === undefined) { // TODO: Should not happen leave it for test now - throw new Error('The "executionData" is not initialized!'); + throw new ApplicationError('`executionData` is not initialized'); } let key: string; @@ -424,11 +425,16 @@ export function getContext( key = 'flow'; } else if (type === 'node') { if (node === undefined) { - throw new Error('The request data of context type "node" the node parameter has to be set!'); + // @TODO: What does this mean? + throw new ApplicationError( + 'The request data of context type "node" the node parameter has to be set!', + ); } key = `node:${node.name}`; } else { - throw new Error(`The context type "${type}" is not know. Only "flow" and node" are supported!`); + throw new ApplicationError('Unknown context type. Only `flow` and `node` are supported.', { + extra: { contextType: type }, + }); } if (runExecutionData.executionData.contextData[key] === undefined) { @@ -530,7 +536,7 @@ export function getParameterResolveOrder( } if (iterations > lastIndexReduction + nodePropertiesArray.length) { - throw new Error( + throw new ApplicationError( 'Could not resolve parameter dependencies. Max iterations reached! Hint: If `displayOptions` are specified in any child parameter of a parent `collection` or `fixedCollection`, remove the `displayOptions` from the child parameter.', ); } @@ -773,9 +779,9 @@ export function getNodeParameters( ) as INodePropertyCollection; if (nodePropertyOptions === undefined) { - throw new Error( - `Could not find property option "${itemName}" for "${nodeProperties.name}"`, - ); + throw new ApplicationError('Could not find property option', { + extra: { propertyOption: itemName, property: nodeProperties.name }, + }); } tempNodePropertiesArray = nodePropertyOptions.values!; @@ -1054,7 +1060,9 @@ export function getNodeInputs( {}, ) || []) as ConnectionTypes[]; } catch (e) { - throw new Error(`Could not calculate inputs dynamically for node "${node.name}"`); + throw new ApplicationError('Could not calculate inputs dynamically for node', { + extra: { nodeName: node.name }, + }); } } @@ -1077,7 +1085,9 @@ export function getNodeOutputs( {}, ) || []) as ConnectionTypes[]; } catch (e) { - throw new Error(`Could not calculate outputs dynamically for node "${node.name}"`); + throw new ApplicationError('Could not calculate outputs dynamically for node', { + extra: { nodeName: node.name }, + }); } } @@ -1258,7 +1268,7 @@ export const tryToParseNumber = (value: unknown): number => { const isValidNumber = !isNaN(Number(value)); if (!isValidNumber) { - throw new Error(`Could not parse '${String(value)}' to number.`); + throw new ApplicationError('Failed to parse value to number', { extra: { value } }); } return Number(value); }; @@ -1282,7 +1292,9 @@ export const tryToParseBoolean = (value: unknown): value is boolean => { } } - throw new Error(`Could not parse '${String(value)}' to boolean.`); + throw new ApplicationError('Failed to parse value as boolean', { + extra: { value }, + }); }; export const tryToParseDateTime = (value: unknown): DateTime => { @@ -1306,7 +1318,7 @@ export const tryToParseDateTime = (value: unknown): DateTime => { return sqlDate; } - throw new Error(`The value "${dateString}" is not a valid date.`); + throw new ApplicationError('Value is not a valid date', { extra: { dateString } }); }; export const tryToParseTime = (value: unknown): string => { @@ -1314,7 +1326,7 @@ export const tryToParseTime = (value: unknown): string => { String(value), ); if (!isTimeInput) { - throw new Error(`The value "${String(value)}" is not a valid time.`); + throw new ApplicationError('Value is not a valid time', { extra: { value } }); } return String(value); }; @@ -1333,11 +1345,11 @@ export const tryToParseArray = (value: unknown): unknown[] => { } if (!Array.isArray(parsed)) { - throw new Error(`The value "${String(value)}" is not a valid array.`); + throw new ApplicationError('Value is not a valid array', { extra: { value } }); } return parsed; } catch (e) { - throw new Error(`The value "${String(value)}" is not a valid array.`); + throw new ApplicationError('Value is not a valid array', { extra: { value } }); } }; @@ -1348,11 +1360,11 @@ export const tryToParseObject = (value: unknown): object => { try { const o = JSON.parse(String(value)); if (typeof o !== 'object' || Array.isArray(o)) { - throw new Error(`The value "${String(value)}" is not a valid object.`); + throw new ApplicationError('Value is not a valid object', { extra: { value } }); } return o; } catch (e) { - throw new Error(`The value "${String(value)}" is not a valid object.`); + throw new ApplicationError('Value is not a valid object', { extra: { value } }); } }; diff --git a/packages/workflow/src/TelemetryHelpers.ts b/packages/workflow/src/TelemetryHelpers.ts index c2eb49dee27c6..058b76a42627f 100644 --- a/packages/workflow/src/TelemetryHelpers.ts +++ b/packages/workflow/src/TelemetryHelpers.ts @@ -9,6 +9,7 @@ import type { INodeTypes, INodeType, } from './Interfaces'; +import { ApplicationError } from './errors/application.error'; const STICKY_NODE_TYPE = 'n8n-nodes-base.stickyNote'; @@ -95,7 +96,7 @@ export function getDomainPath(raw: string, urlParts = URL_PARTS_REGEX): string { try { const url = new URL(raw); - if (!url.hostname) throw new Error('Malformed URL'); + if (!url.hostname) throw new ApplicationError('Malformed URL'); return sanitizeRoute(url.pathname); } catch { diff --git a/packages/workflow/src/Workflow.ts b/packages/workflow/src/Workflow.ts index ea146900b0103..6ec06db20cadd 100644 --- a/packages/workflow/src/Workflow.ts +++ b/packages/workflow/src/Workflow.ts @@ -51,6 +51,7 @@ import * as ObservableObject from './ObservableObject'; import { RoutingNode } from './RoutingNode'; import { Expression } from './Expression'; import { NODES_WITH_RENAMABLE_CONTENT } from './Constants'; +import { ApplicationError } from './errors/application.error'; function dedupe(arr: T[]): T[] { return [...new Set(arr)]; @@ -113,7 +114,10 @@ export class Workflow { // expression resolution also then when the unknown node // does not get used. continue; - // throw new Error(`The node type "${node.type}" of node "${node.name}" is not known.`); + // throw new ApplicationError(`Node with unknown node type`, { + // tags: { nodeType: node.type }, + // extra: { node }, + // }); } // Add default values @@ -312,15 +316,15 @@ export class Workflow { key = 'global'; } else if (type === 'node') { if (node === undefined) { - throw new Error( + throw new ApplicationError( 'The request data of context type "node" the node parameter has to be set!', ); } key = `node:${node.name}`; } else { - throw new Error( - `The context type "${type}" is not know. Only "global" and node" are supported!`, - ); + throw new ApplicationError('Unknown context type. Only `global` and `node` are supported.', { + extra: { contextType: type }, + }); } if (this.staticData[key] === undefined) { @@ -1092,13 +1096,17 @@ export class Workflow { const nodeType = this.nodeTypes.getByNameAndVersion(node.type, node.typeVersion); if (nodeType === undefined) { - throw new Error(`The node type "${node.type}" of node "${node.name}" is not known.`); + throw new ApplicationError('Node with unknown node type', { + extra: { nodeName: node.name }, + tags: { nodeType: node.type }, + }); } if (!nodeType.trigger) { - throw new Error( - `The node type "${node.type}" of node "${node.name}" does not have a trigger function defined.`, - ); + throw new ApplicationError('Node type does not have a trigger function defined', { + extra: { nodeName: node.name }, + tags: { nodeType: node.type }, + }); } if (mode === 'manual') { @@ -1169,13 +1177,17 @@ export class Workflow { const nodeType = this.nodeTypes.getByNameAndVersion(node.type, node.typeVersion); if (nodeType === undefined) { - throw new Error(`The node type "${node.type}" of node "${node.name}" is not known.`); + throw new ApplicationError('Node with unknown node type', { + extra: { nodeName: node.name }, + tags: { nodeType: node.type }, + }); } if (!nodeType.poll) { - throw new Error( - `The node type "${node.type}" of node "${node.name}" does not have a poll function defined.`, - ); + throw new ApplicationError('Node type does not have a poll function defined', { + extra: { nodeName: node.name }, + tags: { nodeType: node.type }, + }); } return nodeType.poll.call(pollFunctions); @@ -1195,9 +1207,13 @@ export class Workflow { ): Promise { const nodeType = this.nodeTypes.getByNameAndVersion(node.type, node.typeVersion); if (nodeType === undefined) { - throw new Error(`The type of the webhook node "${node.name}" is not known.`); + throw new ApplicationError('Unknown node type of webhook node', { + extra: { nodeName: node.name }, + }); } else if (nodeType.webhook === undefined) { - throw new Error(`The node "${node.name}" does not have any webhooks defined.`); + throw new ApplicationError('Node does not have any webhooks defined', { + extra: { nodeName: node.name }, + }); } const context = nodeExecuteFunctions.getExecuteWebhookFunctions( @@ -1241,7 +1257,9 @@ export class Workflow { const nodeType = this.nodeTypes.getByNameAndVersion(node.type, node.typeVersion); if (nodeType === undefined) { - throw new Error(`Node type "${node.type}" is not known so can not run it!`); + throw new ApplicationError('Node type is unknown so cannot run it', { + tags: { nodeType: node.type }, + }); } let connectionInputData: INodeExecutionData[] = []; diff --git a/packages/workflow/src/WorkflowDataProxy.ts b/packages/workflow/src/WorkflowDataProxy.ts index 3ead091171539..96c77452a2f9c 100644 --- a/packages/workflow/src/WorkflowDataProxy.ts +++ b/packages/workflow/src/WorkflowDataProxy.ts @@ -29,6 +29,7 @@ import type { Workflow } from './Workflow'; import { augmentArray, augmentObject } from './AugmentObject'; import { deepCopy } from './utils'; import { getGlobalState } from './GlobalState'; +import { ApplicationError } from './errors/application.error'; export function isResourceLocatorValue(value: unknown): value is INodeParameterResourceLocator { return Boolean( @@ -190,7 +191,9 @@ export class WorkflowDataProxy { if (name[0] === '&') { const key = name.slice(1); if (!that.siblingParameters.hasOwnProperty(key)) { - throw new Error(`Could not find sibling parameter "${key}" on node "${nodeName}"`); + throw new ApplicationError('Could not find sibling parameter on node', { + extra: { nodeName, parameter: key }, + }); } returnValue = that.siblingParameters[key]; } else { @@ -300,8 +303,8 @@ export class WorkflowDataProxy { const taskData = that.runExecutionData.resultData.runData[nodeName][runIndex].data!; if (!taskData.main?.length || taskData.main[0] === null) { - // throw new Error(`No data found for item-index: "${itemIndex}"`); - throw new ExpressionError('No data found from "main" input.', { + // throw new ApplicationError('No data found for item-index', { extra: { itemIndex } }); + throw new ExpressionError('No data found from `main` input', { runIndex: that.runIndex, itemIndex: that.itemIndex, }); @@ -765,7 +768,7 @@ export class WorkflowDataProxy { if (itemInput >= taskData.source.length) { // `Could not resolve pairedItem as the defined node input '${itemInput}' does not exist on node '${sourceData!.previousNode}'.` // Actual error does not matter as it gets caught below and `null` will be returned - throw new Error('Not found'); + throw new ApplicationError('Not found'); } return getPairedItem(destinationNodeName, taskData.source[itemInput], item); diff --git a/packages/workflow/src/utils.ts b/packages/workflow/src/utils.ts index 790903c1b7b97..2f9f17743e22a 100644 --- a/packages/workflow/src/utils.ts +++ b/packages/workflow/src/utils.ts @@ -1,5 +1,6 @@ import FormData from 'form-data'; import type { BinaryFileType, JsonObject } from './Interfaces'; +import { ApplicationError } from './errors/application.error'; const readStreamClasses = new Set(['ReadStream', 'Readable', 'ReadableStream']); @@ -77,7 +78,7 @@ export const jsonParse = (jsonString: string, options?: JSONParseOptions): if (options?.fallbackValue !== undefined) { return options.fallbackValue; } else if (options?.errorMessage) { - throw new Error(options.errorMessage); + throw new ApplicationError(options.errorMessage); } throw error; diff --git a/packages/workflow/test/Helpers.ts b/packages/workflow/test/Helpers.ts index f5562eb5dc6a2..c076708cfe4ca 100644 --- a/packages/workflow/test/Helpers.ts +++ b/packages/workflow/test/Helpers.ts @@ -39,6 +39,7 @@ import { WorkflowHooks } from '@/WorkflowHooks'; import * as NodeHelpers from '@/NodeHelpers'; import { deepCopy } from '@/utils'; import { getGlobalState } from '@/GlobalState'; +import { ApplicationError } from '@/errors/application.error'; export interface INodeTypesObject { [key: string]: INodeType; @@ -55,14 +56,14 @@ export class Credentials extends ICredentials { getData(): ICredentialDataDecryptedObject { if (this.data === undefined) { - throw new Error('No data is set so nothing can be returned.'); + throw new ApplicationError('No data is set so nothing can be returned'); } return JSON.parse(this.data); } getDataToSave(): ICredentialsEncrypted { if (this.data === undefined) { - throw new Error('No credentials were set to save.'); + throw new ApplicationError('No credentials were set to save'); } return { @@ -135,13 +136,15 @@ export function getNodeParameter( ): NodeParameterValue | INodeParameters | NodeParameterValue[] | INodeParameters[] | object { const nodeType = workflow.nodeTypes.getByNameAndVersion(node.type, node.typeVersion); if (nodeType === undefined) { - throw new Error(`Node type "${node.type}" is not known so can not return parameter value!`); + throw new ApplicationError('Node type is unknown so cannot return parameter value', { + tags: { nodeType: node.type }, + }); } const value = get(node.parameters, parameterName, fallbackValue); if (value === undefined) { - throw new Error(`Could not get parameter "${parameterName}"!`); + throw new ApplicationError('Could not get parameter', { extra: { parameterName } }); } let returnData; @@ -211,12 +214,15 @@ export function getExecuteFunctions( } if (inputData[inputName].length < inputIndex) { - throw new Error(`Could not get input index "${inputIndex}" of input "${inputName}"!`); + throw new ApplicationError('Could not get input index', { + extra: { inputIndex, inputName }, + }); } if (inputData[inputName][inputIndex] === null) { - // return []; - throw new Error(`Value "${inputIndex}" of input "${inputName}" did not get set!`); + throw new ApplicationError('Value of input did not get set', { + extra: { inputIndex, inputName }, + }); } return inputData[inputName][inputIndex] as INodeExecutionData[]; @@ -387,21 +393,23 @@ export function getExecuteSingleFunctions( } if (inputData[inputName].length < inputIndex) { - throw new Error(`Could not get input index "${inputIndex}" of input "${inputName}"!`); + throw new ApplicationError('Could not get input index', { + extra: { inputIndex, inputName }, + }); } const allItems = inputData[inputName][inputIndex]; if (allItems === null) { - // return []; - throw new Error(`Value "${inputIndex}" of input "${inputName}" did not get set!`); + throw new ApplicationError('Value of input did not get set', { + extra: { inputIndex, inputName }, + }); } if (allItems[itemIndex] === null) { - // return []; - throw new Error( - `Value "${inputIndex}" of input "${inputName}" with itemIndex "${itemIndex}" did not get set!`, - ); + throw new ApplicationError('Value of input with item index did not get set', { + extra: { inputIndex, inputName, itemIndex }, + }); } return allItems[itemIndex]; diff --git a/packages/workflow/test/utils.test.ts b/packages/workflow/test/utils.test.ts index ce481d22cb896..a0867f5517667 100644 --- a/packages/workflow/test/utils.test.ts +++ b/packages/workflow/test/utils.test.ts @@ -1,3 +1,4 @@ +import { ApplicationError } from '@/errors/application.error'; import { jsonParse, jsonStringify, deepCopy, isObjectEmpty, fileTypeFromMimeType } from '@/utils'; describe('isObjectEmpty', () => { @@ -58,7 +59,11 @@ describe('isObjectEmpty', () => { const { calls } = keySpy.mock; const assertCalls = (count: number) => { - if (calls.length !== count) throw new Error(`Object.keys was called ${calls.length} times`); + if (calls.length !== count) { + throw new ApplicationError('`Object.keys()` was called an unexpected number of times', { + extra: { times: calls.length }, + }); + } }; assertCalls(0);