Merge pull request #726 from jshayes/add-warning-logs

feat(@nestjs/scheduler): Add a warning when using non static providers
nestjs · Nov 16, 2021 · 85f3c0e · 85f3c0e
2 parents e15d1cc + 6b33111
commit 85f3c0e
Show file tree

Hide file tree

Showing 8 changed files with 253 additions and 16 deletions.
diff --git a/lib/schedule.explorer.ts b/lib/schedule.explorer.ts
@@ -26,19 +26,20 @@ export class ScheduleExplorer implements OnModuleInit {
       ...this.discoveryService.getControllers(),
       ...this.discoveryService.getProviders(),
     ];
-    instanceWrappers
-      .filter((wrapper) => wrapper.isDependencyTreeStatic())
-      .forEach((wrapper: InstanceWrapper) => {
-        const { instance } = wrapper;
-        if (!instance || !Object.getPrototypeOf(instance)) {
-          return;
-        }
-        this.metadataScanner.scanFromPrototype(
-          instance,
-          Object.getPrototypeOf(instance),
-          (key: string) => this.lookupSchedulers(instance, key),
-        );
-      });
+    instanceWrappers.forEach((wrapper: InstanceWrapper) => {
+      const { instance } = wrapper;
+      if (!instance || !Object.getPrototypeOf(instance)) {
+        return;
+      }
+      this.metadataScanner.scanFromPrototype(
+        instance,
+        Object.getPrototypeOf(instance),
+        (key: string) =>
+          wrapper.isDependencyTreeStatic()
+            ? this.lookupSchedulers(instance, key)
+            : this.warnForNonStaticProviders(wrapper, instance, key),
+      );
+    });
   }
 
   lookupSchedulers(instance: Record<string, Function>, key: string) {
@@ -87,6 +88,36 @@ export class ScheduleExplorer implements OnModuleInit {
     }
   }
 
