Skip to content

Commit

Permalink
Merge pull request #12020 from nestjs/fix/throw-circular-custom-provi…
Browse files Browse the repository at this point in the history
…ders

fix(core): throw on circular factories and custom providers
  • Loading branch information
kamilmysliwiec committed Jul 17, 2023
2 parents dba6fa4 + 1fea632 commit 4c37a2d
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 17 deletions.
63 changes: 63 additions & 0 deletions integration/injector/e2e/circular-custom-providers.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { Test } from '@nestjs/testing';
import { expect } from 'chai';

import { Controller, Injectable, Module } from '@nestjs/common';

class B {}

@Injectable()
class A {
constructor(b: B) {}
}

@Injectable()
class BImpl {
constructor(a: A) {}
}

@Controller()
class AppController {
constructor(a: A) {}
}

@Module({
imports: [],
controllers: [AppController],
providers: [A, { provide: B, useClass: BImpl }],
})
export class AppModule {}

describe('Circular custom providers', () => {
it('should throw an exception (useClass + regular provider)', async () => {
try {
const builder = Test.createTestingModule({
imports: [AppModule],
});
await builder.compile();

expect(true).to.be.eql(false);
} catch (err) {
expect(err.message).to.be.eql(
'A circular dependency has been detected inside "A". Please, make sure that each side of a bidirectional relationships are decorated with "forwardRef()". Note that circular relationships between custom providers (e.g., factories) are not supported since functions cannot be called more than once.',
);
}
});

it('should throw an exception (2 factories)', async () => {
try {
const builder = Test.createTestingModule({
providers: [
{ provide: 'ABC', useFactory: () => ({}), inject: ['DEF'] },
{ provide: 'DEF', useFactory: () => ({}), inject: ['ABC'] },
],
});
await builder.compile();

expect(true).to.be.eql(false);
} catch (err) {
expect(err.message).to.be.eql(
'A circular dependency has been detected inside "ABC". Please, make sure that each side of a bidirectional relationships are decorated with "forwardRef()". Note that circular relationships between custom providers (e.g., factories) are not supported since functions cannot be called more than once.',
);
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ export class CircularDependencyException extends RuntimeException {
constructor(context?: string) {
const ctx = context ? ` inside ${context}` : ``;
super(
`A circular dependency has been detected${ctx}. Please, make sure that each side of a bidirectional relationships are decorated with "forwardRef()".`,
`A circular dependency has been detected${ctx}. Please, make sure that each side of a bidirectional relationships are decorated with "forwardRef()". Note that circular relationships between custom providers (e.g., factories) are not supported since functions cannot be called more than once.`,
);
}
}
41 changes: 28 additions & 13 deletions packages/core/injector/injector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
} from '@nestjs/common/utils/shared.utils';
import { iterate } from 'iterare';
import { performance } from 'perf_hooks';
import { CircularDependencyException } from '../errors/exceptions';
import { RuntimeException } from '../errors/exceptions/runtime.exception';
import { UndefinedDependencyException } from '../errors/exceptions/undefined-dependency.exception';
import { UnknownDependenciesException } from '../errors/exceptions/unknown-dependencies.exception';
Expand All @@ -35,6 +36,7 @@ import {
PropertyMetadata,
} from './instance-wrapper';
import { Module } from './module';
import { SettlementSignal } from './settlement-signal';

/**
* The type of an injectable dependency
Expand Down Expand Up @@ -111,14 +113,21 @@ export class Injector {
this.getContextId(contextId, wrapper),
inquirerId,
);

if (instanceHost.isPending) {
const settlementSignal = wrapper.settlementSignal;
if (inquirer && settlementSignal?.isCycle(inquirer.id)) {
throw new CircularDependencyException(`"${wrapper.name}"`);
}

return instanceHost.donePromise.then((err?: unknown) => {
if (err) {
throw err;
}
});
}
const done = this.applyDoneHook(instanceHost);

const settlementSignal = this.applySettlementSignal(instanceHost, wrapper);
const token = wrapper.token || wrapper.name;

const { inject } = wrapper;
Expand All @@ -127,7 +136,7 @@ export class Injector {
throw new RuntimeException();
}
if (instanceHost.isResolved) {
return done();
return settlementSignal.complete();
}
try {
const t0 = this.getNowTimestamp();
Expand All @@ -149,7 +158,7 @@ export class Injector {
);
this.applyProperties(instance, properties);
wrapper.initTime = this.getNowTimestamp() - t0;
done();
settlementSignal.complete();
};
await this.resolveConstructorParams<T>(
wrapper,
Expand All @@ -161,7 +170,7 @@ export class Injector {
inquirer,
);
} catch (err) {
done(err);
settlementSignal.error(err);
throw err;
}
}
Expand Down Expand Up @@ -237,15 +246,16 @@ export class Injector {
await this.loadEnhancersPerContext(wrapper, contextId, wrapper);
}

public applyDoneHook<T>(
wrapper: InstancePerContext<T>,
): (err?: unknown) => void {
let done: (err?: unknown) => void;
wrapper.donePromise = new Promise<unknown>((resolve, reject) => {
done = resolve;
});
wrapper.isPending = true;
return done;
public applySettlementSignal<T>(
instancePerContext: InstancePerContext<T>,
host: InstanceWrapper<T>,
) {
const settlementSignal = new SettlementSignal();
instancePerContext.donePromise = settlementSignal.asPromise();
instancePerContext.isPending = true;
host.settlementSignal = settlementSignal;

return settlementSignal;
}

public async resolveConstructorParams<T>(
Expand Down Expand Up @@ -457,6 +467,8 @@ export class Injector {
inquirerId,
);
if (!instanceHost.isResolved && !instanceWrapper.forwardRef) {
inquirer?.settlementSignal?.insertRef(instanceWrapper.id);

await this.loadProvider(
instanceWrapper,
instanceWrapper.host ?? moduleRef,
Expand Down Expand Up @@ -581,6 +593,7 @@ export class Injector {
}
this.printLookingForProviderLog(name, relatedModule);
moduleRegistry.push(relatedModule.id);

const { providers, exports } = relatedModule;
if (!exports.has(name) || !providers.has(name)) {
const instanceRef = await this.lookupComponentInImports(
Expand Down Expand Up @@ -609,6 +622,8 @@ export class Injector {
inquirerId,
);
if (!instanceHost.isResolved && !instanceWrapperRef.forwardRef) {
wrapper.settlementSignal?.insertRef(instanceWrapperRef.id);

await this.loadProvider(
instanceWrapperRef,
relatedModule,
Expand Down
10 changes: 7 additions & 3 deletions packages/core/injector/instance-wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
isValueProvider,
} from './helpers/provider-classifier';
import { Module } from './module';
import { SettlementSignal } from './settlement-signal';

export const INSTANCE_METADATA_SYMBOL = Symbol.for('instance_metadata:cache');
export const INSTANCE_ID_SYMBOL = Symbol.for('instance_metadata:id');
Expand Down Expand Up @@ -63,13 +64,13 @@ export class InstanceWrapper<T = any> {
public readonly host?: Module;
public readonly isAlias: boolean = false;
public readonly subtype?: EnhancerSubtype;

public scope?: Scope = Scope.DEFAULT;
public metatype: Type<T> | Function;
public inject?: FactoryProvider['inject'];
public forwardRef?: boolean;
public durable?: boolean;
public initTime?: number;
public settlementSignal?: SettlementSignal;

private static logger: LoggerService = new Logger(InstanceWrapper.name);

Expand Down Expand Up @@ -104,8 +105,11 @@ export class InstanceWrapper<T = any> {
}

get isNotMetatype(): boolean {
const isFactory = this.metatype && !isNil(this.inject);
return !this.metatype || isFactory;
return !this.metatype || this.isFactory;
}

get isFactory(): boolean {
return this.metatype && !isNil(this.inject);
}

get isTransient(): boolean {
Expand Down
56 changes: 56 additions & 0 deletions packages/core/injector/settlement-signal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* SettlementSignal is used to signal the resolution of a provider/instance.
* Calling `complete` or `error` will resolve the promise returned by `asPromise`.
* Can be used to detect circular dependencies.
*/
export class SettlementSignal {
private readonly _refs = new Set();
private readonly settledPromise: Promise<unknown>;
private settleFn!: (err?: unknown) => void;

constructor() {
this.settledPromise = new Promise<unknown>(resolve => {
this.settleFn = resolve;
});
}

/**
* Resolves the promise returned by `asPromise`.
*/
public complete() {
this.settleFn();
}

/**
* Rejects the promise returned by `asPromise` with the given error.
* @param err Error to reject the promise returned by `asPromise` with.
*/
public error(err: unknown) {
this.settleFn(err);
}

/**
* Returns a promise that will be resolved when `complete` or `error` is called.
* @returns Promise that will be resolved when `complete` or `error` is called.
*/
public asPromise() {
return this.settledPromise;
}

/**
* Inserts a wrapper id that the host of this signal depends on.
* @param wrapperId Wrapper id to insert.
*/
public insertRef(wrapperId: string) {
this._refs.add(wrapperId);
}

/**
* Check if relationship is circular.
* @param wrapperId Wrapper id to check.
* @returns True if relationship is circular, false otherwise.
*/
public isCycle(wrapperId: string) {
return this._refs.has(wrapperId);
}
}

0 comments on commit 4c37a2d

Please sign in to comment.