Bring Rack::Mock cookie parsing into SPEC (fixes #1629)

[mpalmer]

#has_key? isn't required to be supported by the object in the
headers element of the response coming back out of the app.
This replaces that call with a transmogrification into a
HeaderHash so we can use an index as normal. The odd
hash[] || "" construction is necessary because HeaderHash#fetch
doesn't respect header name case insensitivity.

[mpalmer]

I'm inclined to think that the circleci error is more that someone hasn't finished configuring that job, rather than the code being broken.

[ioquatix]

@tenderlove can you please disable CircleCI it's irrelevant now.

[ioquatix]

HeaderHash#fetch in theory is an internal API.

I am happy for you to include in this PR any changes you think make sense regarding that interface in terms of making the main change more straight forward.

Maybe HeaderHash#each_value(header_key) which yields items from the array or tries to split it into an array would make sense. I'm open to ideas.

[ioquatix]

I see, the behaviour here is a bit... messed up.

[mpalmer]

Yes, HeaderHash is a bit... under-specified, compared to the reality of headers and their oddities, as I notice has been discussed at some length. I am in violent agreement that "considering HTTP headers as general metadata is generally a mistake, in my experience. There are too many things which have specific semantic behaviour which requires specific things to handle."

I'm a bit down on HeaderHash overall, after discovering it doesn't handle [["set-cookie", "foo=bar"], ["Set-Cookie", "baz=wombat"]] correctly (though that's something for another PR). Looking at its history, the joining of arrays by newlines has always been on a bit of an "as-needed" basis, and I'm loathe to change the behaviour in #fetch, because it appears to be used heavily and garbage collection, etc.

I've just pushed a new version of the PR, with an extra commit to make HeaderHash#fetch case-insensitive. I also tried a different approach to handling "could be a string, could be an array of strings, who knows?" in the cookie parsing. It's at least differently ugly this time. Let me know which approach you prefer; I'm equally unhappy with both of them.

[lukaso]

If you want to work around the case insensitivity error seen here plus a strange base64 encoded error is saw in a cookie, add this to your capybara configuration file:

module Rack
  module Utils
    # Makes fetch case insensitive
    class HeaderHash
      def fetch(*args)
        patch_bug(super(args.first))
      rescue KeyError
        patch_bug(super(@names[args.first.downcase], *args[1..-1]))
      end

      private

      # this cookie from cloudflare with base64 encoded value that ends in
      # '=', kills cookie handling
      # Not patch received for this part yet.
      # '__cf_bm=somebase64encodedstringwithequalsatthened='
      def patch_bug(string)
        string.gsub(/=; /, '= ; ')
      end
    end
  end
end

The fix to the second error is to be added to this line:

https://github.com/rack/rack/blob/master/lib/rack/mock.rb#L260

Where attribute_value.strip but attribute_value will be nil.

[tenderlove]

I've been trying to figure out how to remove Circle for the past 20 min. I am going to stop now. I'll try again later. If anyone has ideas how to remove it I'm all ears. I couldn't find any settings in the Circle UI, and I don't see any integrations in GitHub settings so I'm kind of at a loss right now.

[ioquatix]

@tenderlove that's weird!? I took a look and can't see anything either. Maybe a webhook? Or some other integration?

[olleolleolle]

@tenderlove Hope this helps:

Settings "Branches" - enter the master branch settings.

There will be named checkboxes there.

[tenderlove]

@olleolleolle is that on the circle ci side? Because I don't see that on the GitHub side. Here is the GitHub side:

CircleCI seems to think that rack/rack is untested:

There's nothing in Integrations:

Nothing in Webhooks:

Nothing in the org level web hooks:

I have no idea why this is happening.

[olleolleolle]

The name of the heading is "Branch Protection Rules".

The master branch has a button Edit which opens a list of "required CI jobs that must pass" - extremely unintuitive. @tenderlove I understand that you missed it. Thanks for the thorough look.

[tenderlove]

@olleolleolle I actually clicked that edit button once and didn't see anything relevant so I didn't post a screenshot. Here it is though:

FWIW, newer pull requests aren't running the CircleCI build afaict. For example this one.

[tenderlove]

For some reason when I try to comment inline on the code I just get errors from GitHub, so I will write in long form here.

I think Hash#fetch can take a block, so we need to support that too.

I noticed this implementation rescues a KeyError rather than checking key?. Do we expect fetch to typically succeed? I'd rather not rescue exceptions as part of our logic, but I could understand for performance (though I'm not sure HeaderHash#fetch is a hotspot).

Finally I think we can use Array#drop to eliminate range allocation.