fix(core): fix di related issues (request scope, modules hierarchy)

Author: kamilmysliwiec

integration/scopes/e2e/request-modules-scope.spec.ts

@@ -0,0 +1,44 @@
+import { INestApplication } from '@nestjs/common';
+import { Test } from '@nestjs/testing';
+import { expect } from 'chai';
+import * as request from 'supertest';
+import { RequestChainModule } from '../src/request-chain/request-chain.module';
+import { RequestChainService } from '../src/request-chain/request-chain.service';
+
+describe('Request scope (modules propagation)', () => {
+  let server;
+  let app: INestApplication;
+
+  before(async () => {
+    const module = await Test.createTestingModule({
+      imports: [RequestChainModule],
+    }).compile();
+
+    app = module.createNestApplication();
+    server = app.getHttpServer();
+    await app.init();
+  });
+
+  describe('when service from parent module is request scoped', () => {
+    before(async () => {
+      const performHttpCall = end =>
+        request(server)
+          .get('/hello')
+          .end((err, res) => {
+            if (err) return end(err);
+            end();
+          });
+      await new Promise(resolve => performHttpCall(resolve));
+      await new Promise(resolve => performHttpCall(resolve));
+      await new Promise(resolve => performHttpCall(resolve));
+    });
+
+    it(`should not fail`, async () => {
+      expect(RequestChainService.COUNTER).to.be.eql(3);
+    });
+  });
+
+  after(async () => {
+    await app.close();
+  });
+});

integration/scopes/src/request-chain/helper/helper.module.ts

@@ -0,0 +1,8 @@
+import { Module } from '@nestjs/common';
+import { HelperService } from './helper.service';
+
+@Module({
+  providers: [HelperService],
+  exports: [HelperService],
+})
+export class HelperModule {}

integration/scopes/src/request-chain/helper/helper.service.ts

@@ -0,0 +1,6 @@
+import { Injectable, Scope } from '@nestjs/common';
+
+@Injectable({ scope: Scope.REQUEST })
+export class HelperService {
+  public noop() {}
+}

integration/scopes/src/request-chain/request-chain.controller.ts

@@ -0,0 +1,12 @@
+import { Controller, Get } from '@nestjs/common';
+import { RequestChainService } from './request-chain.service';
+
+@Controller('hello')
+export class RequestChainController {
+  constructor(private readonly chainService: RequestChainService) {}
+
+  @Get()
+  greeting(): void {
+    this.chainService.call();
+  }
+}

integration/scopes/src/request-chain/request-chain.module.ts

@@ -0,0 +1,11 @@
+import { Module } from '@nestjs/common';
+import { HelperModule } from './helper/helper.module';
+import { RequestChainController } from './request-chain.controller';
+import { RequestChainService } from './request-chain.service';
+
+@Module({
+  imports: [HelperModule],
+  providers: [RequestChainService],
+  controllers: [RequestChainController],
+})
+export class RequestChainModule {}

integration/scopes/src/request-chain/request-chain.service.ts

@@ -0,0 +1,15 @@
+import { Injectable } from '@nestjs/common';
+import { HelperService } from './helper/helper.service';
+
+@Injectable()
+export class RequestChainService {
+  static COUNTER = 0;
+  constructor(private readonly helperService: HelperService) {
+    helperService.noop();
+    RequestChainService.COUNTER += 1;
+  }
+
+  call() {
+    this.helperService.noop();
+  }
+}

packages/core/helpers/external-context-creator.ts

@@ -1,19 +1,23 @@
 import { ForbiddenException, ParamData } from '@nestjs/common';
 import { CUSTOM_ROUTE_AGRS_METADATA } from '@nestjs/common/constants';
-import { Controller, PipeTransform } from '@nestjs/common/interfaces';
+import {
+  ContextType,
+  Controller,
+  PipeTransform,
+} from '@nestjs/common/interfaces';
 import { isEmpty, isFunction } from '@nestjs/common/utils/shared.utils';
+import { ExternalExceptionFilterContext } from '../exceptions/external-exception-filter-context';
 import { FORBIDDEN_MESSAGE } from '../guards/constants';
 import { GuardsConsumer } from '../guards/guards-consumer';
 import { GuardsContextCreator } from '../guards/guards-context-creator';
+import { STATIC_CONTEXT } from '../injector/constants';
 import { NestContainer } from '../injector/container';
 import { Module } from '../injector/module';
 import { ModulesContainer } from '../injector/modules-container';
 import { InterceptorsConsumer } from '../interceptors/interceptors-consumer';
 import { InterceptorsContextCreator } from '../interceptors/interceptors-context-creator';
 import { PipesConsumer } from '../pipes/pipes-consumer';
 import { PipesContextCreator } from '../pipes/pipes-context-creator';
