Only save the session if we're actually writing to it [#2703 state:resolved]

Signed-off-by: Joshua Peek <josh@joshpeek.com>

3 files changed, 40 insertions, 1 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
index 03761b10bd..6d109f4624 100644
--- a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
@@ -15,6 +15,7 @@ module ActionDispatch
           @by = by
           @env = env
           @loaded = false
+          @updated = false
         end
 
         def session_id
@@ -32,6 +33,7 @@ module ActionDispatch
         def []=(key, value)
           load! unless @loaded
           super(key.to_s, value)
+          @updated = true
         end
 
         def to_hash
@@ -79,6 +81,10 @@ module ActionDispatch
             @loaded
           end
 
+          def updated?
+            @updated
+          end
+
           def load!
             stale_session_check! do
               id, session = @by.send(:load_session, @env)
@@ -147,7 +153,10 @@ module ActionDispatch
         options = env[ENV_SESSION_OPTIONS_KEY]
 
         if !session_data.is_a?(AbstractStore::SessionHash) || session_data.send(:loaded?) || options[:expire_after]
-          session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.send(:loaded?)
+          if session_data.is_a?(AbstractStore::SessionHash)
+            session_data.send(:load!) if !session_data.send(:loaded?)
+            return response if !session_data.send(:updated?)
+          end
 
           sid = options[:id] || generate_sid
 
diff --git a/actionpack/test/activerecord/active_record_store_test.rb b/actionpack/test/activerecord/active_record_store_test.rb
index 47f8496181..9ac33b3417 100644
--- a/actionpack/test/activerecord/active_record_store_test.rb
+++ b/actionpack/test/activerecord/active_record_store_test.rb
@@ -22,6 +22,11 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest
       render :text => "foo: #{session[:foo].inspect}"
     end
 
+    def set_cookie_and_get_session_value
+      cookies["kittens"] = { :value => "fluffy" }
+      render :text => "foo: #{session[:foo].inspect}"
+    end
+
     def get_session_id
       session[:foo]
       render :text => "#{request.session_options[:id]}"
@@ -78,6 +83,23 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest
     end
   end
 
+  def test_getting_session_value_does_not_set_cookie
+    with_test_route_set do
+      get '/get_session_value'
+      assert_response :success
+      assert_equal "", headers["Set-Cookie"]
+    end
+  end
+
+  def test_getting_session_value_and_setting_a_cookie_doesnt_delete_all_cookies
+    with_test_route_set do
+      get '/set_cookie_and_get_session_value'
+      assert_response :success
+      assert_equal 'foo: nil', response.body
+      assert_equal({"kittens" => "fluffy"}, response.cookies)
+    end
+  end
+
   def test_setting_session_value_after_session_reset
     with_test_route_set do
       get '/set_session_value'
diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb
index 7561c93e4a..278f2c83ea 100644
--- a/actionpack/test/dispatch/session/mem_cache_store_test.rb
+++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb
@@ -61,6 +61,14 @@ class MemCacheStoreTest < ActionController::IntegrationTest
       end
     end
 
+    def test_getting_session_value_does_not_set_cookie
+      with_test_route_set do
+        get '/get_session_value'
+        assert_response :success
+        assert_equal "", headers["Set-Cookie"]
+      end
+    end
+
     def test_setting_session_value_after_session_reset
       with_test_route_set do
         get '/set_session_value'