From e252a36e3f7ad930d33ddc0a8f0be738ff060b43 Mon Sep 17 00:00:00 2001 From: czlonkowski <56956555+czlonkowski@users.noreply.github.com> Date: Thu, 2 Oct 2025 15:09:10 +0200 Subject: [PATCH] fix: resolve issues #248 and #249 - settings validation and addConnection errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #248: Settings validation error - Add callerPolicy to workflowSettingsSchema to support valid n8n property - Implement settings filtering in cleanWorkflowForUpdate() to prevent API errors - Filter out UI-only properties like timeSavedPerExecution - Preserve only whitelisted settings properties - Add comprehensive unit tests for settings filtering Issue #249: Misleading error messages for addConnection - Enhanced validateAddConnection() with parameter validation - Detect common mistakes like using sourceNodeId/targetNodeId instead of source/target - Provide helpful error messages with correct parameter names - List available nodes when source/target not found - Add unit tests for all error scenarios All tests passing (183 total): - n8n-validation: 73/73 tests (100% coverage) - workflow-diff-engine: 110/110 tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/services/n8n-validation.ts | 34 ++++- src/services/workflow-diff-engine.ts | 30 +++- tests/unit/services/n8n-validation.test.ts | 120 +++++++++++++++ .../services/workflow-diff-engine.test.ts | 141 +++++++++++++++++- 4 files changed, 316 insertions(+), 9 deletions(-) diff --git a/src/services/n8n-validation.ts b/src/services/n8n-validation.ts index af8247c..9448234 100644 --- a/src/services/n8n-validation.ts +++ b/src/services/n8n-validation.ts @@ -45,6 +45,7 @@ export const workflowSettingsSchema = z.object({ saveExecutionProgress: z.boolean().default(true), executionTimeout: z.number().optional(), errorWorkflow: z.string().optional(), + callerPolicy: z.enum(['any', 'workflowsFromSameOwner', 'workflowsFromAList']).optional(), }); // Default settings for workflow creation @@ -95,14 +96,17 @@ export function cleanWorkflowForCreate(workflow: Partial): Partial { ...cleanedWorkflow } = workflow as any; + // Filter settings to only include valid properties (Issue #248 fix) + // Prevents "settings must NOT have additional properties" API errors + if (cleanedWorkflow.settings) { + const allowedSettingsKeys = [ + 'executionOrder', + 'timezone', + 'saveDataErrorExecution', + 'saveDataSuccessExecution', + 'saveManualExecutions', + 'saveExecutionProgress', + 'executionTimeout', + 'errorWorkflow', + 'callerPolicy', + ]; + + const filteredSettings: any = {}; + for (const key of allowedSettingsKeys) { + if (key in cleanedWorkflow.settings) { + filteredSettings[key] = cleanedWorkflow.settings[key]; + } + } + cleanedWorkflow.settings = filteredSettings; + } + return cleanedWorkflow; } diff --git a/src/services/workflow-diff-engine.ts b/src/services/workflow-diff-engine.ts index d4d2432..3d00210 100644 --- a/src/services/workflow-diff-engine.ts +++ b/src/services/workflow-diff-engine.ts @@ -362,16 +362,36 @@ export class WorkflowDiffEngine { // Connection operation validators private validateAddConnection(workflow: Workflow, operation: AddConnectionOperation): string | null { + // Check for common parameter mistakes (Issue #249) + const operationAny = operation as any; + if (operationAny.sourceNodeId || operationAny.targetNodeId) { + const wrongParams: string[] = []; + if (operationAny.sourceNodeId) wrongParams.push('sourceNodeId'); + if (operationAny.targetNodeId) wrongParams.push('targetNodeId'); + + return `Invalid parameter(s): ${wrongParams.join(', ')}. Use 'source' and 'target' instead. Example: {type: "addConnection", source: "Node Name", target: "Target Name"}`; + } + + // Check for missing required parameters + if (!operation.source) { + return `Missing required parameter 'source'. The addConnection operation requires both 'source' and 'target' parameters. Check that you're using 'source' (not 'sourceNodeId').`; + } + if (!operation.target) { + return `Missing required parameter 'target'. The addConnection operation requires both 'source' and 'target' parameters. Check that you're using 'target' (not 'targetNodeId').`; + } + const sourceNode = this.findNode(workflow, operation.source, operation.source); const targetNode = this.findNode(workflow, operation.target, operation.target); - + if (!sourceNode) { - return `Source node not found: ${operation.source}`; + const availableNodes = workflow.nodes.map(n => n.name).join(', '); + return `Source node not found: "${operation.source}". Available nodes: ${availableNodes}`; } if (!targetNode) { - return `Target node not found: ${operation.target}`; + const availableNodes = workflow.nodes.map(n => n.name).join(', '); + return `Target node not found: "${operation.target}". Available nodes: ${availableNodes}`; } - + // Check if connection already exists const sourceOutput = operation.sourceOutput || 'main'; const existing = workflow.connections[sourceNode.name]?.[sourceOutput]; @@ -383,7 +403,7 @@ export class WorkflowDiffEngine { return `Connection already exists from "${sourceNode.name}" to "${targetNode.name}"`; } } - + return null; } diff --git a/tests/unit/services/n8n-validation.test.ts b/tests/unit/services/n8n-validation.test.ts index 7805585..a07cd0e 100644 --- a/tests/unit/services/n8n-validation.test.ts +++ b/tests/unit/services/n8n-validation.test.ts @@ -359,6 +359,126 @@ describe('n8n-validation', () => { const cleaned = cleanWorkflowForUpdate(workflow); expect(cleaned).not.toHaveProperty('settings'); }); + + it('should filter out UI-only settings properties like timeSavedPerExecution (Issue #248)', () => { + const workflow = { + name: 'Test Workflow', + nodes: [], + connections: {}, + settings: { + executionOrder: 'v1' as const, + saveDataSuccessExecution: 'none' as const, + timeSavedPerExecution: 5, // UI-only property - should be removed + unknownProperty: 'test', // Unknown property - should be removed + }, + } as any; + + const cleaned = cleanWorkflowForUpdate(workflow); + + // Should keep valid properties + expect(cleaned.settings).toHaveProperty('executionOrder', 'v1'); + expect(cleaned.settings).toHaveProperty('saveDataSuccessExecution', 'none'); + + // Should remove invalid properties + expect(cleaned.settings).not.toHaveProperty('timeSavedPerExecution'); + expect(cleaned.settings).not.toHaveProperty('unknownProperty'); + }); + + it('should preserve callerPolicy property in settings (Issue #248)', () => { + const workflow = { + name: 'Test Workflow', + nodes: [], + connections: {}, + settings: { + executionOrder: 'v1' as const, + saveDataSuccessExecution: 'none' as const, + callerPolicy: 'workflowsFromSameOwner' as const, + errorWorkflow: 'N2O2nZy3aUiBRGFN', + }, + } as any; + + const cleaned = cleanWorkflowForUpdate(workflow); + + // All these properties should be preserved + expect(cleaned.settings).toHaveProperty('executionOrder', 'v1'); + expect(cleaned.settings).toHaveProperty('saveDataSuccessExecution', 'none'); + expect(cleaned.settings).toHaveProperty('callerPolicy', 'workflowsFromSameOwner'); + expect(cleaned.settings).toHaveProperty('errorWorkflow', 'N2O2nZy3aUiBRGFN'); + }); + + it('should handle settings with both valid and invalid properties (Issue #248)', () => { + const workflow = { + name: 'Test Workflow', + nodes: [], + connections: {}, + settings: { + executionOrder: 'v1' as const, + timezone: 'America/New_York', + timeSavedPerExecution: 10, // Invalid - should be removed + callerPolicy: 'any' as const, // Valid - should be kept + uiProperty: { nested: 'value' }, // Invalid - should be removed + saveExecutionProgress: true, // Valid - should be kept + }, + } as any; + + const cleaned = cleanWorkflowForUpdate(workflow); + + // Valid properties should be kept + expect(cleaned.settings).toHaveProperty('executionOrder', 'v1'); + expect(cleaned.settings).toHaveProperty('timezone', 'America/New_York'); + expect(cleaned.settings).toHaveProperty('callerPolicy', 'any'); + expect(cleaned.settings).toHaveProperty('saveExecutionProgress', true); + + // Invalid properties should be removed + expect(cleaned.settings).not.toHaveProperty('timeSavedPerExecution'); + expect(cleaned.settings).not.toHaveProperty('uiProperty'); + }); + + it('should handle empty settings object', () => { + const workflow = { + name: 'Test Workflow', + nodes: [], + connections: {}, + settings: {}, + } as any; + + const cleaned = cleanWorkflowForUpdate(workflow); + expect(cleaned.settings).toEqual({}); + }); + + it('should preserve all whitelisted settings properties', () => { + const workflow = { + name: 'Test Workflow', + nodes: [], + connections: {}, + settings: { + executionOrder: 'v0' as const, + timezone: 'UTC', + saveDataErrorExecution: 'all' as const, + saveDataSuccessExecution: 'none' as const, + saveManualExecutions: false, + saveExecutionProgress: false, + executionTimeout: 300, + errorWorkflow: 'error-workflow-id', + callerPolicy: 'workflowsFromAList' as const, + }, + } as any; + + const cleaned = cleanWorkflowForUpdate(workflow); + + // All whitelisted properties should be preserved + expect(cleaned.settings).toEqual({ + executionOrder: 'v0', + timezone: 'UTC', + saveDataErrorExecution: 'all', + saveDataSuccessExecution: 'none', + saveManualExecutions: false, + saveExecutionProgress: false, + executionTimeout: 300, + errorWorkflow: 'error-workflow-id', + callerPolicy: 'workflowsFromAList', + }); + }); }); }); diff --git a/tests/unit/services/workflow-diff-engine.test.ts b/tests/unit/services/workflow-diff-engine.test.ts index 5e69be6..ca2f1a9 100644 --- a/tests/unit/services/workflow-diff-engine.test.ts +++ b/tests/unit/services/workflow-diff-engine.test.ts @@ -580,11 +580,150 @@ describe('WorkflowDiffEngine', () => { }; const result = await diffEngine.applyDiff(baseWorkflow, request); - + expect(result.success).toBe(true); expect(result.workflow!.connections['IF'].false).toBeDefined(); expect(result.workflow!.connections['IF'].false[0][0].node).toBe('Slack'); }); + + it('should reject addConnection with wrong parameter sourceNodeId instead of source (Issue #249)', async () => { + const operation: any = { + type: 'addConnection', + sourceNodeId: 'webhook-1', // Wrong parameter name! + target: 'http-1' + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [operation] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(false); + expect(result.errors![0].message).toContain('Invalid parameter(s): sourceNodeId'); + expect(result.errors![0].message).toContain("Use 'source' and 'target' instead"); + }); + + it('should reject addConnection with wrong parameter targetNodeId instead of target (Issue #249)', async () => { + const operation: any = { + type: 'addConnection', + source: 'webhook-1', + targetNodeId: 'http-1' // Wrong parameter name! + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [operation] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(false); + expect(result.errors![0].message).toContain('Invalid parameter(s): targetNodeId'); + expect(result.errors![0].message).toContain("Use 'source' and 'target' instead"); + }); + + it('should reject addConnection with both wrong parameters (Issue #249)', async () => { + const operation: any = { + type: 'addConnection', + sourceNodeId: 'webhook-1', // Wrong! + targetNodeId: 'http-1' // Wrong! + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [operation] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(false); + expect(result.errors![0].message).toContain('Invalid parameter(s): sourceNodeId, targetNodeId'); + expect(result.errors![0].message).toContain("Use 'source' and 'target' instead"); + }); + + it('should show helpful error with available nodes when source is missing (Issue #249)', async () => { + const operation: any = { + type: 'addConnection', + // source is missing entirely + target: 'http-1' + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [operation] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(false); + expect(result.errors![0].message).toContain("Missing required parameter 'source'"); + expect(result.errors![0].message).toContain("not 'sourceNodeId'"); + }); + + it('should show helpful error with available nodes when target is missing (Issue #249)', async () => { + const operation: any = { + type: 'addConnection', + source: 'webhook-1', + // target is missing entirely + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [operation] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(false); + expect(result.errors![0].message).toContain("Missing required parameter 'target'"); + expect(result.errors![0].message).toContain("not 'targetNodeId'"); + }); + + it('should list available nodes when source node not found (Issue #249)', async () => { + const operation: AddConnectionOperation = { + type: 'addConnection', + source: 'non-existent-node', + target: 'http-1' + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [operation] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(false); + expect(result.errors![0].message).toContain('Source node not found: "non-existent-node"'); + expect(result.errors![0].message).toContain('Available nodes:'); + expect(result.errors![0].message).toContain('Webhook'); + expect(result.errors![0].message).toContain('HTTP Request'); + expect(result.errors![0].message).toContain('Slack'); + }); + + it('should list available nodes when target node not found (Issue #249)', async () => { + const operation: AddConnectionOperation = { + type: 'addConnection', + source: 'webhook-1', + target: 'non-existent-node' + }; + + const request: WorkflowDiffRequest = { + id: 'test-workflow', + operations: [operation] + }; + + const result = await diffEngine.applyDiff(baseWorkflow, request); + + expect(result.success).toBe(false); + expect(result.errors![0].message).toContain('Target node not found: "non-existent-node"'); + expect(result.errors![0].message).toContain('Available nodes:'); + expect(result.errors![0].message).toContain('Webhook'); + expect(result.errors![0].message).toContain('HTTP Request'); + expect(result.errors![0].message).toContain('Slack'); + }); }); describe('RemoveConnection Operation', () => {