Skip to content

Commit

Permalink
refactor: unify error body handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@ronag
ronag committed Apr 12, 2023
1 parent b20405e commit c6fdc69
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 92 deletions.
56 changes: 14 additions & 42 deletions lib/api/api-request.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ const Readable = require('./readable')
const {
InvalidArgumentError,
RequestAbortedError,
ResponseStatusCodeError
} = require('../core/errors')
const util = require('../core/util')
const { getResolveErrorBodyCallback } = require('./util')
const { AsyncResource } = require('async_hooks')
const { addSignal, removeSignal } = require('./abort-signal')

Expand Down Expand Up @@ -73,40 +73,39 @@ class RequestHandler extends AsyncResource {
}

onHeaders (statusCode, rawHeaders, resume, statusMessage) {
const { callback, opaque, abort, context } = this
const { callback, opaque, abort, context, responseHeaders } = this

const headers = responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)

if (statusCode < 200) {
if (this.onInfo) {
const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)
this.onInfo({ statusCode, headers })
}
return
}

const parsedHeaders = util.parseHeaders(rawHeaders)
const parsedHeaders = responseHeaders === 'raw' ? util.parseHeaders(rawHeaders) : headers
const contentType = parsedHeaders['content-type']
const body = new Readable(resume, abort, contentType)

this.callback = null
this.res = body
const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)

if (callback !== null) {
if (this.throwOnError && statusCode >= 400) {
this.runInAsyncScope(getResolveErrorBodyCallback, null,
{ callback, body, contentType, statusCode, statusMessage, headers }
)
return
} else {
this.runInAsyncScope(callback, null, null, {
statusCode,
headers,
trailers: this.trailers,
opaque,
body,
context
})
}

this.runInAsyncScope(callback, null, null, {
statusCode,
headers,
trailers: this.trailers,
opaque,
body,
context
})
}
}

Expand Down Expand Up @@ -153,33 +152,6 @@ class RequestHandler extends AsyncResource {
}
}

async function getResolveErrorBodyCallback ({ callback, body, contentType, statusCode, statusMessage, headers }) {
if (statusCode === 204 || !contentType) {
body.dump()
process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers))
return
}

try {
if (contentType.startsWith('application/json')) {
const payload = await body.json()
process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers, payload))
return
}

if (contentType.startsWith('text/')) {
const payload = await body.text()
process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers, payload))
return
}
} catch (err) {
// Process in a fallback if error
}

body.dump()
process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers))
}

function request (opts, callback) {
if (callback === undefined) {
return new Promise((resolve, reject) => {
Expand Down
86 changes: 37 additions & 49 deletions lib/api/api-stream.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ const {
InvalidArgumentError,
InvalidReturnValueError,
RequestAbortedError,
ResponseStatusCodeError
} = require('../core/errors')
const util = require('../core/util')
const { getResolveErrorBodyCallback } = require('./util')
const { AsyncResource } = require('async_hooks')
const { addSignal, removeSignal } = require('./abort-signal')

Expand Down Expand Up @@ -79,49 +79,53 @@ class StreamHandler extends AsyncResource {
}

onHeaders (statusCode, rawHeaders, resume, statusMessage) {
const { factory, opaque, context, callback } = this
const { factory, opaque, context, callback, responseHeaders } = this

const headers = responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)

if (statusCode < 200) {
if (this.onInfo) {
const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)
this.onInfo({ statusCode, headers })
}
return
}

this.factory = null
const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)
const res = this.runInAsyncScope(factory, null, {
statusCode,
headers,
opaque,
context
})

let res

if (this.throwOnError && statusCode >= 400) {
const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)
const chunks = []
const pt = new PassThrough()
pt
.on('data', (chunk) => chunks.push(chunk))
.on('end', () => {
const payload = Buffer.concat(chunks).toString('utf8')
this.runInAsyncScope(
callback,
null,
new ResponseStatusCodeError(
`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`,
statusCode,
headers,
payload
)
)
})
.on('error', (err) => {
this.onError(err)
})
this.res = pt
return
const parsedHeaders = responseHeaders === 'raw' ? util.parseHeaders(rawHeaders) : headers
const contentType = parsedHeaders['content-type']
res = new PassThrough()

this.runInAsyncScope(getResolveErrorBodyCallback, null,
{ callback, body: res, contentType, statusCode, statusMessage, headers }
)
} else {
res = this.runInAsyncScope(factory, null, {
statusCode,
headers,
opaque,
context
})

// TODO: Avoid finished. It registers an unnecessary amount of listeners.
finished(res, { readable: false }, (err) => {
const { callback, res, opaque, trailers, abort } = this

this.res = null
if (err || !res.readable) {
util.destroy(res, err)
}

this.callback = null
this.runInAsyncScope(callback, null, err || null, { opaque, trailers })

if (err) {
abort()
}
})
}

if (
Expand All @@ -134,22 +138,6 @@ class StreamHandler extends AsyncResource {
}

res.on('drain', resume)
// TODO: Avoid finished. It registers an unnecessary amount of listeners.
finished(res, { readable: false }, (err) => {
const { callback, res, opaque, trailers, abort } = this

this.res = null
if (err || !res.readable) {
util.destroy(res, err)
}

this.callback = null
this.runInAsyncScope(callback, null, err || null, { opaque, trailers })

if (err) {
abort()
}
})

this.res = res

Expand Down
42 changes: 42 additions & 0 deletions lib/api/util.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
const assert = require('assert')
const {
ResponseStatusCodeError
} = require('../core/errors')

async function getResolveErrorBodyCallback ({ callback, body, contentType, statusCode, statusMessage, headers }) {
assert(body)

if (statusCode === 204 || !contentType) {
body.dump()
process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers))
return
}

try {
if (contentType.startsWith('application/json')) {
const payload = await body.json()
process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers, payload))
return
}

if (contentType.startsWith('text/')) {
const payload = await body.text()
process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers, payload))
return
}
} catch (err) {
// Process in a fallback if error
}

let limit = 0
for await (const chunk of body) {
limit += chunk.length
if (limit > 128 * 1024) {
break
}
}

process.nextTick(callback, new ResponseStatusCodeError(`Response status code ${statusCode}${statusMessage ? `: ${statusMessage}` : ''}`, statusCode, headers))
}

module.exports = { getResolveErrorBodyCallback }
2 changes: 1 addition & 1 deletion test/client-stream.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@ test('stream legacy needDrain', (t) => {
})
})

test('steam throwOnError', (t) => {
test('stream throwOnError', (t) => {
t.plan(2)

const errStatusCode = 500
Expand Down

0 comments on commit c6fdc69

Please sign in to comment.