Allow multiple SetCookie headers in response

[Kamirus]

The current implementation does not allow setting more than one cookie. Only one SetCookie header is allowed in response.

Is this behaviour intended? Or am I missing something?

[cprussin]

Hmm yeah, it does look like that's a limitation. You could combine the Set-Cookies into a single header (http://tools.ietf.org/html/rfc2109), although I'm not sure if that syntax is accepted by enough user agents for your use case.

Probably we should change the Headers newtype to map to an array of strings instead of a single string: https://github.com/cprussin/purescript-httpure/blob/master/src/HTTPure/Headers.purs#L28. PRs welcome if you have the time!

[paluh]

Probably we should change the Headers newtype to map to an array of strings instead of a single string: https://github.com/cprussin/purescript-httpure/blob/master/src/HTTPure/Headers.purs#L28. PRs welcome if you have the time!

This seems like the best approach I think. The PS node binding API is prepared for this:

https://github.com/purescript-node/purescript-node-http/blob/v5.0.2/src/Node/HTTP.purs#L94

[paluh]

The question remains if we really want to use Map here. Foreign.Object could be more consistent with the underling nodejs API:

https://nodejs.org/api/http.html#http_response_getheaders

I'm not sure how nodejs and js agnostic this lib tries to be... but from my perspective it would be cool if we could stay in this one realm and cover it well.

[paluh]

Regarding Foreign.Object we have efficient, single allocation way to build them in PS:

headers = Object.fromHomogeneous
  { location: ["/login"]
  , "set-cookie": ["sessionid=klkadfa.lkjl"]
  }

There is also a possiblity to use a fast builder in such a scenario.

[paluh]

P.S. I've just found this: https://github.com/cprussin/purescript-httpure/issues/113 so probably Map to Object switch won't happen or... we can still discuss what are the main design goals for this library.

[paluh]

@cprussin

I'm not sure how we should really type headers as we have somewhat strange situation where request headers are String values with a single exception:

https://nodejs.org/api/http.html#http_message_headers

On the other hand response header can be easily defined multiple times:

https://nodejs.org/api/http.html#http_response_setheader_name_value

This leads to somewhat inconvenient situation because in general headers should be represented as Map String (NonEmtpyArray String) (or Object) but we don't want and won't get this kind of representation for nearly all request headers...

Do you think that we should the value to NonEmtpyArray String and provide more convenient API on the top like: getHeader :: String -> String -> String which flattens things out?

[paluh]

Additionally it seems that PS bindings don't really care about this special case and provide flat String based API for request headers:

requestHeaders :: Request -> Object String
requestHeaders = _.headers <<< unsafeCoerce

Taking this into account maybe in the case of our HTTPure.Request there is no value in wrapping everything in Map and every single value in a NonEmptyArray just to preserve consistency of the Headers type between request and response (because these types are clearly inconsistent). Maybe in the case of a Request we can leave this original Object underneath and provide a trivial interface on top of it like this getHeader below:

type Request =
  { method :: Method.Method
  , path :: Path.Path
  , query :: Query.Query
  ...
  , getHeader :: String → Maybe String
  }

[paluh]

In general single Set-Cookie limitation is a huge blocker in a real life and as pointed here setting multiple values for set-cookie values in a single header is not recommended:

This is also what the latest RFC 6265 reflects:

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into a single header field. The usual mechanism for folding HTTP headers fields (i.e., as defined in [RFC2616]) might change the semantics of the Set-Cookie header field because the %x2C (",") character is used by Set-Cookie in a way that conflicts with such folding.

[cprussin]

Yeah, we should definitely come up with a fix here.

I don't love the idea of replacing headers on the Request with getHeader, since there are totally valid use cases for looking at the headers as an entire set. My proposal would be to stop using message.headers from Node and instead use message.rawHeaders: https://nodejs.org/api/http.html#http_message_rawheaders. Then we could easily normalize on Map String (NonEmtpyArray String), and not do the magic duplicate handling that Node has under the hood (we could instead provide helper functions for known headers like Age, Authorization, Set-Cookie, etc if the API feels clunky without them). This way the request & response header types can remain symmetrical and IMO behave with less magic.

That said, I also don't feel completely compelled to continue using the same types for Request and Response everywhere (i.e. we could just make RequestHeaders and ResponseHeaders that are not identical).

