From d7dda3f5de52925e554455f9f10e51bd173ea856 Mon Sep 17 00:00:00 2001 From: Ria Scholz <123465523+riascho@users.noreply.github.com> Date: Tue, 28 Jan 2025 14:19:51 +0100 Subject: [PATCH] feat(Summarize Node): Turns error when field not found in items into warning (#11889) Co-authored-by: Dana Lee Co-authored-by: Elias Meire --- .../Transform/Summarize/Summarize.node.ts | 47 ++++++----- .../test/unitTests/checkIfFieldExists.test.ts | 53 ++++++++++++ .../Summarize/test/unitTests/execute.test.ts | 80 +++++++++++++++++++ .../test/workflow.summarize_hint.json | 70 ++++++++++++++++ .../nodes/Transform/Summarize/utils.ts | 14 ++-- 5 files changed, 237 insertions(+), 27 deletions(-) create mode 100644 packages/nodes-base/nodes/Transform/Summarize/test/unitTests/checkIfFieldExists.test.ts create mode 100644 packages/nodes-base/nodes/Transform/Summarize/test/unitTests/execute.test.ts create mode 100644 packages/nodes-base/nodes/Transform/Summarize/test/workflow.summarize_hint.json diff --git a/packages/nodes-base/nodes/Transform/Summarize/Summarize.node.ts b/packages/nodes-base/nodes/Transform/Summarize/Summarize.node.ts index 83ded6d7af..a73ed285c4 100644 --- a/packages/nodes-base/nodes/Transform/Summarize/Summarize.node.ts +++ b/packages/nodes-base/nodes/Transform/Summarize/Summarize.node.ts @@ -5,6 +5,8 @@ import { type INodeExecutionData, type INodeType, type INodeTypeDescription, + NodeExecutionOutput, + type NodeExecutionHint, } from 'n8n-workflow'; import { @@ -16,7 +18,6 @@ import { fieldValueGetter, splitData, } from './utils'; -import { generatePairedItemData } from '../../../utils/utilities'; export class Summarize implements INodeType { description: INodeTypeDescription = { @@ -25,7 +26,7 @@ export class Summarize implements INodeType { icon: 'file:summarize.svg', group: ['transform'], subtitle: '', - version: 1, + version: [1, 1.1], description: 'Sum, count, max, etc. across items', defaults: { name: 'Summarize', @@ -248,7 +249,12 @@ export class Summarize implements INodeType { type: 'boolean', default: false, description: - "Whether to continue if field to summarize can't be found in any items and return single empty item, owerwise an error would be thrown", + "Whether to continue if field to summarize can't be found in any items and return single empty item, otherwise an error would be thrown", + displayOptions: { + hide: { + '@version': [{ _cnd: { gte: 1.1 } }], + }, + }, }, { displayName: 'Disable Dot Notation', @@ -314,20 +320,6 @@ export class Summarize implements INodeType { const nodeVersion = this.getNode().typeVersion; - if (nodeVersion < 2.1) { - try { - checkIfFieldExists.call(this, newItems, fieldsToSummarize, getValue); - } catch (error) { - if (options.continueIfFieldNotFound) { - const itemData = generatePairedItemData(items.length); - - return [[{ json: {}, pairedItem: itemData }]]; - } else { - throw error; - } - } - } - const aggregationResult = splitData( fieldsToSplitBy, newItems, @@ -336,6 +328,21 @@ export class Summarize implements INodeType { getValue, ); + const fieldsNotFound: NodeExecutionHint[] = []; + try { + checkIfFieldExists.call(this, newItems, fieldsToSummarize, getValue); + } catch (error) { + if (nodeVersion > 1 || options.continueIfFieldNotFound) { + const fieldNotFoundHint: NodeExecutionHint = { + message: error instanceof Error ? error.message : String(error), + location: 'outputPane', + }; + fieldsNotFound.push(fieldNotFoundHint); + } else { + throw error; + } + } + if (options.outputFormat === 'singleItem') { const executionData: INodeExecutionData = { json: aggregationResult, @@ -343,7 +350,7 @@ export class Summarize implements INodeType { item: index, })), }; - return [[executionData]]; + return new NodeExecutionOutput([[executionData]], fieldsNotFound); } else { if (!fieldsToSplitBy.length) { const { pairedItems, ...json } = aggregationResult; @@ -353,7 +360,7 @@ export class Summarize implements INodeType { item: index, })), }; - return [[executionData]]; + return new NodeExecutionOutput([[executionData]], fieldsNotFound); } const returnData = aggregationToArray(aggregationResult, fieldsToSplitBy); const executionData = returnData.map((item) => { @@ -365,7 +372,7 @@ export class Summarize implements INodeType { })), }; }); - return [executionData]; + return new NodeExecutionOutput([executionData], fieldsNotFound); } } } diff --git a/packages/nodes-base/nodes/Transform/Summarize/test/unitTests/checkIfFieldExists.test.ts b/packages/nodes-base/nodes/Transform/Summarize/test/unitTests/checkIfFieldExists.test.ts new file mode 100644 index 0000000000..b5a3636712 --- /dev/null +++ b/packages/nodes-base/nodes/Transform/Summarize/test/unitTests/checkIfFieldExists.test.ts @@ -0,0 +1,53 @@ +import { NodeOperationError, type IExecuteFunctions, type IDataObject } from 'n8n-workflow'; + +import { checkIfFieldExists, type Aggregations } from '../../utils'; + +describe('Test Summarize Node, checkIfFieldExists', () => { + let mockExecuteFunctions: IExecuteFunctions; + + beforeEach(() => { + mockExecuteFunctions = { + getNode: jest.fn().mockReturnValue({ name: 'test-node' }), + } as unknown as IExecuteFunctions; + }); + + const items = [{ a: 1 }, { b: 2 }, { c: 3 }]; + + it('should not throw error if all fields exist', () => { + const aggregations: Aggregations = [ + { aggregation: 'sum', field: 'a' }, + { aggregation: 'count', field: 'c' }, + ]; + const getValue = (item: IDataObject, field: string) => item[field]; + expect(() => { + checkIfFieldExists.call(mockExecuteFunctions, items, aggregations, getValue); + }).not.toThrow(); + }); + + it('should throw NodeOperationError if any field does not exist', () => { + const aggregations: Aggregations = [ + { aggregation: 'sum', field: 'b' }, + { aggregation: 'count', field: 'd' }, + ]; + const getValue = (item: IDataObject, field: string) => item[field]; + expect(() => { + checkIfFieldExists.call(mockExecuteFunctions, items, aggregations, getValue); + }).toThrow(NodeOperationError); + }); + + it("should throw NodeOperationError with error message containing the field name that doesn't exist", () => { + const aggregations: Aggregations = [{ aggregation: 'count', field: 'D' }]; + const getValue = (item: IDataObject, field: string) => item[field]; + expect(() => { + checkIfFieldExists.call(mockExecuteFunctions, items, aggregations, getValue); + }).toThrow("The field 'D' does not exist in any items"); + }); + + it('should not throw error if field is empty string', () => { + const aggregations: Aggregations = [{ aggregation: 'count', field: '' }]; + const getValue = (item: IDataObject, field: string) => item[field]; + expect(() => { + checkIfFieldExists.call(mockExecuteFunctions, items, aggregations, getValue); + }).not.toThrow(); + }); +}); diff --git a/packages/nodes-base/nodes/Transform/Summarize/test/unitTests/execute.test.ts b/packages/nodes-base/nodes/Transform/Summarize/test/unitTests/execute.test.ts new file mode 100644 index 0000000000..d675e4be6d --- /dev/null +++ b/packages/nodes-base/nodes/Transform/Summarize/test/unitTests/execute.test.ts @@ -0,0 +1,80 @@ +import type { MockProxy } from 'jest-mock-extended'; +import { mock } from 'jest-mock-extended'; +import type { IExecuteFunctions } from 'n8n-workflow'; +import { NodeExecutionOutput, NodeOperationError } from 'n8n-workflow'; + +import { Summarize } from '../../Summarize.node'; +import type { Aggregations } from '../../utils'; + +let summarizeNode: Summarize; +let mockExecuteFunctions: MockProxy; + +describe('Test Summarize Node, execute', () => { + beforeEach(() => { + summarizeNode = new Summarize(); + mockExecuteFunctions = mock({ + getNode: jest.fn().mockReturnValue({ name: 'test-node' }), + getNodeParameter: jest.fn(), + getInputData: jest.fn(), + helpers: { + constructExecutionMetaData: jest.fn().mockReturnValue([]), + }, + }); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should handle field not found with hints if version > 1', async () => { + mockExecuteFunctions.getInputData.mockReturnValue([{ json: { someField: 1 } }]); + mockExecuteFunctions.getNode.mockReturnValue({ + id: '1', + name: 'test-node', + type: 'test-type', + position: [0, 0], + parameters: {}, + typeVersion: 1.1, + }); + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce({}) // options + .mockReturnValueOnce('') // fieldsToSplitBy + .mockReturnValueOnce([{ field: 'nonexistentField', aggregation: 'sum' }]); // fieldsToSummarize + + const result = await summarizeNode.execute.call(mockExecuteFunctions); + + expect(result).toBeInstanceOf(NodeExecutionOutput); + expect(result).toEqual([[{ json: { sum_nonexistentField: 0 }, pairedItem: [{ item: 0 }] }]]); + expect((result as NodeExecutionOutput).getHints()).toEqual([ + { + location: 'outputPane', + message: "The field 'nonexistentField' does not exist in any items", + }, + ]); + }); + + it('should throw error if node version is < 1.1 and fields not found', async () => { + const items = [{ json: { a: 1, b: 2, c: 3 } }]; + const aggregations: Aggregations = [ + { aggregation: 'sum', field: 'b' }, + { aggregation: 'count', field: 'd' }, + ]; + + mockExecuteFunctions.getNode.mockReturnValue({ + id: '1', + name: 'test-node', + type: 'test-type', + position: [0, 0], + parameters: {}, + typeVersion: 1, + }); + mockExecuteFunctions.getInputData.mockReturnValue(items); + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce({}) // options + .mockReturnValueOnce('') // fieldsToSplitBy + .mockReturnValueOnce(aggregations); // fieldsToSummarize + await expect(async () => { + await summarizeNode.execute.bind(mockExecuteFunctions)(); + }).rejects.toThrow(NodeOperationError); + }); +}); diff --git a/packages/nodes-base/nodes/Transform/Summarize/test/workflow.summarize_hint.json b/packages/nodes-base/nodes/Transform/Summarize/test/workflow.summarize_hint.json new file mode 100644 index 0000000000..a0fb6399ad --- /dev/null +++ b/packages/nodes-base/nodes/Transform/Summarize/test/workflow.summarize_hint.json @@ -0,0 +1,70 @@ +{ + "nodes": [ + { + "parameters": {}, + "type": "n8n-nodes-base.manualTrigger", + "typeVersion": 1, + "position": [520, -80], + "id": "9ff1d8e0-ebe8-4c52-931a-e482e14ac911", + "name": "When clicking ‘Test workflow’" + }, + { + "parameters": { + "category": "randomData", + "randomDataSeed": "ria", + "randomDataCount": 5 + }, + "type": "n8n-nodes-base.debugHelper", + "typeVersion": 1, + "position": [700, -80], + "id": "50cea846-f8ed-4b5a-a14a-c11bf4ba0f5d", + "name": "DebugHelper2" + }, + { + "parameters": { + "fieldsToSummarize": { + "values": [ + { + "field": "passwor" + } + ] + }, + "options": {} + }, + "type": "n8n-nodes-base.summarize", + "typeVersion": 1.1, + "position": [900, -80], + "id": "d39fedc9-62e1-45bc-8c9a-d20f2b63b543", + "name": "Summarize1" + } + ], + "connections": { + "When clicking ‘Test workflow’": { + "main": [ + [ + { + "node": "DebugHelper2", + "type": "main", + "index": 0 + } + ] + ] + }, + "DebugHelper2": { + "main": [ + [ + { + "node": "Summarize1", + "type": "main", + "index": 0 + } + ] + ] + } + }, + "pinData": {}, + "meta": { + "templateCredsSetupCompleted": true, + "instanceId": "ee90fdf8d57662f949e6c691dc07fa0fd2f66e1eee28ed82ef06658223e67255" + } +} diff --git a/packages/nodes-base/nodes/Transform/Summarize/utils.ts b/packages/nodes-base/nodes/Transform/Summarize/utils.ts index 0094d4d8ef..9a67ad0b98 100644 --- a/packages/nodes-base/nodes/Transform/Summarize/utils.ts +++ b/packages/nodes-base/nodes/Transform/Summarize/utils.ts @@ -231,20 +231,20 @@ export function splitData( const [firstSplitKey, ...restSplitKeys] = splitKeys; const groupedData = data.reduce((acc, item) => { - let keyValuee = getValue(item, firstSplitKey) as string; + let keyValue = getValue(item, firstSplitKey) as string; - if (typeof keyValuee === 'object') { - keyValuee = JSON.stringify(keyValuee); + if (typeof keyValue === 'object') { + keyValue = JSON.stringify(keyValue); } - if (options.skipEmptySplitFields && typeof keyValuee !== 'number' && !keyValuee) { + if (options.skipEmptySplitFields && typeof keyValue !== 'number' && !keyValue) { return acc; } - if (acc[keyValuee] === undefined) { - acc[keyValuee] = [item]; + if (acc[keyValue] === undefined) { + acc[keyValue] = [item]; } else { - (acc[keyValuee] as IDataObject[]).push(item); + (acc[keyValue] as IDataObject[]).push(item); } return acc; }, {} as IDataObject);