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

Multi-valued header support #28

Open
chrisheller opened this issue Mar 1, 2020 · 5 comments
Open

Multi-valued header support #28

chrisheller opened this issue Mar 1, 2020 · 5 comments

Comments

@chrisheller
Copy link
Contributor

The readHeaders procs in Rosencrantz are currently setup to work with single-valued headers, but don't handle multi-valued headers. This can be worked around by creating a copy of readHeaders that accepts HttpHeaderValues as the parameter type (instead of string).

proc readHeaders*(s1: string, p: proc(h1: HttpHeaderValues): Handler): Handler =
  proc h(req: ref Request, ctx: Context): Future[Context] {.async.} =
    if req.headers.hasKey(s1):
      let handler = p(req.headers[s1])
      let newCtx = await handler(req, ctx)
      return newCtx
    else:
      return ctx.reject()

  return h

In the readHeaders proc in Rosencrantz, the call works because req.headers[s1] (which is an HttpHeaderValues object) gets converted to a string by the toString() converter from the standard library's httpcore module. That converter just grabs the first value for the header though. That generally works since most headers are single-valued.

I'm not sure what would be the best way to add this now though. I tried changing the signature like this, but that wouldn't compile for some reason.

proc readHeaders*(s1: string, p: proc(h1: string|HttpHeaderValues): Handler): Handler =

Adding duplicate procs seems like the wrong way to deal with it though. Maybe a macro to handle duplication might work; I haven't tried that though.

@andreaferretti
Copy link
Owner

it could make sense to deprecate the string version and just use the HttpHeaderValues. When Rosencrantz was started, there was no such thing, and the stdlib httpserver only worked with non repeated headers. I will have a look in the next days

@andreaferretti
Copy link
Owner

While investigating this, I found a related issue in httpcore

@andreaferretti
Copy link
Owner

That said, I don't know what to do. The reason your approach does not work is that the or goes on the outside of the proc, not on the inside. That is, your version

proc readHeaders*(s1: string, p: proc(h1: string|HttpHeaderValues): Handler): Handler =

takes a p which accepts a string or an HttpHeaderValues, while the correct version is

proc readHeaders*(s1: string, p: (proc(h1: string): Handler) or (proc(h1: HttpHeaderValues): Handler)): Handler =

which takes either a p: string => Handler or a p: HttpHeaderValues => Handler.

Still, this does not work. I created a branch to track this - it compiles but the test fails. I am really not sure why - it may even be a bug in httpserver. I also tried the repeated version of the procs just to be sure, but it fails to read multiple headers anyway

@andreaferretti
Copy link
Owner

I submitted a PR to Nim to handle repeated values nim-lang/Nim#13575

@andreaferretti
Copy link
Owner

Ok, it turns out it was a misunderstanding on my part. Now the above procs work with repeated headers in the repeated-headers branch.

@chrisheller If you want, please have a look if this works for you. If so, I will document and merge this

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

No branches or pull requests

2 participants