From b6a1f5b9e1ba2f3366b86a8bff3f3f36612a3519 Mon Sep 17 00:00:00 2001 From: KaKa Date: Thu, 27 Oct 2022 00:50:24 +0800 Subject: [PATCH 1/6] feat: support async trailer --- docs/Reference/Reply.md | 13 +++++--- lib/reply.js | 29 ++++++++++++++++-- test/reply-trailers.test.js | 60 ++++++++++++++++++++++++++----------- 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/docs/Reference/Reply.md b/docs/Reference/Reply.md index c669790927..3c2c4c3e5e 100644 --- a/docs/Reference/Reply.md +++ b/docs/Reference/Reply.md @@ -234,9 +234,6 @@ as soon as possible. *Note: The header `Transfer-Encoding: chunked` will be added once you use the trailer. It is a hard requirement for using trailer in Node.js.* -*Note: Currently, the computation function only supports synchronous function. -That means `async-await` and `promise` are not supported.* - ```js reply.trailer('server-timing', function() { return 'db;dur=53, app;dur=47.2' @@ -246,7 +243,15 @@ const { createHash } = require('crypto') // trailer function also recieve two argument // @param {object} reply fastify reply // @param {string|Buffer|null} payload payload that already sent, note that it will be null when stream is sent -reply.trailer('content-md5', function(reply, payload) { +// @param {function} done callback to set trailer value +reply.trailer('content-md5', function(reply, payload, done) { + const hash = createHash('md5') + hash.update(payload) + done(null, hash.disgest('hex')) +}) + +// when you prefer async-await +reply.trailer('content-md5', async function(reply, payload) { const hash = createHash('md5') hash.update(payload) return hash.disgest('hex') diff --git a/lib/reply.js b/lib/reply.js index 68f02de06d..2f4e6f347c 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -673,11 +673,36 @@ function sendTrailer (payload, res, reply) { if (reply[kReplyTrailers] === null) return const trailerHeaders = Object.keys(reply[kReplyTrailers]) const trailers = {} + let handled = 0 for (const trailerName of trailerHeaders) { if (typeof reply[kReplyTrailers][trailerName] !== 'function') continue - trailers[trailerName] = reply[kReplyTrailers][trailerName](reply, payload) + handled-- + + function cb (err, value) { + // TODO: we may protect multiple callback calls + // or mixing async-await with callback + handled++ + + // we can safely ignore error for trailer + // since it does affect the client + // we log in here only for debug usage + if (err) reply.log.debug(err) + else trailers[trailerName] = value + + // add trailers when all handler handled + if (handled === 0) { + res.addTrailers(trailers) + } + } + + const result = reply[kReplyTrailers][trailerName](reply, payload, cb) + if (typeof result === 'object' && typeof result.then === 'function') { + result.then((v) => cb(null, v), cb) + } else if (result !== null && result !== undefined) { + // TODO: should be deprecated + cb(null, result) + } } - res.addTrailers(trailers) } function sendStreamTrailer (payload, res, reply) { diff --git a/test/reply-trailers.test.js b/test/reply-trailers.test.js index e9ae60cdf7..ee08946807 100644 --- a/test/reply-trailers.test.js +++ b/test/reply-trailers.test.js @@ -12,8 +12,8 @@ test('send trailers when payload is empty string', t => { const fastify = Fastify() fastify.get('/', function (request, reply) { - reply.trailer('ETag', function (reply, payload) { - return 'custom-etag' + reply.trailer('ETag', function (reply, payload, done) { + done(null, 'custom-etag') }) reply.send('') }) @@ -36,8 +36,8 @@ test('send trailers when payload is empty buffer', t => { const fastify = Fastify() fastify.get('/', function (request, reply) { - reply.trailer('ETag', function (reply, payload) { - return 'custom-etag' + reply.trailer('ETag', function (reply, payload, done) { + done(null, 'custom-etag') }) reply.send(Buffer.alloc(0)) }) @@ -60,8 +60,8 @@ test('send trailers when payload is undefined', t => { const fastify = Fastify() fastify.get('/', function (request, reply) { - reply.trailer('ETag', function (reply, payload) { - return 'custom-etag' + reply.trailer('ETag', function (reply, payload, done) { + done(null, 'custom-etag') }) reply.send(undefined) }) @@ -88,11 +88,11 @@ test('send trailers when payload is json', t => { const md5 = hash.digest('hex') fastify.get('/', function (request, reply) { - reply.trailer('Content-MD5', function (reply, payload) { + reply.trailer('Content-MD5', function (reply, payload, done) { t.equal(data, payload) const hash = createHash('md5') hash.update(payload) - return hash.digest('hex') + done(null, hash.digest('hex')) }) reply.send(data) }) @@ -116,9 +116,9 @@ test('send trailers when payload is stream', t => { const fastify = Fastify() fastify.get('/', function (request, reply) { - reply.trailer('ETag', function (reply, payload) { + reply.trailer('ETag', function (reply, payload, done) { t.same(payload, null) - return 'custom-etag' + done(null, 'custom-etag') }) const stream = Readable.from([JSON.stringify({ hello: 'world' })]) reply.send(stream) @@ -137,6 +137,30 @@ test('send trailers when payload is stream', t => { }) }) +test('send trailers when using async-await', t => { + t.plan(5) + + const fastify = Fastify() + + fastify.get('/', function (request, reply) { + reply.trailer('ETag', async function (reply, payload) { + return 'custom-etag' + }) + reply.send('') + }) + + fastify.inject({ + method: 'GET', + url: '/' + }, (error, res) => { + t.error(error) + t.equal(res.statusCode, 200) + t.equal(res.headers.trailer, 'etag') + t.equal(res.trailers.etag, 'custom-etag') + t.notHas(res.headers, 'content-length') + }) +}) + test('removeTrailer', t => { t.plan(6) @@ -144,12 +168,12 @@ test('removeTrailer', t => { fastify.get('/', function (request, reply) { reply.removeTrailer('ETag') // remove nothing - reply.trailer('ETag', function (reply, payload) { - return 'custom-etag' + reply.trailer('ETag', function (reply, payload, done) { + done(null, 'custom-etag') }) - reply.trailer('Should-Not-Call', function (reply, payload) { + reply.trailer('Should-Not-Call', function (reply, payload, done) { t.fail('it should not called as this trailer is removed') - return 'should-not-call' + done(null, 'should-not-call') }) reply.removeTrailer('Should-Not-Call') reply.send(undefined) @@ -175,13 +199,13 @@ test('hasTrailer', t => { fastify.get('/', function (request, reply) { t.equal(reply.hasTrailer('ETag'), false) - reply.trailer('ETag', function (reply, payload) { - return 'custom-etag' + reply.trailer('ETag', function (reply, payload, done) { + done(null, 'custom-etag') }) t.equal(reply.hasTrailer('ETag'), true) - reply.trailer('Should-Not-Call', function (reply, payload) { + reply.trailer('Should-Not-Call', function (reply, payload, done) { t.fail('it should not called as this trailer is removed') - return 'should-not-call' + done(null, 'should-not-call') }) t.equal(reply.hasTrailer('Should-Not-Call'), true) reply.removeTrailer('Should-Not-Call') From 34b6c06480e222ec843d429c33876ad4dae7a7a3 Mon Sep 17 00:00:00 2001 From: KaKa Date: Thu, 27 Oct 2022 00:52:39 +0800 Subject: [PATCH 2/6] test: increase coverage --- test/reply-trailers.test.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/reply-trailers.test.js b/test/reply-trailers.test.js index ee08946807..cdef77e61a 100644 --- a/test/reply-trailers.test.js +++ b/test/reply-trailers.test.js @@ -161,6 +161,30 @@ test('send trailers when using async-await', t => { }) }) +test('should be deprecated usage', t => { + t.plan(5) + + const fastify = Fastify() + + fastify.get('/', function (request, reply) { + reply.trailer('ETag', function (reply, payload) { + return 'custom-etag' + }) + reply.send('') + }) + + fastify.inject({ + method: 'GET', + url: '/' + }, (error, res) => { + t.error(error) + t.equal(res.statusCode, 200) + t.equal(res.headers.trailer, 'etag') + t.equal(res.trailers.etag, 'custom-etag') + t.notHas(res.headers, 'content-length') + }) +}) + test('removeTrailer', t => { t.plan(6) From 9606aaa42024241b8785dd9d77ebc9611512dfea Mon Sep 17 00:00:00 2001 From: KaKa Date: Thu, 27 Oct 2022 00:55:41 +0800 Subject: [PATCH 3/6] docs: add note about error --- docs/Reference/Reply.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/Reference/Reply.md b/docs/Reference/Reply.md index 3c2c4c3e5e..2ba7c8e616 100644 --- a/docs/Reference/Reply.md +++ b/docs/Reference/Reply.md @@ -234,6 +234,9 @@ as soon as possible. *Note: The header `Transfer-Encoding: chunked` will be added once you use the trailer. It is a hard requirement for using trailer in Node.js.* +*Note: Any error passed to `done` callback will be ignored. If you interested +in the error, you may turn on `debug` level logging.* + ```js reply.trailer('server-timing', function() { return 'db;dur=53, app;dur=47.2' From 1c8b85279daafe7d23f807d353dbb921c46940eb Mon Sep 17 00:00:00 2001 From: KaKa Date: Thu, 27 Oct 2022 01:12:55 +0800 Subject: [PATCH 4/6] test: increase coverage --- lib/reply.js | 1 + test/reply-trailers.test.js | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/lib/reply.js b/lib/reply.js index 2f4e6f347c..dc0c8371a7 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -690,6 +690,7 @@ function sendTrailer (payload, res, reply) { else trailers[trailerName] = value // add trailers when all handler handled + /* istanbul ignore else */ if (handled === 0) { res.addTrailers(trailers) } diff --git a/test/reply-trailers.test.js b/test/reply-trailers.test.js index cdef77e61a..5f3e23d627 100644 --- a/test/reply-trailers.test.js +++ b/test/reply-trailers.test.js @@ -161,6 +161,30 @@ test('send trailers when using async-await', t => { }) }) +test('error in trailers should be ignored', t => { + t.plan(5) + + const fastify = Fastify() + + fastify.get('/', function (request, reply) { + reply.trailer('ETag', function (reply, payload, done) { + done('error') + }) + reply.send('') + }) + + fastify.inject({ + method: 'GET', + url: '/' + }, (error, res) => { + t.error(error) + t.equal(res.statusCode, 200) + t.equal(res.headers.trailer, 'etag') + t.notHas(res.trailers, 'etag') + t.notHas(res.headers, 'content-length') + }) +}) + test('should be deprecated usage', t => { t.plan(5) From a50f26a09d34c3fec51a2db332f42865a2d73641 Mon Sep 17 00:00:00 2001 From: KaKa Date: Thu, 27 Oct 2022 01:13:03 +0800 Subject: [PATCH 5/6] docs: wording --- docs/Reference/Reply.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Reference/Reply.md b/docs/Reference/Reply.md index 2ba7c8e616..b5d690ae83 100644 --- a/docs/Reference/Reply.md +++ b/docs/Reference/Reply.md @@ -235,7 +235,7 @@ as soon as possible. trailer. It is a hard requirement for using trailer in Node.js.* *Note: Any error passed to `done` callback will be ignored. If you interested -in the error, you may turn on `debug` level logging.* +in the error, you can turn on `debug` level logging.* ```js reply.trailer('server-timing', function() { From 383d300bf2014ada6f050e5cb4f4740d340c14ea Mon Sep 17 00:00:00 2001 From: KaKa Date: Thu, 27 Oct 2022 12:22:15 +0800 Subject: [PATCH 6/6] feat: deprecation --- lib/reply.js | 3 ++- lib/warnings.js | 2 ++ test/reply-trailers.test.js | 11 +++++++++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/reply.js b/lib/reply.js index dc0c8371a7..04fbd0fe8a 100644 --- a/lib/reply.js +++ b/lib/reply.js @@ -700,7 +700,8 @@ function sendTrailer (payload, res, reply) { if (typeof result === 'object' && typeof result.then === 'function') { result.then((v) => cb(null, v), cb) } else if (result !== null && result !== undefined) { - // TODO: should be deprecated + // TODO: should be removed in fastify@5 + warning.emit('FSTDEP013') cb(null, result) } } diff --git a/lib/warnings.js b/lib/warnings.js index 76d15e1513..25fe27be8a 100644 --- a/lib/warnings.js +++ b/lib/warnings.js @@ -23,4 +23,6 @@ warning.create('FastifyDeprecation', 'FSTDEP011', 'Variadic listen method is dep warning.create('FastifyDeprecation', 'FSTDEP012', 'Request#context property access is deprecated. Please use "Request#routeConfig" or "Request#routeSchema" instead for accessing Route settings. The "Request#context" will be removed in `fastify@5`.') +warning.create('FastifyDeprecation', 'FSTDEP013', 'Direct return of "trailers" function is deprecated. Please use "callback" or "async-await" for return value. The support of direct return will removed in `fastify@5`.') + module.exports = warning diff --git a/test/reply-trailers.test.js b/test/reply-trailers.test.js index 5f3e23d627..ee59ed3353 100644 --- a/test/reply-trailers.test.js +++ b/test/reply-trailers.test.js @@ -185,8 +185,8 @@ test('error in trailers should be ignored', t => { }) }) -test('should be deprecated usage', t => { - t.plan(5) +test('should emit deprecation warning when using direct return', t => { + t.plan(7) const fastify = Fastify() @@ -197,6 +197,13 @@ test('should be deprecated usage', t => { reply.send('') }) + process.on('warning', onWarning) + function onWarning (warning) { + t.equal(warning.name, 'FastifyDeprecation') + t.equal(warning.code, 'FSTDEP013') + } + t.teardown(() => process.removeListener('warning', onWarning)) + fastify.inject({ method: 'GET', url: '/'