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

feat_req(http/routes): add a accept filter #5883

Open
lowlighter opened this issue Aug 31, 2024 · 7 comments
Open

feat_req(http/routes): add a accept filter #5883

lowlighter opened this issue Aug 31, 2024 · 7 comments

Comments

@lowlighter
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Current implementation is supporting http method discriminant probably covers most use-case, but there's also one more use-case which I feel like is pretty common is Accept header discriminant.

Sometimes, same route path may serve different content based on Accept headers (for example a nice page for users when text/html is present, and a json object for developers when application/json is passed.

Describe the solution you'd like

Example: sending different format based on requested accept types:

const routes: Route[] = [
  {
    pattern: new URLPattern({ pathname: "/api/foo" }),
    accept: ["application/xml"]
    handler: () => new Response("<foo>bar</foo>"),
  },
  {
    pattern: new URLPattern({ pathname: "/api/foo" }),
    accept: ["application/json"]
    handler: () => new Response('{"foo":{"bar"}}'),
  }
];

Behaviour could be:

  • If accept is not specified, keep current behaviour, ie: execute handler regardless of client header
  • If accept is specified, only execute handler if one of the requested types is matching the specified array

Which also means that it's possible to specify the same route multiple time with different accept headers, and possible specify one without if you want a default handler for this route

Describe alternatives you've considered

Not using route or handle in default handler but less elegant

@iuioiua
Copy link
Contributor

iuioiua commented Sep 2, 2024

AFAIK, quite a lot of other routers in the JS ecosystem just match the URL and method. accept header matching can be easily done within the matched route handler.

@Leokuma
Copy link

Leokuma commented Sep 23, 2024

This addition seemed obvious to me at first, but as pointed out, routers usually don't support this. Then I started to wonder why they don't, and I came up with the arguments below.

If we support accept, then probably for consistency we should also support basically every other header such as accept-encoding, content-language, content-range, cookie etc. Matching them is not trivial: it will often be domain-specific. For example, how would we match content-range? The user might also want to use some complex logic that would check different headers and conditions at the same time. The user can also create custom headers, and we would have to consider that to create the types.

Those things can become hard to express in terms of a router - and arguably they go beyond a router's purpose. A router is intended to handle routes - "paths", to put it in a simplistic way.

@lowlighter
Copy link
Contributor Author

If we support accept, then probably for consistency we should also support basically every other header such as accept-encoding, content-language, content-range, cookie etc. Matching them is not trivial: it will often be domain-specific. For example, how would we match content-range? The user might also want to use some complex logic that would check different headers and conditions at the same time. The user can also create custom headers, and we would have to consider that to create the types.

It wouldn't necessarly mean supporting all headers, I didn't explicitely mentioned it but I was actually targetting for content negotiation filtering, (which is kind of common, scoped but also standardized), but indeed it'd at least means thataccept-language and accept-encoding could be candidate if implemented.

Cookies, custom and others headers, etc. are not part officially part of content negotiation so would remain user's responsability if they want custom processing

But anyways as @iuioiua mentioned, you can achieve something somewhat similar with the following:

  {
    pattern: new URLPattern({ pathname: "/api/foo" }),
    handler: request => {
      if (accepts(request.headers, "application/xml")
        return new Response("<foo>bar</foo>")
      if (accepts(request.headers, "application/json")
        return new Response('{"foo":{"bar"}}')
      return new Response(null, {status: 406 })
    }
  },

But it does make the code a bit less readable if you have to duplicate the checks for each route.

Maybe the solution would be some kind of simple middleware system, but I don't know if it's possible to do something that stays generic and simple enough. After a certain point it may just be better to just use a fully fledged router

@Leokuma
Copy link

Leokuma commented Sep 23, 2024

I didn't know about Content Negotiation. That makes sense.

I guess the behavior for wildcards like accept-encoding: * would be that they only match exactly accept-encoding: *, not accept-encoding: gzip? So inside the accept-encoding: * route the dev can somehow forward the request to gzip, br or whatever. That sort of behavior resembles middlewares actually, as you mentioned.

If my route is accept-encoding: * and I receive a request without any accept-encoding header, then there's no match. Is it confusing? Would the route accept-encoding: * be expected to match any accept-encoding even if the header is not present at all?

Also, the request can have many encodings in no particular order: accept-encoding: deflate, gzip;q=1.0, *;q=0.5. If multiple routes match, will the router decide the route based on the value of q? What if there's no q?

This could work, but I think we should consider those "edge cases" in advance and explain the behavior in the docs so that it's clear for users 👍.

@lowlighter
Copy link
Contributor Author

I guess the behavior for wildcards like accept-encoding: * would be that they only match exactly accept-encoding: *, not accept-encoding: gzip? So inside the accept-encoding: * route the dev can somehow forward the request to gzip, br or whatever. That sort of behavior resembles middlewares actually, as you mentioned.

I'd say it'd match any, but it'll imply that users orders their routes from most specific to most generic, but this is already the case with routes anyway as they early break on first matching route:

for (const route of routes) {
const match = route.pattern.exec(request.url);

But tbh Accept-Encoding seems pretty low level and I think that Accept and Accept-Language would be the most used

If my route is accept-encoding: * and I receive a request without any accept-encoding header, then there's no match.

Hum it depends, some browser will automatically appends the wildcard, and it can be implicit when not specified

Also, the request can have many encodings in no particular order: accept-encoding: deflate, gzip;q=1.0, *;q=0.5. If multiple routes match, will the router decide the route based on the value of q? What if there's no q?

The parsing is already implemented in std/http/negotiation so it'd mostly get back to the original post idea where you pass an array of possible values, and it matches depending on what the accepts*() functions returns, which is already the preferred requested type by the client

@Leokuma
Copy link

Leokuma commented Sep 24, 2024

Thanks for the clarification.

I just thought of another concern: the fact that we have to replicate the route to be able to handle a different header value. Could it end up growing exponentially? If our API can return XML and JSON in English and French, then I guess we would need to have each route replicated 4 times, one for each combination of accept and accept-language. If you want to change the path of one route, you would need to change it in 4 places:

const routes: Route[] = [
  {
    pattern: new URLPattern({ pathname: "/api/foo" }),
    accept: ["application/xml"],
    acceptLanguage: ["fr"],
    handler: () => new Response("<foo>bonjour</foo>"),
  },
  {
    pattern: new URLPattern({ pathname: "/api/foo" }),
    accept: ["application/xml"],
    acceptLanguage: ["en"],
    handler: () => new Response("<foo>hello</foo>"),
  },
  {
    pattern: new URLPattern({ pathname: "/api/foo" }),
    accept: ["application/json"],
    acceptLanguage: ["fr"],
    handler: () => new Response('{"foo":{"bonjour"}}'),
  },
  {
    pattern: new URLPattern({ pathname: "/api/foo" }),
    accept: ["application/json"],
    acceptLanguage: ["en"],
    handler: () => new Response('{"foo":{"hello"}}'),
  }
];

And if you want to respond in French by default in case the request doesn't specify a language, you'd have to duplicate the routes once more without acceptLanguage - or maybe allow an empty string in the array in order to make it default, like acceptLanguage: ["", "fr"].

Now I'm inclined towards a middleware approach too, but as you said, it's hard to make it generic. There was an initiative before but it was canceled I think.

@iuioiua
Copy link
Contributor

iuioiua commented Sep 24, 2024

Now I'm inclined towards a middleware approach too, but as you said, it's hard to make it generic. There was an initiative before but it was canceled I think.

#1295

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

Successfully merging a pull request may close this issue.

3 participants