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.

Sign up for GitHub

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

Jump to bottom

fix(core): Fix $getWorkflowStaticData on task runners #12153

Merged
merged 15 commits into from
Dec 11, 2024
Merged

fix(core): Fix $getWorkflowStaticData on task runners #12153

merged 15 commits into from
Dec 11, 2024

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Dec 11, 2024

Summary

If the script relies on $getWorkflowStaticData, have the runner resolve and also send back to update requester-side.

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/PAY-2358
#12125

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Dec 11, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 11, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/workflow/src/Workflow.ts 0.00% 3 Missing ⚠️
...ages/cli/src/runners/task-managers/task-manager.ts 33.33% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

tomi
tomi reviewed Dec 11, 2024
View reviewed changes
...ages/@n8n/task-runner/src/js-task-runner/built-ins-parser/__tests__/built-ins-parser.test.ts Outdated Show resolved Hide resolved
packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts Outdated Show resolved Hide resolved
packages/cli/src/runners/task-managers/task-manager.ts Outdated
// if the runner sent back static data, it may have changed, so update it
if (incomingStaticData) {
workflow.setStaticData(incomingStaticData);
workflow.staticData.__dataChanged = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this inside the Workflow class as it's an implementation detail of it?

Copy link
Contributor Author

@ivov ivov Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I did not include it there because the original logic in the constructor did not mark as changed and now both the constructor and this callsite use setStaticData, i.e. to avoid changing preexisting behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's because the use case is different. The constructor initializes the workflow class, which makes sense that the static data hasn't changed at that point. Here we are explicitly saying "the static data changed, here is the new static data". So the operation is different: initialize static data vs set/override/change static data.

packages/cli/src/utils.ts Outdated Show resolved Hide resolved
@ivov ivov added the release/backport Changes that need to be backported to older releases. label Dec 11, 2024
tomi
tomi reviewed Dec 11, 2024
View reviewed changes
packages/cli/src/runners/task-managers/task-manager.ts Outdated Show resolved Hide resolved
packages/cli/src/runners/task-managers/task-manager.ts Outdated
// if the runner sent back static data, it may have changed, so update it
if (incomingStaticData) {
workflow.setStaticData(incomingStaticData);
workflow.staticData.__dataChanged = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

That's because the use case is different. The constructor initializes the workflow class, which makes sense that the static data hasn't changed at that point. Here we are explicitly saying "the static data changed, here is the new static data". So the operation is different: initialize static data vs set/override/change static data.

packages/cli/src/runners/task-managers/task-manager.ts Outdated Show resolved Hide resolved
packages/cli/src/workflows/workflow-static-data.service.ts Outdated Show resolved Hide resolved
tomi
tomi approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments 💟 Works nicely now 👏

@github-actions GitHub Actions
Copy link
Contributor

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

@cypress Cypress
Copy link

cypress bot commented Dec 11, 2024 •
edited
Loading

n8n    Run #8301

Run Properties:  status check passed Passed #8301  •  git commit f596151530: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project n8n
Branch Review pay-2358-getworkflowstaticdata-not-working-on-task-runners
Run status status check passed Passed #8301
Run duration 04m 38s
Commit git commit f596151530: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Committer Tomi Turtiainen
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 480
View all changes introduced in this branch ↗︎

@github-actions GitHub Actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@tomi tomi merged commit b479f14 into master Dec 11, 2024
37 checks passed
@tomi tomi deleted the pay-2358-getworkflowstaticdata-not-working-on-task-runners branch December 11, 2024 18:48
ivov added a commit that referenced this pull request Dec 12, 2024
@ivov @tomi
fix(core): Fix $getWorkflowStaticData on task runners (#12153)
79ae828 
Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Dec 12, 2024
Merged
ivov added a commit that referenced this pull request Dec 12, 2024
@ivov @tomi
fix(core): Fix $getWorkflowStaticData on task runners (#12153)
a769097 
Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>
netroy pushed a commit that referenced this pull request Dec 12, 2024
@ivov @tomi @netroy
fix(core): Fix $getWorkflowStaticData on task runners (#12153)
26435f7 
Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>
@janober
Copy link
Member

janober commented Dec 12, 2024

Got released with n8n@1.71.3

tomi added a commit that referenced this pull request Dec 12, 2024
@ivov @tomi
fix(core): Fix $getWorkflowStaticData on task runners (#12153)
215cb22 
Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>
This was referenced Dec 12, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomi tomi tomi approved these changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team release/backport Changes that need to be backported to older releases. Released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivov @janober @tomi