Add Fastify-allow plugin reference

[mattbishop]

Checklist

• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[RafaelGSS]

LGTM. Very usefull plugin!

[zekth]

lgtm

[mcollina]

Great plugin! May I ask for a change? I think by default it should not add a new not found handler but rather provide an utility that could be used to implement such behavior. All the applications I have worked with have a custom not found handler.. which will make this plugin significantly less useful.

[mattbishop]

Interesting, I didn't know that it was common to replace the notFoundHandler. Do you have a plugin that does this well? My first thought is to expose a method routeNotFoundHandler with the same interface as notFoundHander. Then people who don't want to automatically send 405s would use the existing options and then get this method to handle themselves.

[mcollina]

Interesting, I didn't know that it was common to replace the notFoundHandler. Do you have a plugin that does this well? My first thought is to expose a method routeNotFoundHandler with the same interface as notFoundHander. Then people who don't want to automatically send 405s would use the existing options and then get this method to handle themselves.

I would not really use such behavior. The best thing I think would be to add a property on the thrown error with the allowed methods

[mattbishop]

I don't think I understand. When would I throw an error? Is there a hook I can use that is for requests that do not match to a route where I could throw an error?

[mcollina]

I don't think I understand. When would I throw an error? Is there a hook I can use that is for requests that do not match to a route where I could throw an error?

I'm sorry I've responded too quickly. I think the best approach would be to add a decorator in the app to fetch all the allowed method of a route.

[mattbishop]

The decorator approach is fine for known routes with handlers like GET, but I don't think it will work for the routes that have one handler but not more, where the spec expects the server to return 405 with an Allow header. For instance:

app.get("/route", ...)

Then the client calls:

HTTP GET /route
... response:
200 OK,  Allow: GET

HTTP POST /route
... response:
405 Method Not Allowed, Allow: GET

The only way I found to respond with the 405 automatically is to replace the notFoundHandler because no route exists for POST:/route. This is important to the usefulness of the plugin. I thought of other ways, like registering a bunch of handlers for all the methods that are not supported but I didn't have access to the routes table at the plugin level.

This may actually be better as a core behaviour of fastify? It is part of the http 1.1 spec.

[mcollina]

Actually you are better off implementing this as a hook and throwing an Error with .statusCode 405, something like the MethodNotAllowed error in https://www.npmjs.com/package/http-errors.

Something like (this works):

import Fastify from 'fastify'
import errors from 'http-errors'

const app = Fastify({ logger: true })

app.addHook('onRequest', async function (req) {
  if (req.method === 'POST') {
    throw new errors.MethodNotAllowed('POST')
  }
})

await app.listen(3000)

Which in pseudocode would become:

import Fastify from 'fastify'
import errors from 'http-errors'

const app = Fastify({ logger: true })

await app.register(import('fastify-allow'))
app.addHook('onRequest', async function (req) {
  if (app.isAllowed(req)) {
    throw new errors.MethodNotAllowed('POST')
  }
})

await app.listen(3000)

[mattbishop]

The 'onRequest' approach was actually my first idea, but I found that it was not called if a request had no registered handler. Find-my-way includes the method in the search for a route, so if fastify has a handler for app.get('/route'...), fastify will trigger onRequest for GET /route, but it will not trigger for POST /route.

The only hook I could find for non-registered routes was the notFoundHandler.

This plugin does allow the developer to turn off the handleNotFound override with a plugin option. Let me take your original suggestion of a utility export and try it out. For those cases where they don't need to override handleNotFound, the plugin works as-is. For those who want a different notFound behaviour, they can turn it off with the plugin option "send405": false. It's the last use case I'm missing–they want their own notFound handler, but they also want to send 405s too.

[mcollina]

The 'onRequest' approach was actually my first idea, but I found that it was not called if a request had no registered handler. Find-my-way includes the method in the search for a route, so if fastify has a handler for app.get('/route'...), fastify will trigger onRequest for GET /route, but it will not trigger for POST /route.

I think we fixed this, this will get triggered all the time.

[mattbishop]

Cool, what version?

…

On Tue, Aug 10, 2021 at 6:33 AM Matteo Collina ***@***.***> wrote: The 'onRequest' approach was actually my first idea, but I found that it was not called if a request had no registered handler. Find-my-way includes the method in the search for a route, so if fastify has a handler for app.get('/route'...), fastify will trigger onRequest for GET /route, but it will not trigger for POST /route. I *think* we fixed this, this will get triggered all the time. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3242 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACY3BJCCU3XGVU4DKQYU43T4ETDZANCNFSM5BY56ZZA> .

[mcollina]

I think it was fixed since the v3 cycle but I might be mistaken. Anyway, I could not reproduce the behavior you are mentioning now.

[mattbishop]

Great news, I was wrong and onRequest is indeed called when a path matches but not the method. I tested both fastify 3.3.0 and 3.18.1:

test("onRequest for unregistered routes", (t) => {
  t.plan(2)
  const app = Fastify()
  app.addHook("onRequest", (request: FastifyRequest, reply: FastifyReply, done: () => void) => {
    t.pass(`onRequest called for ${request.method} ${request.url}`)
    done()
  })
  const url = "/a-route"
  app.get(url, (request: FastifyRequest, reply: FastifyReply) => {
    reply.send()
  })

  app.inject({method: "GET", url})
  app.inject({method: "DELETE", url})
})

TAP version 13
# onRequest for unregistered routes
ok 1 onRequest called for GET /a-route
ok 2 onRequest called for DELETE /a-route

So I can take out the notFoundHandler approach altogether. I'll post in this issue when I do that.

[mattbishop]

I have refactored to not use the noFoundHandler. Thanks again for the direction.

[mcollina]

lgtm

[github-actions]

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.