support all http methods about fastify

[mcollina]

I think we can close this :).

[mcollina]

I think we should support 'TRACE' and 'CONNECT' as methods but not add any route, possibly creating a plugin that is spec compliant instead. I see most of our users not needing those.

[mcollina]

I would commit the current state, but git hooks prevent me from doing so before the tests work)

You can skip git hooks with git commit -n.

[jakearchibald]

@mcollina

What methods did you need to add that were not there?

All of them 😄

I'm creating a demo where the user can set whatever method they like to see if the browser allow the request through (it's a CORS demo).

@jsumners

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

Nah, I'm ok. I'm not even sure if it's in the spirit of this library. I was able to do what I wanted in express with:

app.options('/resource', (req, res) => {
  // Handle preflight
});

app.all('/resource', (req, res) => {
  // Handle main request
});

Edit: Express might support more methods, but it doesn't support arbitrary ones due to limitations in Node's HTTP implementation.

[jakearchibald]

I might be talking shit here, I'm not sure express supports any method either, it might just support more 😄

[jakearchibald]

That's what we're talking about.

[mcollina]

It's likely not the main use case, but it would be good for the framework to be flexible enough. At the minimum we should support all methods that Node.js is supporting.

[jsumners]

@mcollina Sure, how should I start working?

See https://gist.github.com/jsumners/461ef7a64545108635cc437fde112721

[delvedor]

Should we implement TRACEand CONNECT as well?

[Radiergummi]

@mcollina @delvedor, as far as I can see in the documentation, CONNECT and TRACE are not supported? I'm building sort of a custom proxy protocol and would like to add a CONNECT endpoint to establish a bi-directional tunnel. I can do that with a separate net socket, but having all the HTTP stuff in fastify would definitely be more desirable :)

[mcollina]

@Radiergummi would you like to send a PR?

[Radiergummi]

I didn't peek into the fastify source yet, but I'll give it a go. Depends on the scope you'd image though:

TL;DR: Either we could simply add explicit support for these two methods or drop the method name restriction altogether.

• TRACE calls are described by MDN like this:

  The final recipient of the request should reflect the message received, excluding some fields described below, back to the client as the message body of a 200 (OK) response with a Content-Type of message/http. The final recipient is either the origin server or the first server to receive a Max-Forwards value of 0 in the request.

  This is something I guess fastify could do out of the box, unless otherwise suppplemented with a custom implementation by the user. Meaning fastify should probably adhere to the docs and mirror the request in the response, but users may provide a custom route handler for TRACE requests to handle it themselves.
  Though I have a feeling this is so obscure it might be better off as a plugin for those that really need support for TRACE.

• CONNECT calls have the following description:

  The HTTP CONNECT method starts two-way communications with the requested resource. It can be used to open a tunnel.

  I think this might be easier to implement, as a handler for this method would only need direct access to the underlying request and response streams and keep them open, but as raw is part of fastify's request and reply, that is there already. Of course fastify could include a higher-level API, but again that might be a better job for a plugin.

---

The other direction a PR might go into would be to simply allow any request method string, which is perfectly legal according to RFC 7231. Fastify could still provide helper methods for the common request methods and keep the Typescript interfaces but union them with the string type.
This would have the additional benefit of making it possible to build a webdav implementation based on fastify, should anyone fancy doing so.

[Eomm]

Though I have a feeling this is so obscure it might be better off as a plugin for those that really need support for TRACE.

I agree, a plugin for this it is already doable

but as raw is part of fastify's request and reply, that is there already

Right now the stream are consumed by the content type parser, so in this case, we should skip this setup and it is doable in a plugin, but the plugin must add a content type parser that could lead to security issues (and I would not force the user to add a content type parser if it is not need at all)

So for this usage, I think it would impact a bit in core

[Radiergummi]

I'm actively working on a PR right now, but a single test is failing for TRACE requests for me... In # Subtest: TRACE returns 413 - Payload Too Large, fastify returns a 400 instead of a 413. Stepping through the tests with the debugger, I noticed after the 413 is correctly sent, something somewhere throws a FST_ERR_CTP_EMPTY_JSON_BODY, right after that node prints

