feat: Improve error experience by metcoder95 · Pull Request #2954 · fastify/fastify

metcoder95 · 2021-03-24T22:35:56Z

This PR tries to improve the error experience by changing to sync registering of routes along with a new error object for Fastify errors.

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 the Developer's Certification of Origin
and the Code of conduct

metcoder95 · 2021-03-29T21:45:55Z

fastify.js

    } else if (name === 'onReady') {
      this[kHooks].validate(name, fn)
      this[kHooks].add(name, fn)
+    } else if (name === 'onRoute') {


I need to move the setup for the onRoute hooks due to the fact that was made for being async, in order to achieve register routes sync, the hooks should also be set in the same way. Please let me know your thoughts

lib/route.js

test/hooks.test.js

mcollina

Can you please target next instead?

lib/route.js

test/hooks.test.js

jsumners · 2021-03-31T12:11:51Z

The description as I write this is "WIP". Therefore, I am marking it as a draft. Please mark it ready for review when you feel it is ready.

metcoder95 · 2021-04-06T21:15:20Z

lib/route.js

+      // There's a bug when ignoreTrailingSlash !== true
+      // See
+      /*
+      └── /prefix
+          └── / (GET)
+              / (HEAD)
+              └── / (GET)
+      **/


This is a pretty interesting error popping up only when ignoreTrailingSlash === true, I didn't dig deep too much but was quite hard to detect. The third route comes out nowhere and the call stack is not really descriptive. I can take a look into it but not sure if it belongs directly to this PR as it happens before targeting to next branch.

I don't understand the error you are describing. As it happens on master as well, open a fresh issue.

Not sure if it happens on main but caused me problems when pointing to next.
What it does is create a third route out of nowhere, as described in the comment. A third route under /prefix// is made, only if ignoreTrailingSlash = true. I'll try to see if I can reproduce it on main

It happens in next without this PR?

Incredibly I was not able to reproduce it on next after a rebase. I'll try to figure out if stills reproducible or maybe was part of my changes before getting it stable

metcoder95 · 2021-04-06T21:16:42Z

test/route.test.js

  t.plan(3)

-  const fastify = Fastify()
+  const fastify = Fastify({ exposeHeadRoutes: false })


I set exposeHeadRoutes to false in order to let the test pass with the correct route, as now if there's a schema validation error, the HEAD route validation is being thrown first, just before the origin GET route

Have you rebased this PR on top of next? I think I fixed this.

Yes, actually rebasing to next popped up the issue. The routes are registered in order, first GET and HEAD as second, but as the schema build happens async, the first that comes up is always for the HEAD one

I have a feeling that we should fix this.

I found the issue, basically in order to receive first the error thrown by the schema validation of the first GET route, is a need to pass the error to the done function when receiving the error from the schema validation. Solved on 1ac09cc

test/404s.test.js

mcollina · 2021-04-07T07:09:10Z

test/route.test.js

  t.plan(3)

-  const fastify = Fastify()
+  const fastify = Fastify({ exposeHeadRoutes: false })


Have you rebased this PR on top of next? I think I fixed this.

mcollina · 2021-04-07T07:09:38Z

test/schema-feature.test.js

 test('Should throw of the schema does not exists in input', t => {
  t.plan(2)
-  const fastify = Fastify()
+  const fastify = Fastify({ exposeHeadRoutes: false })


what happens if this is set to true?

The error validates a throw on an unexisting schema reference, but the test is waiting for the error to be thrown on the original GET /:id endpoint. But because of the order of the routes being loaded (first GET , second HEAD), the validation always fails on the HEAD route throwing exactly the same error but just for the sibling route

Can you add a test with exposeHeadRoutes: true?

test/throw.test.js

mcollina · 2021-04-07T07:10:52Z

lib/route.js

+      // There's a bug when ignoreTrailingSlash !== true
+      // See
+      /*
+      └── /prefix
+          └── / (GET)
+              / (HEAD)
+              └── / (GET)
+      **/


I don't understand the error you are describing. As it happens on master as well, open a fresh issue.

test/hooks.test.js

test/throw.test.js

test/hooks.test.js

metcoder95 · 2021-04-13T21:00:19Z

~~There was a recent update that changed the behavior of fast-json-stringify?~~

It's not because of that, basically on route.js the compileSchemasForSerialization is throwing twice being only the last one captured by avvio, the first one is the one expected from the test but not being able to achieve it

Basically, the same as the previous bugs. The sibling HEAD route is throwing the correct error, meanwhile the original GET one is throwing the new error showed below 😅

The tests is receiving a new error:

-Failed building the serialization schema for GET: /:id, due to error Cannot read property 'type' of undefined
          
+Failed building the serialization schema for GET: /:id, due to error schema is invalid: data.properties['name'] should be object,boolean

metcoder95 · 2021-04-13T22:33:03Z

lib/route.js

+        const { exposeHeadRoute } = opts
+        const hasRouteExposeHeadRouteFlag = exposeHeadRoute != null
+        const shouldExposeHead = hasRouteExposeHeadRouteFlag ? exposeHeadRoute : globalExposeHeadRoutes
+
+        if (shouldExposeHead && options.method === 'GET' && !headRouteExists) {
+          const onSendHandlers = parseHeadOnSendHandlers(opts.onSend)
+          prepareRoute.call(this, 'HEAD', path, { ...opts, onSend: onSendHandlers })
+        } else if (headRouteExists && exposeHeadRoute) {
+          warning.emit('FSTDEP007')


The error stated on this comment was caused by the order of how the sibling HEAD route was being registered for each GET one.

As each GET route was creating the sibling HEAD one before registering a handler for the after hook, the HEAD route was registering first the hook handler, changing the order on how the validations should be done.

I moved the register of the sibling route on the after route hook. Not sure if is the best or should do it after registering the hook handler and not within. Looking forward to having feedback 🙂

Good for me

lib/route.js

jsumners · 2021-04-15T20:58:33Z

test/404s.test.js

-  fastify.listen(0, err => {
-    t.ok(err)
-  })
+    t.fail('setNotFoundHandler should not interfer duplicated route error')


"interfer" is not a word I recognize.

This one and here solved in 5bfeeb 👍

I'm still seeing this on GitHub.

I made another typo, sorry 😓
Done in 0659eb5

mcollina · 2021-04-16T14:10:00Z

test/schema-feature.test.js

 test('Should throw of the schema does not exists in input', t => {
  t.plan(2)
-  const fastify = Fastify()
+  const fastify = Fastify({ exposeHeadRoutes: true })


it is exposeHeadRoutes is true by default.

Suggested change

const fastify = Fastify({ exposeHeadRoutes: true })

const fastify = Fastify()

Done in 0659eb5

RafaelGSS

Can you add a new test to not call onRoute twice when duplicate routes?

lib/route.js

RafaelGSS · 2021-04-16T20:28:09Z

lib/route.js

+      // There's a bug when ignoreTrailingSlash !== true
+      // See
+      /*
+      └── /prefix
+          └── / (GET)
+              / (HEAD)
+              └── / (GET)
+      **/


It happens in next without this PR?

lib/route.js

metcoder95 · 2021-04-24T21:22:09Z

Personally, I'm ok with the current approach, I don't see any cases that call onRoute twice on a duplicate route would be bad or dangerous. However, the current documentation says:

OnRoute
Triggered when a new route is registered here

Perhaps, I'd just change to "Triggered before a new route is registered" just to avoid confusion. What do you all think of that?

Sure, sounds good to me, has more sense as is indeed triggered before registering the route rather than after it. Let me update the documentation about it 👍

can you merge/rebase on top of next? I've merged master in next.
Done, is passing now!

metcoder95 · 2021-04-24T21:48:21Z

I added the changes for the documentation and remove the comments on the code, feel free to take another look and share feedback 🙂

mcollina

lgtm

RafaelGSS

LGTM.

mcollina · 2021-04-30T14:09:15Z

Any more feedbacks on this? @delvedor @jsumners @fastify/core?

jsumners

LGTM

github-actions · 2022-05-12T00:22:16Z

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.

Base automatically changed from master to main March 26, 2021 12:02

metcoder95 marked this pull request as ready for review March 29, 2021 21:44

metcoder95 commented Mar 29, 2021

View reviewed changes

lib/route.js Outdated Show resolved Hide resolved

metcoder95 commented Mar 29, 2021

View reviewed changes

test/hooks.test.js Outdated Show resolved Hide resolved

mcollina requested changes Mar 30, 2021

View reviewed changes

lib/route.js Show resolved Hide resolved

lib/route.js Outdated Show resolved Hide resolved

test/hooks.test.js Outdated Show resolved Hide resolved

mcollina added this to the v4.0.0 milestone Mar 30, 2021

mcollina added the v4.x Issue or pr related to Fastify v4 label Mar 30, 2021

jsumners marked this pull request as draft March 31, 2021 12:11

metcoder95 force-pushed the improve_error_experience branch 2 times, most recently from 3ae7553 to c26bd0e Compare April 1, 2021 20:05

metcoder95 changed the base branch from main to next April 1, 2021 20:11

metcoder95 force-pushed the improve_error_experience branch 2 times, most recently from f9c50f2 to 4b313e1 Compare April 1, 2021 21:35

metcoder95 marked this pull request as ready for review April 6, 2021 21:09

metcoder95 requested a review from mcollina April 6, 2021 21:13

metcoder95 commented Apr 6, 2021

View reviewed changes

mcollina requested changes Apr 7, 2021

View reviewed changes

jsumners reviewed Apr 7, 2021

View reviewed changes

test/hooks.test.js Outdated Show resolved Hide resolved

RafaelGSS reviewed Apr 8, 2021

View reviewed changes

test/throw.test.js Outdated Show resolved Hide resolved

test/hooks.test.js Show resolved Hide resolved

metcoder95 commented Apr 13, 2021

View reviewed changes

metcoder95 requested review from RafaelGSS, jsumners and mcollina April 15, 2021 20:52

jsumners reviewed Apr 15, 2021

View reviewed changes

mcollina reviewed Apr 16, 2021

View reviewed changes

RafaelGSS reviewed Apr 16, 2021

View reviewed changes

metcoder95 added 16 commits April 24, 2021 23:04

feat: PoC sync route registration

8bc9304

chore: add new error code

9025c31

chore: move onRoute hook setup async

f226f0a

chore: move HEAD route exposure async

db22298

chore: implement new error code

15235d4

chore(test): add todos

08350b3

chore(test): remove constraints from error message

ccae116

refactor: delete afterRouteAdded fn

2235cb0

fix: check before adding a new HEAD route

4d88722

fix: address tests changes

4671ba8

fix: pass error objects to done callback

5b87885

chore(test): enable exposeHeadRoutes back again

1ed326f

refactor: add sibling HEAD route on after hook

b5118fb

fix: remove comments

a1dc3e2

test: address changes

e56af4f

route: improve validation of error message

0d51d43

metcoder95 force-pushed the improve_error_experience branch from a58880e to 0d51d43 Compare April 24, 2021 21:13

metcoder95 added 2 commits April 24, 2021 23:27

route: remove comments

53029e7

docs: update docs for new error

1be28a0

mcollina approved these changes Apr 24, 2021

View reviewed changes

RafaelGSS approved these changes Apr 25, 2021

View reviewed changes

jsumners approved these changes Apr 30, 2021

View reviewed changes

mcollina merged commit a36f6f3 into fastify:next May 1, 2021

metcoder95 deleted the improve_error_experience branch May 11, 2021 20:34

mcollina mentioned this pull request May 4, 2022

Update to Fastify v4 fastify/fastify-compress#224

Merged

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2022

	const fastify = Fastify({ exposeHeadRoutes: true })
	const fastify = Fastify()

Uh oh!

Conversation

metcoder95 commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsumners commented Mar 31, 2021

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

metcoder95 commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

RafaelGSS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metcoder95 commented Mar 24, 2021 •

edited

Loading

metcoder95 commented Apr 13, 2021 •

edited

Loading