issue #95 - adding onError hook (#96)

* 1. adding onError hook
2. Fix to failed service unavailable error test
3. add .idea/ to git ignore file

* fixed code review comments:
1. index.ts - error type was changed from string to Error
2. onErrorNoOp was renamed to onErrorDefault
3. rollback timeout errors during tests

* fixed code review comments:
1. using httpStatus instead of magic numbers
2. using fake-timers instead of setTimeout in tests

* fixed code review comments:
1. namespaced imports are supposed to be above the non-namespaced ones
2. added type check to to .ts test

* rollback modifying file

* fixed code review comments:
1. added onError to .ts test

* fixed code review comments:
1. added onError to .ts test

* fixed code review comments:
1. added onError to .ts test

* fixed sporadic failed test

* code review:
using http-errors package

* code review:
1. removed dev dependency http-status
2. onError reply.send(error)
3. fixed readme.md accordingly

* code review:
1. wrap the error in json object
2. updated README.md

* code review:
1. updated onError in index.d.ts
2. updated README.md

Co-authored-by: Yohay <yohay.go@moonactive.com>

Author: yohayg

.gitignore

@@ -58,7 +58,8 @@ typings/
 .env
 
 .vscode/
+.idea/
 
 # lockfiles
 yarn.lock
-package-lock.json
+package-lock.json

README.md

@@ -179,6 +179,13 @@ Called when an http response is received from the source.
 The default behavior is `reply.send(res)`, which will be disabled if the
 option is specified.
 
+#### `onError(reply, error)`
+
+Called when an http response is received with error from the source.
+The default behavior is `reply.send(error)`, which will be disabled if the
+option is specified.
+It must reply the error.
+
 #### `rewriteHeaders(headers)`
 
 Called to rewrite the headers of the response, before them being copied

index.d.ts

@@ -35,6 +35,10 @@ export interface FastifyReplyFromHooks {
     reply: FastifyReply<RawServerBase>,
     res: RawReplyDefaultExpression<RawServerBase>
   ) => void;
