Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accept two optional args in Rack::Session::Abstract::SessionHash#fetch #1024

Closed

Conversation

hfaulds
Copy link

@hfaulds hfaulds commented Mar 2, 2016

I got bitten by a bug in rails that lead me to this class. They inherit from Rack::Session::Abstract::SessionHash for some session hash classes relating to forgery protection (https://github.com/rails/rails/blob/e7feaff70f13b56a0507e9f4dfaf3ebc361cb8e6/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L135). Whilst in the normal session hash they have implemented fetch with two optional arguments (https://github.com/rails/rails/blob/e7feaff70f13b56a0507e9f4dfaf3ebc361cb8e6/actionpack/lib/action_dispatch/request/session.rb#L133-L140). They also have a separate session hash class for testing which implements the two optional args for fetch.

I had some shared code between controllers called session.fetch with the default argument, this caused exceptions for controllers that used forgery protection it also didn't exception in the tests for these controllers.

Whilst this is a bug in rails it seems like this class in rack was the right place to fix it and properly mimic the behaviour of Ruby's Hash.

I would have added another spec but it outputs the warning rack/lib/rack/session/abstract/id.rb:66: warning: block supersedes default value argument in the middle of the test suite. Given the current tests for this class it seemed better to just not include it than to hide the warning.

it "returns the correct value using a block and default" do
  assert_equal 'missing', hash.fetch(:missing, :default) { |el| el.to_s }
end

@ioquatix
Copy link
Member

It looks like this has been merged. Thanks for your contribution.

@ioquatix ioquatix closed this Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants