FBCM-5157 Allow multiple SetCookie headers in response

[arthurxavierx]

We add the HTTPure.MultiHeaders module and use it in HTTPure.Request and HTTPure.Response for reading and writing headers with multiple values.

We chose this approach instead of adding support for duplicated headers to HTTPure.Headers in order to avoid a large breaking change.

Resolves #157.

[davezuch]

This looks great! I just have a few questions/comments below.

[davezuch]

Thanks for making those changes! This looks great, but I do think we should add something to docs/Examples before merging.

[arthurxavierx]

This looks great, but I do think we should add something to docs/Examples before merging.

Adding an example for MultiHeaders caught a bug: turns out that the request.headersDistinct-based implementation didn't work even with more recent node.js versions. I fixed it by using [request.rawHeaders] instead.

[davezuch]

Adding an example for MultiHeaders caught a bug: turns out that the request.headersDistinct-based implementation didn't work even with more recent node.js versions. I fixed it by using [request.rawHeaders] instead.

Ah, nice catch! So it looks like the new implementation will have the desired behavior when there are duplicate Set-Cookie headers, but now I'm concerned about the handling for certain headers, and the asymmetry now between HTTPure.Headers and HTTPure.MultiHeaders. Specifically, request.headers—which HTTPure.Headers is implemented in terms of—lists these behaviors:

• Duplicates of age, authorization, content-length, content-type, etag, expires, from, host, if-modified-since, if-unmodified-since, last-modified, location, max-forwards, proxy-authorization, referer, retry-after, server, or user-agent are discarded.

I think the behavior here makes sense. HTTPure.Headers discards duplicates, while HTTPure.MultiHeaders allows the user to decide what to do with them.

• set-cookie is always an array. Duplicates are added to the array.

For HTTPure.MultiHeaders we now have the desired behavior, but with HTTPure.Headers we will have a runtime TypeError if we try to access this, since it will be an array, not a string. Perhaps in the FFI we should have a special case here that joins these into a string?

• For duplicate cookie headers, the values are joined together with ; .

Seems like the HTTPure.Headers behavior is what we want, but what about HTTPure.MultiHeaders? Will these ever come as a single header string that we need to parse—splitting at ;? Even then, the user still has to parse key value pairs from the string. Would it make sense to go one step further and provide an HTTPure.Cookies module that gives access to a fully parsed Map of cookies?

• For all other headers, the values are joined together with , .

I think the behavior here is fine on both ends.

[arthurxavierx]

Even then, the user still has to parse key value pairs from the string. Would it make sense to go one step further and provide an HTTPure.Cookies module that gives access to a fully parsed Map of cookies?

I don't think that should be done in this PR.

For HTTPure.MultiHeaders we now have the desired behavior, but with HTTPure.Headers we will have a runtime TypeError if we try to access this, since it will be an array, not a string. Perhaps in the FFI we should have a special case here that joins these into a string?

We could just filter out set-cookies in HTTPure.Headers.read and specify in the docs that set-cookies headers should always be read from multiHeaders; what do you think?

[davezuch]

Even then, the user still has to parse key value pairs from the string. Would it make sense to go one step further and provide an HTTPure.Cookies module that gives access to a fully parsed Map of cookies?

I don't think that should be done in this PR.

Agreed. Created an issue (#198) for the idea.

For HTTPure.MultiHeaders we now have the desired behavior, but with HTTPure.Headers we will have a runtime TypeError if we try to access this, since it will be an array, not a string. Perhaps in the FFI we should have a special case here that joins these into a string?

We could just filter out set-cookies in HTTPure.Headers.read and specify in the docs that set-cookies headers should always be read from multiHeaders; what do you think?

That works as well.

[arthurxavierx]

@davezuch done

[davezuch]

This is great! I think it's good to go. I just want to point out the asymmetry of how the headers will be used on the request and response sides, to make sure we think it makes sense.

When creating a response, we're not going have duplicate headers in the HTTPure.Headers and HTTPure.MultiHeaders, we might construct something like:

response
  { headers =
      HTTPure.Headers.header "Host" "127.0.0.1:8000"
      <> HTTPure.Headers.header "Accept" "*/*"
  , multiHeaders =
      HTTPure.MultiHeaders.header "Cookie" "foo=bar"
      <> HTTPure.MultiHeaders.header "Cookie" "Authorization=\"Token XXX\""
  }

Meanwhile, when reading a request, we will have duplicate headers. The same headers in a request would look something like:

request
  { headers =
      HTTPure.Headers.header "Host" "127.0.0.1:8000"
      <> HTTPure.Headers.header "Accept" "*/*"
      <> HTTPure.Headers.header "Cookie" "foo=bar; Authorization=\"Token XXX\""
  , multiHeaders =
      HTTPure.MultiHeaders.header "Host" "127.0.0.1:8000"
      <> HTTPure.MultiHeaders.header "Accept" "*/*"
      <> HTTPure.MultiHeaders.header "Cookie" "foo=bar"
      <> HTTPure.MultiHeaders.header "Cookie" "Authorization=\"Token XXX\""
  }

That seem right?

[arthurxavierx]

Yes, that seems about right to me. The reason is that we want to avoid breaking changes (except for the show one). If we only had MultiHeaders in Request, then users would have to consume headers as Array String with lookup, as opposed to the current String.

[davezuch]

Yeah, I agree, I just wanted to make sure we're on the same page. As you mentioned, we will need to release this as a breaking change due to the change in behavior for show.

[arthurxavierx]

@davezuch Do you know what's the release process for this library? Looks like we should be running nix-shell --run generate-bower and nix-shell --run generate-docs before merging this?

[cprussin]

@arthurxavierx it's documented in https://github.com/citizennet/purescript-httpure/blob/main/Releasing.md

[cprussin]

Although it looks like some steps have changed now that the registry is a thing. Maybe see https://github.com/purescript-contrib/governance/blob/main/pursuit-preregistry.md and update the releasing guide accordingly? (probably just swap the link in step 4 to point to this page instead)?

[arthurxavierx]

Oh I tried searching the repo for a file like that but missed it. Thanks @cprussin!