Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support HTTP QUERY method #5615

Closed
2 tasks done
jonchurch opened this issue Apr 20, 2024 · 4 comments
Closed
2 tasks done

Support HTTP QUERY method #5615

jonchurch opened this issue Apr 20, 2024 · 4 comments

Comments

@jonchurch
Copy link
Member

jonchurch commented Apr 20, 2024

Honestly we might already, but opening this issue to track landing support and writing tests.

Node 21 added support for a new method! QUERY which is awesome, looks to have landed here nodejs/node#51719 in 21.7.2 (changelog)

@wesleytodd any idea what needs to be done to router to support QUERY? If this Issue belongs at router plz do move it!
We may actually already support this ¯_(ツ)_/¯ and it's just that a test is failing.

Im unable to get it to work in Node 21.7.3 currently, and left a comment in the node tracking issue for support nodejs/node#51562

TODO:

@jonchurch
Copy link
Member Author

jonchurch commented Apr 20, 2024

Edit: this will light up once it fully lights up in Node, see the Node repro at the bottom

Here's my test harness rn, I don't think we support this automatically

✨✨✨

const express = require('express');
const app = express();
const router = express.Router();

app.use(express.json());

// Test using app.query (if this is valid)
try {
  app.query('/test-app-query', (req, res) => {
    console.log({ body: req.body });
    res.status(200).json({ data: req.body });
  });
  console.log("app.query() tested and set up.");
} catch (error) {
  console.error("app.query() failed:", error);
}

// Test using router.query (if this is valid)
try {
  router.query('/test-router-query', (req, res) => {
    console.log({ body: req.body });
    res.status(200).json({ data: req.body });
  });
  app.use(router);
  console.log("router.query() tested and set up.");
} catch (error) {
  console.error("router.query() failed:", error);
}

// Fallback general middleware to check if QUERY method is getting through
app.use('/test', (req, res, next) => {
  if (req.method === 'QUERY') {
    console.log('Received QUERY method via general handler');
    res.status(200).json({ received: req.body });
  } else {
    next();
  }
});

// General error handler
app.use((err, req, res, next) => {
  console.error('Error Handler:', { err });
  res.status(500).send('Server Error');
});

app.listen(3000, () => {
  console.log("Server listening on port 3000");
});

These are the curl requests Im using:

curl -i -X QUERY -H "Content-Type: application/json" -d '{"foo": "bar"}' http://localhost:3000/test-app-query
curl -i -X QUERY -H "Content-Type: application/json" -d '{"foo": "bar"}' http://localhost:3000/test-router-query
curl -i -X QUERY -H "Content-Type: application/json" -d '{"foo": "bar"}' http://localhost:3000/test

They each give me a response of:

HTTP/1.1 404 Not Found
X-Powered-By: Express
Content-Security-Policy: default-src 'none'
X-Content-Type-Options: nosniff
Content-Type: text/html; charset=utf-8
Content-Length: 159
Date: Sat, 20 Apr 2024 22:33:30 GMT
Connection: keep-alive
Keep-Alive: timeout=5

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Cannot undefined /test-app-query</pre>
</body>
</html>

CURL shouldn't matter here, but just in case:

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

Maybe node really doesn't support this yet? Check this out:

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
curl -i -X QUERY -H "Content-Type: application/json" -d '{"foo": "bar"}' http://localhost:3000

HTTP/1.1 405 Method Not Allowed
Content-Type: text/plain
Date: Sat, 20 Apr 2024 23:22:16 GMT
Connection: keep-alive
Keep-Alive: timeout=5
Transfer-Encoding: chunked

Method Not Allowed

@sheplu
Copy link
Member

sheplu commented Apr 21, 2024

I may be wrong but I do think this is supported by express? I do remember that it worked with a manually build version of Node.js but not with the latest release @wesleytodd

@wesleytodd
Copy link
Member

Yeah that is quite possible. I thought the version with this fixed was supposed to go out with the security patch but maybe I misunderstood and it was set to land later? Based on this yes I am thinking that is the case: nodejs/node#51562 (comment)

jonchurch added a commit to jonchurch/express that referenced this issue Apr 29, 2024
jonchurch added a commit to jonchurch/express that referenced this issue Apr 29, 2024
jonchurch added a commit to jonchurch/express that referenced this issue Apr 29, 2024
jonchurch added a commit to jonchurch/express that referenced this issue Apr 29, 2024
RobinTail added a commit to RobinTail/express-zod-api that referenced this issue May 6, 2024
This serves two purposes:

- Enforcing constraints on its actual usage in `walkRouting()`,
- Should ease adoption of the new `QUERY` method in the future, see:
  - expressjs/express#5615
  - nodejs/node#51562
RobinTail added a commit to RobinTail/express-zod-api that referenced this issue May 6, 2024
This serves two purposes:

- Enforcing constraints on its actual usage in `walkRouting()`,
- Should ease adoption of the new `QUERY` method in the future, see:
  - expressjs/express#5615
  - nodejs/node#51562
RobinTail added a commit to RobinTail/express-zod-api that referenced this issue May 6, 2024
This serves two purposes:

- Enforcing constraints on its actual usage in `walkRouting()`,
- Should ease adoption of the new `QUERY` method in the future, see:
  - expressjs/express#5615
  - nodejs/node#51562
adarshRsinha added a commit to adarshRsinha/express that referenced this issue May 22, 2024
@jonchurch
Copy link
Member Author

Looks like this is now passing in Node v22.2.0, which includes this commit nodejs/node@65c8380

I don't think support will come to node 21, as the change was considered semver major by Node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants