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 & function nodes): Update function nodes to work with binary-data-mode 'filesystem'. #3845

Merged
merged 9 commits into from
Sep 11, 2022
Merged

fix(core & function nodes): Update function nodes to work with binary-data-mode 'filesystem'. #3845

merged 9 commits into from
Sep 11, 2022

Conversation

rhyswilliamsza
Copy link
Contributor

@rhyswilliamsza rhyswilliamsza commented Aug 6, 2022 •
edited
Loading

Hello! 👋

This is a PR addressing issue #3618.

Describe the bug
FunctionItem getBinaryData() returns empty data payloads when default binary data mode set to filesystem, though data is visible in the node input.

To Reproduce
Steps to reproduce the behavior:

  1. Set N8N_DEFAULT_BINARY_DATA_MODE to filesystem
  2. Tie together a stream that includes a node that outputs binary data, as well as a FunctionItem node that accepts this binary data.
  3. Call getBinaryData() in your function. The data element is empty and the binary data is inaccessible.

Expected behavior
If N8N_DEFAULT_BINARY_DATA_MODE is left to memory, the getBinaryData() helper method correctly returns binary data.

Added ✨

  • Added an additional helper method to NodeExecuteFunctions.ts, setBinaryDataBuffer(data: IBinaryData, binaryData: Buffer): Promise<IBinaryData>, which allows a node to store binary data when the IBinaryData object already exists. Previously, one had to recreate this object using prepareBinaryData.
  • FunctionItem & Function nodes: getBinaryDataAsync(...) helper method has been added. This uses the configured BinaryDataManager to retrieve binary data.
  • FunctionItem & Function nodes: setBinaryDataAsync(...) helper method has been added. This uses the configured BinaryDataManager to store binary data.

Deprecated 🙅

  • FunctionItem node: getBinaryData(...) has been deprecated. Users should migrate to getBinaryDataAsync(...) instead.
  • FunctionItem node: setBinaryData(...) has been deprecated. Users should migrate to setBinaryDataAsync(...) instead.

@n8n-assistant n8n-assistant bot added community Authored by a community member core Enhancement outside /nodes-base and /editor-ui node/improvement New feature or request labels Aug 6, 2022
@janober
Copy link
Member

janober commented Aug 10, 2022

Looks good from what I can see. Would be great if you could finish what you think is still outstanding and we can then review.

Thanks a lot for taking care of that. Is very appreciated!

rhyswilliamsza
rhyswilliamsza commented Aug 24, 2022
View reviewed changes
packages/nodes-base/nodes/FunctionItem/FunctionItem.node.ts
getNodeParameter: this.getNodeParameter,
getWorkflowStaticData: this.getWorkflowStaticData,
helpers: this.helpers,
item: item.json,
setBinaryData: (data: IBinaryKeyData) => {
getBinaryDataAsync: async (): Promise<IBinaryKeyData | undefined> => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getBinaryDataAsync: async ()

The Function node requires one to pass in the item here. It isn't necessary in the FunctionItem node, however perhaps we should align the two for better a better user experience when switching between them? Though - to be honest - I quite like it this way if we think about the nodes independently.

@rhyswilliamsza rhyswilliamsza marked this pull request as ready for review August 24, 2022 10:56
J4NS-R
J4NS-R reviewed Aug 25, 2022
View reviewed changes
Copy link

@J4NS-R J4NS-R left a comment

Choose a reason for hiding this comment

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

Just some code style things to consider. Also, I agree we need tests for this feature.

packages/nodes-base/nodes/Function/Function.node.ts Show resolved Hide resolved
packages/nodes-base/nodes/FunctionItem/FunctionItem.node.ts Outdated Show resolved Hide resolved
@csuermann
Copy link
Contributor

Thanks for the contribution! We're tracking it internally under N8N-4435 and will be looking into it shortly.

LizaOkennedy9oo
LizaOkennedy9oo reviewed Aug 26, 2022
View reviewed changes
Copy link

@LizaOkennedy9oo LizaOkennedy9oo left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ahsanv
Copy link
Member

ahsanv commented Sep 6, 2022

Thank you very much for the contribution @rhyswilliamsza !
Can you please rebase / merge with master?

ahsanv
ahsanv approved these changes Sep 11, 2022
View reviewed changes
Copy link
Member

@ahsanv ahsanv left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@ahsanv ahsanv merged commit f6064ef into n8n-io:master Sep 11, 2022
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Sep 11, 2022
@rhyswilliamsza rhyswilliamsza deleted the fix/function-item-binary-data branch September 13, 2022 15:36
@rhyswilliamsza
Copy link
Contributor Author

Thank you very much for the contribution @rhyswilliamsza ! Can you please rebase / merge with master?

Apologies, I have been away and just got back to this. Thank you for merging!

@janober
Copy link
Member

janober commented Sep 15, 2022

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Sep 15, 2022
valya pushed a commit to valya/n8n that referenced this pull request Nov 8, 2022
@rhyswilliamsza
fix(core & function nodes): Update function nodes to work with binary…
1ab66e5 
…-data-mode 'filesystem'. (n8n-io#3845)

* Initial Fix

* Self-Review n8n-io#1

* Lint

* Added support for FunctionItem. Minor updates.

* Self-review

* review comments. Added testing.

* Self Review

* Fixed memory handling on data manager use.

* Fixes for unnecessary memory leaks.
@saurabharch saurabharch mentioned this pull request Feb 11, 2024
Closed
@reconsumeralization reconsumeralization mentioned this pull request Feb 11, 2024
Open
@johnperez416 johnperez416 mentioned this pull request Feb 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LizaOkennedy9oo LizaOkennedy9oo LizaOkennedy9oo left review comments

@J4NS-R J4NS-R J4NS-R left review comments

@ahsanv ahsanv ahsanv approved these changes

Assignees
No one assigned
Labels
community Authored by a community member core Enhancement outside /nodes-base and /editor-ui node/improvement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rhyswilliamsza @janober @csuermann @ahsanv @J4NS-R @LizaOkennedy9oo