+  onError?: (
+      reply: FastifyReply<RawServerBase>,
+      error: { error: Error }
+  ) => void;
   body?: unknown;
   rewriteHeaders?: (
     headers: Http2IncomingHttpHeaders | IncomingHttpHeaders

index.js

@@ -5,6 +5,7 @@ const URL = require('url').URL
 const lru = require('tiny-lru')
 const querystring = require('querystring')
 const Stream = require('stream')
+const createError = require('http-errors')
 const buildRequest = require('./lib/request')
 const {
   filterPseudoHeaders,
@@ -34,6 +35,7 @@ module.exports = fp(function from (fastify, opts, next) {
     const onResponse = opts.onResponse
     const rewriteHeaders = opts.rewriteHeaders || headersNoOp
     const rewriteRequestHeaders = opts.rewriteRequestHeaders || requestHeadersNoOp
+    const onError = opts.onError || onErrorDefault
 
     if (!source) {
       source = req.url
@@ -103,11 +105,11 @@ module.exports = fp(function from (fastify, opts, next) {
         this.request.log.warn(err, 'response errored')
         if (!this.sent) {
           if (err.code === 'ERR_HTTP2_STREAM_CANCEL' || err.code === 'ENOTFOUND') {
-            this.code(503).send(new Error('Service Unavailable'))
+            onError(this, { error: new createError.ServiceUnavailable() })
           } else if (err instanceof TimeoutError || err.code === 'UND_ERR_REQUEST_TIMEOUT') {
-            this.code(504).send(new Error('Gateway Timeout'))
+            onError(this, { error: new createError.GatewayTimeout() })
           } else {
-            this.code(500).send(err)
+            onError(this, { error: createError(500, err) })
           }
         }
         return
@@ -165,3 +167,7 @@ function headersNoOp (headers) {
 function requestHeadersNoOp (originalReq, headers) {
   return headers
 }
+
+function onErrorDefault (reply, { error }) {
+  reply.send(error)
+}

package.json

@@ -7,7 +7,7 @@
   "scripts": {
     "typescript": "tsd",
     "coverage": "tap -j8 test/*.js --cov",
-    "test": "standard | snazzy && tap test/*.js && npm run typescript"
+    "test": "standard | snazzy && tap --ts test/* && npm run typescript"
   },
   "repository": {
     "type": "git",
@@ -29,7 +29,9 @@
   },
   "homepage": "https://github.com/fastify/fastify-reply-from#readme",
   "devDependencies": {
+    "@sinonjs/fake-timers": "^6.0.1",
     "@types/node": "^14.0.4",
+    "@types/tap": "^14.10.0",
     "fastify": "^3.0.0",
     "got": "^11.1.3",
     "h2url": "^0.2.0",
@@ -50,7 +52,8 @@
     "pump": "^3.0.0",
     "semver": "^7.2.1",
     "tiny-lru": "^7.0.0",
-    "undici": "^1.0.0"
+    "undici": "^1.0.0",
+    "http-errors": "^1.8.0"
   },
   "tsd": {
     "directory": "test"

test/http-timeout.js

@@ -4,6 +4,9 @@ const t = require('tap')
 const Fastify = require('fastify')
 const From = require('..')
 const got = require('got')
+const FakeTimers = require('@sinonjs/fake-timers')
+
+const clock = FakeTimers.createClock()
 
 t.autoend(false)
 
@@ -13,10 +16,10 @@ t.tearDown(target.close.bind(target))
 target.get('/', (request, reply) => {
   t.pass('request arrives')
 
-  setTimeout(() => {
+  clock.setTimeout(() => {
     reply.status(200).send('hello world')
     t.end()
-  }, 1000)
+  }, 200)
 })
 
 async function main () {
@@ -43,7 +46,7 @@ async function main () {
       error: 'Gateway Timeout',
       message: 'Gateway Timeout'
     })
-
+    clock.tick(200)
     return
   }
 

test/index.test-d.ts

@@ -1,10 +1,12 @@
 import replyFrom, { FastifyReplyFromOptions } from "../";
-import fastify from "fastify";
+import fastify, {FastifyReply, RawServerBase} from "fastify";
 import { AddressInfo } from "net";
 import { IncomingHttpHeaders } from "http2";
 import { expectType } from 'tsd';
 import * as http from 'http';
 import * as http2 from 'http2';
+// @ts-ignore
+import tap from 'tap'
 
 const fullOptions: FastifyReplyFromOptions = {
   base: "http://example2.com",
@@ -38,52 +40,68 @@ const fullOptions: FastifyReplyFromOptions = {
     pipelining: 10
   }
 };
+tap.autoend(false);
 
-const server = fastify();
+async function main() {
+  const server = fastify();
 
-server.register(replyFrom);
+  server.register(replyFrom);
 
-server.register(replyFrom, {});
+  server.register(replyFrom, {});
 
-server.register(replyFrom, { http2: true });
+  server.register(replyFrom, {http2: true});
 
-server.register(replyFrom, fullOptions);
+  server.register(replyFrom, fullOptions);
 
-server.get("/v1", (erquest, reply) => {
-  reply.from();
-});
-server.get("/v3", (erquest, reply) => {
-  reply.from("/v3", {
-    body: { hello: "world" },
-    rewriteRequestHeaders(req, headers) {
-      expectType<http.IncomingMessage | http2.Http2ServerRequest>(req);
-      return headers;
-    }
+  server.get("/v1", (request, reply) => {
+      reply.from();
+  });
+  server.get("/v3", (request, reply) => {
+      reply.from("/v3", {
+          body: {hello: "world"},
+          rewriteRequestHeaders(req, headers) {
+              expectType<http.IncomingMessage | http2.Http2ServerRequest>(req);
+              return headers;
+          }
+      });
   });
-});
 
 // http2
-const instance = fastify({ http2: true });
-
-const target = fastify({ http2: true });
+  const instance = fastify({http2: true});
+  // @ts-ignore
+  tap.tearDown(instance.close.bind(instance));
+  const target = fastify({http2: true});
+  // @ts-ignore
+  tap.tearDown(target.close.bind(target));
+  instance.get("/", (request, reply) => {
+      reply.from();
+  });
 
-instance.get("/", (request, reply) => {
-  reply.from();
-});
+  instance.get("/http2", (request, reply) => {
+      reply.from("/", {
+          rewriteHeaders(headers) {
+              return headers;
+          },
+          rewriteRequestHeaders(req, headers: IncomingHttpHeaders) {
+              return headers;
+          },
+          onError(reply: FastifyReply<RawServerBase>, error) {
+              return reply.send(error.error);
+          }
+      });
+  });
 
-instance.get("/http2", (request, reply) => {
-  reply.from("/", {
-    rewriteHeaders(headers) {
-      return headers;
-    },
-    rewriteRequestHeaders(req, headers: IncomingHttpHeaders) {
-      return headers;
-    }
+  await target.listen(0);
+  const port = (target.server.address() as AddressInfo).port;
+  instance.register(replyFrom, {
+      base: `http://localhost:${port}`,
+      http2: true,
+      rejectUnauthorized: false
   });
-});
+  await instance.listen(0);
+
+  tap.pass('done')
+  tap.end();
+}
 
-instance.register(replyFrom, {
-  base: `http://localhost:${(target.server.address() as AddressInfo).port}`,
-  http2: true,
-  rejectUnauthorized: false
-});
+main();

test/on-error.js

@@ -0,0 +1,62 @@
+'use strict'
+
+const t = require('tap')
+const Fastify = require('fastify')
+const From = require('..')
+const got = require('got')
+const FakeTimers = require('@sinonjs/fake-timers')
+
+const clock = FakeTimers.createClock()
+
+t.autoend(false)
+
+const target = Fastify()
+t.tearDown(target.close.bind(target))
+
+target.get('/', (request, reply) => {
+  t.pass('request arrives')
+
+  clock.setTimeout(() => {
+    reply.status(200).send('hello world')
+    t.end()
+  }, 1000)
+})
+
+async function main () {
+  await target.listen(0)
+
+  const instance = Fastify()
+  t.tearDown(instance.close.bind(instance))
+
+  instance.register(From, { http: { requestOptions: { timeout: 100 } } })
+
+  instance.get('/', (request, reply) => {
+    reply.from(`http://localhost:${target.server.address().port}/`,
+      {
+        onError: (reply, { error }) => {
+          t.equal(error.status, 504)
+          reply.code(error.status).send(error)
+        }
+      })
+  })
+
+  await instance.listen(0)
+
+  try {
+    await got.get(`http://localhost:${instance.server.address().port}/`, { retry: 0 })
+  } catch (err) {
+    t.equal(err.response.statusCode, 504)
+    t.match(err.response.headers['content-type'], /application\/json/)
+    t.deepEqual(JSON.parse(err.response.body), {
+      statusCode: 504,
+      error: 'Gateway Timeout',
+      message: 'Gateway Timeout'
+    })
+    clock.tick(1000)
+    return
+  }
+
+  t.fail()
+}
+
+main()

test/undici-timeout.js

@@ -4,6 +4,9 @@ const t = require('tap')
 const Fastify = require('fastify')
 const From = require('..')
 const got = require('got')
+const FakeTimers = require('@sinonjs/fake-timers')
+
+const clock = FakeTimers.createClock()
 
 t.autoend(false)
 
@@ -13,7 +16,7 @@ t.tearDown(target.close.bind(target))
 target.get('/', (request, reply) => {
   t.pass('request arrives')
 
-  setTimeout(() => {
+  clock.setTimeout(() => {
     reply.status(200).send('hello world')
     t.end()
   }, 1000)
@@ -48,7 +51,7 @@ async function main () {
       error: 'Gateway Timeout',
       message: 'Gateway Timeout'
     })
-
+    clock.tick(1000)
     return
   }