Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unify server error response across interceptors #555

Merged
merged 2 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 9 additions & 20 deletions src/interceptors/ClientRequest/NodeClientRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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<unknown, Response | undefined>(async () => {
// Notify the interceptor about the request.
// This will call any "request" listeners the users have.
this.logger.info(
Expand Down Expand Up @@ -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
Expand All @@ -275,27 +277,14 @@ export class NodeClientRequest extends ClientRequest {
// Treat them as request errors.
if (isNodeLikeError(resolverResult.error)) {
this.errorWith(resolverResult.error)
} else {
// 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))

return this
}

Expand Down
29 changes: 6 additions & 23 deletions src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -94,36 +95,18 @@ export function createXMLHttpRequestProxy({
resolverResult.error
)

// Treat thrown Responses as mocked responses.
if (resolverResult.error instanceof Response) {
this.respondWith(resolverResult.error)
return
}

// Treat unhandled exceptions in the "request" listener
// as 500 server errors.
// Unhandled exceptions in the request listeners are
// synonymous to unhandled exceptions on the server.
// Those are represented as 500 error responses.
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',
},
}
)
createServerErrorResponse(resolverResult.error)
)

/**
* @todo Consider forwarding this error to the stderr as well
* since not all consumers are expecting to handle errors.
* If they don't, this error will be swallowed.
*/
// xhrRequestController.errorWith(resolverResult.error)
return
}

Expand Down
62 changes: 26 additions & 36 deletions src/interceptors/fetch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<HttpRequestEventMap> {
static symbol = Symbol('fetch')
Expand Down Expand Up @@ -109,28 +110,30 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> {
return response
}

const resolverResult = await until(async () => {
const listenersFinished = emitAsync(this.emitter, 'request', {
request: interactiveRequest,
requestId,
})
const resolverResult = await until<unknown, Response | undefined>(
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)
Expand All @@ -142,23 +145,10 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> {
return respondWith(resolverResult.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',
},
}
)
// 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
Expand All @@ -177,7 +167,7 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> {

/**
* Set the cause of the request promise rejection to the
* network error Response instance. This different from Undici.
* network error Response instance. This differs from Undici.
* Undici will forward the "response.error" custom property
* as the rejection reason but for "Response.error()" static method
* "response.error" will equal to undefined, making "cause" an empty Error.
Expand Down
24 changes: 24 additions & 0 deletions src/utils/responseUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
}
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/**
* @see https://github.com/mswjs/msw/issues/355
*/
import { it, expect, beforeAll, afterEach, afterAll } from 'vitest'
import { vi, it, expect, beforeAll, afterEach, afterAll } from 'vitest'
import axios from 'axios'
import { XMLHttpRequestInterceptor } from '../../../../src/interceptors/XMLHttpRequest'
import { createXMLHttpRequest } from '../../../helpers'
Expand Down Expand Up @@ -77,3 +77,20 @@ it('treats a thrown Response instance as a mocked response', async () => {
expect(request.response).toBe('hello world')
expect(request.responseText).toBe('hello world')
})

it('treats Response.error() as a network error', async () => {
interceptor.on('request', ({ request }) => {
request.respondWith(Response.error())
})

const requestErrorListener = vi.fn()
const request = await createXMLHttpRequest((request) => {
request.responseType = 'text'
request.open('GET', 'http://localhost/api')
request.addEventListener('error', requestErrorListener)
request.send()
})

expect(request.status).toBe(0)
expect(requestErrorListener).toHaveBeenCalledTimes(1)
})
16 changes: 16 additions & 0 deletions test/modules/fetch/fetch-exception.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,19 @@ it('treats a thrown Response as a mocked response', async () => {
expect(response.status).toBe(200)
expect(await response.text()).toBe('hello world')
})

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')
.then(() => {
throw new Error('Must not resolve')
})
.catch<TypeError & { cause?: unknown }>((error) => error)

expect(requestError.name).toBe('TypeError')
expect(requestError.message).toBe('Failed to fetch')
expect(requestError.cause).toBeInstanceOf(Response)
})
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { vi, it, expect, beforeAll, afterEach, afterAll } from 'vitest'
import http from 'node:http'
import { ClientRequestInterceptor } from '../../../../src/interceptors/ClientRequest'
import { waitForClientRequest } from '../../../helpers'
import { DeferredPromise } from '@open-draft/deferred-promise'

const interceptor = new ClientRequestInterceptor()

Expand Down
Loading