PATCH: Clean up the two ways to send the header fields.

12 views
Skip to first unread message

Isaac Z. Schlueter

unread,
Jan 24, 2010, 4:48:47 AM1/24/10
to nodejs
With this patch, you can use any of these methods to same multiple
header fields with the same field name:

[["key", "val"], ["key", "val2"]]
[["key", "val", "val2"]]
["key", ["val", "val2"]]
{"key" : ["val", "val2"]}

Previously, when using an object to send the header fields, you had to
do some crazy hack like this:
{ x : ["key", "val"], y : ["key", "val2"] }
which is just plain weird.

http://github.com/isaacs/node/commit/3439395cdd14f58e5c2441b0a68d89085d80b8bd

--i

Isaac Z. Schlueter

unread,
Jan 27, 2010, 4:31:39 PM1/27/10
to nodejs, r...@tinyclouds.org
I updated this so that it will stick to the JSGI spec a bit better,
and be consistent with request and response headers.

http://github.com/isaacs/node/commits/cleanup-header-object

--i

Isaac Schlueter

unread,
Jan 30, 2010, 8:57:52 PM1/30/10
to Felix Geisendörfer, Ryan Dahl, nod...@googlegroups.com
Hey, Felix.

It seems like the multipart library doesn't handle cases where there's
multiple instances of a single header field, causing my patch to break
the multipart test. I'm pretty sure it would have broken
(differently) using multiple headers with comma-separation, as well,
so maybe it's good to expose this now.

The issue is that you can have cases where a header is set multiple
times. It's kind of crazy when you do this for content-type or
content-encoding, but it happens, and in those cases, it should be a
"last one wins" if I understand the spec correctly.

My planned API is for the response to support either a string or an
array. The request header values are always set to an array, because
the benchmarks show it's faster if you can remove the isArray test.
So, I've updated it to use whatever the last item in the array is for
content-(type|length).

I was able to fix it so that it passes the test, but I'm concerned
because it looks like it's setting headers to strings in another area
without checking to see if they might actually be arrays. Would you
mind taking a look at this patch and let me know what you think?

http://gist.github.com/290838

Thanks!

--i


On Fri, Jan 29, 2010 at 18:26, <r...@tinyclouds.org> wrote:
> I just tried your patch
> http://github.com/isaacs/node/tree/jsgi-header-fast
> but it broke the multipart test. any ideas?

Jonas Pfenniger

unread,
Jan 31, 2010, 7:50:01 AM1/31/10
to nod...@googlegroups.com
IMO the best option is the first one. Programatically, you don't need
to know what is already in your headers for adding a new-one. Eventual
lookup is O(n), which is probably acceptable given the number of
headers. It also guarantees the headers order, which is nice if you
want to diff messages.

At a higher-level, you probably want a Headers object that correctly
prepend unspecified headers with "X-" and make sure "Set-Cookie"
headers are not concatenated (because of browser bugs).
Some headers may also be set differently, like the ones that have a
weight ratio.

> --
> You received this message because you are subscribed to the Google Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com.
> To unsubscribe from this group, send email to nodejs+un...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/nodejs?hl=en.
>
>

--
Jonas Pfenniger (zimbatm) <jo...@pfenniger.name>

Felix Geisendörfer

unread,
Jan 31, 2010, 9:51:16 AM1/31/10
to nodejs
Hi Isaacs,

so basically your patch is turning each req.headers[header] element
into an array rather than a string?

If that's the case, you're fix looks good. I also agree that this
should be handled for the headers found in the 'parts' themselves.

My only question is if its nice to always have to write:

req.headers['content-type'].slice(-1)[0];

IMHO the situation where a header field is set multiple times is the
edge case, so its kind of annoying to get punished for that when
expecting the regular case.

-- fg

Reply all
Reply to author
Forward
0 new messages