mirror of
https://github.com/czlonkowski/n8n-mcp.git
synced 2026-01-30 06:22:04 +00:00
fix: add warnings for If/Switch node connection parameters (issue #360)
Implemented a warning system to guide users toward using smart parameters (branch="true"/"false" for If nodes, case=N for Switch nodes) instead of sourceIndex, which can lead to incorrect branch routing. Changes: - Added warnings property to WorkflowDiffResult interface - Warnings generated when sourceIndex used with If/Switch nodes - Enhanced tool documentation with CRITICAL pitfalls - Added regression tests reproducing issue #360 - Version bump to 2.22.1 The branch parameter functionality works correctly - this fix adds helpful warnings to prevent users from accidentally using the less intuitive sourceIndex parameter. Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
49
CHANGELOG.md
49
CHANGELOG.md
@@ -7,6 +7,55 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
|
|
||||||
## [Unreleased]
|
## [Unreleased]
|
||||||
|
|
||||||
|
### 🐛 Bug Fixes
|
||||||
|
|
||||||
|
**Issue #360: Enhanced Warnings for If/Switch Node Connection Parameters**
|
||||||
|
|
||||||
|
Fixed issue where users could unintentionally place multiple If node connections on the same branch (TRUE/FALSE) when using `sourceIndex` parameter instead of the recommended `branch` parameter. The system now provides helpful warnings to guide users toward better practices.
|
||||||
|
|
||||||
|
#### What Was Fixed
|
||||||
|
|
||||||
|
1. **New Warning System**:
|
||||||
|
- Warns when using `sourceIndex` with If nodes - suggests `branch="true"` or `branch="false"` instead
|
||||||
|
- Warns when using `sourceIndex` with Switch nodes - suggests `case=N` instead
|
||||||
|
- Explains the correct branch structure: `main[0]=TRUE branch, main[1]=FALSE branch`
|
||||||
|
|
||||||
|
2. **Enhanced Documentation**:
|
||||||
|
- Added **CRITICAL** pitfalls to `n8n_update_partial_workflow` tool documentation
|
||||||
|
- Clear guidance that using `sourceIndex=0` for multiple connections puts them ALL on the TRUE branch
|
||||||
|
- Examples showing correct vs. incorrect usage
|
||||||
|
|
||||||
|
3. **Type System Improvements**:
|
||||||
|
- Added `warnings` field to `WorkflowDiffResult` interface
|
||||||
|
- Warnings are non-blocking (operations still succeed)
|
||||||
|
- Differentiated from errors for better UX
|
||||||
|
|
||||||
|
#### Behavior
|
||||||
|
|
||||||
|
The existing `branch` parameter works correctly and has comprehensive test coverage:
|
||||||
|
- `branch="true"` → routes to `main[0]` (TRUE path)
|
||||||
|
- `branch="false"` → routes to `main[1]` (FALSE path)
|
||||||
|
|
||||||
|
The issue was that users who didn't know about the `branch` parameter would naturally use `sourceIndex`, which led to incorrect branch routing.
|
||||||
|
|
||||||
|
#### Example Warning
|
||||||
|
|
||||||
|
```
|
||||||
|
Connection to If node "Check Condition" uses sourceIndex=0.
|
||||||
|
Consider using branch="true" or branch="false" for better clarity.
|
||||||
|
If node outputs: main[0]=TRUE branch, main[1]=FALSE branch.
|
||||||
|
```
|
||||||
|
|
||||||
|
#### Test Coverage
|
||||||
|
|
||||||
|
- Added regression tests that reproduce the exact issue from #360
|
||||||
|
- Verify warnings are generated for If and Switch nodes
|
||||||
|
- Confirm existing smart parameter tests still pass
|
||||||
|
|
||||||
|
**Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en**
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
### ✨ New Features
|
### ✨ New Features
|
||||||
|
|
||||||
**Auto-Update Node Versions with Smart Migration**
|
**Auto-Update Node Versions with Smart Migration**
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"name": "n8n-mcp",
|
"name": "n8n-mcp",
|
||||||
"version": "2.22.0",
|
"version": "2.22.1",
|
||||||
"description": "Integration between n8n workflow automation and Model Context Protocol (MCP)",
|
"description": "Integration between n8n workflow automation and Model Context Protocol (MCP)",
|
||||||
"main": "dist/index.js",
|
"main": "dist/index.js",
|
||||||
"types": "dist/index.d.ts",
|
"types": "dist/index.d.ts",
|
||||||
|
|||||||
@@ -272,6 +272,8 @@ Please choose a different name.
|
|||||||
'Use "updates" property for updateNode operations: {type: "updateNode", updates: {...}}',
|
'Use "updates" property for updateNode operations: {type: "updateNode", updates: {...}}',
|
||||||
'Smart parameters (branch, case) only work with IF and Switch nodes - ignored for other node types',
|
'Smart parameters (branch, case) only work with IF and Switch nodes - ignored for other node types',
|
||||||
'Explicit sourceIndex overrides smart parameters (branch, case) if both provided',
|
'Explicit sourceIndex overrides smart parameters (branch, case) if both provided',
|
||||||
|
'**CRITICAL**: For If nodes, ALWAYS use branch="true"/"false" instead of sourceIndex. Using sourceIndex=0 for multiple connections will put them ALL on the TRUE branch (main[0]), breaking your workflow logic!',
|
||||||
|
'**CRITICAL**: For Switch nodes, ALWAYS use case=N instead of sourceIndex. Using same sourceIndex for multiple connections will put them on the same case output.',
|
||||||
'cleanStaleConnections removes ALL broken connections - cannot be selective',
|
'cleanStaleConnections removes ALL broken connections - cannot be selective',
|
||||||
'replaceConnections overwrites entire connections object - all previous connections lost',
|
'replaceConnections overwrites entire connections object - all previous connections lost',
|
||||||
'**Auto-sanitization behavior**: Binary operators (equals, contains) automatically have singleValue removed; unary operators (isEmpty, isNotEmpty) automatically get singleValue:true added',
|
'**Auto-sanitization behavior**: Binary operators (equals, contains) automatically have singleValue removed; unary operators (isEmpty, isNotEmpty) automatically get singleValue:true added',
|
||||||
|
|||||||
@@ -38,6 +38,8 @@ const logger = new Logger({ prefix: '[WorkflowDiffEngine]' });
|
|||||||
export class WorkflowDiffEngine {
|
export class WorkflowDiffEngine {
|
||||||
// Track node name changes during operations for connection reference updates
|
// Track node name changes during operations for connection reference updates
|
||||||
private renameMap: Map<string, string> = new Map();
|
private renameMap: Map<string, string> = new Map();
|
||||||
|
// Track warnings during operation processing
|
||||||
|
private warnings: WorkflowDiffValidationError[] = [];
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Apply diff operations to a workflow
|
* Apply diff operations to a workflow
|
||||||
@@ -47,8 +49,9 @@ export class WorkflowDiffEngine {
|
|||||||
request: WorkflowDiffRequest
|
request: WorkflowDiffRequest
|
||||||
): Promise<WorkflowDiffResult> {
|
): Promise<WorkflowDiffResult> {
|
||||||
try {
|
try {
|
||||||
// Reset rename tracking for this diff operation
|
// Reset tracking for this diff operation
|
||||||
this.renameMap.clear();
|
this.renameMap.clear();
|
||||||
|
this.warnings = [];
|
||||||
|
|
||||||
// Clone workflow to avoid modifying original
|
// Clone workflow to avoid modifying original
|
||||||
const workflowCopy = JSON.parse(JSON.stringify(workflow));
|
const workflowCopy = JSON.parse(JSON.stringify(workflow));
|
||||||
@@ -114,6 +117,7 @@ export class WorkflowDiffEngine {
|
|||||||
? 'Validation successful. All operations are valid.'
|
? 'Validation successful. All operations are valid.'
|
||||||
: `Validation completed with ${errors.length} errors.`,
|
: `Validation completed with ${errors.length} errors.`,
|
||||||
errors: errors.length > 0 ? errors : undefined,
|
errors: errors.length > 0 ? errors : undefined,
|
||||||
|
warnings: this.warnings.length > 0 ? this.warnings : undefined,
|
||||||
applied: appliedIndices,
|
applied: appliedIndices,
|
||||||
failed: failedIndices
|
failed: failedIndices
|
||||||
};
|
};
|
||||||
@@ -126,6 +130,7 @@ export class WorkflowDiffEngine {
|
|||||||
operationsApplied: appliedIndices.length,
|
operationsApplied: appliedIndices.length,
|
||||||
message: `Applied ${appliedIndices.length} operations, ${failedIndices.length} failed (continueOnError mode)`,
|
message: `Applied ${appliedIndices.length} operations, ${failedIndices.length} failed (continueOnError mode)`,
|
||||||
errors: errors.length > 0 ? errors : undefined,
|
errors: errors.length > 0 ? errors : undefined,
|
||||||
|
warnings: this.warnings.length > 0 ? this.warnings : undefined,
|
||||||
applied: appliedIndices,
|
applied: appliedIndices,
|
||||||
failed: failedIndices
|
failed: failedIndices
|
||||||
};
|
};
|
||||||
@@ -213,7 +218,8 @@ export class WorkflowDiffEngine {
|
|||||||
success: true,
|
success: true,
|
||||||
workflow: workflowCopy,
|
workflow: workflowCopy,
|
||||||
operationsApplied,
|
operationsApplied,
|
||||||
message: `Successfully applied ${operationsApplied} operations (${nodeOperations.length} node ops, ${otherOperations.length} other ops)`
|
message: `Successfully applied ${operationsApplied} operations (${nodeOperations.length} node ops, ${otherOperations.length} other ops)`,
|
||||||
|
warnings: this.warnings.length > 0 ? this.warnings : undefined
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
@@ -685,6 +691,24 @@ export class WorkflowDiffEngine {
|
|||||||
sourceIndex = operation.case;
|
sourceIndex = operation.case;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Validation: Warn if using sourceIndex with If/Switch nodes without smart parameters
|
||||||
|
if (sourceNode && operation.sourceIndex !== undefined && operation.branch === undefined && operation.case === undefined) {
|
||||||
|
if (sourceNode.type === 'n8n-nodes-base.if') {
|
||||||
|
this.warnings.push({
|
||||||
|
operation: -1, // Not tied to specific operation index in request
|
||||||
|
message: `Connection to If node "${operation.source}" uses sourceIndex=${operation.sourceIndex}. ` +
|
||||||
|
`Consider using branch="true" or branch="false" for better clarity. ` +
|
||||||
|
`If node outputs: main[0]=TRUE branch, main[1]=FALSE branch.`
|
||||||
|
});
|
||||||
|
} else if (sourceNode.type === 'n8n-nodes-base.switch') {
|
||||||
|
this.warnings.push({
|
||||||
|
operation: -1, // Not tied to specific operation index in request
|
||||||
|
message: `Connection to Switch node "${operation.source}" uses sourceIndex=${operation.sourceIndex}. ` +
|
||||||
|
`Consider using case=N for better clarity (case=0 for first output, case=1 for second, etc.).`
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
return { sourceOutput, sourceIndex };
|
return { sourceOutput, sourceIndex };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -170,6 +170,7 @@ export interface WorkflowDiffResult {
|
|||||||
success: boolean;
|
success: boolean;
|
||||||
workflow?: any; // Updated workflow if successful
|
workflow?: any; // Updated workflow if successful
|
||||||
errors?: WorkflowDiffValidationError[];
|
errors?: WorkflowDiffValidationError[];
|
||||||
|
warnings?: WorkflowDiffValidationError[]; // Non-blocking warnings (e.g., parameter suggestions)
|
||||||
operationsApplied?: number;
|
operationsApplied?: number;
|
||||||
message?: string;
|
message?: string;
|
||||||
applied?: number[]; // Indices of successfully applied operations (when continueOnError is true)
|
applied?: number[]; // Indices of successfully applied operations (when continueOnError is true)
|
||||||
|
|||||||
@@ -1418,6 +1418,113 @@ describe('WorkflowDiffEngine', () => {
|
|||||||
expect(result.workflow!.connections['Switch']['main'][2][0].node).toBe('Handler');
|
expect(result.workflow!.connections['Switch']['main'][2][0].node).toBe('Handler');
|
||||||
expect(result.workflow!.connections['Switch']['main'][1]).toEqual([]);
|
expect(result.workflow!.connections['Switch']['main'][1]).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should warn when using sourceIndex with If node (issue #360)', async () => {
|
||||||
|
const addIF: any = {
|
||||||
|
type: 'addNode',
|
||||||
|
node: {
|
||||||
|
name: 'Check Condition',
|
||||||
|
type: 'n8n-nodes-base.if',
|
||||||
|
position: [400, 300]
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
const addSuccess: any = {
|
||||||
|
type: 'addNode',
|
||||||
|
node: {
|
||||||
|
name: 'Success Handler',
|
||||||
|
type: 'n8n-nodes-base.set',
|
||||||
|
position: [600, 200]
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
const addError: any = {
|
||||||
|
type: 'addNode',
|
||||||
|
node: {
|
||||||
|
name: 'Error Handler',
|
||||||
|
type: 'n8n-nodes-base.set',
|
||||||
|
position: [600, 400]
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
// BAD: Using sourceIndex with If node (reproduces issue #360)
|
||||||
|
const connectSuccess: any = {
|
||||||
|
type: 'addConnection',
|
||||||
|
source: 'Check Condition',
|
||||||
|
target: 'Success Handler',
|
||||||
|
sourceIndex: 0 // Should use branch="true" instead
|
||||||
|
};
|
||||||
|
|
||||||
|
const connectError: any = {
|
||||||
|
type: 'addConnection',
|
||||||
|
source: 'Check Condition',
|
||||||
|
target: 'Error Handler',
|
||||||
|
sourceIndex: 0 // Should use branch="false" instead - both will end up in main[0]!
|
||||||
|
};
|
||||||
|
|
||||||
|
const request: WorkflowDiffRequest = {
|
||||||
|
id: 'test-workflow',
|
||||||
|
operations: [addIF, addSuccess, addError, connectSuccess, connectError]
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = await diffEngine.applyDiff(baseWorkflow, request);
|
||||||
|
|
||||||
|
expect(result.success).toBe(true);
|
||||||
|
|
||||||
|
// Should produce warnings
|
||||||
|
expect(result.warnings).toBeDefined();
|
||||||
|
expect(result.warnings!.length).toBe(2);
|
||||||
|
expect(result.warnings![0].message).toContain('Consider using branch="true" or branch="false"');
|
||||||
|
expect(result.warnings![0].message).toContain('If node outputs: main[0]=TRUE branch, main[1]=FALSE branch');
|
||||||
|
expect(result.warnings![1].message).toContain('Consider using branch="true" or branch="false"');
|
||||||
|
|
||||||
|
// Both connections end up in main[0] (the bug behavior)
|
||||||
|
expect(result.workflow!.connections['Check Condition']['main'][0].length).toBe(2);
|
||||||
|
expect(result.workflow!.connections['Check Condition']['main'][0][0].node).toBe('Success Handler');
|
||||||
|
expect(result.workflow!.connections['Check Condition']['main'][0][1].node).toBe('Error Handler');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should warn when using sourceIndex with Switch node', async () => {
|
||||||
|
const addSwitch: any = {
|
||||||
|
type: 'addNode',
|
||||||
|
node: {
|
||||||
|
name: 'Switch',
|
||||||
|
type: 'n8n-nodes-base.switch',
|
||||||
|
position: [400, 300]
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
const addHandler: any = {
|
||||||
|
type: 'addNode',
|
||||||
|
node: {
|
||||||
|
name: 'Handler',
|
||||||
|
type: 'n8n-nodes-base.set',
|
||||||
|
position: [600, 300]
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
// BAD: Using sourceIndex with Switch node
|
||||||
|
const connect: any = {
|
||||||
|
type: 'addConnection',
|
||||||
|
source: 'Switch',
|
||||||
|
target: 'Handler',
|
||||||
|
sourceIndex: 1 // Should use case=1 instead
|
||||||
|
};
|
||||||
|
|
||||||
|
const request: WorkflowDiffRequest = {
|
||||||
|
id: 'test-workflow',
|
||||||
|
operations: [addSwitch, addHandler, connect]
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = await diffEngine.applyDiff(baseWorkflow, request);
|
||||||
|
|
||||||
|
expect(result.success).toBe(true);
|
||||||
|
|
||||||
|
// Should produce warning
|
||||||
|
expect(result.warnings).toBeDefined();
|
||||||
|
expect(result.warnings!.length).toBe(1);
|
||||||
|
expect(result.warnings![0].message).toContain('Consider using case=N for better clarity');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('AddConnection with sourceIndex (Phase 0 Fix)', () => {
|
describe('AddConnection with sourceIndex (Phase 0 Fix)', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user