Merge pull request #2 from Maklestiguan/improvement_suggestions

temarusanov · web-flow · commit 03e8a020ab18 · 2021-12-28T17:52:12.000+07:00
feat: new error log with id, now with tests!

BREAKING CHANGE: logger.error() now logs object except of string message
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -12,7 +12,6 @@ module.exports = {
     'plugin:@typescript-eslint/eslint-recommended',
     'plugin:@typescript-eslint/recommended',
     'prettier',
-    'prettier/@typescript-eslint',
   ],
   root: true,
   env: {
@@ -111,6 +110,7 @@ module.exports = {
   },
   "ignorePatterns": [
     ".eslintrc.js",
+    "*.mocks.ts",
     '[0-9]*.ts' // not to lint migrations files, example: 152325322-create-db.ts
   ],
 };
diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml
@@ -12,4 +12,5 @@ jobs:
       - uses: actions/setup-node@v2
         with:
           node-version: 14
-      - run: npm ci
+      - run: npm ci
+      - run: npm test
diff --git a/lib/filters/response.exception-filter.spec.ts b/lib/filters/response.exception-filter.spec.ts
@@ -0,0 +1,101 @@
+import { Test, TestingModule } from '@nestjs/testing'
+import { BadRequestException, HttpStatus } from '@nestjs/common'
+
+import { ResponseFilter } from './response.exception-filter'
+import { mockArgumentsHost, mockLogger } from '../test/__mocks__/common.mocks'
+
+describe('Response exception filter suite', () => {
+    let exceptionFilter: ResponseFilter
+
+    describe('when stacktrace enabled', () => {
+        beforeEach(async () => {
+            jest.clearAllMocks()
+            const module: TestingModule = await Test.createTestingModule({
+                providers: [
+                    {
+                        provide: ResponseFilter,
+                        useValue: new ResponseFilter(),
+                    },
+                ],
+            })
+                .setLogger(mockLogger)
+                .compile()
+
+            exceptionFilter = module.get<ResponseFilter>(ResponseFilter)
+        })
+
+        it('should be defined', () => {
+            expect(exceptionFilter).toBeDefined()
+        })
+
+        it('should have stack in error object', () => {
+            const response = exceptionFilter.catch(
+                new Error('random error'),
+                mockArgumentsHost,
+            )
+            expect(response).toBeDefined()
+            expect(response.data).toBeUndefined()
+            expect(response.error).toBeDefined()
+            expect(response.error.stack).toBeDefined()
+        })
+    })
+
+    describe('when stacktrace disabled | stacktrace does not matter', () => {
+        beforeEach(async () => {
+            jest.clearAllMocks()
+            const module: TestingModule = await Test.createTestingModule({
+                providers: [
+                    {
+                        provide: ResponseFilter,
+                        useValue: new ResponseFilter({ stack: false }),
+                    },
+                ],
+            })
+                .setLogger(mockLogger)
+                .compile()
+
+            exceptionFilter = module.get<ResponseFilter>(ResponseFilter)
+        })
+
+        it('should be defined', () => {
+            expect(exceptionFilter).toBeDefined()
+        })
+
+        it('should not have stack in error object', () => {
+            const response = exceptionFilter.catch(
+                new Error('random error'),
+                mockArgumentsHost,
+            )
+            expect(response).toBeDefined()
+            expect(response.data).toBeUndefined()
+            expect(response.error).toBeDefined()
+            expect(response.error.stack).toBeUndefined()
+        })
+
+        it('should set status to INTERNAL_SERVER_ERROR if error is not instanceof HttpException', () => {
+            const response = exceptionFilter.catch(
+                new Error('I am not an HttpException'),
+                mockArgumentsHost,
+            )
+            expect(response).toBeDefined()
+            expect(response.data).toBeUndefined()
+            expect(response.error).toBeDefined()
+            expect(response.error.code).toEqual(
+                HttpStatus.INTERNAL_SERVER_ERROR,
+            )
+        })
+
+        it('should set given status when error is instanceof HttpException', () => {
+            const response = exceptionFilter.catch(
+                new BadRequestException(
+                    'I am an HttpException with status code 400',
+                ),
+                mockArgumentsHost,
+            )
+            expect(response).toBeDefined()
+            expect(response.data).toBeUndefined()
+            expect(response.error).toBeDefined()
+            expect(response.error.code).toEqual(HttpStatus.BAD_REQUEST)
+        })
+    })
+})
diff --git a/lib/filters/response.exception-filter.ts b/lib/filters/response.exception-filter.ts
@@ -1,64 +1,80 @@
-import { Catch, ArgumentsHost, Logger, HttpException } from '@nestjs/common'
+import {
+    Catch,
+    ArgumentsHost,
+    Logger,
+    HttpException,
+    HttpStatus,
+} from '@nestjs/common'
 import { v4 as uuid } from 'uuid'
 import { GqlExceptionFilter } from '@nestjs/graphql'
