fix: resolve issues #248 and #249 - settings validation and addConnection errors

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 <noreply@anthropic.com>
2026-02-06 05:23:08 +00:00 · 2025-10-02 15:09:10 +02:00
parent 39e13c451f
commit e252a36e3f
4 changed files with 316 additions and 9 deletions
--- 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',
+        });
+      });
    });
  });

--- 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', () => {