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

fix: Overhaul expression error messages related to paired item #8765

Merged
merged 8 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
27 changes: 20 additions & 7 deletions packages/editor-ui/src/components/Error/NodeErrorView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -300,23 +300,36 @@ export default defineComponent({

<style lang="scss">
.error-header {
margin-bottom: 10px;
margin-bottom: var(--spacing-s);
}

.error-message {
color: var(--color-ndv-ouptut-error-font);
font-weight: bold;
font-size: 1.1rem;
font-weight: var(--font-weight-bold);
font-size: var(--font-size-l);
}

.error-description {
margin-top: 10px;
font-size: 1rem;
margin-top: var(--spacing-s);
font-size: var(--font-size-m);

ul {
padding: var(--spacing-s) 0;
padding-left: var(--spacing-l);
}

code {
font-size: var(--font-size-xs);
color: var(--color-text-dark);
background: var(--color-background-medium);
padding: var(--spacing-5xs);
border-radius: var(--border-radius-base);
}
}

.error-details__summary {
font-weight: 600;
font-size: 16px;
font-weight: var(--font-weight-bold);
font-size: var(--font-size-m);
cursor: pointer;
outline: none;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/editor-ui/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,8 @@ export const ALLOWED_HTML_TAGS = [
'a',
'br',
'i',
'ul',
'li',
'em',
'small',
'details',
Expand Down
195 changes: 101 additions & 94 deletions packages/workflow/src/WorkflowDataProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,20 +276,23 @@ 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,
elsmr marked this conversation as resolved.
Show resolved Hide resolved
description: `The node <strong>'${nodeName}'</strong> doesn't exist, but it's used in an expression here.`,
elsmr marked this conversation as resolved.
Show resolved Hide resolved
});
}

if (
!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',
description: `An expression references the node <strong>'${nodeName}'</strong>, but it hasn't been executed yet. Either change the expression, or re-wire your workflow to make sure that node executes first.`,
nodeCause: nodeName,
});
}
Expand Down Expand Up @@ -374,7 +377,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,
description: `The node <strong>'${nodeName}'</strong> doesn't exist, but it's used in an expression here.`,
});
}

if (['binary', 'data', 'json'].includes(name)) {
Expand Down Expand Up @@ -594,9 +602,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,
description: `The node <strong>'${nodeName}'</strong> doesn't exist, but it's used in an expression here.`,
});
}

Expand Down Expand Up @@ -671,10 +681,10 @@ 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 <strong>'${nodeName}'</strong> and execute the workflow again.`;
context.descriptionTemplate = `To fetch the data for the expression under '%%PARAMETER%%', you must unpin the node <strong>'${nodeName}'</strong> and execute the workflow again.`;
context.description =
'The <a href=”https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-errors/”>item-matching</a> data in that node may be stale. It is needed by an expression in this node that uses <code>.item</code>.';
}

if (context.moreInfoLink && (pinData || isScriptingNode(nodeName, that.workflow))) {
Expand All @@ -693,6 +703,59 @@ export class WorkflowDataProxy {
});
};

const createInvalidPairedItemError = ({
nodeName,
itemIndex = 0,
runIndex = 0,
}: {
nodeName: string;
itemIndex?: number;
runIndex?: number;
}) => {
const itemIndexMessage = `item ${itemIndex} ${
runIndex ? `of run ${runIndex.toString()} ` : ''
}`;
return createExpressionError('Can’t get data for expression', {
messageTemplate: 'Expression info invalid',
functionality: 'pairedItem',
functionOverrides: {
message: 'Can’t get data',
},
nodeCause: nodeName,
description: `An expression here won't work because it uses <code>.item</code> and n8n can't figure out the <a href=”https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-errors/”>matching item</a>. This is because the node <strong>'${nodeName}'</strong> returned incorrect matching information (for ${itemIndexMessage}). <br/><br/>Try using <code>.first()</code>, <code>.last()</code> or <code>.all()[index]</code> instead of <code>.item</code>.`,
type: 'paired_item_invalid_info',
});
};

const createMissingPairedItemError = (nodeName: 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: nodeName,
description: isScriptingNode(nodeName, that.workflow)
? `An expression here won't work because it uses <code>.item</code> and n8n can't figure out the <a href="https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-errors/">matching item</a>. You can either: <ul><li>Add the <a href="https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-code-node/">missing information</a> to the node <strong>'${nodeName}'</strong></li><li>Or use <code>.first()</code>, <code>.last()</code> or <code>.all()[index]</code> instead of <code>.item</code></li></ul>`
: `An expression here won't work because it uses <code>.item</code> and n8n can't figure out the <a href=”https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-errors/”>matching item</a>. The node <strong>'${nodeName}'</strong> didn't return enough information.`,
causeDetailed: `Missing pairedItem data (node ‘${nodeName}’ probably didn’t supply it)`,
type: 'paired_item_no_info',
});
};

