disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
351 stars 72 forks source link

Add support for comma-separated headers in SimpleRestJson #1489

Open ghostbuster91 opened 7 months ago

ghostbuster91 commented 7 months ago

According to the RFC:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

It should be possible to send multiple values for a single header by passing them as comma separated list.

However, SimpleRestJson smithy4s treats all such values as a single text value.

repository to reproduce: https://github.com/ghostbuster91/demos/tree/multi-header

calling the endpoint should return {"weather":"2"} but it returns:

$ curl localhost:8080/weather -X POST -H "X-Foo: max-age=3600, must-revalidate"                                       
{"weather":"1"}%

smithy4s version: 0.18.15

Baccata commented 7 months ago

Thanks for the repro.

I'm not sure whether this should be a smithy4s issue or http4s issue though. @kubukoz, thoughts ?

kubukoz commented 7 months ago

Smells like http4s...

kubukoz commented 7 months ago

ugh I was distracted when submitting the comment, should've elaborated a bit.

http4s appears to just have a single Raw header for an X-Foo like the above, so we'd have to apply extra parsing on it. This seems to me like something that belongs in http4s, because in virtually every other area it gives us solid pre-parsed, structured representations of HTTP messages.

Baccata commented 7 months ago

Let's open in http4s. I'm not opposed to fixing it here if they offer a rationale for why they shouldn't do it, but I'd rather we asked first

ghostbuster91 commented 7 months ago

I opened an issue in http4s