[cprussin]

Also, with regards to using Object: my preference is not to. Ideally I'd like the HTTPure API to be platform-agnostic--it's a stretch but it would be great to eventually be able to support running HTTPure on other purescript backends besides just node. That was most of my thinking behind preferring Map over Object.

Additionally, although probably weaker reasoning, I just think the Map type is much more pleasant to work with.

[paluh]

A the beginning I want to emphasis that I really appreciate your work and I'm quite happy HTTPure user :-)

Let me summarize how it looks from my limited perspective (Of course I have mainly our use cases in mind where we want to gain from the nodejs performance as much as we can using PS):

• If we use Object as a representation of Headers we don't need any additional type in the case of a request headers and can stick to this String value which is really correct from the RFC perspective. I think that we can concatenate this pathological case of Set-Cookie header in the request if it would be ever present.

• I think that using message.rawHeaders and reimplementing pairing and building a Map which adds additional overhead (map + Array + Tuple + Map + NonEmptyArray ) per request because there is one pathological case or because we don't want to use preexisting API with Object is rather bad deal.

• l agree that having foldable representation for headers is possibly much nicer than an interface wrapper. I just wanted to propose something which is lightweight but provides and interface which mimics preexisting "lookup" API. I think that it is also the main use case for the headers. The proposed interface is also backend agnostic. To be honest I don't really like this proposition too...

• Working with Object in the test environment has readability benefits because it is really easy and cheap to build it at once (single and safe unsafeCoerce under the hood):

Object.fromHomogeneous
 { cookies: 'x=y'
 , 'content-length': '8'
 , 'user-Agent': 'Mozilla/5.0'
 , host: 'example.com'  
 }

• It is somewhat inconsistent that this library binds directly to the node and has modules which are strictly node related and usage of the native Object is a problem in the API.

I'm really afraid that staying somewhere in between nodejs and assumption that this lib is JS agnostic we loose the power to provide lightweight, convenient and well typed APIs around nodejs. That is sad from my perspective. It is like avoiding success at all cost and forcing nodejs user to take purescript-express path instead. I think that if you really want to keep this library nodejs agnostic you should state that clearly in the project objectives. I think it would be reasonable to exercise this assumption by providing a core which has nothing (!) to do with nodejs. Then check what lefts and judge if it is worth it. Then maybe build a concrete implementation for nodejs and some separate backend. In other ways we are loosing the power but I don't think that we are gaining any real portability.

Sorry, but that is how I look at these things as a user which cares about performance of the base layer of HTTP apps.

[cprussin]

I'd like to separate discussions around ergonomics from those around performance. I don't trust theories around what should be more or less performant without actual measurements, and I think to be able to select what's worth benchmarking we need to start from the API we'd ideally want, assuming we can make it fast enough to be acceptable (for instance, I'm very dubious to make decisions based on the fear that rawHeaders comes with any significant measurable overhead without actually measuring that implementation). So I'm going to ignore the performance bit for now with the intention on re-evaluating with benchmarks once we're settled on what the ideal ergonomic API is.

My opinion has always been that the Map api is more ergonomic than the Object one, but I'm having trouble following your comment to see if that's true for you as well. Do you prefer Object purely for performance reasons, or is there an actual ergonomic preference as well?

To be clear, I'm not saying that being platform-agnostic is a definitive goal or necessesity. It's more of a nice to have--all else being equal I'd prefer to lean towards public HTTPure APIs that aren't specific to Node (this also comes from an ergonomic preference--in general I strongly prefer working with standard FP types and not Node types). Portability is a nice idea but certainly not an immediate goal--the underlying goal is to have a very ergonomic and pleasant API.

[cprussin]

To be clear: I am very willing to sacrifice some amount (the exact amount to be defied) of performance for the sake of more ergonomic and clear APIs (if your goal is absolute maximal performance then the only way is to bind directly to the Node APIs anyways, anything that wraps those APIs comes with added overhead and presumably the only reason to pay that overhead is in order to expose more pleasant APIs).

[cprussin]

One more note, based on these last two comments, where I'd like to start is:

1. Some idea of acceptable performance (comparative benchmarks against other frameworks maybe???)

2. Some idea of what we believe is the most pleasant API to interact with here

And then we can see if #2 can fit into #1. If not, we can iterate.

[paluh]

Some idea of acceptable performance (comparative benchmarks against other frameworks maybe???)

I'm afraid that I don't like this approach that in general we shouldn't care about CPU cycles per request till we introduce something which is so bad that it is reflected in the benchmarks. Do we really have so many spare CPU cycles when using PS + nodejs? Folding Array, building Tuples and wrapping values in NonEmptyArray to build this final Map has clearly allocation and CPU overhead per every request. There is also additional overhead for every lookup which is constant in the case of an Object and logarithmic in the case of a Map with some additional constant factor for sure (how large?). It is also interesting to see how nodejs developers care about performance when building this headers Object.

Some idea of what we believe is the most pleasant API to interact with here

Could you please tell me what is the difference in the API of the Map String String vs Object String which makes the difference for you? We are not going to modify this Map so we don't really benefit from the fact that Map is designed to be immutable structure. On the other hand, simple debug print (read traceM) or a construction of an Object during testing (like I've pointed above) are much more pleasant when we compare them to the Map counterparts.

