Skip to content

Commit

Permalink
fix(core): Improve cyclic dependency check in the DI container (#12600)
Browse files Browse the repository at this point in the history
  • Loading branch information
netroy authored Jan 15, 2025
1 parent eceee7f commit c3c4a20
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 11 deletions.
7 changes: 6 additions & 1 deletion packages/@n8n/di/jest.config.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,7 @@
/** @type {import('jest').Config} */
module.exports = require('../../../jest.config');
module.exports = {
...require('../../../jest.config'),
transform: {
'^.+\\.ts$': ['ts-jest', { isolatedModules: false }],
},
};
17 changes: 17 additions & 0 deletions packages/@n8n/di/src/__tests__/circular-depedency.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { ServiceA } from './fixtures/ServiceA';
import { ServiceB } from './fixtures/ServiceB';
import { Container } from '../di';

describe('DI Container', () => {
describe('circular dependency', () => {
it('should detect multilevel circular dependencies', () => {
expect(() => Container.get(ServiceA)).toThrow(
'[DI] Circular dependency detected in ServiceB at index 0.\nServiceA -> ServiceB',
);

expect(() => Container.get(ServiceB)).toThrow(
'[DI] Circular dependency detected in ServiceB at index 0.\nServiceB',
);
});
});
});
8 changes: 8 additions & 0 deletions packages/@n8n/di/src/__tests__/fixtures/ServiceA.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// eslint-disable-next-line import/no-cycle
import { ServiceB } from './ServiceB';
import { Service } from '../../di';

@Service()
export class ServiceA {
constructor(readonly b: ServiceB) {}
}
8 changes: 8 additions & 0 deletions packages/@n8n/di/src/__tests__/fixtures/ServiceB.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// eslint-disable-next-line import/no-cycle
import { ServiceA } from './ServiceA';
import { Service } from '../../di';

@Service()
export class ServiceB {
constructor(readonly a: ServiceA) {}
}
19 changes: 9 additions & 10 deletions packages/@n8n/di/src/di.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,6 @@ class ContainerClass {

if (metadata?.instance) return metadata.instance as T;

// Check for circular dependencies before proceeding with instantiation
if (resolutionStack.includes(type)) {
throw new DIError(
`Circular dependency detected. ${resolutionStack.map((t) => t.name).join(' -> ')}`,
);
}

// Add current type to resolution stack before resolving dependencies
resolutionStack.push(type);

Expand All @@ -96,9 +89,15 @@ class ContainerClass {
} else {
const paramTypes = (Reflect.getMetadata('design:paramtypes', type) ??
[]) as Constructable[];
const dependencies = paramTypes.map(<P>(paramType: Constructable<P>) =>
this.get(paramType),
);

const dependencies = paramTypes.map(<P>(paramType: Constructable<P>, index: number) => {
if (paramType === undefined) {
throw new DIError(
`Circular dependency detected in ${type.name} at index ${index}.\n${resolutionStack.map((t) => t.name).join(' -> ')}\n`,
);
}
return this.get(paramType);
});
// Create new instance with resolved dependencies
instance = new (type as Constructable)(...dependencies) as T;
}
Expand Down

0 comments on commit c3c4a20

Please sign in to comment.