Merge pull request #20851 from tomprats/indifferent-sessions

Give Sessions Indifferent Access

actionpack/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Update session to have indifferent access across multiple requests
+
+        session[:deep][:hash] = "Magic"
+
+        session[:deep][:hash] == "Magic"
+        session[:deep]["hash"] == "Magic"
+
+    *Tom Prats*
+
 *   Add application/gzip as a default mime type.
 
     *Mehmet Emin İNAÇ*

actionpack/lib/action_controller/test_case.rb

@@ -176,7 +176,7 @@ class TestSession < Rack::Session::Abstract::SessionHash #:nodoc:
     def initialize(session = {})
       super(nil, nil)
       @id = SecureRandom.hex(16)
-      @data = stringify_keys(session)
+      @data = session.with_indifferent_access
       @loaded = true
     end
 

actionpack/lib/action_dispatch/request/session.rb

@@ -9,7 +9,7 @@ class Session # :nodoc:
 
       # Singleton object used to determine if an optional param wasn't specified
       Unspecified = Object.new
-      
+
       # Creates a session hash, merging the properties of the previous session if any
       def self.create(store, req, default_options)
         session_was = find req
@@ -61,7 +61,7 @@ def values_at(*args); @delegate.values_at(*args); end
       def initialize(by, req)
         @by       = by
         @req      = req
-        @delegate = {}
+        @delegate = {}.with_indifferent_access
         @loaded   = false
         @exists   = nil # we haven't checked yet
       end
@@ -88,13 +88,13 @@ def destroy
       # nil if the given key is not found in the session.
       def [](key)
         load_for_read!
-        @delegate[key.to_s]
+        @delegate[key]
       end
 
       # Returns true if the session has the given key or false.
       def has_key?(key)
         load_for_read!
-        @delegate.key?(key.to_s)
+        @delegate.key?(key)
       end
       alias :key? :has_key?
       alias :include? :has_key?
@@ -112,7 +112,7 @@ def values
       # Writes given value to given key of the session.
       def []=(key, value)
         load_for_write!
-        @delegate[key.to_s] = value
+        @delegate[key] = value
       end
 
       # Clears the session.
@@ -139,13 +139,13 @@ def to_hash
       #   # => {"session_id"=>"e29b9ea315edf98aad94cc78c34cc9b2", "foo" => "bar"}
       def update(hash)
         load_for_write!
-        @delegate.update stringify_keys(hash)
+        @delegate.update hash
       end
 
       # Deletes given key from the session.
       def delete(key)
         load_for_write!
-        @delegate.delete key.to_s
+        @delegate.delete key
       end
 
       # Returns value of the given key from the session, or raises +KeyError+
@@ -165,9 +165,9 @@ def delete(key)
       def fetch(key, default=Unspecified, &block)
         load_for_read!
         if default == Unspecified
-          @delegate.fetch(key.to_s, &block)
+          @delegate.fetch(key, &block)
         else
-          @delegate.fetch(key.to_s, default, &block)
+          @delegate.fetch(key, default, &block)
         end
       end
 
@@ -211,15 +211,9 @@ def load_for_write!
       def load!
         id, session = @by.load_session @req
         options[:id] = id
-        @delegate.replace(stringify_keys(session))
+        @delegate.replace(session)
         @loaded = true
       end
-
-      def stringify_keys(other)
-        other.each_with_object({}) { |(key, value), hash|
-          hash[key.to_s] = value
-        }
-      end
     end
   end
 end

actionpack/test/dispatch/request/session_test.rb

@@ -105,6 +105,16 @@ def test_fetch
         end
       end
 
+      def test_indifferent_access
+        s = Session.create(store, req, {})
+
+        s[:one] = { test: "deep" }
+        s[:two] = { "test" => "deep" }
+
+        assert_equal 'deep', s[:one]["test"]
+        assert_equal 'deep', s[:two][:test]
+      end
+
       private
       def store
         Class.new {

actionpack/test/dispatch/session/abstract_store_test.rb

@@ -46,6 +46,22 @@ def test_new_session_object_is_merged_with_old
         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

actionpack/test/dispatch/session/cache_store_test.rb

@@ -12,6 +12,11 @@ def set_session_value
       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
@@ -21,6 +26,14 @@ 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: "#{request.session.id}"
     end
@@ -160,6 +173,22 @@ def test_prevents_session_fixation
     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|

actionpack/test/dispatch/session/cookie_store_test.rb

@@ -24,10 +24,23 @@ def set_session_value
       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
@@ -81,6 +94,15 @@ def test_getting_session_value
     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
@@ -332,6 +354,18 @@ def test_session_store_with_all_domains
     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

actionpack/test/dispatch/session/mem_cache_store_test.rb

@@ -13,6 +13,11 @@ def set_session_value
       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
@@ -22,6 +27,14 @@ 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: "#{request.session.id}"
     end
@@ -179,6 +192,24 @@ def test_prevents_session_fixation
     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

actionpack/test/dispatch/session/test_session_test.rb

@@ -60,4 +60,11 @@ def test_fetch_returns_block_value
     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