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

@std/http/route is doesn't automatically route HEAD requests #5993

Closed
LitoMore opened this issue Sep 16, 2024 · 5 comments · Fixed by #6003
Closed

@std/http/route is doesn't automatically route HEAD requests #5993

LitoMore opened this issue Sep 16, 2024 · 5 comments · Fixed by #6003
Labels
http suggestion a suggestion yet to be agreed

Comments

@LitoMore
Copy link
Contributor

LitoMore commented Sep 16, 2024

It's difficult to handle the HEAD request method on route(). For example:

import { route } from '@std/http/route';

const routes = [
	{
		pattern: new URLPattern({ pathname: '/' }),
		handler: homepageHandler,
	},
	{
		pattern: new URLPattern({ pathname: '/favicon.ico' }),
		handler: faviconHandler,
	},
	{
		pattern: new URLPattern({ pathname: '/:iconSlug/:color?/:darkModeColor?' }),
		handler: iconHandler,
	},
];

The route configuration above only supports GET request method. You have to add three extra routes with { method: "HEAD", ... }.

What I expect is that all configured methods allow HEAD requests by default. And add an extra option to let users choose strict mode that excludes HEAD.

Or we can change it method option to methods, so that users can define multiple handlers for one handler:

// @std/http/route.ts

return (request: Request, info?: Deno.ServeHandlerInfo) => {
  for (const route of routes) {
    const match = route.pattern.exec(request.url);
-   if (match && request.method === (route.method ?? "GET")) {
+   if (match && (route.methods ?? ["HEAD", "GET"]).includes(request.method)) {
      return route.handler(request, info, match);
    }
  }
  return defaultHandler(request, info);
};

or use add option allowHead:

// @std/http/route.ts

return (request: Request, info?: Deno.ServeHandlerInfo) => {
  for (const route of routes) {
    const match = route.pattern.exec(request.url);
-   if (match && request.method === (route.method ?? "GET")) {
+   if (match && [route.allowHead && 'HEAD' , route.method ?? 'GET'].filter(Boolean).includes(request.method)) {
      return route.handler(request, info, match);
    }
  }
  return defaultHandler(request, info);
};

Oak's route.get() handles the HEAD method as expected. The same is true for other frameworks, and we may need to consider polishing this part of the experience for developers.

@iuioiua iuioiua changed the title @std/http/route is not friendly for HEAD request method @std/http/route is doesn't automatically route HEAD requests Sep 16, 2024
@iuioiua
Copy link
Collaborator

iuioiua commented Sep 16, 2024

route() was very intentionally designed to be basic. It not automatically handling HEAD requests is fine and not a bug.

@iuioiua iuioiua added suggestion a suggestion yet to be agreed http labels Sep 16, 2024
@kt3k
Copy link
Member

kt3k commented Sep 17, 2024

What I expect is that all configured methods allow HEAD requests by default. And add an extra option to let users choose strict mode that excludes HEAD.

I think HEAD handler and GET handler shouldn't be exactly the same in general as HEAD response should have empty body.

Oak's route.get() handles the HEAD method as expected.

What is the response body for HEAD request in oak? Does it become empty, or is it the same as GET response?

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 17, 2024

I think HEAD handler and GET handler shouldn't be exactly the same in general as HEAD response should have empty body.

As https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD said: any representation headers that describe the erroneous body are assumed to describe the response body that a GET request would have received.

If you manually change the response body to empty, its Content-Length will also change. Which might not be a correct behavior for some cases.

Whether a response body and Content-Length need to be returned is decided by the endpoint provider themselves, rather than us thinking that a HEAD request is unreasonable just because we see a response body.

What is the response body for HEAD request in oak? Does it become empty, or is it the same as GET response?

It's nothing to do with Oak. You don't need to care about the response body. Tools like cURL, Postman, and RapidAPI will handle this themselves.

@kt3k
Copy link
Member

kt3k commented Sep 17, 2024

RFC 9110 says: The HEAD method is identical to GET except that the server MUST NOT send content in the response.

ref: https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.2

What this suggestion tries to promote is providing the same handler for GET and HEAD, but that violates the above rule.

As https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD said: any representation headers that describe the erroneous body are assumed to describe the response body that a GET request would have received.

This rule looks like for the clients to work around the non-compliant servers. I don't think the servers should intentionally depend on the workaround.

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 17, 2024

rfc-editor.org/rfc/rfc9110.html#section-9.3.2

Thank you for finding this out. This is fine for no HEAD on default behavior. My main question was whether we could provide developers with a more convenient way to handle HEAD requests.

Currently, we have to define the route twice for these two methods. It would be good if we could have method: ['HEAD', 'GET'] as an option for handling multiple methods.

We could change the method option to string | string[], for example:

// route.ts
match &&
	(Array.isArray(route.method)
		? route.method.includes(request.method)
		: (route.method ?? 'GET') === request.method)
type Route = {
	pattern: URLPattern;
	method?: string | string[]; // default to `'GET'`
	handler: Handler
};

const exampleHandler: Handler = (requset) => {
	const body = runSomeLogic();
	return new Response(request.method === 'HEAD' ? null : body);
};

const route: Route = [
	{
		method: ['HEAD', 'GET'],
		handler: exmapleHandler,
	},
];

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http suggestion a suggestion yet to be agreed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants