From 6216a092ccfe6422f113db906a52fe8ffdafdbe6 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Fri, 26 Feb 2016 03:06:38 +1030 Subject: Revert "Update Session to utilize indiffernt access" This reverts commit 45a75a3fcc96b22954caf69be2df4e302b134d7a. HWIAs are better than silently deeply-stringified hashes... but that's a reaction to a shortcoming of one particular session store: we should not break the basic behaviour of other, more featureful, session stores in the process. Fixes #23884 --- actionpack/lib/action_controller/test_case.rb | 2 +- actionpack/lib/action_dispatch/request/session.rb | 22 +++++++++----- actionpack/test/dispatch/request/session_test.rb | 2 +- .../test/dispatch/session/abstract_store_test.rb | 16 ---------- .../test/dispatch/session/cache_store_test.rb | 29 ------------------ .../test/dispatch/session/cookie_store_test.rb | 34 ---------------------- .../test/dispatch/session/mem_cache_store_test.rb | 31 -------------------- .../test/dispatch/session/test_session_test.rb | 7 ----- 8 files changed, 16 insertions(+), 127 deletions(-) diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb index 6548ce326b..700317614f 100644 --- a/actionpack/lib/action_controller/test_case.rb +++ b/actionpack/lib/action_controller/test_case.rb @@ -176,7 +176,7 @@ module ActionController def initialize(session = {}) super(nil, nil) @id = SecureRandom.hex(16) - @data = session.with_indifferent_access + @data = stringify_keys(session) @loaded = true end diff --git a/actionpack/lib/action_dispatch/request/session.rb b/actionpack/lib/action_dispatch/request/session.rb index 38d0da3e67..f6428af636 100644 --- a/actionpack/lib/action_dispatch/request/session.rb +++ b/actionpack/lib/action_dispatch/request/session.rb @@ -88,13 +88,13 @@ module ActionDispatch # nil if the given key is not found in the session. def [](key) load_for_read! - @delegate[key] + @delegate[key.to_s] end # Returns true if the session has the given key or false. def has_key?(key) load_for_read! - @delegate.key?(key) + @delegate.key?(key.to_s) end alias :key? :has_key? alias :include? :has_key? @@ -112,7 +112,7 @@ module ActionDispatch # Writes given value to given key of the session. def []=(key, value) load_for_write! - @delegate[key] = value + @delegate[key.to_s] = value end # Clears the session. @@ -139,13 +139,13 @@ module ActionDispatch # # => {"session_id"=>"e29b9ea315edf98aad94cc78c34cc9b2", "foo" => "bar"} def update(hash) load_for_write! - @delegate.update hash + @delegate.update stringify_keys(hash) end # Deletes given key from the session. def delete(key) load_for_write! - @delegate.delete key + @delegate.delete key.to_s end # Returns value of the given key from the session, or raises +KeyError+ @@ -165,9 +165,9 @@ module ActionDispatch def fetch(key, default=Unspecified, &block) load_for_read! if default == Unspecified - @delegate.fetch(key, &block) + @delegate.fetch(key.to_s, &block) else - @delegate.fetch(key, default, &block) + @delegate.fetch(key.to_s, default, &block) end end @@ -211,9 +211,15 @@ module ActionDispatch def load! id, session = @by.load_session @req options[:id] = id - @delegate.replace(session) + @delegate.replace(stringify_keys(session)) @loaded = true end + + def stringify_keys(other) + other.each_with_object({}) { |(key, value), hash| + hash[key.to_s] = value + } + end end end end diff --git a/actionpack/test/dispatch/request/session_test.rb b/actionpack/test/dispatch/request/session_test.rb index 3433d82791..3fc4ffd71c 100644 --- a/actionpack/test/dispatch/request/session_test.rb +++ b/actionpack/test/dispatch/request/session_test.rb @@ -105,7 +105,7 @@ module ActionDispatch end end - def test_indifferent_access + def test_with_indifferent_access s = Session.create(store, req, {}) s[:one] = { test: "deep" } diff --git a/actionpack/test/dispatch/session/abstract_store_test.rb b/actionpack/test/dispatch/session/abstract_store_test.rb index c9ce5cad42..d38d1bbce6 100644 --- a/actionpack/test/dispatch/session/abstract_store_test.rb +++ b/actionpack/test/dispatch/session/abstract_store_test.rb @@ -46,22 +46,6 @@ module ActionDispatch assert_equal session.to_hash, session1.to_hash end - def test_previous_session_has_indifferent_access - env = {} - as = MemoryStore.new app - as.call(env) - - assert @env - session = Request::Session.find ActionDispatch::Request.new @env - session[:foo] = { bar: "baz" } - - as.call(@env) - session = Request::Session.find ActionDispatch::Request.new @env - - assert_equal session[:foo][:bar], "baz" - assert_equal session[:foo]["bar"], "baz" - end - private def app(&block) @env = nil diff --git a/actionpack/test/dispatch/session/cache_store_test.rb b/actionpack/test/dispatch/session/cache_store_test.rb index b911392cf1..dbb996973d 100644 --- a/actionpack/test/dispatch/session/cache_store_test.rb +++ b/actionpack/test/dispatch/session/cache_store_test.rb @@ -12,11 +12,6 @@ class CacheStoreTest < ActionDispatch::IntegrationTest head :ok end - def set_deep_session_value - session[:foo] = { bar: "baz" } - head :ok - end - def set_serialized_session_value session[:foo] = SessionAutoloadTest::Foo.new head :ok @@ -26,14 +21,6 @@ class CacheStoreTest < ActionDispatch::IntegrationTest render plain: "foo: #{session[:foo].inspect}" end - def get_deep_session_value_with_symbol - render plain: "foo: { bar: #{session[:foo][:bar].inspect} }" - end - - def get_deep_session_value_with_string - render plain: "foo: { \"bar\" => #{session[:foo]["bar"].inspect} }" - end - def get_session_id render plain: "#{request.session.id}" end @@ -173,22 +160,6 @@ class CacheStoreTest < ActionDispatch::IntegrationTest end end - def test_previous_session_has_indifferent_access - with_test_route_set do - get '/set_deep_session_value' - assert_response :success - assert cookies['_session_id'] - - get '/get_deep_session_value_with_symbol' - assert_response :success - assert_equal 'foo: { bar: "baz" }', response.body - - get '/get_deep_session_value_with_string' - assert_response :success - assert_equal 'foo: { "bar" => "baz" }', response.body - end - end - private def with_test_route_set with_routing do |set| diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb index 71402b021a..f07e215e3a 100644 --- a/actionpack/test/dispatch/session/cookie_store_test.rb +++ b/actionpack/test/dispatch/session/cookie_store_test.rb @@ -24,23 +24,10 @@ class CookieStoreTest < ActionDispatch::IntegrationTest render plain: Rack::Utils.escape(Verifier.generate(session.to_hash)) end - def set_deep_session_value - session[:foo] = { bar: "baz" } - render plain: Rack::Utils.escape(Verifier.generate(session.to_hash)) - end - def get_session_value render plain: "foo: #{session[:foo].inspect}" end - def get_deep_session_value_with_symbol - render plain: "foo: { bar: #{session[:foo][:bar].inspect} }" - end - - def get_deep_session_value_with_string - render plain: "foo: { \"bar\" => #{session[:foo]["bar"].inspect} }" - end - def get_session_id render plain: "id: #{request.session.id}" end @@ -94,15 +81,6 @@ class CookieStoreTest < ActionDispatch::IntegrationTest end end - def test_session_indifferent_access - with_test_route_set do - cookies[SessionKey] = SignedBar - get '/get_session_value' - assert_response :success - assert_equal 'foo: "bar"', response.body - end - end - def test_getting_session_id with_test_route_set do cookies[SessionKey] = SignedBar @@ -354,18 +332,6 @@ class CookieStoreTest < ActionDispatch::IntegrationTest end end - def test_previous_session_has_indifferent_access - with_test_route_set do - get '/set_deep_session_value' - - get '/get_deep_session_value_with_symbol' - assert_equal 'foo: { bar: "baz" }', response.body - - get '/get_deep_session_value_with_string' - assert_equal 'foo: { "bar" => "baz" }', response.body - end - end - private # Overwrite get to send SessionSecret in env hash diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb index 2e6b42856f..3fed9bad4f 100644 --- a/actionpack/test/dispatch/session/mem_cache_store_test.rb +++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb @@ -13,11 +13,6 @@ class MemCacheStoreTest < ActionDispatch::IntegrationTest head :ok end - def set_deep_session_value - session[:foo] = { bar: "baz" } - head :ok - end - def set_serialized_session_value session[:foo] = SessionAutoloadTest::Foo.new head :ok @@ -27,14 +22,6 @@ class MemCacheStoreTest < ActionDispatch::IntegrationTest render plain: "foo: #{session[:foo].inspect}" end - def get_deep_session_value_with_symbol - render plain: "foo: { bar: #{session[:foo][:bar].inspect} }" - end - - def get_deep_session_value_with_string - render plain: "foo: { \"bar\" => #{session[:foo]["bar"].inspect} }" - end - def get_session_id render plain: "#{request.session.id}" end @@ -192,24 +179,6 @@ class MemCacheStoreTest < ActionDispatch::IntegrationTest rescue Dalli::RingError => ex skip ex.message, ex.backtrace end - - def test_previous_session_has_indifferent_access - with_test_route_set do - get '/set_deep_session_value' - assert_response :success - assert cookies['_session_id'] - - get '/get_deep_session_value_with_symbol' - assert_response :success - assert_equal 'foo: { bar: "baz" }', response.body - - get '/get_deep_session_value_with_string' - assert_response :success - assert_equal 'foo: { "bar" => "baz" }', response.body - end - rescue Dalli::RingError => ex - skip ex.message, ex.backtrace - end rescue LoadError, RuntimeError, Dalli::DalliError $stderr.puts "Skipping MemCacheStoreTest tests. Start memcached and try again." end diff --git a/actionpack/test/dispatch/session/test_session_test.rb b/actionpack/test/dispatch/session/test_session_test.rb index 332c2ae3c8..3e61d123e3 100644 --- a/actionpack/test/dispatch/session/test_session_test.rb +++ b/actionpack/test/dispatch/session/test_session_test.rb @@ -60,11 +60,4 @@ class ActionController::TestSessionTest < ActiveSupport::TestCase session = ActionController::TestSession.new(one: '1') assert_equal(2, session.fetch('2') { |key| key.to_i }) end - - def test_fetch_returns_indifferent_access - session = ActionController::TestSession.new(three: { two: '1' }) - three = session.fetch(:three) - assert_equal('1', three[:two]) - assert_equal('1', three["two"]) - end end -- cgit v1.2.3