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

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