test/trace.test.js 2> (node:430773) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [Socket]. Use emitter.setMaxListeners() to increase limit
test/trace.test.js 2>     at _addListener (events.js:389:17)
test/trace.test.js 2>     at Socket.addListener (events.js:405:10)
test/trace.test.js 2>     at Socket.Readable.on (_stream_readable.js:852:35)
test/trace.test.js 2>     at Socket.socketListenerWrap [as on] (_http_server.js:831:54)
test/trace.test.js 2>     at Socket.socketOnError (_http_server.js:599:8)
test/trace.test.js 2>     at onParserExecuteCommon (_http_server.js:618:19)
test/trace.test.js 2>     at onParserExecute (_http_server.js:577:3)
test/trace.test.js 2>     at HTTPParser.callbackTrampoline (internal/async_hooks.js:129:14)

The test file is copied from the PUT tests:

'use strict'

const t = require('tap')
require('./helper').payloadMethod('trace', t)
require('./input-validation').payloadMethod('trace', t)

Which I would have guessed should work out of the box. Any ideas?
(I would commit the current state, but git hooks prevent me from doing so before the tests work)

[Eomm]

FST_ERR_CTP_EMPTY_JSON_BODY

This is throw when the content type of the response is application/json but the reply.send() has been run

After the commit open a PR so we could help you to check it 👍

[Radiergummi]

Draft PR is open, see #2454.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jakearchibald]

I had to switch back to using expressjs for a project because fastify doesn't support adding routes for arbitrary HTTP methods. The thing I was working on was a lower-level test thing, so maybe that's fine. Just thought I'd let you know!

Edit: Express might support more methods, but it doesn't support arbitrary ones due to limitations in Node's HTTP implementation.

[mcollina]

What methods did you need to add that were not there?

[jsumners]

I had to switch back to using expressjs for a project because fastify doesn't support adding routes for arbitrary HTTP methods. The thing I was working on was a lower-level test thing, so maybe that's fine. Just thought I'd let you know!

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[mcollina]

We support the ones Node support in the router: https://github.com/delvedor/find-my-way/blob/c561d220e6c0c93b76b72a7461dcdc8620158ba2/index.js#L102, not sure how we can support more 😅.

Maybe there is some specific behavior that we are missing?

[jakearchibald]

Maybe I'm reading it wrong, but require('http').METHODS is:

[
  'ACL',         'BIND',       'CHECKOUT',
  'CONNECT',     'COPY',       'DELETE',
  'GET',         'HEAD',       'LINK',
  'LOCK',        'M-SEARCH',   'MERGE',
  'MKACTIVITY',  'MKCALENDAR', 'MKCOL',
  'MOVE',        'NOTIFY',     'OPTIONS',
  'PATCH',       'POST',       'PROPFIND',
  'PROPPATCH',   'PURGE',      'PUT',
  'REBIND',      'REPORT',     'SEARCH',
  'SOURCE',      'SUBSCRIBE',  'TRACE',
  'UNBIND',      'UNLINK',     'UNLOCK',
  'UNSUBSCRIBE'
]

Whereas fastify only supports a subset of that https://github.com/fastify/fastify/blob/main/lib/route.js#L7.

But yes, you're ultimately restricted by Node here. My new plan is to use Deno which doesn't seem to have that restriction, but I might be talking shit again.

Sorry for the noise!

[Radiergummi]

Well according to the HTTP spec, the method can be an arbitrary string: RFC 7231, section 4.1.

[mcollina]

I think we can relax those checks, they are mostly for developer experience. I don't think there are any actual limitations in supporting those.

I can add an option if you'd like.

[jakearchibald]

🤷‍♂️

The thing I'm building ended up needing something lower level than Node was able to support, so I'd totally understand if you didn't see this as a good fit for fastify.

[hungtcs]

Please support WebDAV http methods. WebDAV extension http methods are very useful, sometimes I need to process WebDAV requests especially some files related applications.

[mcollina]

@hungtcs would you like to work on it? There #3635 which is not complete but a good starting point for two.

[hungtcs]

@hungtcs would you like to work on it? There #3635 which is not complete but a good starting point for two.

@mcollina Sure, how should I start working?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.