fix(editor): Use pinned data to resolve expressions in unexecuted nodes #9693

ivov · 2024-06-11T07:13:15Z

Ensure that expressions always resolve to a defined value based on pinned data, even when node is not executed.
Ensure pinned data only affects manual executions in expressions resolving

Before

After

* master: fix(editor): Add back credential type icon (no-changelog) (#9704) feat(editor): Add canvas edge toolbar hover show/hide support (no-changelog) (#9699) ci: Fix custom docker builds (no-changelog) (#9702) test: Fix e2e for projects missing instance owner (no-changelog) (#9703) ci: Refactor e2e tests to be less flaky (no-changelog) (#9695) feat(editor): Add move resources option to workflows and credentials on (#9654) fix: Introduce `HooksService` (#8962) fix(editor): Improve large data warning in input/output panel (#9671) ci(editor): Enforce type-safety in @n8n/chat builds as well (no-changelog) (#9685) fix(editor): Un-skip workflow save test (no-changelog) (#9698) refactor(core): Remove more dead code from event bus (no-changelog) (#9697) ci: Remove unused WaitTracker mocking (no-changelog) (#9694) feat: Update NPS Value Survey (#9638) refactor(core): Remove event bus channel (no-changelog) (#9663) refactor(core): Remove event bus helpers (no-changelog) (#9690)

mutdmour · 2024-06-18T14:48:16Z

packages/editor-ui/src/composables/useWorkflowHelpers.ts

-	const workflowsStore = useWorkflowsStore();
-
-	if (workflowsStore.shouldReplaceInputDataWithPinData) {
-		const parentPinData = parentNode.reduce<INodeExecutionData[]>((acc, parentNodeName, index) => {
-			const pinData = workflowsStore.pinDataByNodeName(parentNodeName);
-
-			if (pinData) {
-				acc.push({
-					json: pinData[0],
-					pairedItem: {
-						item: index,
-						input: 1,
-					},
-				});
-			}
-
-			return acc;
-		}, []);
-
-		if (parentPinData.length > 0) {
-			if (connectionInputData && connectionInputData.length > 0) {
-				parentPinData.forEach((parentPinDataEntry) => {
-					connectionInputData![0].json = {
-						...connectionInputData![0].json,
-						...parentPinDataEntry.json,
-					};
-				});
-			} else {
-				connectionInputData = parentPinData;
-			}
-		}
-	}
-


cleaning this up, not necessary since WorkflowProxy now supports pinned data directly

mutdmour · 2024-06-18T15:44:27Z

packages/workflow/test/fixtures/WorkflowDataProxy/pindata_workflow.json

@@ -0,0 +1,106 @@
+{


example workflow to test this

ivov

Thanks for tackling this! A bit spent today, will continue with fresh eyes tomorrow.

ivov · 2024-06-19T14:50:02Z

packages/workflow/src/WorkflowDataProxy.ts

+	}: { nodeName?: string; branchIndex?: number; runIndex?: number; shortSyntax?: boolean } = {}) {
+		if (nodeName === undefined) {
+			return this.connectionInputData;
+		}


When is nodeName not defined? I'm looking at all four callsites for getNodeExecutionOrPinnedData and all of them have nodeName as string. Is this first check needed?

The original is setting defaults for some of the args - should we do that as well here?

removed

getNodeExecutionData does the same. So I have left it for that.

ivov · 2024-06-19T15:00:17Z

packages/workflow/src/WorkflowDataProxyHelpers.ts

+export function getPinDataIfManualExecution(
+	workflow: Workflow,
+	nodeName: string,
+	mode: WorkflowExecuteMode,
+): INodeExecutionData[] | undefined {
+	const pinData = workflow.getPinDataOfNode(nodeName);
+	if (pinData && mode === 'manual') {
+		return pinData;
+	}
+
+	return undefined;
+}


Can we do less work by checking for mode before getting pin data?

Any reason not to make this a method of Workflow? We're trying to move away from *Helpers everywhere.

Would it make sense to make the mode check part of the existing getPinDataOfNode? Pin data should only be relevant for manual executions.

done

I am not a fan of having to use that and this everywhere. Makes it harder to test and refactor. As well as understand what and when a value is set. That's why I prefer pure functions here.

I could see this being used without a manual execution, for example to validate or render data. Though it's not used currently as so. I would keep as is.

The point about that is valid, but I don't see how moving this method out of Workflow helps there. Can we please move this method into Workflow? 🙏🏽

We have to start somewhere breaking this apart. I don't want to balloon this PR with refactoring at the same time. I would opt to keep it.

I'm torn - Workflow is very much convoluted, but I think having more -Helper files is not the way. In the end, this is not worth blocking over and we can revisit when we make an effort to simplify this class.

Let's deal with this another time.
We've started writing tests and refactoring Workflow, and it's possible that a lot of the code in here could be moved into the frontend, and we can then simplify this class.

ivov · 2024-06-19T15:01:26Z

packages/editor-ui/src/composables/useWorkflowHelpers.ts

@@ -349,39 +348,6 @@ function connectionInputData(
 		}
 	}

