-
Notifications
You must be signed in to change notification settings - Fork 53.3k
feat: Binary data merge and expressions simplification #23270
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
base: master
Are you sure you want to change the base?
Conversation
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
node-3854-add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
… on default filesystem, binary data access by property fixes
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-component-for-binaries-included-in-json-schema-view
…add-binarydata-merge
This comment has been minimized.
This comment has been minimized.
…add-binarydata-merge
…add-binarydata-merge
…add-binarydata-merge
…8n-io/n8n into node-3854-add-binarydata-merge
…add-binarydata-merge
This comment has been minimized.
This comment has been minimized.
…add-binarydata-merge
…add-binarydata-merge
…add-binarydata-merge
…add-binarydata-merge
dlavrenuek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great and huge piece of work @michael-radency 💪 ! Thank you for implementing the bg improvements!
Adding my review so far, will continue tomorrow.
| ).rejects.toThrow('Provided parameter is not a string or binary data object.'); | ||
| }); | ||
|
|
||
| it('should throw error when path is undefined in combined mode', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name is a bit misleading because path is 'data.file' and also defined, but the value is undefined. May be something like?
| it('should throw error when path is undefined in combined mode', async () => { | |
| it('should throw error when path resolves to undefined in combined mode', async () => { |
| it('should detect ASCII encoding', () => { | ||
| const asciiBuffer = Buffer.from('Simple ASCII text 123'); | ||
| const encoding = detectBinaryEncoding(asciiBuffer); | ||
| expect(encoding).toBeDefined(); | ||
| // ASCII is often detected as UTF-8, ASCII, or similar encodings | ||
| expect(['UTF-8', 'ASCII', 'ascii', 'windows-1252', 'ISO-8859-1']).toContain(encoding); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional suggestion: The test file is saved in utf8, so all text inside is also utf8. Ascii can be enforced the same way it is done in other tests with different encodings but there is also an easy way to just use Buffer.from().toString('ascii'). The used chardet library uses only uppercase for the charset. Also the readme does not state that it supports ascii, it obviously does support it as ASCII. Does the following also work on your machine?
| it('should detect ASCII encoding', () => { | |
| const asciiBuffer = Buffer.from('Simple ASCII text 123'); | |
| const encoding = detectBinaryEncoding(asciiBuffer); | |
| expect(encoding).toBeDefined(); | |
| // ASCII is often detected as UTF-8, ASCII, or similar encodings | |
| expect(['UTF-8', 'ASCII', 'ascii', 'windows-1252', 'ISO-8859-1']).toContain(encoding); | |
| }); | |
| it('should detect ASCII encoding', () => { | |
| const asciiBuffer = Buffer.from(Buffer.from('Simple ASCII text 123').toString('ascii')); | |
| const encoding = detectBinaryEncoding(asciiBuffer); | |
| expect(encoding).toBeDefined(); | |
| expect(encoding).toBe("ASCII"); | |
| }); |
| }); | ||
| }); | ||
|
|
||
| describe('getBinaryDataBuffer with combined binary mode', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test coverage 🏅
| }, | ||
| ); | ||
| } | ||
| if (binaryMode === 'separate' || binaryMode === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR we will have two modes: "separate" and "combined". This condition suggests that "separate" is the default behavior if unset or specificly defined as "separate", otherwise it will be handled as "conbined". This is confusing in my opinion especially if the available modes are extended in the future.
The bahaviour should be more clear when the condition is reversed as
if (binaryMode === BINARY_MODE_COMBINED) {..} else {...}
or if used with switch instead of if:
switch (binaryMode) {
case BINARY_MODE_COMBINED: { ... }
case "separate":
default: { ... }
}
What do you think?
| } else { | ||
| const itemData = inputData.main[inputIndex]![itemIndex].json; | ||
| const binaryData = get(itemData, parameterData); | ||
| if (binaryData === undefined || !isBinaryValue(binaryData)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isBinaryValue already checks internally if the passed value is an object, so we can safely remove the check if it's undefined
| title: 'Error downloading file', | ||
| message: 'File could not be downloaded', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add these text to localization
| const isDownloadHovered = ref(false); | ||
| const downloadIconColor = computed(() => (isDownloadHovered.value ? 'primary' : 'text-base')); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of complexity for a hover effect. We should simply use the CSS hover instead of using vue state+rendering logic.
| const isDownloadHovered = ref(false); | |
| const downloadIconColor = computed(() => (isDownloadHovered.value ? 'primary' : 'text-base')); |
| <div | ||
| :class="$style.download" | ||
| @click="downloadBinaryData" | ||
| @mouseenter="isDownloadHovered = true" | ||
| @mouseleave="isDownloadHovered = false" | ||
| > | ||
| <N8nIcon icon="download" size="large" :color="downloadIconColor" /> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color and hover color can be defined in the css class. Also I suggest using either a button or a link here to make it accessible
| <div | |
| :class="$style.download" | |
| @click="downloadBinaryData" | |
| @mouseenter="isDownloadHovered = true" | |
| @mouseleave="isDownloadHovered = false" | |
| > | |
| <N8nIcon icon="download" size="large" :color="downloadIconColor" /> | |
| </div> | |
| <a :class="$style.download" @click="downloadBinaryData"> | |
| <N8nIcon icon="download" size="large" /> | |
| </a> |
| cursor: pointer; | ||
| flex-shrink: 0; | ||
| opacity: 0; | ||
| transition: opacity 0.2s ease-in-out; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a button or link, cursor is not needed. To keep the colors as you had them following will work:
| cursor: pointer; | |
| flex-shrink: 0; | |
| opacity: 0; | |
| transition: opacity 0.2s ease-in-out; | |
| flex-shrink: 0; | |
| opacity: 0; | |
| transition: all 0.2s ease-in-out; | |
| color: var(--color-text-base); | |
| &:hover { | |
| color: var(--color--primary--shade-1); | |
| } |
It would be even better to use a separate component "icon button" if such one exists
| @@ -1,16 +1,35 @@ | |||
| <script setup lang="ts"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: continue review here
Summary
This PR introduces a new combined binary data mode for n8n workflows that changes how binary data is accessed and where it is included in items. Instead of storing binary data separately from the item's JSON data under a
binarykey, the combined mode automatically includes binary entries directly within the item's JSON data under a_filesproperty, simplifying expressions for data access. Binary entries can be included at any level of nesting in the item's JSON data and can also be placed inside arrays. Items will be displayed in the UI in Schema and Table views with options to preview and download binary items.1. New Binary Data Mode
New Binary Mode Setting
BINARY_IN_JSON_PROPERTY = '_files'constantbinaryMode: 'separate' | 'combined'binaryMode: 'combined', binary data is automatically converted to be accessible via$json._filesinstead of$binary2. UI/UX Improvements
Binary Data Viewer Components
BinaryDataViewModal.vue: Modal for previewing binary files (images, videos, PDFs, JSON, HTML, markdown, text)BinaryEntryDataTable.vue: Table row component for displaying binary file metadata with download and preview actionsVirtualSchemaItem.vue: Enhanced schema view to recognize and display binary data embedded in JSONWorkflow Settings
binaryMode: 'combined', which enables simplified expressions and inclusion of binaries into JSON3. Expression Simplification
Workflow Data Proxy (
packages/workflow/src/workflow-data-proxy.ts)$itemnow behaves like$jsonand$datain combined mode, returning the JSON portion of items directly$input.item,$input.first(),$input.last(), and$input.all()return JSON data directly in combined mode$('nodeName').first(),$('nodeName').last(),$('nodeName').all(), and$('nodeName').pairedItem()return JSON data in combined modefullItemparameter for$('nodeName', fullItem)to force returning full item structure even in combined modeCode Node Autocomplete
n8n-once-for-all-items-combined.d.ts,n8n-once-for-each-item-combined.d.ts.jsonsuggestion for item data access in combined modeawaitwarnings, as it was falsely warning thatawaitcould not be used inside Code nodes4. Backend Changes
Binary Data Conversion
binaryModesettingconvertBinaryDatathat moves binaries to the JSON_filesproperty, automatically converting base64 data to entries based on filesystem modeHelper Functions
getBinaryDataBuffer()to work with both modesassertBinaryData()now support binary data access by path from bothbinaryandjsonpropertiesRelated Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/NODE-3854/add-binarydata-component-for-binaries-included-in-json-schema-view
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)