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 1 commit
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
7 changes: 6 additions & 1 deletion src/interceptors/ClientRequest/NodeClientRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,10 @@ export class NodeClientRequest extends ClientRequest {
// Treat them as request errors.
if (isNodeLikeError(resolverResult.error)) {
this.errorWith(resolverResult.error)
} else {
return this
}

if (resolverResult.error instanceof Error) {
// Coerce unhandled exceptions in the "request" listeners
// as 500 responses.
this.respondWith(
Expand All @@ -294,8 +297,10 @@ export class NodeClientRequest extends ClientRequest {
}
)
)
return this
}

this.errorWith(resolverResult.error)
return this
}

Expand Down
45 changes: 22 additions & 23 deletions src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,31 +99,30 @@ export function createXMLHttpRequestProxy({
return
}

// 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',
},
}
if (resolverResult.error instanceof Error) {
// Treat unhandled exceptions in the "request" listener
// as 500 server errors.
xhrRequestController.respondWith(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument of treating exceptions as 500 responses is good, see: #516. TL;DR, if exceptions are preserved, they will be handled by the higher request client. Not only different clients handle those exceptions differently, but that harms the observability of the thrown error.

I also think that treating unhandled exceptions as 500 responses is more similar to the actual server, and you write these request listeners from the server's respective.

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
}

/**
* @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)
// Otherwise, error the request with the thrown data.
xhrRequestController.errorWith(resolverResult.error)
Copy link
Member Author

@kettanaito kettanaito Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument for translating any other (non-error) data to request errors is a bit weaker. There should be a way to trigger request errors.

Now that I think about it, we do have that way! It's this:

request.respondWith(Response.error())

This translates to a request error across different clients. I think we need to remove non-error handling changes and treat non-errors as 500 responses anyway.

So the three cases become this:

  • Throw a Response, it gets used as a mocked response.
  • Respond with Response.error(), it gets used as a request error.
  • Throw anything unhandled, it gets coerces to a 500 error response.

I think this makes the most sense.

return
}

Expand Down
40 changes: 22 additions & 18 deletions src/interceptors/fetch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,23 +142,27 @@ 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',
},
}
)
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))
}

const mockedResponse = resolverResult.data
Expand All @@ -177,7 +181,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
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('forwards thrown non-errors as request errors', async () => {
interceptor.on('request', () => {
throw 123
})

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 thrown non-errors a network error', async () => {
interceptor.on('request', () => {
throw 123
})

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).toBe(123)
})
15 changes: 15 additions & 0 deletions test/modules/http/compliance/http-unhandled-exception.test.ts
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 Expand Up @@ -49,3 +50,17 @@ 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<Error>()
request.on('error', (error) => requestErrorPromise.resolve(error))

const requestError = await requestErrorPromise

expect(requestError).toBe(123)
})
Loading