My proposition is to use existing purescript-node getHeaders. It means: no additional code on our side, no testing and zero additional overhead per request and possibly a quick PR and merge.

[cprussin]

I didn't at all say that we should ignore performance. I said that making tradeoffs that sacrifice the ergonomics of the pbublic API to support assumed theoretical performance benefits with no benchmarks and no concept of acceptable performance is not useful. For one, without actual measurements, claims about theoretical performance are dubious at best. For two, if you only care to absolutely optimize theoretical performance at the cost of the niceness of the API, then Node itself is absolutely not an acceptable tradeoff, regardless of what frameworks you use or do not use.

It is also interesting to see how nodejs developers care about performance when building this headers Object.

There are very important things to note about the thread you linked:

1. This was optimization work applied after selecting (what they thought was) an ergonomic public API--not a public API selected around theoretical performance benefits.
2. This thread comes with benchmarks (and in fact was closed without merging due to the benchmarks not working out as expected).

Let me be clear: theoretical pitches of performance are not useful. If you want to talk performance, I want to see benchmarks, and I want to come up with an acceptable bar for performance (and ideally I want to talk performance in terms of optimizing the public API that was selected to maximize ergonomics).

Could you please tell me what is the difference in the API of the Map String String vs Object String which makes the difference for you? We are not going to modify this Map so we don't really benefit from the fact that Map is designed to be immutable structure.

This is a far more interesting (and concrete) point to me. I'd like to focus on this point and see if we can align here.

With request headers, you are right that there aren't going to be a lot of mutations. But there absolutely will be with response headers. So as long as we are using the same types for request headers and response headers, we do need to cater to a nice API for mutations.

Additionally, I think the semantics around Map String (NonEmptyArray String) are significantly nicer than having to deal with the special cases in Node--for instance, that multiple incoming Cookies get magically turned into a single string separated by semicolons, and that some other subset of cookies will drop any duplicate values entirely is not an API I've ever found ergonomic and I don't feel compelled to propagate that API in HTTPure.

I can buy the argument about cleaner semantics of building objects, the Map apis here are a bit clunky...

[cprussin]

I'm curious to look into what http frameworks in Haskell do for header types too, IMO that's a better example to follow than the low-level Node bindings.

[cprussin]

This thread also brings up another interesting point, which is that currently HTTPure doesn't give you any escape hatches to resort back to low-level bindings for custom use cases or opinions that diverge from what the library does. Should we?

[cprussin]

@paluh and I spoke offline to come up with a solution. He's going to put in a PR for the following changes:

1. We're going to split up the RequestHeaders and ResponseHeaders types.
2. RequestHeaders will be a newtype around Object and will be populated by using the purescript-node-http getHeaders binding.
3. ResponseHeaders will become a Map CaseInsensitiveString (NonEmptyArray String).

We're going to leave it in PR and @paluh will work on his app code against the PR branch for a few days to see if there are any pain points introduced by that API that we aren't aware of. Assuming that investigation goes well, we'll merge the PR a few days after he opens it.

@paluh please drop in a message if there's anything I'm forgetting here.

[paluh]

It seems fine. Thanks!

[davezuch]

Was any progress ever made on this?

[cprussin]

@davezuch Unfortunately no, PRs welcome but in the meantime I haven't had a chance to work on this yet.