Fix cookie header duplication (#522)

* Add failing spec for multiple Set-Cookie headers bug

* When ActionDispatch::Cookies is available from Rails tries to use it.
Otherwise parse current cookies set on response and replace it using Rack directly.

* Keep compatibility with Rack ~> 1.6.
New methods like Rack::Utils#make_delete_cookie_header were only added on 2.0.0

Author: andrehjr

lib/split/persistence/cookie_adapter.rb

@@ -6,6 +6,7 @@ module Persistence
     class CookieAdapter
 
       def initialize(context)
+        @context = context
         @request, @response = context.request, context.response
         @cookies = @request.cookies
         @expires = Time.now + cookie_length_config
@@ -30,13 +31,40 @@ def keys
       private
 
       def set_cookie(value = {})
-        @response.set_cookie :split.to_s, default_options.merge(value: JSON.generate(value))
+        cookie_key = :split.to_s
+        cookie_value = default_options.merge(value: JSON.generate(value))
+        if action_dispatch?
+          @context.cookies[cookie_key] = cookie_value
+        else
+          set_cookie_via_rack(cookie_key, cookie_value)
+        end
       end
 
       def default_options
         { expires: @expires, path: '/' }
       end
 
+      def set_cookie_via_rack(key, value)
+        delete_cookie_header!(@response.header, key, value)
+        Rack::Utils.set_cookie_header!(@response.header, key, value)
+      end
+
+      # Use Rack::Utils#make_delete_cookie_header after Rack 2.0.0
+      def delete_cookie_header!(header, key, value)
+        cookie_header = header['Set-Cookie']
+        case cookie_header
+        when nil, ''
+          cookies = []
+        when String
+          cookies = cookie_header.split("\n")
+        when Array
+          cookies = cookie_header
+        end
+
+        cookies.reject! { |cookie| cookie =~ /\A#{Rack::Utils.escape(key)}=/ }
+        header['Set-Cookie'] = cookies.join("\n")
+      end
+
       def hash
         @hash ||= begin
           if cookies = @cookies[:split.to_s]
@@ -55,6 +83,9 @@ def cookie_length_config
         Split.configuration.persistence_cookie_length
       end
 
+      def action_dispatch?
+        defined?(Rails) && @response.is_a?(ActionDispatch::Response)
+      end
     end
   end
 end

spec/persistence/cookie_adapter_spec.rb

@@ -7,7 +7,7 @@
   let(:env) { Rack::MockRequest.env_for("http://example.com:8080/") }
   let(:request) { Rack::Request.new(env) }
   let(:response) { Rack::MockResponse.new(200, {}, "") }
-  let(:context) { double(request: request, response: response) }
+  let(:context) { double(request: request, response: response, cookies: CookiesMock.new) }
   subject { Split::Persistence::CookieAdapter.new(context) }
 
   describe "#[] and #[]=" do
@@ -40,4 +40,23 @@
     expect(subject["my_key"]).to eq("my_value")
   end
 
+  it "puts multiple experiments in a single cookie" do
+    subject["foo"] = "FOO"
+    subject["bar"] = "BAR"
+    expect(context.response.headers["Set-Cookie"]).to match(/\Asplit=%7B%22foo%22%3A%22FOO%22%2C%22bar%22%3A%22BAR%22%7D; path=\/; expires=[a-zA-Z]{3}, \d{2} [a-zA-Z]{3} \d{4} \d{2}:\d{2}:\d{2} -0000\Z/)
+  end
+
+  it "ensure other added cookies are not overriden" do
+    context.response.set_cookie 'dummy', 'wow'
+    subject["foo"] = "FOO"
+    expect(context.response.headers["Set-Cookie"]).to include("dummy=wow")
+    expect(context.response.headers["Set-Cookie"]).to include("split=")
+  end
+
+  it "uses ActionDispatch::Cookie when available for cookie writing" do
+    allow(subject).to receive(:action_dispatch?).and_return(true)
+    subject["foo"] = "FOO"
+    expect(subject['foo']).to eq('FOO')
+  end
+
 end