+  warnForNonStaticProviders(
+    wrapper: InstanceWrapper<any>,
+    instance: Record<string, Function>,
+    key: string,
+  ) {
+    const methodRef = instance[key];
+    const metadata = this.metadataAccessor.getSchedulerType(methodRef);
+
+    switch (metadata) {
+      case SchedulerType.CRON: {
+        this.logger.warn(
+          `Cannot register cron job "${wrapper.name}@${key}" because it is defined in a non static provider.`,
+        );
+        break;
+      }
+      case SchedulerType.TIMEOUT: {
+        this.logger.warn(
+          `Cannot register timeout "${wrapper.name}@${key}" because it is defined in a non static provider.`,
+        );
+        break;
+      }
+      case SchedulerType.INTERVAL: {
+        this.logger.warn(
+          `Cannot register interval "${wrapper.name}@${key}" because it is defined in a non static provider.`,
+        );
+        break;
+      }
+    }
+  }
+
   private wrapFunctionInTryCatchBlocks(methodRef: Function, instance: object) {
     return async (...args: unknown[]) => {
       try {

diff --git a/tests/e2e/cron-jobs.spec.ts b/tests/e2e/cron-jobs.spec.ts
@@ -1,9 +1,10 @@
-import { INestApplication } from '@nestjs/common';
+import { INestApplication, Logger } from '@nestjs/common';
 import { Test } from '@nestjs/testing';
 import sinon from 'sinon';
 import { SchedulerRegistry } from '../../lib/scheduler.registry';
 import { AppModule } from '../src/app.module';
 import { CronService } from '../src/cron.service';
+import { RequestScopedCronService } from '../src/request-scoped-cron.service';
 import { nullPrototypeObjectProvider } from '../src/null-prototype-object.provider';
 
 const deleteAllRegisteredJobsExceptOne = (
@@ -186,6 +187,57 @@ describe('Cron', () => {
     expect(service.doesExists('dynamic')).toEqual(false);
   });
 
+  it(`should not log a warning when the provider is not request scoped`, async () => {
+    const logger = {
+      log: jest.fn(),
+      error: jest.fn(),
+      warn: jest.fn(),
+    };
+    Logger.overrideLogger(logger);
+    jest.spyOn(logger, 'warn');
+
+    await app.init();
+
+    expect(logger.warn).not.toHaveBeenCalled();
+  });
+
+  afterEach(async () => {
+    await app.close();
+  });
+});
+
+describe('Cron - Request Scoped Provider', () => {
+  let app: INestApplication;
+  let clock: sinon.SinonFakeTimers;
+
+  beforeEach(async () => {
+    const module = await Test.createTestingModule({
+      imports: [AppModule.registerRequestScopedCron()],
+    }).compile();
+
+    app = module.createNestApplication();
+    clock = sinon.useFakeTimers({ now: 1577836800000 }); // 2020-01-01T00:00:00.000Z
+  });
+
+  it(`should log a warning when trying to register a cron in a request scoped provider`, async () => {
+    const logger = {
+      log: jest.fn(),
+      error: jest.fn(),
+      warn: jest.fn(),
+    };
+    Logger.overrideLogger(logger);
+    jest.spyOn(logger, 'warn');
+
+    await app.init();
+    const registry = app.get(SchedulerRegistry);
+
+    expect(registry.getCronJobs()).toEqual(new Map());
+    expect(logger.warn).toHaveBeenCalledWith(
+      'Cannot register cron job "RequestScopedCronService@handleCron" because it is defined in a non static provider.',
+      'Scheduler',
+    );
+  });
+
   afterEach(async () => {
     await app.close();
   });

diff --git a/tests/e2e/interval.spec.ts b/tests/e2e/interval.spec.ts
@@ -1,9 +1,10 @@
-import { INestApplication } from '@nestjs/common';
+import { INestApplication, Logger } from '@nestjs/common';
 import { Test } from '@nestjs/testing';
 import { SchedulerRegistry } from '../../lib/scheduler.registry';
 import { AppModule } from '../src/app.module';
 import { IntervalService } from '../src/interval.service';
 import { nullPrototypeObjectProvider } from '../src/null-prototype-object.provider';
+import { RequestScopedIntervalService } from '../src/request-scoped-interval.service';
 
 describe('Interval', () => {
   let app: INestApplication;
@@ -112,6 +113,56 @@ describe('Interval', () => {
     expect(service.doesExists('dynamic')).toEqual(false);
   });
 
+  it(`should not log a warning when the provider is not request scoped`, async () => {
+    const logger = {
+      log: jest.fn(),
+      error: jest.fn(),
+      warn: jest.fn(),
+    };
+    Logger.overrideLogger(logger);
+    jest.spyOn(logger, 'warn');
+
+    await app.init();
+
+    expect(logger.warn).not.toHaveBeenCalledWith();
+  });
+
+  afterEach(async () => {
+    await app.close();
+  });
+});
+
+describe('Interval - Request Scoped', () => {
+  let app: INestApplication;
+
+  beforeEach(async () => {
+    const module = await Test.createTestingModule({
+      imports: [AppModule.registerRequestScopedInterval()],
+    }).compile();
+
+    app = module.createNestApplication();
+    jest.useFakeTimers();
+  });
+
+  it(`should log a warning when trying to register an interval in a request scoped provider`, async () => {
+    const logger = {
+      log: jest.fn(),
+      error: jest.fn(),
+      warn: jest.fn(),
+    };
+    Logger.overrideLogger(logger);
+    jest.spyOn(logger, 'warn');
+
+    await app.init();
+    const registry = app.get(SchedulerRegistry);
+
+    expect(registry.getIntervals()).toEqual([]);
+    expect(logger.warn).toHaveBeenCalledWith(
+      'Cannot register interval "RequestScopedIntervalService@handleInterval" because it is defined in a non static provider.',
+      'Scheduler',
+    );
+  });
+
   afterEach(async () => {
     await app.close();
   });

diff --git a/tests/e2e/timeout.spec.ts b/tests/e2e/timeout.spec.ts
@@ -1,8 +1,9 @@
-import { INestApplication } from '@nestjs/common';
+import { INestApplication, Logger } from '@nestjs/common';
 import { Test } from '@nestjs/testing';
 import { SchedulerRegistry } from '../../lib/scheduler.registry';
 import { AppModule } from '../src/app.module';
 import { nullPrototypeObjectProvider } from '../src/null-prototype-object.provider';
+import { RequestScopedTimeoutService } from '../src/request-scoped-timeout.service';
 import { TimeoutService } from '../src/timeout.service';
 
 describe('Timeout', () => {
@@ -112,6 +113,56 @@ describe('Timeout', () => {
     expect(service.doesExists('dynamic')).toEqual(false);
   });
 
+  it(`should not log a warning when the provider is not request scoped`, async () => {
+    const logger = {
+      log: jest.fn(),
+      error: jest.fn(),
+      warn: jest.fn(),
+    };
+    Logger.overrideLogger(logger);
+    jest.spyOn(logger, 'warn');
+
+    await app.init();
+
+    expect(logger.warn).not.toHaveBeenCalledWith();
+  });
+
+  afterEach(async () => {
+    await app.close();
+  });
+});
+
+describe('Timeout - Request Scoped', () => {
+  let app: INestApplication;
+
+  beforeEach(async () => {
+    const module = await Test.createTestingModule({
+      imports: [AppModule.registerRequestScopedTimeout()],
+    }).compile();
+
+    app = module.createNestApplication();
+    jest.useFakeTimers();
+  });
+
+  it(`should log a warning when trying to register an timeout in a request scoped provider`, async () => {
+    const logger = {
+      log: jest.fn(),
+      error: jest.fn(),
+      warn: jest.fn(),
+    };
+    Logger.overrideLogger(logger);
+    jest.spyOn(logger, 'warn');
+
+    await app.init();
+    const registry = app.get(SchedulerRegistry);
+
+    expect(registry.getTimeouts()).toEqual([]);
+    expect(logger.warn).toHaveBeenCalledWith(
+      'Cannot register timeout "RequestScopedTimeoutService@handleTimeout" because it is defined in a non static provider.',
+      'Scheduler',
+    );
+  });
+
   afterEach(async () => {
     await app.close();
   });

diff --git a/tests/src/app.module.ts b/tests/src/app.module.ts
@@ -2,6 +2,9 @@ import { DynamicModule, Module } from '@nestjs/common';
 import { ScheduleModule } from '../../lib/schedule.module';
 import { CronService } from './cron.service';
 import { IntervalService } from './interval.service';
+import { RequestScopedCronService } from './request-scoped-cron.service';
+import { RequestScopedIntervalService } from './request-scoped-interval.service';
+import { RequestScopedTimeoutService } from './request-scoped-timeout.service';
 import { TimeoutService } from './timeout.service';
 
 @Module({})
@@ -14,6 +17,14 @@ export class AppModule {
     };
   }
 
+  static registerRequestScopedTimeout(): DynamicModule {
+    return {
+      module: AppModule,
+      imports: [ScheduleModule.forRoot()],
+      providers: [RequestScopedTimeoutService],
+    };
+  }
+
   static registerInterval(): DynamicModule {
     return {
       module: AppModule,
@@ -22,11 +33,27 @@ export class AppModule {
     };
   }
 
+  static registerRequestScopedInterval(): DynamicModule {
+    return {
+      module: AppModule,
+      imports: [ScheduleModule.forRoot()],
+      providers: [RequestScopedIntervalService],
+    };
+  }
+
   static registerCron(): DynamicModule {
     return {
       module: AppModule,
       imports: [ScheduleModule.forRoot()],
       providers: [CronService],
     };
   }
+
+  static registerRequestScopedCron(): DynamicModule {
+    return {
+      module: AppModule,
+      imports: [ScheduleModule.forRoot()],
+      providers: [RequestScopedCronService],
+    };
+  }
 }
diff --git a/tests/src/request-scoped-cron.service.ts b/tests/src/request-scoped-cron.service.ts
@@ -0,0 +1,9 @@
+import { Injectable, Scope } from '@nestjs/common';
+import { Cron } from '../../lib/decorators';
+import { CronExpression } from '../../lib/enums';
+
+@Injectable({ scope: Scope.REQUEST })
+export class RequestScopedCronService {
+  @Cron(CronExpression.EVERY_MINUTE)
+  handleCron() {}
+}
diff --git a/tests/src/request-scoped-interval.service.ts b/tests/src/request-scoped-interval.service.ts
@@ -0,0 +1,8 @@
+import { Injectable, Scope } from '@nestjs/common';
+import { Interval } from '../../lib/decorators';
+
+@Injectable({ scope: Scope.REQUEST })
+export class RequestScopedIntervalService {
+  @Interval('test', 2500)
+  handleInterval() {}
+}
diff --git a/tests/src/request-scoped-timeout.service.ts b/tests/src/request-scoped-timeout.service.ts
@@ -0,0 +1,8 @@
+import { Injectable, Scope } from '@nestjs/common';
+import { Timeout } from '../../lib/decorators';
+
+@Injectable({ scope: Scope.REQUEST })
+export class RequestScopedTimeoutService {
+  @Timeout('test', 2500)
+  handleTimeout() {}
+}