-import { ExternalExceptionFilterContext } from '../exceptions/external-exception-filter-context';
-import { STATIC_CONTEXT } from '../injector/constants';
 import { ContextUtils, ParamProperties } from './context-utils';
 import { ExternalErrorProxy } from './external-proxy';
 import { HandlerMetadataStorage } from './handler-metadata-storage';
@@ -80,7 +84,10 @@ export class ExternalContextCreator {
     );
   }
 
-  public create<T extends ParamsMetadata = ParamsMetadata>(
+  public create<
+    TParamsMetadata extends ParamsMetadata = ParamsMetadata,
+    TContext extends ContextType = ContextType
+  >(
     instance: Controller,
     callback: (...args: any[]) => any,
     methodName: string,
@@ -93,14 +100,12 @@ export class ExternalContextCreator {
       guards: true,
       filters: true,
     },
+    contextType: TContext = 'http' as TContext,
   ) {
     const module = this.getContextModuleName(instance.constructor);
-    const { argsLength, paramtypes, getParamsMetadata } = this.getMetadata<T>(
-      instance,
-      methodName,
-      metadataKey,
-      paramsFactory,
-    );
+    const { argsLength, paramtypes, getParamsMetadata } = this.getMetadata<
+      TParamsMetadata
+    >(instance, methodName, metadataKey, paramsFactory);
     const pipes = this.pipesContextCreator.create(
       instance,
       callback,
@@ -138,7 +143,7 @@ export class ExternalContextCreator {
       : [];
 
     const fnCanActivate = options.guards
-      ? this.createGuardsFn(guards, instance, callback)
+      ? this.createGuardsFn(guards, instance, callback, contextType)
       : null;
     const fnApplyPipes = this.createPipesFn(pipes, paramsOptions);
     const handler = (initialArgs: any[], ...args: any[]) => async () => {
@@ -159,6 +164,7 @@ export class ExternalContextCreator {
         instance,
         callback,
         handler(initialArgs, ...args),
+        contextType,
       );
       return this.transformToResult(result);
     };
@@ -316,17 +322,19 @@ export class ExternalContextCreator {
     return resultOrDeffered;
   }
 
-  public createGuardsFn(
+  public createGuardsFn<TContext extends ContextType = ContextType>(
     guards: any[],
     instance: Controller,
     callback: (...args: any[]) => any,
+    contextType?: TContext,
   ): Function | null {
     const canActivateFn = async (args: any[]) => {
-      const canActivate = await this.guardsConsumer.tryActivate(
+      const canActivate = await this.guardsConsumer.tryActivate<TContext>(
         guards,
         args,
         instance,
         callback,
+        contextType,
       );
       if (!canActivate) {
         throw new ForbiddenException(FORBIDDEN_MESSAGE);

packages/core/injector/injector.ts

@@ -358,10 +358,8 @@ export class Injector {
       wrapper,
       contextId,
       inquirer,
+      keyOrIndex,
     );
-    isString(keyOrIndex)
-      ? wrapper.addPropertiesMetadata(keyOrIndex, instanceWrapper)
-      : wrapper.addCtorMetadata(keyOrIndex, instanceWrapper);
 
     return this.resolveComponentHost(
       module,
@@ -419,6 +417,7 @@ export class Injector {
     wrapper: InstanceWrapper<T>,
     contextId = STATIC_CONTEXT,
     inquirer?: InstanceWrapper,
+    keyOrIndex?: string | number,
   ): Promise<InstanceWrapper<T>> {
     const { name } = dependencyContext;
     if (wrapper && wrapper.name === name) {
@@ -428,23 +427,28 @@ export class Injector {
         module,
       );
     }
-    const scanInExports = () =>
-      this.lookupComponentInExports(
-        dependencyContext,
-        module,
-        wrapper,
-        contextId,
-        inquirer,
-      );
-    return providers.has(name) ? providers.get(name) : scanInExports();
+    if (providers.has(name)) {
+      const instanceWrapper = providers.get(name);
+      this.addDependencyMetadata(keyOrIndex, wrapper, instanceWrapper);
+      return instanceWrapper;
+    }
+    return this.lookupComponentInParentModules(
+      dependencyContext,
+      module,
+      wrapper,
+      contextId,
+      inquirer,
+      keyOrIndex,
+    );
   }
 
-  public async lookupComponentInExports<T = any>(
+  public async lookupComponentInParentModules<T = any>(
     dependencyContext: InjectorDependencyContext,
     module: Module,
     wrapper: InstanceWrapper<T>,
     contextId = STATIC_CONTEXT,
     inquirer?: InstanceWrapper,
+    keyOrIndex?: string | number,
   ) {
     const instanceWrapper = await this.lookupComponentInImports(
       module,
@@ -453,6 +457,7 @@ export class Injector {
       [],
       contextId,
       inquirer,
+      keyOrIndex,
     );
     if (isNil(instanceWrapper)) {
       throw new UnknownDependenciesException(
@@ -471,6 +476,7 @@ export class Injector {
     moduleRegistry: any[] = [],
     contextId = STATIC_CONTEXT,
     inquirer?: InstanceWrapper,
+    keyOrIndex?: string | number,
     isTraversing?: boolean,
   ): Promise<any> {
     let instanceWrapperRef: InstanceWrapper = null;
@@ -499,14 +505,17 @@ export class Injector {
           moduleRegistry,
           contextId,
           inquirer,
+          keyOrIndex,
           true,
         );
         if (instanceRef) {
+          this.addDependencyMetadata(keyOrIndex, wrapper, instanceRef);
           return instanceRef;
         }
         continue;
       }
       instanceWrapperRef = providers.get(name);
+      this.addDependencyMetadata(keyOrIndex, wrapper, instanceWrapperRef);
 
       const inquirerId = this.getInquirerId(inquirer);
       const instanceHost = instanceWrapperRef.getInstanceByContextId(
@@ -752,4 +761,14 @@ export class Injector {
   ) {
     return param === INQUIRER && parentInquirer;
   }
+
+  private addDependencyMetadata(
+    keyOrIndex: number | string,
+    hostWrapper: InstanceWrapper,
+    instanceWrapper: InstanceWrapper,
+  ) {
+    isString(keyOrIndex)
+      ? hostWrapper.addPropertiesMetadata(keyOrIndex, instanceWrapper)
+      : hostWrapper.addCtorMetadata(keyOrIndex, instanceWrapper);
+  }
 }

packages/core/injector/module-token-factory.ts

@@ -13,11 +13,11 @@ export class ModuleTokenFactory {
     const moduleScope = this.reflectScope(metatype);
     const isSingleScoped = moduleScope === true;
     const opaqueToken = {
-      module: metatype,
+      module: this.getModuleName(metatype),
       dynamic: this.getDynamicMetadataToken(dynamicModuleMetadata),
       scope: isSingleScoped ? this.getScopeStack(scope) : moduleScope,
     };
-    return hash(opaqueToken);
+    return hash(opaqueToken, { ignoreUnknown: true });
   }
 
   public getDynamicMetadataToken(
@@ -45,6 +45,10 @@ export class ModuleTokenFactory {
     return stack.map(module => module.name);
   }
 
+  public getModuleName(metatype: Type<any>): string {
+    return metatype.name;
+  }
+
   private reflectScope(metatype: Type<any>) {
     const scope = Reflect.getMetadata(SHARED_MODULE_METADATA, metatype);
     return scope ? scope : 'global';

packages/core/test/injector/injector.spec.ts

@@ -266,12 +266,12 @@ describe('Injector', () => {
   describe('lookupComponent', () => {
     let lookupComponentInImports: sinon.SinonStub;
     const metatype = { name: 'test', metatype: { name: 'test' } };
-    const wrapper: any = {
+    const wrapper = new InstanceWrapper({
       name: 'Test',
-      metatype,
+      metatype: metatype as any,
       instance: null,
       isResolved: false,
-    };
+    });
     beforeEach(() => {
       lookupComponentInImports = sinon.stub();
       (injector as any).lookupComponentInImports = lookupComponentInImports;
@@ -303,10 +303,9 @@ describe('Injector', () => {
         collection as any,
         null,
         { name, index: 0, dependencies: [] },
-        {
-          ...wrapper,
+        Object.assign(wrapper, {
           name,
-        },
+        }),
       );
       expect(result).to.eventually.be.rejected;
     });
@@ -374,7 +373,7 @@ describe('Injector', () => {
       const result = await injector.lookupComponentInImports(
         module as any,
         null,
-        null,
+        new InstanceWrapper(),
       );
       expect(result).to.be.eq(null);
     });
@@ -443,7 +442,7 @@ describe('Injector', () => {
       await injector.lookupComponentInImports(
         module as any,
         metatype as any,
-        null,
+        new InstanceWrapper(),
       );
       expect(loadProvider.called).to.be.true;
     });