From c46fd4d457450efbf0cf690188f06c2951e0bf34 Mon Sep 17 00:00:00 2001 From: Artem Zakharchenko Date: Wed, 17 Apr 2024 15:32:21 +0200 Subject: [PATCH] fix: consistent exception handling --- .../ClientRequest/NodeClientRequest.ts | 30 ++------- .../XMLHttpRequest/XMLHttpRequestProxy.ts | 32 +++------- src/interceptors/fetch/index.ts | 64 ++++++++----------- src/utils/responseUtils.ts | 24 +++++++ .../xhr-middleware-exception.test.ts | 6 +- test/modules/fetch/fetch-exception.test.ts | 8 +-- .../http-unhandled-exception.test.ts | 14 ---- 7 files changed, 71 insertions(+), 107 deletions(-) diff --git a/src/interceptors/ClientRequest/NodeClientRequest.ts b/src/interceptors/ClientRequest/NodeClientRequest.ts index 1bf9be91..1067ce44 100644 --- a/src/interceptors/ClientRequest/NodeClientRequest.ts +++ b/src/interceptors/ClientRequest/NodeClientRequest.ts @@ -23,6 +23,7 @@ import { isPropertyAccessible } from '../../utils/isPropertyAccessible' import { isNodeLikeError } from '../../utils/isNodeLikeError' import { INTERNAL_REQUEST_ID_HEADER_NAME } from '../../Interceptor' import { createRequestId } from '../../createRequestId' +import { createServerErrorResponse } from '../../utils/responseUtils' export type Protocol = 'http' | 'https' @@ -220,7 +221,7 @@ export class NodeClientRequest extends ClientRequest { // Execute the resolver Promise like a side-effect. // Node.js 16 forces "ClientRequest.end" to be synchronous and return "this". - until(async () => { + until(async () => { // Notify the interceptor about the request. // This will call any "request" listeners the users have. this.logger.info( @@ -266,6 +267,7 @@ export class NodeClientRequest extends ClientRequest { resolverResult.error ) + // Treat thrown Responses as mocked responses. if (resolverResult.error instanceof Response) { this.respondWith(resolverResult.error) return @@ -278,29 +280,11 @@ export class NodeClientRequest extends ClientRequest { return this } - if (resolverResult.error instanceof Error) { - // Coerce unhandled exceptions in the "request" listeners - // as 500 responses. - this.respondWith( - new Response( - JSON.stringify({ - name: resolverResult.error.name, - message: resolverResult.error.message, - stack: resolverResult.error.stack, - }), - { - status: 500, - statusText: 'Unhandled Exception', - headers: { - 'Content-Type': 'application/json', - }, - } - ) - ) - return this - } + // Unhandled exceptions in the request listeners are + // synonymous to unhandled exceptions on the server. + // Those are represented as 500 error responses. + this.respondWith(createServerErrorResponse(resolverResult.error)) - this.errorWith(resolverResult.error) return this } diff --git a/src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts b/src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts index 9325f394..a7ee0e55 100644 --- a/src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts +++ b/src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts @@ -4,6 +4,7 @@ import { XMLHttpRequestEmitter } from '.' import { toInteractiveRequest } from '../../utils/toInteractiveRequest' import { emitAsync } from '../../utils/emitAsync' import { XMLHttpRequestController } from './XMLHttpRequestController' +import { createServerErrorResponse } from '../../utils/responseUtils' export interface XMLHttpRequestProxyOptions { emitter: XMLHttpRequestEmitter @@ -94,35 +95,18 @@ export function createXMLHttpRequestProxy({ resolverResult.error ) + // Treat thrown Responses as mocked responses. if (resolverResult.error instanceof Response) { this.respondWith(resolverResult.error) return } + // Unhandled exceptions in the request listeners are + // synonymous to unhandled exceptions on the server. + // Those are represented as 500 error responses. + xhrRequestController.respondWith( + createServerErrorResponse(resolverResult.error) + ) - if (resolverResult.error instanceof Error) { - // Treat unhandled exceptions in the "request" listener - // as 500 server errors. - xhrRequestController.respondWith( - new Response( - JSON.stringify({ - name: resolverResult.error.name, - message: resolverResult.error.message, - stack: resolverResult.error.stack, - }), - { - status: 500, - statusText: 'Unhandled Exception', - headers: { - 'Content-Type': 'application/json', - }, - } - ) - ) - return - } - - // Otherwise, error the request with the thrown data. - xhrRequestController.errorWith(resolverResult.error) return } diff --git a/src/interceptors/fetch/index.ts b/src/interceptors/fetch/index.ts index e03a35fd..0643ad47 100644 --- a/src/interceptors/fetch/index.ts +++ b/src/interceptors/fetch/index.ts @@ -8,6 +8,7 @@ import { emitAsync } from '../../utils/emitAsync' import { isPropertyAccessible } from '../../utils/isPropertyAccessible' import { canParseUrl } from '../../utils/canParseUrl' import { createRequestId } from '../../createRequestId' +import { createServerErrorResponse } from '../../utils/responseUtils' export class FetchInterceptor extends Interceptor { static symbol = Symbol('fetch') @@ -109,28 +110,30 @@ export class FetchInterceptor extends Interceptor { return response } - const resolverResult = await until(async () => { - const listenersFinished = emitAsync(this.emitter, 'request', { - request: interactiveRequest, - requestId, - }) + const resolverResult = await until( + async () => { + const listenersFinished = emitAsync(this.emitter, 'request', { + request: interactiveRequest, + requestId, + }) - await Promise.race([ - requestAborted, - // Put the listeners invocation Promise in the same race condition - // with the request abort Promise because otherwise awaiting the listeners - // would always yield some response (or undefined). - listenersFinished, - requestController.responsePromise, - ]) + await Promise.race([ + requestAborted, + // Put the listeners invocation Promise in the same race condition + // with the request abort Promise because otherwise awaiting the listeners + // would always yield some response (or undefined). + listenersFinished, + requestController.responsePromise, + ]) - this.logger.info('all request listeners have been resolved!') + this.logger.info('all request listeners have been resolved!') - const mockedResponse = await requestController.responsePromise - this.logger.info('event.respondWith called with:', mockedResponse) + const mockedResponse = await requestController.responsePromise + this.logger.info('event.respondWith called with:', mockedResponse) - return mockedResponse - }) + return mockedResponse + } + ) if (requestAborted.state === 'rejected') { return Promise.reject(requestAborted.rejectionReason) @@ -142,27 +145,10 @@ export class FetchInterceptor extends Interceptor { return respondWith(resolverResult.error) } - if (resolverResult.error instanceof Error) { - // Treat unhandled exceptions from the "request" listeners - // as 500 errors from the server. Fetch API doesn't respect - // Node.js internal errors so no special treatment for those. - return new Response( - JSON.stringify({ - name: resolverResult.error.name, - message: resolverResult.error.message, - stack: resolverResult.error.stack, - }), - { - status: 500, - statusText: 'Unhandled Exception', - headers: { - 'Content-Type': 'application/json', - }, - } - ) - } - - return Promise.reject(createNetworkError(resolverResult.error)) + // Unhandled exceptions in the request listeners are + // synonymous to unhandled exceptions on the server. + // Those are represented as 500 error responses. + return createServerErrorResponse(resolverResult.error) } const mockedResponse = resolverResult.data diff --git a/src/utils/responseUtils.ts b/src/utils/responseUtils.ts index 105048b5..ba9ef7f4 100644 --- a/src/utils/responseUtils.ts +++ b/src/utils/responseUtils.ts @@ -13,3 +13,27 @@ export const RESPONSE_STATUS_CODES_WITHOUT_BODY = new Set([ export function isResponseWithoutBody(status: number): boolean { return RESPONSE_STATUS_CODES_WITHOUT_BODY.has(status) } + +/** + * Creates a generic 500 Unhandled Exception response. + */ +export function createServerErrorResponse(body: unknown): Response { + return new Response( + JSON.stringify( + body instanceof Error + ? { + name: body.name, + message: body.message, + stack: body.stack, + } + : body + ), + { + status: 500, + statusText: 'Unhandled Exception', + headers: { + 'Content-Type': 'application/json', + }, + } + ) +} diff --git a/test/modules/XMLHttpRequest/compliance/xhr-middleware-exception.test.ts b/test/modules/XMLHttpRequest/compliance/xhr-middleware-exception.test.ts index b5c73920..b4cc1bac 100644 --- a/test/modules/XMLHttpRequest/compliance/xhr-middleware-exception.test.ts +++ b/test/modules/XMLHttpRequest/compliance/xhr-middleware-exception.test.ts @@ -78,9 +78,9 @@ it('treats a thrown Response instance as a mocked response', async () => { expect(request.responseText).toBe('hello world') }) -it('forwards thrown non-errors as request errors', async () => { - interceptor.on('request', () => { - throw 123 +it('treats Response.error() as a network error', async () => { + interceptor.on('request', ({ request }) => { + request.respondWith(Response.error()) }) const requestErrorListener = vi.fn() diff --git a/test/modules/fetch/fetch-exception.test.ts b/test/modules/fetch/fetch-exception.test.ts index 2cbecaca..ff9d888f 100644 --- a/test/modules/fetch/fetch-exception.test.ts +++ b/test/modules/fetch/fetch-exception.test.ts @@ -46,9 +46,9 @@ it('treats a thrown Response as a mocked response', async () => { expect(await response.text()).toBe('hello world') }) -it('treats thrown non-errors a network error', async () => { - interceptor.on('request', () => { - throw 123 +it('treats Response.error() as a network error', async () => { + interceptor.on('request', ({ request }) => { + request.respondWith(Response.error()) }) const requestError = await fetch('http://localhost:3001/resource') @@ -59,5 +59,5 @@ it('treats thrown non-errors a network error', async () => { expect(requestError.name).toBe('TypeError') expect(requestError.message).toBe('Failed to fetch') - expect(requestError.cause).toBe(123) + expect(requestError.cause).toBeInstanceOf(Response) }) diff --git a/test/modules/http/compliance/http-unhandled-exception.test.ts b/test/modules/http/compliance/http-unhandled-exception.test.ts index 407c9576..af7bfcc9 100644 --- a/test/modules/http/compliance/http-unhandled-exception.test.ts +++ b/test/modules/http/compliance/http-unhandled-exception.test.ts @@ -50,17 +50,3 @@ it('treats unhandled interceptor errors as 500 responses', async () => { stack: expect.any(String), }) }) - -it('treats thrown non-errors as request errors', async () => { - interceptor.on('request', () => { - throw 123 - }) - - const request = http.get('http://localhost/resource') - const requestErrorPromise = new DeferredPromise() - request.on('error', (error) => requestErrorPromise.resolve(error)) - - const requestError = await requestErrorPromise - - expect(requestError).toBe(123) -})