-	const workflowsStore = useWorkflowsStore();
-
-	if (workflowsStore.shouldReplaceInputDataWithPinData) {


I see another use of shouldReplaceInputDataWithPinData in useWorkflowHelpers - that we cannot simplify also?

I checked that. That was actually necessary to set the input data for $input and $json to resolve for example.

ivov · 2024-06-19T15:31:04Z

packages/editor-ui/src/stores/workflows.store.ts

@@ -360,7 +360,7 @@ export const useWorkflowsStore = defineStore(STORES.WORKFLOWS, () => {
 	function getCurrentWorkflow(copyData?: boolean): Workflow {
 		const nodes = getNodes();
 		const connections = allConnections.value;
-		const cacheKey = JSON.stringify({ nodes, connections });
+		const cacheKey = JSON.stringify({ nodes, connections, pinData: pinnedWorkflowData.value });


Max pin data size I think defaults to 12 MB and can be customized to be even higher. getCurrentWorkflow seems to be called from multiple spots.

Wondering - Could we have performance issues from frequently stringifying a very large JS object? Is there a limit to how long a key can be?

There's no limit. I don't have a quick solution here. But removing this caching is much worse for performance.

This whole method is performance nightmare, and one of the leading culprits behind UI performance issues. Adding pinData isn't going to make it terribly worse than it already is.

That said, we need to make the Workflow class updatable, and update the properties in it, and get rid of this caching code. We can't just keep adding more properties in here.

At some point we need to own this code across packages, and fix it properly.
I've re-opened the ticket to fix this.

packages/workflow/src/WorkflowDataProxy.ts

packages/workflow/test/WorkflowDataProxy.test.ts

github-actions · 2024-06-25T07:24:21Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2024-06-25T07:24:28Z

3 flaky tests on run #5670 ↗︎

0	397	0	0	3

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n	Commit: `a55ed16355`
Status: Passed	Duration: 05:01 💡
Started: Jun 25, 2024 3:08 PM	Ended: Jun 25, 2024 3:13 PM

5-ndv.cy.ts • 2 flaky tests

View Output Video

Test		Artifacts
NDV > should not retrieve remote options when required params throw errors		`Screenshots` `Video`
NDV > Stop listening for trigger event from NDV		`Screenshots` `Video`

10-undo-redo.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Undo/Redo > should undo/redo adding connected nodes		`Test Replay` `Screenshots` `Video`

Review all test suite changes for PR #9693 ↗︎

github-actions · 2024-06-25T15:13:59Z

✅ All Cypress E2E specs passed

ivov · 2024-06-27T08:45:58Z

@mutdmour Thank you for fixing this. Since I "authored" this PR my review doesn't count for CI, so feel free to merge.

mutdmour · 2024-06-27T08:50:08Z

Thank you @ivov for the review

janober · 2024-06-27T10:36:35Z

Got released with n8n@1.48.0

fix(editor): Account for pin data in expressions

73d2a29

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jun 11, 2024

MiloradFilipovic added 4 commits June 11, 2024 13:38

💄 Updating expression labels when nodes are not executed

871c76f

✅ Started adding tests

845e147

💩 Cleaning up tests, updating fixtures

cdac4de

MiloradFilipovic self-assigned this Jun 12, 2024

mutdmour added 16 commits June 13, 2024 17:43

Merge branch 'master' of github.com:n8n-io/n8n into ado-2111

25a92d1

simplify approach

8eee7db

Merge branch 'master' of github.com:n8n-io/n8n into ado-2111

eeca703

fix: tests

8878d07

fix: add tests

48c184c

fix: fix order

606a476

fix: improve funcitonality

8ec62c5

fix: and others

6e3eaf6

fix: refactor to support

d0616fe

refactor: clean up

c7e11b4

refactor: clean up

41130fb

fix: remove message change

1c6464f

Merge branch 'master' of github.com:n8n-io/n8n into ado-2111

6be02ab

fix: remvoe console

2c76145

refactor: params

1300377

test: add e2e tests

d98073d

mutdmour marked this pull request as ready for review June 18, 2024 15:42

Merge branch 'master' of github.com:n8n-io/n8n into ado-2111

a8ea01a

mutdmour reviewed Jun 18, 2024

View reviewed changes

mutdmour changed the title ~~fix(editor): Account for pin data in expressions~~ fix(editor): Use pin data to resolve expressions Jun 18, 2024

mutdmour changed the title ~~fix(editor): Use pin data to resolve expressions~~ fix(editor): Use pin data to resolve expressions in unexecuted nodes Jun 18, 2024

fix: add pinned data to cache key

8ca2a56

test: add test for bug with unpinning node

3b34c1f

mutdmour changed the title ~~fix(editor): Use pin data to resolve expressions in unexecuted nodes~~ fix(editor): Use pinned data to resolve expressions in unexecuted nodes Jun 19, 2024

mutdmour added 2 commits June 19, 2024 11:47

test: fix unit tests

51364ce

fix: lint

8549a1c

ivov commented Jun 19, 2024

View reviewed changes

ivov commented Jun 20, 2024

View reviewed changes

mutdmour added 3 commits June 21, 2024 17:12

merge: fix conflicts

cf6f3aa

fix: address feedback

4d6c6a8

Merge branch 'master' of github.com:n8n-io/n8n into ado-2111

f3f75dc

mutdmour previously approved these changes Jun 25, 2024

View reviewed changes

mutdmour added 2 commits June 25, 2024 16:31

merge in master

f436051

test: update tests

a55ed16

mutdmour dismissed their stale review via a55ed16 June 25, 2024 15:02

mutdmour approved these changes Jun 25, 2024

View reviewed changes

mutdmour merged commit 6cb3072 into master Jun 27, 2024
30 checks passed

mutdmour deleted the ado-2111 branch June 27, 2024 08:49

github-actions bot mentioned this pull request Jun 27, 2024

🚀 Release 1.48.0 #9879

Merged

janober added the Released label Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(editor): Use pinned data to resolve expressions in unexecuted nodes #9693

fix(editor): Use pinned data to resolve expressions in unexecuted nodes #9693

ivov commented Jun 11, 2024 •

edited by mutdmour

Loading

mutdmour Jun 18, 2024

mutdmour Jun 18, 2024

ivov left a comment

ivov Jun 19, 2024 •

edited

Loading

mutdmour Jun 21, 2024

ivov Jun 19, 2024

mutdmour Jun 21, 2024

netroy Jun 24, 2024

mutdmour Jun 24, 2024

ivov Jun 27, 2024

netroy Jun 27, 2024

ivov Jun 19, 2024

mutdmour Jun 21, 2024

ivov Jun 19, 2024

mutdmour Jun 21, 2024

netroy Jun 24, 2024

github-actions bot commented Jun 25, 2024

cypress bot commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024

ivov commented Jun 27, 2024

mutdmour commented Jun 27, 2024

janober commented Jun 27, 2024

fix(editor): Use pinned data to resolve expressions in unexecuted nodes #9693

fix(editor): Use pinned data to resolve expressions in unexecuted nodes #9693

Conversation

ivov commented Jun 11, 2024 • edited by mutdmour Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

ivov Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 25, 2024

cypress bot commented Jun 25, 2024 • edited Loading

3 flaky tests on run #5670 ↗︎

Review all test suite changes for PR #9693 ↗︎

github-actions bot commented Jun 25, 2024

ivov commented Jun 27, 2024

mutdmour commented Jun 27, 2024

janober commented Jun 27, 2024

ivov commented Jun 11, 2024 •

edited by mutdmour

Loading

ivov Jun 19, 2024 •

edited

Loading

cypress bot commented Jun 25, 2024 •

edited

Loading