Require the response headers be an unfrozen hash in SPEC

[ioquatix]

This is stricter than what was previously required. However,
non-hash response headers would break most of the middleware
that accesses response headers.

Middleware in many cases adds or removes headers, so require
the hash not be frozen, so that this can be done efficiently.

Fixes #1222

[ioquatix]

I'm happy with this as a starting point for #1592

[ioquatix]

@bryanmacfarlane sorry to ping you directly.... but I just don't understand why this is going wrong. We have a workflow set up for push and pull_request, and now we seem to get 2x the jobs executing, i.e. for both push and pull_request. What are we doing wrong here? Thanks and sorry to bother you.

[ioquatix]

These changes ensure that if middleware uses Rack::Response for manipulating headers, that it will work as expected, even if the header case is not lower case. But should we also issue a warning?

e.g.

set_header('Foo', 'bar')
Header key "Foo" is not lower case, this behaviour will be deprecated.

[mpalmer]

If you only state that "headers must be a hash", then adding another value to the hash when there's already a value there is... fraught with peril. If "headers must be a hash" is where SPEC goes, I'd suggest going a bit further and saying "headers must be a Hash.new { |h, k| h[k] = [] }" (and "all values must be arrays of strings"). Then set_header is []= and add_header is <<.

[ioquatix]

@mpalmer I actually agree with you but I received a lot of push back on this point. #1598

However, I still believe it's the right approach to use Array, at least in cases like this.

[mpalmer]

There's a lot going on in that linked issue, and you're right that there's some opinion against the proposal you made there. However, it's discussing a slightly different point to what we're talking about in this issue. Over in #1597, you're proposing that arrays be allowed header values, but not requiring them, within the context of not otherwise changing the format of headers in SPEC. I agree with Jeremy, et al, that just making that change isn't worthwhile -- it's just complicating all code that has to handle headers, without really providing any benefit.

This issue, though, is about mandating a more restrictive interface for headers, at which case it's worth doing the job properly and embracing the array of values. I don't think you can sanely expect everything that interacts with headers, a hash in which some keys could legitimately have multiple values -- and for which the "join" semantics differ between keys -- to implement the correct "join with a comma, except if you don't, but only if there's already a value" logic.

Yes, everyone could just use HeaderHash, but if you specify an interface that essentially requires HeaderHash, without mandating that HeaderHash is necessary, you're de facto mandating using HeaderHash anyway, at which point you may as well just say "you must use HeaderHash" and be done with it. But I agree with what Bo said, and I too would prefer it if the Rack specification didn't require me to use the rack gem, so sticking to simple data structures with well-described semantics for usage would be my preference. But, then again, my ultimate preference would be to use "arrays of two-element arrays" for headers, so perhaps my judgment is severely suspect.

[ioquatix]

I've reverted this back to the original commit which was co-authored by @jeremyevans and I believe we agree on, so I'll push forward with this in the first instance.