From 45461c8cb57aa22697e58c6c52a25ab1d6d633d9 Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Thu, 21 Mar 2024 16:59:22 +0100 Subject: [PATCH] fix: Overhaul expression error messages related to paired item (#8765) --- .../src/components/Error/NodeErrorView.vue | 31 ++- packages/editor-ui/src/constants.ts | 2 + .../src/plugins/i18n/locales/en.json | 11 ++ packages/workflow/src/WorkflowDataProxy.ts | 181 ++++++++---------- .../workflow/src/errors/expression.error.ts | 2 + .../workflow/test/WorkflowDataProxy.test.ts | 10 +- 6 files changed, 135 insertions(+), 102 deletions(-) diff --git a/packages/editor-ui/src/components/Error/NodeErrorView.vue b/packages/editor-ui/src/components/Error/NodeErrorView.vue index 6496f7abc07f6..f893a326477e7 100644 --- a/packages/editor-ui/src/components/Error/NodeErrorView.vue +++ b/packages/editor-ui/src/components/Error/NodeErrorView.vue @@ -22,6 +22,7 @@ import { sanitizeHtml } from '@/utils/htmlUtils'; import { useAIStore } from '@/stores/ai.store'; import { MAX_DISPLAY_DATA_SIZE } from '@/constants'; import VueMarkdown from 'vue-markdown-render'; +import type { BaseTextKey } from '@/plugins/i18n'; const props = defineProps({ error: { @@ -188,6 +189,21 @@ function getErrorDescription(): string { }), ); } + + if (props.error.context?.descriptionKey) { + const interpolate = { + nodeCause: props.error.context.nodeCause as string, + runIndex: (props.error.context.runIndex as string) ?? '0', + itemIndex: (props.error.context.itemIndex as string) ?? '0', + }; + return sanitizeHtml( + i18n.baseText( + `nodeErrorView.description.${props.error.context.descriptionKey as string}` as BaseTextKey, + { interpolate }, + ), + ); + } + if (!props.error.context?.descriptionTemplate) { return sanitizeHtml(props.error.description ?? ''); } @@ -397,7 +413,7 @@ function copySuccess() {
@@ -661,6 +677,19 @@ function copySuccess() { &__header-description { padding: 0 var(--spacing-s) var(--spacing-3xs) var(--spacing-s); font-size: var(--font-size-xs); + + ul { + padding: var(--spacing-s) 0; + padding-left: var(--spacing-l); + } + + code { + font-size: var(--font-size-xs); + color: var(--color-text-base); + background: var(--color-background-base); + padding: var(--spacing-5xs); + border-radius: var(--border-radius-base); + } } &__debugging { diff --git a/packages/editor-ui/src/constants.ts b/packages/editor-ui/src/constants.ts index 2d59fd8cd8d01..86f4c4360f722 100644 --- a/packages/editor-ui/src/constants.ts +++ b/packages/editor-ui/src/constants.ts @@ -664,6 +664,8 @@ export const ALLOWED_HTML_TAGS = [ 'a', 'br', 'i', + 'ul', + 'li', 'em', 'small', 'details', diff --git a/packages/editor-ui/src/plugins/i18n/locales/en.json b/packages/editor-ui/src/plugins/i18n/locales/en.json index 0d6455d76f3ed..aac976ad5ef66 100644 --- a/packages/editor-ui/src/plugins/i18n/locales/en.json +++ b/packages/editor-ui/src/plugins/i18n/locales/en.json @@ -1033,6 +1033,17 @@ "nodeErrorView.theErrorCauseIsTooLargeToBeDisplayed": "The error cause is too large to be displayed", "nodeErrorView.time": "Time", "nodeErrorView.inputPanel.previousNodeError.title": "Error running node '{nodeName}'", + "nodeErrorView.description.pairedItemInvalidInfo": "An expression here won't work because it uses .item and n8n can't figure out the matching item. This is because the node '{nodeCause}' returned incorrect matching information (for item {itemIndex} of run {runIndex}).

Try using .first(), .last() or .all()[index] instead of .item.", + "nodeErrorView.description.pairedItemNoInfo": "An expression here won't work because it uses .item and n8n can't figure out the matching item. The node '{nodeCause}' didn't return enough information.", + "nodeErrorView.description.pairedItemNoInfoCodeNode": "An expression here won't work because it uses .item and n8n can't figure out the matching item. You can either: ", + "nodeErrorView.description.pairedItemNoConnection": "There is no connection back to the node '{nodeCause}', but it's used in an expression here.

Please wire up the node (there can be other nodes in between).", + "nodeErrorView.description.pairedItemNoConnectionCodeNode": "There is no connection back to the node '{nodeCause}', but it's used in code here.

Please wire up the node (there can be other nodes in between).", + "nodeErrorView.description.noNodeExecutionData": "An expression references the node '{nodeCause}', but it hasn't been executed yet. Either change the expression, or re-wire your workflow to make sure that node executes first.", + "nodeErrorView.description.nodeNotFound": "The node '{nodeCause}' doesn't exist, but it's used in an expression here.", + "nodeErrorView.description.noInputConnection": "This node has no input data. Please make sure this node is connected to another node.", + "nodeErrorView.description.pairedItemMultipleMatches": "An expression here won't work because it uses .item and n8n can't figure out the matching item. (There are multiple possible matches)

Try using .first(), .last() or .all()[index] instead of .item or reference a different node.", + "nodeErrorView.description.pairedItemMultipleMatchesCodeNode": "The code here won't work because it uses .item and n8n can't figure out the matching item. (There are multiple possible matches)

Try using .first(), .last() or .all()[index] instead of .item or reference a different node.", + "nodeErrorView.description.pairedItemPinned": "The item-matching data in that node may be stale. It is needed by an expression in this node that uses .item.", "nodeErrorView.debugError.button": "Ask AI ✨", "nodeErrorView.debugError.loading": "Asking AI.. ✨", "nodeErrorView.debugError.feedback.reload": "Regenerate answer", diff --git a/packages/workflow/src/WorkflowDataProxy.ts b/packages/workflow/src/WorkflowDataProxy.ts index a5f2bb7fde86d..c4b043b67fdc3 100644 --- a/packages/workflow/src/WorkflowDataProxy.ts +++ b/packages/workflow/src/WorkflowDataProxy.ts @@ -271,9 +271,11 @@ export class WorkflowDataProxy { } if (!that.workflow.getNode(nodeName)) { - throw new ExpressionError(`"${nodeName}" node doesn't exist`, { + throw new ExpressionError("Referenced node doesn't exist", { runIndex: that.runIndex, itemIndex: that.itemIndex, + nodeCause: nodeName, + descriptionKey: 'nodeNotFound', }); } @@ -281,10 +283,11 @@ export class WorkflowDataProxy { !that.runExecutionData.resultData.runData.hasOwnProperty(nodeName) && !that.workflow.getPinDataOfNode(nodeName) ) { - throw new ExpressionError(`no data, execute "${nodeName}" node first`, { + throw new ExpressionError('Referenced node is unexecuted', { runIndex: that.runIndex, itemIndex: that.itemIndex, type: 'no_node_execution_data', + descriptionKey: 'noNodeExecutionData', nodeCause: nodeName, }); } @@ -369,7 +372,12 @@ export class WorkflowDataProxy { name = name.toString(); if (!node) { - throw new ExpressionError(`"${nodeName}" node doesn't exist`); + throw new ExpressionError("Referenced node doesn't exist", { + runIndex: that.runIndex, + itemIndex: that.itemIndex, + nodeCause: nodeName, + descriptionKey: 'nodeNotFound', + }); } if (['binary', 'data', 'json'].includes(name)) { @@ -380,8 +388,7 @@ export class WorkflowDataProxy { throw new ExpressionError('No execution data available', { messageTemplate: 'No execution data available to expression under ‘%%PARAMETER%%’', - description: - 'This node has no input data. Please make sure this node is connected to another node.', + descriptionKey: 'noInputConnection', nodeCause: nodeName, runIndex: that.runIndex, itemIndex: that.itemIndex, @@ -589,9 +596,11 @@ export class WorkflowDataProxy { const nodeName = name.toString(); if (that.workflow.getNode(nodeName) === null) { - throw new ExpressionError(`"${nodeName}" node doesn't exist`, { + throw new ExpressionError("Referenced node doesn't exist", { runIndex: that.runIndex, itemIndex: that.itemIndex, + nodeCause: nodeName, + descriptionKey: 'nodeNotFound', }); } @@ -666,10 +675,9 @@ export class WorkflowDataProxy { if (!context) { context = {}; } - message = `‘Node ${nodeName}‘ must be unpinned to execute`; + message = `Unpin '${nodeName}' to execute`; context.messageTemplate = undefined; - context.description = `To fetch the data for the expression, you must unpin the node '${nodeName}' and execute the workflow again.`; - context.descriptionTemplate = `To fetch the data for the expression under '%%PARAMETER%%', you must unpin the node '${nodeName}' and execute the workflow again.`; + context.descriptionKey = 'pairedItemPinned'; } if (context.moreInfoLink && (pinData || isScriptingNode(nodeName, that.workflow))) { @@ -688,6 +696,48 @@ export class WorkflowDataProxy { }); }; + const createInvalidPairedItemError = ({ nodeName }: { nodeName: string }) => { + return createExpressionError("Can't get data for expression", { + messageTemplate: 'Expression info invalid', + functionality: 'pairedItem', + functionOverrides: { + message: "Can't get data", + }, + nodeCause: nodeName, + descriptionKey: 'pairedItemInvalidInfo', + type: 'paired_item_invalid_info', + }); + }; + + const createMissingPairedItemError = (nodeCause: string) => { + return createExpressionError("Can't get data for expression", { + messageTemplate: 'Info for expression missing from previous node', + functionality: 'pairedItem', + functionOverrides: { + message: "Can't get data", + }, + nodeCause, + descriptionKey: isScriptingNode(nodeCause, that.workflow) + ? 'pairedItemNoInfoCodeNode' + : 'pairedItemNoInfo', + causeDetailed: `Missing pairedItem data (node '${nodeCause}' probably didn't supply it)`, + type: 'paired_item_no_info', + }); + }; + + const createNoConnectionError = (nodeCause: string) => { + return createExpressionError('Invalid expression', { + messageTemplate: 'No path back to referenced node', + functionality: 'pairedItem', + descriptionKey: isScriptingNode(nodeCause, that.workflow) + ? 'pairedItemNoConnectionCodeNode' + : 'pairedItemNoConnection', + type: 'paired_item_no_connection', + moreInfoLink: true, + nodeCause, + }); + }; + const getPairedItem = ( destinationNodeName: string, incomingSourceData: ISourceData | null, @@ -736,42 +786,15 @@ export class WorkflowDataProxy { const source = taskData?.source ?? []; if (pairedItem.item >= previousNodeOutputData.length) { - throw createExpressionError('Can’t get data for expression', { - messageTemplate: 'Can’t get data for expression under ‘%%PARAMETER%%’ field', - functionality: 'pairedItem', - functionOverrides: { - message: 'Can’t get data', - }, - nodeCause: nodeBeforeLast, - description: `In node ‘${nodeBeforeLast!}’, output item ${ - currentPairedItem.item || 0 - } ${ - sourceData.previousNodeRun - ? `of run ${(sourceData.previousNodeRun || 0).toString()} ` - : '' - }points to an input item on node ‘${ - sourceData.previousNode - }‘ that doesn’t exist.`, - type: 'paired_item_invalid_info', - moreInfoLink: true, + throw createInvalidPairedItemError({ + nodeName: sourceData.previousNode, }); } const itemPreviousNode: INodeExecutionData = previousNodeOutputData[pairedItem.item]; if (itemPreviousNode.pairedItem === undefined) { - throw createExpressionError('Can’t get data for expression', { - messageTemplate: 'Can’t get data for expression under ‘%%PARAMETER%%’ field', - functionality: 'pairedItem', - functionOverrides: { - message: 'Can’t get data', - }, - nodeCause: sourceData.previousNode, - description: `To fetch the data from other nodes that this expression needs, more information is needed from the node ‘${sourceData.previousNode}’`, - causeDetailed: `Missing pairedItem data (node ‘${sourceData.previousNode}’ probably didn’t supply it)`, - type: 'paired_item_no_info', - moreInfoLink: true, - }); + throw createMissingPairedItemError(sourceData.previousNode); } if (Array.isArray(itemPreviousNode.pairedItem)) { @@ -804,13 +827,17 @@ export class WorkflowDataProxy { } throw createExpressionError('Invalid expression', { - messageTemplate: 'Invalid expression under ‘%%PARAMETER%%’', + messageTemplate: `Multiple matching items for expression [item ${ + currentPairedItem.item || 0 + }]`, functionality: 'pairedItem', functionOverrides: { - description: `The code uses data in the node ‘${destinationNodeName}’ but there is more than one matching item in that node`, - message: 'Invalid code', + message: `Multiple matching items for code [item ${currentPairedItem.item || 0}]`, }, - description: `The expression uses data in the node ‘${destinationNodeName}’ but there is more than one matching item in that node`, + nodeCause: destinationNodeName, + descriptionKey: isScriptingNode(destinationNodeName, that.workflow) + ? 'pairedItemMultipleMatchesCodeNode' + : 'pairedItemMultipleMatches', type: 'paired_item_multiple_matches', }); } @@ -833,17 +860,7 @@ export class WorkflowDataProxy { if (itemInput >= source.length) { if (source.length === 0) { // A trigger node got reached, so looks like that that item can not be resolved - throw createExpressionError('Invalid expression', { - messageTemplate: 'Invalid expression under ‘%%PARAMETER%%’', - functionality: 'pairedItem', - functionOverrides: { - description: `The code uses data in the node ‘${destinationNodeName}’ but there is no path back to it. Please check this node is connected to it (there can be other nodes in between).`, - message: 'Invalid code', - }, - description: `The expression uses data in the node ‘${destinationNodeName}’ but there is no path back to it. Please check this node is connected to it (there can be other nodes in between).`, - type: 'paired_item_no_connection', - moreInfoLink: true, - }); + throw createNoConnectionError(destinationNodeName); } throw createExpressionError('Can’t get data for expression', { messageTemplate: 'Can’t get data for expression under ‘%%PARAMETER%%’ field', @@ -905,24 +922,8 @@ export class WorkflowDataProxy { } if (pairedItem.item >= taskData.data!.main[previousNodeOutput]!.length) { - throw createExpressionError('Can’t get data for expression', { - messageTemplate: 'Can’t get data for expression under ‘%%PARAMETER%%’ field', - functionality: 'pairedItem', - functionOverrides: { - message: 'Can’t get data', - }, - nodeCause: nodeBeforeLast, - description: `In node ‘${nodeBeforeLast!}’, output item ${ - currentPairedItem.item || 0 - } ${ - sourceData.previousNodeRun - ? `of run ${(sourceData.previousNodeRun || 0).toString()} ` - : '' - }points to an input item on node ‘${ - sourceData.previousNode - }‘ that doesn’t exist.`, - type: 'paired_item_invalid_info', - moreInfoLink: true, + throw createInvalidPairedItemError({ + nodeName: sourceData.previousNode, }); } @@ -937,7 +938,12 @@ export class WorkflowDataProxy { const referencedNode = that.workflow.getNode(nodeName); if (referencedNode === null) { - throw createExpressionError(`"${nodeName}" node doesn't exist`); + throw createExpressionError("Referenced node doesn't exist", { + runIndex: that.runIndex, + itemIndex: that.itemIndex, + nodeCause: nodeName, + descriptionKey: 'nodeNotFound', + }); } const ensureNodeExecutionData = () => { @@ -945,8 +951,11 @@ export class WorkflowDataProxy { !that?.runExecutionData?.resultData?.runData.hasOwnProperty(nodeName) && !that.workflow.getPinDataOfNode(nodeName) ) { - throw createExpressionError(`no data, execute "${nodeName}" node first`, { + throw createExpressionError('Referenced node is unexecuted', { + runIndex: that.runIndex, + itemIndex: that.itemIndex, type: 'no_node_execution_data', + descriptionKey: 'noNodeExecutionData', nodeCause: nodeName, }); } @@ -989,17 +998,7 @@ export class WorkflowDataProxy { } const parentNodes = that.workflow.getParentNodes(contextNode); if (!parentNodes.includes(nodeName)) { - throw createExpressionError('Invalid expression', { - messageTemplate: 'Invalid expression under ‘%%PARAMETER%%’', - functionality: 'pairedItem', - functionOverrides: { - description: `The code uses data in the node ‘${nodeName}’ but there is no path back to it. Please check this node is connected to it (there can be other nodes in between).`, - message: `No path back to node ‘${nodeName}’`, - }, - description: `The expression uses data in the node ‘${nodeName}’ but there is no path back to it. Please check this node is connected to it (there can be other nodes in between).`, - nodeCause: nodeName, - type: 'paired_item_no_connection', - }); + throw createNoConnectionError(nodeName); } ensureNodeExecutionData(); @@ -1037,17 +1036,7 @@ export class WorkflowDataProxy { const pairedItem = input.pairedItem as IPairedItemData; if (pairedItem === undefined) { - throw createExpressionError('Can’t get data for expression', { - messageTemplate: 'Can’t get data for expression under ‘%%PARAMETER%%’ field', - functionality: 'pairedItem', - functionOverrides: { - description: `To fetch the data from other nodes that this code needs, more information is needed from the node ‘${that.activeNodeName}‘`, - message: 'Can’t get data', - }, - description: `To fetch the data from other nodes that this expression needs, more information is needed from the node ‘${that.activeNodeName}‘`, - causeDetailed: `Missing pairedItem data (node ‘${that.activeNodeName}‘ probably didn’t supply it)`, - itemIndex, - }); + throw createMissingPairedItemError(that.activeNodeName); } if (!that.executeData?.source) { diff --git a/packages/workflow/src/errors/expression.error.ts b/packages/workflow/src/errors/expression.error.ts index 96a93376bb316..ac73010957153 100644 --- a/packages/workflow/src/errors/expression.error.ts +++ b/packages/workflow/src/errors/expression.error.ts @@ -5,6 +5,7 @@ export interface ExpressionErrorOptions { cause?: Error; causeDetailed?: string; description?: string; + descriptionKey?: string; descriptionTemplate?: string; functionality?: 'pairedItem'; itemIndex?: number; @@ -38,6 +39,7 @@ export class ExpressionError extends ExecutionBaseError { const allowedKeys = [ 'causeDetailed', 'descriptionTemplate', + 'descriptionKey', 'functionality', 'itemIndex', 'messageTemplate', diff --git a/packages/workflow/test/WorkflowDataProxy.test.ts b/packages/workflow/test/WorkflowDataProxy.test.ts index 7137af6d57737..98f0a3959c1db 100644 --- a/packages/workflow/test/WorkflowDataProxy.test.ts +++ b/packages/workflow/test/WorkflowDataProxy.test.ts @@ -162,7 +162,7 @@ describe('WorkflowDataProxy', () => { } catch (error) { expect(error).toBeInstanceOf(ExpressionError); const exprError = error as ExpressionError; - expect(exprError.message).toEqual('"does not exist" node doesn\'t exist'); + expect(exprError.message).toEqual("Referenced node doesn't exist"); done(); } }); @@ -193,7 +193,7 @@ describe('WorkflowDataProxy', () => { } catch (error) { expect(error).toBeInstanceOf(ExpressionError); const exprError = error as ExpressionError; - expect(exprError.message).toEqual('no data, execute "Impossible" node first'); + expect(exprError.message).toEqual('Referenced node is unexecuted'); expect(exprError.context.type).toEqual('no_node_execution_data'); done(); } @@ -221,7 +221,7 @@ describe('WorkflowDataProxy', () => { } catch (error) { expect(error).toBeInstanceOf(ExpressionError); const exprError = error as ExpressionError; - expect(exprError.message).toEqual('no data, execute "Impossible if" node first'); + expect(exprError.message).toEqual('Referenced node is unexecuted'); expect(exprError.context.type).toEqual('no_node_execution_data'); done(); } @@ -263,7 +263,7 @@ describe('WorkflowDataProxy', () => { } catch (error) { expect(error).toBeInstanceOf(ExpressionError); const exprError = error as ExpressionError; - expect(exprError.message).toEqual('Can’t get data for expression'); + expect(exprError.message).toEqual("Can't get data for expression"); expect(exprError.context.type).toEqual('paired_item_no_info'); done(); } @@ -277,7 +277,7 @@ describe('WorkflowDataProxy', () => { } catch (error) { expect(error).toBeInstanceOf(ExpressionError); const exprError = error as ExpressionError; - expect(exprError.message).toEqual('Can’t get data for expression'); + expect(exprError.message).toEqual("Can't get data for expression"); expect(exprError.context.type).toEqual('paired_item_invalid_info'); done(); }