Merge pull request #10479 from micalevisk/feat/verbose-wrong-controller-error

feat(core): display class's name on request mapping exceptions

Author: kamilmysliwiec

.gitignore

@@ -8,6 +8,13 @@ node_modules/
 /.devcontainer
 *.code-workspace
 
+# Vim
+[._]*.s[a-v][a-z]
+[._]*.sw[a-p]
+[._]s[a-rt-v][a-z]
+[._]ss[a-gi-z]
+[._]sw[a-p]
+
 # bundle
 packages/**/*.d.ts
 packages/**/*.js

packages/core/errors/exceptions/unknown-request-mapping.exception.ts

@@ -1,8 +1,9 @@
+import type { Type } from '@nestjs/common';
 import { RuntimeException } from './runtime.exception';
 import { UNKNOWN_REQUEST_MAPPING } from '../messages';
 
 export class UnknownRequestMappingException extends RuntimeException {
-  constructor() {
-    super(UNKNOWN_REQUEST_MAPPING);
+  constructor(metatype: Type) {
+    super(UNKNOWN_REQUEST_MAPPING(metatype));
   }
 }

packages/core/errors/messages.ts

@@ -172,8 +172,14 @@ export const INVALID_CLASS_SCOPE_MESSAGE = (
     name || 'This class'
   } is marked as a scoped provider. Request and transient-scoped providers can't be used in combination with "get()" method. Please, use "resolve()" instead.`;
 
+export const UNKNOWN_REQUEST_MAPPING = (metatype: Type) => {
+  const className = metatype.name;
+  return className
+    ? `An invalid controller has been detected. "${className}" does not have the @Controller() decorator but it is being listed in the "controllers" array of some module.`
+    : `An invalid controller has been detected. Perhaps, one of your controllers is missing the @Controller() decorator.`;
+};
+
 export const INVALID_MIDDLEWARE_CONFIGURATION = `An invalid middleware configuration has been passed inside the module 'configure()' method.`;
-export const UNKNOWN_REQUEST_MAPPING = `An invalid controller has been detected. Perhaps, one of your controllers is missing @Controller() decorator.`;
 export const UNHANDLED_RUNTIME_EXCEPTION = `Unhandled Runtime Exception.`;
 export const INVALID_EXCEPTION_FILTER = `Invalid exception filters (@UseFilters()).`;
 export const MICROSERVICES_PACKAGE_NOT_FOUND_EXCEPTION = `Unable to load @nestjs/microservices package. (Please make sure that it's already installed.)`;

packages/core/router/router-explorer.ts

@@ -116,7 +116,7 @@ export class RouterExplorer {
     const path = Reflect.getMetadata(PATH_METADATA, metatype);
 
     if (isUndefined(path)) {
-      throw new UnknownRequestMappingException();
+      throw new UnknownRequestMappingException(metatype);
     }
     if (Array.isArray(path)) {
       return path.map(p => addLeadingSlash(p));

packages/core/test/exceptions/external-exception-filter-context.spec.ts

@@ -1,5 +1,6 @@
 import { expect } from 'chai';
 import * as sinon from 'sinon';
+import { ExceptionFilter } from '../../../common';
 import { Catch } from '../../../common/decorators/core/catch.decorator';
 import { UseFilters } from '../../../common/decorators/core/exception-filters.decorator';
 import { ApplicationConfig } from '../../application-config';
@@ -13,7 +14,10 @@ describe('ExternalExceptionFilterContext', () => {
 
   class CustomException {}
   @Catch(CustomException)
-  class ExceptionFilter {
+  class ExceptionFilter implements ExceptionFilter {
+    public catch(exc, res) {}
+  }
+  class ClassWithNoMetadata implements ExceptionFilter {
     public catch(exc, res) {}
   }
 
@@ -59,6 +63,11 @@ describe('ExternalExceptionFilterContext', () => {
         exceptionFilter.reflectCatchExceptions(new ExceptionFilter()),
       ).to.be.eql([CustomException]);
     });
+    it('should return an empty array when metadata was found', () => {
+      expect(
+        exceptionFilter.reflectCatchExceptions(new ClassWithNoMetadata()),
+      ).to.be.eql([]);
+    });
   });
   describe('createConcreteContext', () => {
     class InvalidFilter {}

packages/core/test/exceptions/external-exceptions-handler.spec.ts

@@ -2,22 +2,23 @@ import { expect } from 'chai';
 import { of } from 'rxjs';
 import * as sinon from 'sinon';
 import { ExternalExceptionsHandler } from '../../exceptions/external-exceptions-handler';
+import { ExternalExceptionFilter } from '../../exceptions/external-exception-filter';
 
 describe('ExternalExceptionsHandler', () => {
   let handler: ExternalExceptionsHandler;
 
   beforeEach(() => {
     handler = new ExternalExceptionsHandler();
+
+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // @ts-ignore The 'logger' property is private but we want to avoid showing useless error logs
+    ExternalExceptionFilter.logger.error = () => {};
   });
 
   describe('next', () => {
-    it('should method returns expected stream with message when exception is unknown', async () => {
+    it('should method returns expected stream with message when exception is unknown', () => {
       const error = new Error();
-      try {
-        await handler.next(error, null);
-      } catch (err) {
-        expect(err).to.be.eql(error);
-      }
+      expect(() => handler.next(error, null)).to.throw(error);
     });
     describe('when "invokeCustomFilters" returns value', () => {
       const observable$ = of(true);
@@ -49,6 +50,7 @@ describe('ExternalExceptionsHandler', () => {
     describe('when filters array is not empty', () => {
       let filters, funcSpy;
       class TestException {}
+      class AnotherTestException {}
 
       beforeEach(() => {
         funcSpy = sinon.spy();
@@ -73,6 +75,12 @@ describe('ExternalExceptionsHandler', () => {
         });
       });
       describe('when filter does not exists in filters array', () => {
+        beforeEach(() => {
+          filters = [
+            { exceptionMetatypes: [AnotherTestException], func: funcSpy },
+          ];
+          (handler as any).filters = filters;
+        });
         it('should not call funcSpy', () => {
           handler.invokeCustomFilters(new TestException(), null);
           expect(funcSpy.notCalled).to.be.true;

packages/core/test/router/router-explorer.spec.ts

@@ -19,6 +19,7 @@ import { RoutePathFactory } from '../../router/route-path-factory';
 import { RouterExceptionFilters } from '../../router/router-exception-filters';
 import { RouterExplorer } from '../../router/router-explorer';
 import { NoopHttpAdapter } from '../utils/noop-adapter.spec';
+import { UnknownRequestMappingException } from '../../errors/exceptions/unknown-request-mapping.exception';
 
 describe('RouterExplorer', () => {
   @Controller('global')
@@ -51,6 +52,8 @@ describe('RouterExplorer', () => {
     public getTestUsingArray() {}
   }
 
+  class ClassWithMissingControllerDecorator {}
+
   let routerBuilder: RouterExplorer;
   let injector: Injector;
   let exceptionsFilter: RouterExceptionFilters;
@@ -315,6 +318,15 @@ describe('RouterExplorer', () => {
         '/global-alias',
       ]);
     });
+
+    it("should throw UnknownRequestMappingException when missing the `@Controller()` decorator in the class, displaying class's name", () => {
+      expect(() =>
+        routerBuilder.extractRouterPath(ClassWithMissingControllerDecorator),
+      ).to.throw(
+        UnknownRequestMappingException,
+        /ClassWithMissingControllerDecorator/,
+      );
+    });
   });
 
   describe('createRequestScopedHandler', () => {