feat(#1930): added option to override onBadUrl callback by alemagio · Pull Request #2106 · fastify/fastify

alemagio · 2020-02-18T19:26:54Z

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

I added this draft to have a feedback.
I think this could be a possible solution to allow the user to configure error handling of the framework. As you can see it is still incomplete I hope the idea is clear.

mcollina · 2020-02-18T21:24:41Z

You should to the validator changes in https://github.com/fastify/fastify/blob/master/build/build-validation.js and re-run that file.

mcollina · 2020-02-18T21:25:25Z

Why did you use a frameworkErrors wrapping object?

alemagio · 2020-02-18T21:34:55Z

Why did you use a frameworkErrors wrapping object?

There are other places where we might want the user to be able to override the default behavior of the error handler so i thought it might have been good to wrap those handlers to replace the content-type header or for other reasons. I just thought it could be clearer to wrap those handlers.
Do you think it might be better having separated options?

mcollina · 2020-02-19T01:00:31Z

The current pattern is to have separate options. I would keep it consistent for now.

delvedor

I don't agree @mcollina, I think that for this case it would be better to have a single function that we call, the user can either decide to use our default handling or provide their own, in the same way we are doing for the default error handler.

const fastify = require('fastify')({
  frameworkErrors (error, req, reply) {
    // custom handling
  }
})

Then, the error object should use our error objects, so the user can either specify a custom handling or return the error directly.
For example:

const fastify = require('fastify')({
  frameworkErrors (error, req, reply) {
    if (err instanceof BadUrlError) {
      // custom handling
    } else {
      reply.send(err)
    }
  }
})

alemagio · 2020-02-19T08:46:37Z

I added an object instead of a simple function because there are cases which, in my opinion, are completely different like clientError and it I think it looks cleaner for the user to override some behavior and leave the default for others.

delvedor

@alemagio the reason why there was this proposal in the first place is that someone might want to return the body with a different content type.
Having a single callback makes it quite easy while having multiple keys to override could be annoying.
I could argue that having a simple function allows you to override a single case as well, as described in my previous comment.

alemagio · 2020-02-24T15:51:17Z

Ok, thanks for the explanation @delvedor .
You're right actually.
I will update my code asap.

alemagio · 2020-02-27T15:04:47Z

Updated the code.
I made the changes suggested by @delvedor , hope is what you had in mind.
I did not rebuild the validation file @mcollina because being a function I don't think it is possible to validate it in a json schema but if I miss something else just let me know.

In any case I'm a noob guys but I enjoyed working on this project and using it.
Any comment is really appreciated and I'll do my best to update the code according to it.

mcollina

Good work! This would just needs docs now.

alemagio · 2020-02-27T15:19:31Z

Good work! This would just needs docs now.

Thanks
I'll update the doc asap

jsumners · 2020-02-28T13:56:20Z

docs/Server.md

+
+ Default: `null`
+
+Configure custom error handlers.


This could use some more information. What is the purpose of configuring such handlers? If I configure a handler, what problem am I solving?

What about:

Fastify provides default error handlers for the most common use cases.
Using this option it is possible to override one or more of those handlers with custom code.

const fastify = require('fastify')({ frameworkErrors (error, req, reply) { if (err instanceof BadUrlError) { const headers = { 'Content-Type': 'text/plain' } res.writeHead(400, headers) return res.end("Provided url is not valid") } else { reply.send(err) } } })

That's better.

Eomm · 2020-02-28T21:06:44Z

docs/Server.md

+      res.writeHead(400, headers)
+      return res.end("Provided url is not valid")
+    } else {
+      reply.send(err)


we don't have reply here

Eomm · 2020-02-28T21:08:47Z

test/router-options.test.js

+  fastify.inject({
+    method: 'GET',
+    url: '/test/%world'
+  })


we should check the result here

I'm sorry I don't understand what do you mean

I mean

fastify.inject({ method: 'GET', url: '/test/%world' }, (err, res) => { t.error(err) t.deepEquals(res.json(), ....) } )

docs/Server.md

Eomm · 2020-02-28T21:15:07Z

fastify.js

    // custom error handling
    setNotFoundHandler: setNotFoundHandler,
    setErrorHandler: setErrorHandler,
+    frameworkErrors: null,


this line is not used

alemagio · 2020-03-04T14:35:32Z

Code updated I think I fixed all the comments

… bad url error in errors section

mcollina

lgtm

Eomm

Add more detail to improve the parameter function signature

Eomm · 2020-03-04T23:04:39Z

test/router-options.test.js

+    },
+    (err, res) => {
+      t.error(err)
+      t.deepEquals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component')


Suggested change

t.deepEquals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component')

t.equals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component')

deepEquals is for object

Eomm · 2020-03-04T23:14:57Z

fastify.js


  function onBadUrl (path, req, res) {
+    if (options.frameworkErrors) {
+      return options.frameworkErrors(new FST_ERR_BAD_URL(path), req, res)


as suggested by delvedor, here we could manage the req and res object like this the following code.

In this way, we will provide a solid and plain API to all the developers, where the frameworkErrors function will receive every time the same input: one fastify error, the enhanced request and the reply.

fastify/lib/fourOhFour.js

Lines 167 to 182 in 911c8c0

function fourOhFourFallBack (req, res) {

// if this happen, we have a very bad bug

// we might want to do some hard debugging

// here, let's print out as much info as

// we can

req.id = genReqId(req)

req.originalUrl = req.url

var childLogger = logger.child({ reqId: req.id })

if (modifyCoreObjects) {

req.log = res.log = childLogger

}

childLogger.info({ req }, 'incoming request')

var request = new Request(null, req, null, req.headers, childLogger)

var reply = new Reply(res, { onSend: [], onError: [] }, request, childLogger)

The main goal would be the logger: everyone wants to log the errors, and if we deliver the plain HTTP-REQ and HTTP-RES the user will have issue to archive this

You should have all the parameters you need in that scope to reflect that logic 💪

I believe I have everything i need in the scope.
So basically after those lines I should pass the newly created request and reply to the frameworkErrors function to expose them to the user right?

can you also do:

const frameworkErrors = options.frameworkErrors // in the function if (frameworkErrors) { return frameworkErrors(new FST_ERR_BAD_URL(path), req, res) }

This will avoid frameworkErrors to be called with this as the options object.

alemagio · 2020-03-05T17:04:05Z

Code and doc and test updated.
@Eomm I just copied from 404 error, I think it is correct just the way it is but let me know (as always 😅 ) if i missed something.

Eomm

@alemagio , please don't hate me 😓 : last suggestion to fix the readme ✨ 😇

docs/Server.md

alemagio · 2020-03-06T09:39:30Z

Updated @Eomm

mcollina

lgtm

github-actions · 2022-02-10T09:07:09Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

delvedor reviewed Feb 19, 2020

View reviewed changes

delvedor added discussion Issues or PRs with this label will never stale semver-minor Issue or PR that should land as semver minor v2.x Issue or pr related to Fastify v2 v3.x Issue or pr related to Fastify v3 labels Feb 19, 2020

delvedor reviewed Feb 24, 2020

View reviewed changes

mcollina reviewed Feb 27, 2020

View reviewed changes

alemagio marked this pull request as ready for review February 28, 2020 13:39

jsumners reviewed Feb 28, 2020

View reviewed changes

Eomm reviewed Feb 28, 2020

View reviewed changes

alemagio added 5 commits March 4, 2020 15:45

feat(fastify#1930): added option to overrind onBadUrl callback

0631956

(feat fastify#1930): created option to override onBadUrl handler

b60ce72

(doc fastify#1930): added paragraph for frameworkErrors option, added…

13d57b1

… bad url error in errors section

(fastify#1930): added better example to doc

834769a

(fastify#1930): improved test case

002e69d

alemagio force-pushed the 1930-configurable-error-handler branch from 96a57f8 to 002e69d Compare March 4, 2020 14:45

mcollina approved these changes Mar 4, 2020

View reviewed changes

Eomm reviewed Mar 4, 2020

View reviewed changes

(fastify#1930): make request and reply logger available for the user

0d87e04

Eomm reviewed Mar 5, 2020

View reviewed changes

docs/Server.md Outdated Show resolved Hide resolved

(doc fastify#1930): fixed doc

89df8fd

Eomm approved these changes Mar 6, 2020

View reviewed changes

Eomm requested a review from delvedor March 6, 2020 10:24

mcollina approved these changes Mar 6, 2020

View reviewed changes

mcollina merged commit 14059cf into fastify:master Mar 6, 2020

jsumners changed the title ~~feat(#1930): added option to overrind onBadUrl callback~~ feat(#1930): added option to override onBadUrl callback Mar 6, 2020

This was referenced Mar 6, 2020

Update: pr#2106 fastify/docs-chinese#169

Merged

文档持续更新List fastify/docs-chinese#18

Closed

Eomm mentioned this pull request Mar 13, 2020

feature: throws object #2134

Merged

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022

	t.deepEquals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component')
	t.equals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component')

	function fourOhFourFallBack (req, res) {
	// if this happen, we have a very bad bug
	// we might want to do some hard debugging
	// here, let's print out as much info as
	// we can
	req.id = genReqId(req)
	req.originalUrl = req.url
	var childLogger = logger.child({ reqId: req.id })
	if (modifyCoreObjects) {
	req.log = res.log = childLogger
	}

	childLogger.info({ req }, 'incoming request')

	var request = new Request(null, req, null, req.headers, childLogger)
	var reply = new Reply(res, { onSend: [], onError: [] }, request, childLogger)

Uh oh!

Conversation

alemagio commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mcollina commented Feb 18, 2020

Uh oh!

mcollina commented Feb 18, 2020

Uh oh!

alemagio commented Feb 18, 2020

Uh oh!

mcollina commented Feb 19, 2020

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

alemagio commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

alemagio commented Feb 24, 2020

Uh oh!

alemagio commented Feb 27, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

alemagio commented Feb 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alemagio Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alemagio commented Mar 4, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alemagio commented Mar 5, 2020

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alemagio commented Mar 6, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 10, 2022

Uh oh!

alemagio commented Feb 18, 2020 •

edited

Loading

alemagio commented Feb 19, 2020 •

edited

Loading

alemagio Feb 28, 2020 •

edited

Loading