const createNoConnectionError = (nodeName: string) => {
return createExpressionError('Invalid expression', {
messageTemplate: 'No path back to referenced node',
functionality: 'pairedItem',
functionOverrides: {
description: `There is no connection back to the node <strong>'${nodeName}'</strong>, but it's used in code here.<br/><br/>Please wire up the node (there can be other nodes in between).`,
},
description: `There is no connection back to the node <strong>'${nodeName}'</strong>, but it's used in an expression here.<br/><br/>Please wire up the node (there can be other nodes in between).`,
type: 'paired_item_no_connection',
moreInfoLink: true,
});
};

const getPairedItem = (
destinationNodeName: string,
incomingSourceData: ISourceData | null,
Expand Down Expand Up @@ -741,42 +804,17 @@ 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 ‘<strong>${nodeBeforeLast!}</strong>’, output item ${
currentPairedItem.item || 0
} ${
sourceData.previousNodeRun
? `of run ${(sourceData.previousNodeRun || 0).toString()} `
: ''
}points to an input item on node ‘<strong>${
sourceData.previousNode
}</strong>‘ that doesn’t exist.`,
type: 'paired_item_invalid_info',
moreInfoLink: true,
throw createInvalidPairedItemError({
nodeName: sourceData.previousNode,
itemIndex: currentPairedItem.item,
runIndex: sourceData.previousNodeRun,
});
}

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 ‘<strong>${sourceData.previousNode}</strong>’`,
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)) {
Expand Down Expand Up @@ -809,13 +847,18 @@ 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 ‘<strong>${destinationNodeName}</strong>’ 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 code here won't work because it uses <code>.item</code> and n8n can't figure out the <a href=”https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-errors/”>matching item</a>. (There are multiple possible matches) <br/><br/>Try using <code>.first()</code>, <code>.last()</code> or <code>.all()[index]</code> instead of <code>.item</code> or <a href=”https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-code-node/”>reference a different node</a>.",
},
description: `The expression uses data in the node ‘<strong>${destinationNodeName}</strong>’ but there is more than one matching item in that node`,
nodeCause: destinationNodeName,
description:
"An expression here won't work because it uses <code>.item</code> and n8n can't figure out the <a href=”https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-errors/”>matching item</a>. (There are multiple possible matches) <br/><br/>Try using <code>.first()</code>, <code>.last()</code> or <code>.all()[index]</code> instead of <code>.item</code> or <a href=”https://docs.n8n.io/data/data-mapping/data-item-linking/item-linking-code-node/”>reference a different node</a>.",
type: 'paired_item_multiple_matches',
});
}
Expand All @@ -838,17 +881,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 ‘<strong>${destinationNodeName}</strong>’ 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 ‘<strong>${destinationNodeName}</strong>’ 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',
Expand Down Expand Up @@ -910,24 +943,10 @@ 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 ‘<strong>${nodeBeforeLast!}</strong>’, output item ${
currentPairedItem.item || 0
} ${
sourceData.previousNodeRun
? `of run ${(sourceData.previousNodeRun || 0).toString()} `
: ''
}points to an input item on node ‘<strong>${
sourceData.previousNode
}</strong>‘ that doesn’t exist.`,
type: 'paired_item_invalid_info',
moreInfoLink: true,
throw createInvalidPairedItemError({
nodeName: sourceData.previousNode,
itemIndex: currentPairedItem.item,
runIndex: sourceData.previousNodeRun,
});
}

Expand All @@ -942,16 +961,24 @@ 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,
description: `The node <strong>'${nodeName}'</strong> doesn't exist, but it's used in an expression here.`,
});
}

const ensureNodeExecutionData = () => {
if (
!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',
description: `An expression references the node <strong>'${nodeName}'</strong>, but it hasn't been executed yet. Either change the expression, or re-wire your workflow to make sure that node executes first.`,
nodeCause: nodeName,
});
}
Expand Down Expand Up @@ -994,17 +1021,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 <strong>‘${nodeName}’</strong> 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 <strong>‘${nodeName}’</strong> 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();
Expand Down Expand Up @@ -1042,17 +1059,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 ‘<strong>${that.activeNodeName}</strong>‘`,
message: 'Can’t get data',
},
description: `To fetch the data from other nodes that this expression needs, more information is needed from the node ‘<strong>${that.activeNodeName}</strong>‘`,
causeDetailed: `Missing pairedItem data (node ‘${that.activeNodeName}‘ probably didn’t supply it)`,
itemIndex,
});
throw createMissingPairedItemError(that.activeNodeName);
}

if (!that.executeData?.source) {
Expand Down
6 changes: 3 additions & 3 deletions packages/workflow/test/WorkflowDataProxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
});
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}
Expand Down
Loading