fukamachi / woo

A fast non-blocking HTTP server on top of libev
http://ultra.wikia.com/wiki/Woo_(kaiju)
MIT License
1.27k stars 96 forks source link

Make all header values be strings #84

Closed hjudt closed 4 years ago

hjudt commented 5 years ago

The content-length header in the hash-table is not a string as the other fields, when it should be. While you would certainly want to use the content-length as a number after being parsed, this can make parsing raw headers more difficult.

Example: https://github.com/fukamachi/woo/blob/master/src/woo.lisp#L328 You also expose content-length as a plist value: https://github.com/fukamachi/woo/blob/master/src/woo.lisp#L244 Maybe the latter could be changed to be the parsed value (therefore a number or nil), while the header remains untouched?

For a counter-example, hunchentoot exposes content-length as string.

I have not looked more intensely at the source code, but i believe content-length is the only header field that gets parsed.

fukamachi commented 4 years ago

What's the problem of it?

hjudt commented 4 years ago

I'll try to explain. It is actually more of a design issue than a problem. Headers are expected to be strings (why doesn't hunchentoot expose the value as a parsed number but leaves it a string and actually delegates it to the application to parse?).

I suppose it is not a problem for applications that build on your framework because they have been written to expect it to be a number, but it is for other frameworks/applications that expect the content-length header to be of type string. In that case, one needs to make a special case and treat woo differently than other servers which only return headers as string.

Edit: I also quickly looked up the definition at Wikipedia and it says,

Header fields are colon-separated key-value pairs in clear-text string format, terminated by a carriage return (CR) and line feed (LF) character sequence.

So actually this is what the server should return, because content-length too is simply a header field. The framework or application then could parse this content-length field. But woo treats it differently, returning the parsed value, thus doing what is actually the responsibility of an framework/application and not that of the server.

Shinmera commented 4 years ago

It's simply a problem of consistency. Why is the content-length entry literally the only header that isn't a string? Either parse all of the headers to their canonical representation -- which isn't feasible due to many custom and non-standard headers -- or none.

fukamachi commented 4 years ago

It is actually more of a design issue than a problem.

Now I got you two are pointing out.

The reason why Content-Length is special is Content-Length has to be parsed in fast-http in any case, so I allowed the parser to just return the value with kindness.

fukamachi commented 4 years ago

It seems that fast-http parses all number header values, not only Content-Length. 😕 https://github.com/fukamachi/fast-http/blob/master/src/fast-http.lisp#L137-L139

fukamachi commented 4 years ago

Merged.