fix(core): Correctly handle error responses for REST controllers

Fixes #187

packages/core/e2e/fixtures/test-plugins.ts

@@ -1,13 +1,15 @@
-import { Injectable, OnApplicationBootstrap, OnModuleDestroy, OnModuleInit } from '@nestjs/common';
+import { Controller, Get, Injectable } from '@nestjs/common';
 import { Query, Resolver } from '@nestjs/graphql';
+import { Permission } from '@vendure/common/lib/generated-shop-types';
 import { LanguageCode } from '@vendure/common/lib/generated-types';
 import {
+    Allow,
     ConfigService,
+    InternalServerError,
     OnVendureBootstrap,
     OnVendureClose,
     OnVendureWorkerBootstrap,
     OnVendureWorkerClose,
-    VendureConfig,
     VendurePlugin,
 } from '@vendure/core';
 import { ConfigModule } from '@vendure/core/dist/config/config.module';
@@ -194,3 +196,27 @@ export class TestPluginWithConfigAndBootstrap implements OnVendureBootstrap, OnV
         TestPluginWithConfigAndBootstrap.boostrapWasCalled.mockClear();
     }
 }
+
+@Controller('test')
+export class TestController {
+    @Get('public')
+    publicRoute() {
+        return 'success';
+    }
+
+    @Allow(Permission.Authenticated)
+    @Get('restricted')
+    restrictedRoute() {
+        return 'success';
+    }
+
+    @Get('bad')
+    badRoute() {
+        throw new InternalServerError('uh oh!');
+    }
+}
+
+@VendurePlugin({
+    controllers: [TestController],
+})
+export class TestRestPlugin {}

packages/core/e2e/plugin.e2e-spec.ts

@@ -13,6 +13,7 @@ import {
     TestPluginWithAllLifecycleHooks,
     TestPluginWithConfigAndBootstrap,
     TestPluginWithProvider,
+    TestRestPlugin,
 } from './fixtures/test-plugins';
 
 describe('Plugins', () => {
@@ -37,6 +38,7 @@ describe('Plugins', () => {
             TestAPIExtensionPlugin,
             TestPluginWithProvider,
             TestLazyExtensionPlugin,
+            TestRestPlugin,
         ],
     });
 
@@ -126,6 +128,39 @@ describe('Plugins', () => {
         expect(result.names).toEqual(['seon', 'linda', 'hong']);
     });
 
+    describe('REST plugins', () => {
+        const restControllerUrl = `http://localhost:${testConfig.port}/test`;
+
+        it('public route', async () => {
+            const response = await shopClient.fetch(restControllerUrl + '/public');
+            const body = await response.text();
+
+            expect(body).toBe('success');
+        });
+
+        it('permission-restricted route forbidden', async () => {
+            const response = await shopClient.fetch(restControllerUrl + '/restricted');
+            expect(response.status).toBe(403);
+            const result = await response.json();
+            expect(result.message).toContain('FORBIDDEN');
+        });
+
+        it('permission-restricted route forbidden allowed', async () => {
+            await shopClient.asUserWithCredentials('hayden.zieme12@hotmail.com', 'test');
+            const response = await shopClient.fetch(restControllerUrl + '/restricted');
+            expect(response.status).toBe(200);
+            const result = await response.text();
+            expect(result).toBe('success');
+        });
+
+        it('handling I18nErrors', async () => {
+            const response = await shopClient.fetch(restControllerUrl + '/bad');
+            expect(response.status).toBe(500);
+            const result = await response.json();
+            expect(result.message).toContain('uh oh!');
+        });
+    });
+
     describe('on app close', () => {
         beforeAll(async () => {
             await server.destroy();

packages/core/src/api/middleware/exception-logger.filter.ts

@@ -9,44 +9,73 @@ import { parseContext } from '../common/parse-context';
  */
 export class ExceptionLoggerFilter implements ExceptionFilter {
     catch(exception: Error | HttpException | I18nError, host: ArgumentsHost) {
+        const { req, res, info, isGraphQL } = parseContext(host);
+        let message = '';
+        let statusCode = 500;
         if (exception instanceof I18nError) {
-            const { code, message, logLevel } = exception;
-            const logMessage = `${code || 'Error'}: ${message}`;
+            const { code, message: msg, logLevel } = exception;
+            message = `${code || 'Error'}: ${msg}`;
+            statusCode = this.errorCodeToStatusCode(code);
+
             switch (logLevel) {
                 case LogLevel.Error:
-                    Logger.error(logMessage, undefined, exception.stack);
+                    Logger.error(message, undefined, exception.stack);
                     break;
                 case LogLevel.Warn:
-                    Logger.warn(logMessage);
+                    Logger.warn(message);
                     break;
                 case LogLevel.Info:
-                    Logger.info(logMessage);
+                    Logger.info(message);
                     break;
                 case LogLevel.Debug:
-                    Logger.debug(logMessage);
+                    Logger.debug(message);
                     break;
                 case LogLevel.Verbose:
-                    Logger.verbose(logMessage);
+                    Logger.verbose(message);
                     break;
             }
         } else if (exception instanceof HttpException) {
             // Handle other Nestjs errors
-            const { req, res, info } = parseContext(host);
-            const status = exception.getStatus();
-            let message = exception.message;
+            statusCode = exception.getStatus();
+            message = exception.message;
             let stack = exception.stack;
-            if (status === 404) {
+            if (statusCode === 404) {
                 message = exception.message.message;
                 stack = undefined;
             }
             Logger.error(message, undefined, stack);
+        }
 
-            res.status(status).json({
-                statusCode: status,
+        if (!isGraphQL) {
+            // In the GraphQL context, we can let the error pass
+            // through to the next layer, where Apollo Server will
+            // return a response for us. But when in the REST context,
+            // we must explicitly send the response, otherwise the server
+            // will hang.
+            res.status(statusCode).json({
+                statusCode,
                 message,
                 timestamp: new Date().toISOString(),
                 path: req.url,
             });
         }
     }
+
+    /**
+     * For a given I18nError.code, returns a corresponding HTTP
+     * status code.
+     */
+    private errorCodeToStatusCode(errorCode: string | undefined): number {
+        switch (errorCode) {
+            case 'FORBIDDEN':
+                return 403;
+            case 'UNAUTHORIZED':
+                return 401;
+            case 'USER_INPUT_ERROR':
+            case 'ILLEGAL_OPERATION':
+                return 400;
+            default:
+                return 500;
+        }
+    }
 }