Add Rack::Request#headers for getting access to HTTP headers

[jeremyevans]

In addition to being backwards compatible, I think this offers
a nicer API:

  request.headers['host']           # env['HTTP_HOST']
  request.headers['content-type']   # env['CONTENT_TYPE']
  request.headers['content-length'] # env['CONTENT_LENGTH']

  request.headers['allow'] = 'GET'     # env['HTTP_ALLOW'] = 'GET'
  request.headers.add('allow', 'POST') # env['HTTP_ALLOW'] #=> %w'GET POST'

  request.headers.has_key?('allow')
  request.headers.fetch('allow')
  request.headers.delete('allow')

  request.headers.each{|key, value| }
  request.headers.to_h

You can compare this to a backwards incompatible proposal to modify
the related *_header methods:

  # This proposal
  request.headers['host']

  # Backwards incompatible alternative
  request.get_header('host')

This only allocates potentially one Rack::Request::Headers object
per request, on the first call to Rack::Request#headers. I don't
think this will be a performance issue. Any code desiring maximum
performance should access env directly.

[ioquatix]

I personally like this kind of interface, but I'm not sure if this fixes the existing semantic issues with get/set_header.

[jeremyevans]

I personally like this kind of interface, but I'm not sure if this fixes the existing semantic issues with get/set_header.

It doesn't attempt to fix the semantic issues. We should document *_header methods as only for backwards compatibility, and recommend using headers instead. If this is accepted, I'll handle the documentation updates, and remove internal use of the *_header methods (Similar to your second commit in #1880).

This allows current code that uses the *_header methods to continue to work. New code can use the headers method for an even nicer interface than previously provided by *_headers. There should be no semantic issues with the headers method.

As long as you like this interface, I think it is a much better path forward than #1880.

[ioquatix]

How do we deprecate the existing methods without introducing the same backwards incompatible change? Basically this doesn't seem to solve the original problem.

I'm also interested in a solution where some hypothetical Rack::Request::Headers can internally multiplex around different env shapes. I think Rack::Request::Headers is insufficient for that purpose, but it's very close to being a good solution. Basically, it's too narrow. The problem that should be solved here is, "given an env, give me a structured interface".

By adopting this PR, it will be more tricky later on to hide this implementation detail. That's why I prefer direct methods on Rack::Request rather than a separate object/instance/interface.

For that reason, I'd like more time to think about the implications of this design and don't want to rush into it for Rack 3.0.

As I've outlined several times, my main concern is the semantic inconsistency between Request#*_headers and Response#*headers and this PR does not solve that problem.

[jeremyevans]

How do we deprecate the existing methods without introducing the same backwards incompatible change? Basically this doesn't seem to solve the original problem.

As I've stated, I don't think we should deprecate. The benefits are very small, and the costs are significant. It's better to just document the methods as existing only for backwards compatibility, and introduce this as a replacement.

I'm also interested in a solution where some hypothetical Rack::Request::Headers can internally multiplex around different env shapes. I think Rack::Request::Headers is insufficient for that purpose, but it's very close to being a good solution. Basically, it's too narrow. The problem that should be solved here is, "given an env, give me a structured interface".

Assuming Rack::Request can deal with different env shapes, this is trivial because #headers could return an instance of a different class with the same API, for that specific env shape. So that seems simple to address.

By adopting this PR, it will be more tricky later on to hide this implementation detail. That's why I prefer direct methods on Rack::Request rather than a separate object/instance/interface.

I disagree. The polymorphic approach described (instance of a separate class) would solve it very simply. Even if you didn't want to use that approach, any way that Rack::Request handled separate shapes inside the *_headers method could be handled by Rack::Request::Headers.

For that reason, I'd like more time to think about the implications of this design and don't want to rush into it for Rack 3.0.

That's fine, as long as you aren't pushing for any *_header method changes in Rack 3.0.

[ioquatix]

I'm fine punting both PRs for later in the Rack 3.x release cycle, but I'd like to hear input from others before making a final decision. I think introducing a new interface has a slightly higher burden of review than fixing an existing one.

I disagree. The polymorphic approach described (instance of a separate class) would solve it very simply. Even if you didn't want to use that approach, any way that Rack::Request handled separate shapes inside the *_headers method could be handled by Rack::Request::Headers.

I agree with you generally that the approach you've described is the right one, but I think we want to be a bit more nuanced with the actual interface and naming, since this is about wrapping an environment (of different shapes), not specifically about headers. I would prefer to consider introducing something like Rack::Environment which has methods for path, method, scheme, get_header, set_header, each_header and a few others. The scope of this interface is a big bigger than just "headers". I like the general idea, but I think we should be a little bit more ambitious and as such I think we should punt it to a later release rather than holding up a 3.0 release.

[jeremyevans]

I'm fine punting both PRs for later in the Rack 3.x release cycle, but I'd like to hear input from others before making a final decision. I think introducing a new interface has a slightly higher burden of review than fixing an existing one.

IMO, you are not fixing an existing interface. What you are proposing here is not a bug fix. You are implementing a new feature by changing the behavior of existing methods in a backwards incompatible manner. IMO, backwards incompatible changes should require a much higher burden of review than backwards compatible new features.

That being said, I agree with you that we should probably focus on Rack 3.0 and revisit these PRs after 3.0 is released.

[ahx]

I like the interface, because it talks like a Hash. I am currently working on a piece of code that picks certain headers from a request and I think this interface would help me a lot making unit tests and implementation easier to understand, because I can just write something like my_code(headers) where headers is request.headers or just a normal Hash in unit test.

[ioquatix]

Yeah, I too would like to see us fix this in 3.1 - I'm okay with this proposal, but I'd like to bounce it around a little more before making a final decision.