Support for 'QUERY' method

[evert]

Version

v20.10.0

Platform

Linux evertbook6f 6.6.11-200.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Jan 10 19:25:59 UTC 2024 x86_64 GNU/Linux

Subsystem

node:http

What steps will reproduce the bug?

The QUERY http method has a fair bit of interest and will likely at some point get standardized. There's a draft here:

https://www.ietf.org/archive/id/draft-ietf-httpbis-safe-method-w-body-02.html

Currently (as far as I'm aware) it's not possible to define custom methods in the Node http package, preventing users from testing with this. Although I understand this was probably done for parser performance reasons, HTTP does have an explicit extension mechanism for new methods and recommendations on how to treat unknown ones, so I do feel it goes a bit counter against the spirit of HTTP to not allow new methods. Browsers will for example let users use any (unregistered) http method in fetch().

That said, I understand that supporting any unknown HTTP method is probably a lot of work, so I'll settle for having support for the QUERY http method!

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

Currently Node returns a 400 Bad Request

Additional information

No response

[aduh95]

@nodejs/http

[ShogunPanda]

This is technically doable, but it will involve a PR in llhttp (the current Node.js HTTP parser) first and then it will be automatically supported by Node.
@evert Woud like to send a PR to llhttp to implement this?

[benjamingr]

cc @jasnell (http QUERY is your spec right?)

[evert]

Yes! Interested in doing this. Will give this a shot

[marco-ippolito]

llhttp supports QUERY with nodejs/llhttp#265, so when released it will be possible to implement this on node

[evert]

Cool, thanks for doing that!

[marco-ippolito]

When this PR #51719 is merged should be possible to support it

[jonchurch]

Hey folks, Im not seeing QUERY support in Node post #51719 landing

Should this have gone out in 21.7.2? based on https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V21.md#21.7.2

Guessing that it might need additional work in node? Expected support because http.METHODS lists QUERY currently

I'm using this as a test and tracking adding support in express in expressjs/express#5615:

// filename: http.js
const http = require('http');

// Do we know about query?
console.log(http.METHODS.includes('QUERY')) // true

// Create an HTTP server
const server = http.createServer((req, res) => {
  // Log the method to the console for verification
  console.log('Received method:', req.method);

  // Check if the method is QUERY
  if (req.method === 'QUERY') {
    res.writeHead(200, { 'Content-Type': 'text/plain' });
    res.end('Received a QUERY method request');
  } else {
    // Respond with Method Not Allowed if the method is not QUERY
    res.writeHead(405, { 'Content-Type': 'text/plain' });
    res.end('Method Not Allowed');
  }
});

// Listen on port 3000
server.listen(3000, () => {
  console.log('Server listening on port 3000');
});

node -v
# v21.7.3
node http.js
curl -i -X QUERY -H "Content-Type: application/json" -d '{"foo": "bar"}' http://localhost:3000

I see req.method as undefined, so fallthrough to my 405 here.

for completeness:

curl --version

curl 8.4.0 (x86_64-apple-darwin23.0) libcurl/8.4.0 (SecureTransport) LibreSSL/3.3.6 zlib/1.2.12 nghttp2/1.58.0
Release-Date: 2023-10-11
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS GSS-API HSTS HTTP2 HTTPS-proxy IPv6 Kerberos Largefile libz MultiSSL NTLM NTLM_WB SPNEGO SSL threadsafe UnixSockets

And I am reproing locally on mac os, but also in GHA ubuntu-latest aka ubuntu-22.04

[marco-ippolito]

@ShogunPanda triaged the issue, and if Im not mistaken its a trivial bug on Node side

[bricss]

Does anyone know where we should look in code to add a path to support the QUERY method?

[marco-ippolito]

Does anyone know where we should look in code to add a path to support the QUERY method?

It should he already supported, there is just a little bug so its not listen in METHODS

[bricss]

Seems like I found the location, it's missing from HTTP_KNOWN_METHODS(V) 🕵️

[bricss]

I'm just not sure if it's the only place where we should be looking for, or is it?

[marco-ippolito]

@ShogunPanda may know more about it

[ShogunPanda]

Yes, I just submitted #52701. To address this.

[jonchurch]

QUERY is now supported in Node 22.2.0 🎉 Thanks!

Any chance #52701 will be backported to 21? QUERY is still failing there, but reported in http.METHODS

[evert]

Thank you!

[marco-ippolito]

QUERY is now supported in Node 22.2.0 🎉 Thanks!

Any chance #52701 will be backported to 21? QUERY is still failing there, but reported in http.METHODS

v21 is EOL and v20 has an old major version of llhttp so I think it's unlikely

[voxpelli]

v21 is EOL and v20 has an old major version of llhttp so I think it's unlikely

Unless QUERY support can be backported to v20 (which would be nice, considering that eg. Fastify v5 supports v20 and up, right @mcollina?) this issue can be closed as completed, right?

[marco-ippolito]

I dont think it will be backported to v20 for the reason stated in my previous comment also v20 going into maintenance mode next month. Closing as resolved