-import { ResponseFilterConfig } from '../interfaces/response-filter-config.interface'
-import { ResponsePayload } from '../interfaces/response-payload.interface'
+import { ResponseFilterConfig, ResponsePayload, ErrorInfo } from '../interfaces'
+
+const NO_DESCRIPTION = 'No description provided'
 
 @Catch()
 export class ResponseFilter implements GqlExceptionFilter {
     private _logger = new Logger(ResponseFilter.name)
 
-    constructor(private readonly _config: ResponseFilterConfig) {}
+    constructor(
+        private readonly _config: ResponseFilterConfig = { stack: true },
+    ) {}
 
-    catch(exception: Error, host: ArgumentsHost): ResponsePayload<unknown> {
+    catch(exception: Error, _host: ArgumentsHost): ResponsePayload<unknown> {
+        const id = uuid()
         const code =
-            exception instanceof HttpException ? exception.getStatus() : 500
+            exception instanceof HttpException
+                ? exception.getStatus()
+                : HttpStatus.INTERNAL_SERVER_ERROR
 
         const { message, description } = this._getErrorInfo(exception)
 
         const { stack } = this._config
 
-        const stackMessage =
-            stack === undefined || stack === true ? exception.stack : undefined
+        const stackMessage = stack ? exception.stack : undefined
 
-        this._logger.error(`${message} (${description})`, exception.stack)
+        this._logger.error(
+            {
+                id,
+                message,
+                description,
+            },
+            exception.stack,
+        )
 
         return {
             error: {
-                id: uuid(),
-                code: code,
+                id,
+                code,
                 message,
                 description,
                 stack: stackMessage,
             },
         }
     }
 
-    private _getErrorInfo(exception: Error): {
-        message: string
-        description: string
-    } {
+    /**
+     * @summary Retrieves `message` and `description` that were originally sent to NestJS' `HttpException` constructor
+     * @param exception caught in `ResponseFilter`
+     * @returns `message` and `description` fields wrapped in object
+     */
+    private _getErrorInfo(exception: Error): ErrorInfo {
         const errorResponse =
-            exception instanceof HttpException
-                ? (exception.getResponse() as any)
-                : null
+            exception instanceof HttpException ? exception.getResponse() : {}
 
-        if (typeof errorResponse === 'string' || errorResponse === null) {
+        if (typeof errorResponse === 'string') {
             return {
                 message: exception.name,
-                description: exception.message as string,
+                description: exception.message,
             }
         }
 
         return {
-            message:
-                'message' in errorResponse
-                    ? errorResponse['message']
-                    : exception.name,
-            description:
-                'description' in errorResponse
-                    ? errorResponse['description']
-                    : 'No description provided',
+            message: errorResponse.hasOwnProperty('message')
+                ? errorResponse['message']
+                : exception.name,
+            description: errorResponse.hasOwnProperty('description')
+                ? errorResponse['description']
+                : NO_DESCRIPTION,
         }
     }
 }
diff --git a/lib/interfaces/error-info.interface.ts b/lib/interfaces/error-info.interface.ts
@@ -0,0 +1,4 @@
+export interface ErrorInfo {
+    message: string
+    description: string
+}
diff --git a/lib/interfaces/index.ts b/lib/interfaces/index.ts
@@ -1,3 +1,4 @@
 export * from './response-error.interface'
 export * from './response-payload.interface'
 export * from './response-filter-config.interface'
+export * from './error-info.interface'
diff --git a/lib/interfaces/response-error.interface.ts b/lib/interfaces/response-error.interface.ts
@@ -2,6 +2,6 @@ export interface ResponseError {
     id: string
     code: number
     message: string
-    description?: string
-    stack?: unknown
+    description: string
+    stack?: string
 }
diff --git a/lib/test/__mocks__/common.mocks.ts b/lib/test/__mocks__/common.mocks.ts
@@ -0,0 +1,31 @@
+export const mockLogger = {
+    log: (message: any, context?: string) => {},
+    error: (message: any, trace?: string, context?: string) => {},
+    warn: (message: any, context?: string) => {},
+    debug: (message: any, context?: string) => {},
+    verbose: (message: any, context?: string) => {},
+}
+
+export const mockJson = jest.fn()
+
+export const mockStatus = jest.fn().mockImplementation(() => ({
+    json: mockJson,
+}))
+
+export const mockGetResponse = jest.fn().mockImplementation(() => ({
+    status: mockStatus,
+}))
+
+export const mockHttpArgumentsHost = jest.fn().mockImplementation(() => ({
+    getResponse: mockGetResponse,
+    getRequest: jest.fn(),
+}))
+
+export const mockArgumentsHost = {
+    switchToHttp: mockHttpArgumentsHost,
+    getArgByIndex: jest.fn(),
+    getArgs: jest.fn(),
+    getType: jest.fn(),
+    switchToRpc: jest.fn(),
+    switchToWs: jest.fn(),
+}
diff --git a/lib/types/response-error.type.ts b/lib/types/response-error.type.ts
@@ -12,8 +12,8 @@ export class ResponseErrorType implements ResponseError {
     @Field()
     message: string
 
-    @Field({ nullable: true })
-    description?: string
+    @Field()
+    description: string
 
     @Field({ nullable: true })
     stack?: string
diff --git a/lib/types/responses-payload.type.ts b/lib/types/responses-payload.type.ts
@@ -4,12 +4,26 @@ import { ResponseError } from '../interfaces/response-error.interface'
 import { ResponsesPayload } from '../interfaces/responses-payload.interface'
 import { ResponseErrorType } from './response-error.type'
 
+export type NullableListOptions = { nullable: true | 'itemsAndList' }
+
+/**
+ *
+ * @param classRef Initial class with `@ObjectType` decorator
+ * @param options nullability options, returning `[Type]!` or `[Type!]!` is disallowed due to nature of response structure -
+ * `data` field will always be (`GraphQL`-wise) null when error is thrown
+ *
+ * You can provide `true` (default) to generate `[Type!]` definition
+ * or `'itemsAndList'` to generate `[Type]` definition in resulting `.graphql` schema file
+ *
+ * @see NullableListOptions
+ */
 export const ResponsesPayloadType = <T>(
     classRef: Type<T>,
+    options: NullableListOptions = { nullable: true },
 ): Type<ResponsesPayload<T>> => {
     @ObjectType('ResponsesPayload', { isAbstract: true })
     class ResponsesPayloadType implements ResponsesPayload<T> {
-        @Field(() => [classRef], { nullable: true })
+        @Field(() => [classRef], { nullable: options.nullable })
         data?: T[]
 
         @Field(() => ResponseErrorType, { nullable: true })
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json

Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,6 @@ export interface ResponseError {`
`2`	`2`	`id: string`
`3`	`3`	`code: number`
`4`	`4`	`message: string`
`5`		`- description?: string`
`6`		`- stack?: unknown`
	`5`	`+ description: string`
	`6`	`+ stack?: string`
`7`	`7`	`}`