Merge pull request rack#957 from jeremy/add-multivalued-header

`Response#add_header` to add to a value to a multivalued header

HISTORY.md

@@ -1,3 +1,12 @@
+Sun Oct 3 18:25:03 2015  Jeremy Daer <jeremydaer@gmail.com>
+
+	* Introduce `Rack::Response::Helpers#add_header` to add a value to a
+	multi-valued response header. Implemented in terms of other
+	`Response#*_header` methods, so it's available to any response-like
+	class that includes the `Helpers` module.
+
+	* Add `Rack::Request#add_header` to match.
+
 Fri Sep  4 18:34:53 2015  Aaron Patterson <tenderlove@ruby-lang.org>
 
 	* `Rack::Session::Abstract::ID` IS DEPRECATED.  Please switch to
@@ -31,6 +40,17 @@ Thu Aug 27 15:43:48 2015  Aaron Patterson <tenderlove@ruby-lang.org>
 	* Tempfiles are automatically closed in the case that there were too
 	many posted.
 
+Thu Aug 27 11:00:03 2015  Aaron Patterson <tenderlove@ruby-lang.org>
+
+	* Added methods for manipulating response headers that don't assume
+	they're stored as a Hash. Response-like classes may include the
+	Rack::Response::Helpers module if they define these methods:
+
+	  * Rack::Response#has_header?
+	  * Rack::Response#get_header
+	  * Rack::Response#set_header
+	  * Rack::Response#delete_header
+
 Mon Aug 24 18:05:23 2015  Aaron Patterson <tenderlove@ruby-lang.org>
 
 	* Introduce Util.get_byte_ranges that will parse the value of the
@@ -55,10 +75,12 @@ Thu Aug 20 16:20:58 2015  Aaron Patterson <tenderlove@ruby-lang.org>
 	data set as CGI parameters, and just any arbitrary data the user wants
 	to associate with a particular request.  New methods:
 
-	  * Rack::Request#get_header
-	  * Rack::Request#set_header
 	  * Rack::Request#has_header?
+	  * Rack::Request#get_header
+	  * Rack::Request#fetch_header
 	  * Rack::Request#each_header
+	  * Rack::Request#set_header
+	  * Rack::Request#delete_header
 
 Thu Jun 18 16:00:05 2015  Aaron Patterson <tenderlove@ruby-lang.org>
 

lib/rack/request.rb

@@ -57,6 +57,12 @@ def initialize(env)
         super()
       end
 
+      # Predicate method to test to see if `name` has been set as request
+      # specific data
+      def has_header?(name)
+        @env.key? name
+      end
+
       # Get a request specific value for `name`.
       def get_header(name)
         @env[name]
@@ -68,25 +74,38 @@ def fetch_header(name, &block)
         @env.fetch(name, &block)
       end
 
-      # Delete a request specific value for `name`.
-      def delete_header(name)
-        @env.delete name
+      # Loops through each key / value pair in the request specific data.
+      def each_header(&block)
+        @env.each(&block)
       end
 
       # Set a request specific value for `name` to `v`
       def set_header(name, v)
         @env[name] = v
       end
 
-      # Predicate method to test to see if `name` has been set as request
-      # specific data
-      def has_header?(name)
-        @env.key? name
+      # Add a header that may have multiple values.
+      #
+      # Example:
+      #   request.add_header 'Accept', 'image/png'
+      #   request.add_header 'Accept', '*/*'
+      #
+      #   assert_equal 'image/png,*/*', request.get_header('Accept')
+      #
+      # http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
+      def add_header key, v
+        if v.nil?
+          get_header key
+        elsif has_header? key
+          set_header key, "#{get_header key},#{v}"
+        else
+          set_header key, v
+        end
       end
 
-      # Loops through each key / value pair in the request specific data.
-      def each_header(&block)
-        @env.each(&block)
+      # Delete a request specific value for `name`.
+      def delete_header(name)
+        @env.delete name
       end
 
       def initialize_copy(other)

lib/rack/response.rb

@@ -99,7 +99,7 @@ def empty?
       @block == nil && @body.empty?
     end
 
-    def have_header?(key);  headers.key? key;   end
+    def has_header?(key);   headers.key? key;   end
     def get_header(key);    headers[key];       end
     def set_header(key, v); headers[key] = v;   end
     def delete_header(key); headers.delete key; end
@@ -132,7 +132,26 @@ def unprocessable?;       status == 422;                        end
       def redirect?;            [301, 302, 303, 307, 308].include? status; end
 
       def include?(header)
-        have_header? header
+        has_header? header
+      end
+
+      # Add a header that may have multiple values.
+      #
+      # Example:
+      #   response.add_header 'Vary', 'Accept-Encoding'
+      #   response.add_header 'Vary', 'Cookie'
+      #
+      #   assert_equal 'Accept-Encoding,Cookie', response.get_header('Vary')
+      #
+      # http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
+      def add_header key, v
+        if v.nil?
+          get_header key
+        elsif has_header? key
+          set_header key, "#{get_header key},#{v}"
+        else
+          set_header key, v
+        end
       end
 
       def content_type
@@ -190,7 +209,7 @@ def initialize status, headers
         @headers = headers
       end
 
-      def have_header?(key);  headers.key? key;   end
+      def has_header?(key);   headers.key? key;   end
       def get_header(key);    headers[key];       end
       def set_header(key, v); headers[key] = v;   end
       def delete_header(key); headers.delete key; end

test/spec_mock.rb

@@ -273,3 +273,70 @@
     }.must_raise Rack::MockRequest::FatalWarning
   end
 end
+
+describe Rack::MockResponse, 'headers' do
+  before do
+    @res = Rack::MockRequest.new(app).get('')
+    @res.set_header 'FOO', '1'
+  end
+
+  it 'has_header?' do
+    lambda { @res.has_header? nil }.must_raise NoMethodError
+
+    @res.has_header?('FOO').must_equal true
+    @res.has_header?('Foo').must_equal true
+  end
+
+  it 'get_header' do
+    lambda { @res.get_header nil }.must_raise NoMethodError
+
+    @res.get_header('FOO').must_equal '1'
+    @res.get_header('Foo').must_equal '1'
+  end
+
+  it 'set_header' do
+    lambda { @res.set_header nil, '1' }.must_raise NoMethodError
+
+    @res.set_header('FOO', '2').must_equal '2'
+    @res.get_header('FOO').must_equal '2'
+
+    @res.set_header('Foo', '3').must_equal '3'
+    @res.get_header('Foo').must_equal '3'
+    @res.get_header('FOO').must_equal '3'
+
+    @res.set_header('FOO', nil).must_be_nil
+    @res.get_header('FOO').must_be_nil
+    @res.has_header?('FOO').must_equal true
+  end
+
+  it 'add_header' do
+    lambda { @res.add_header nil, '1' }.must_raise NoMethodError
+
+    # Sets header on first addition
+    @res.add_header('FOO', '1').must_equal '1,1'
+    @res.get_header('FOO').must_equal '1,1'
+
+    # Ignores nil additions
+    @res.add_header('FOO', nil).must_equal '1,1'
+    @res.get_header('FOO').must_equal '1,1'
+
+    # Converts additions to strings
+    @res.add_header('FOO', 2).must_equal '1,1,2'
+    @res.get_header('FOO').must_equal '1,1,2'
+
+    # Respects underlying case-sensitivity
+    @res.add_header('Foo', 'yep').must_equal '1,1,2,yep'
+    @res.get_header('Foo').must_equal '1,1,2,yep'
+    @res.get_header('FOO').must_equal '1,1,2,yep'
+  end
+
+  it 'delete_header' do
+    lambda { @res.delete_header nil }.must_raise NoMethodError
+
+    @res.delete_header('FOO').must_equal '1'
+    @res.has_header?('FOO').must_equal false
+
+    @res.has_header?('Foo').must_equal false
+    @res.delete_header('Foo').must_be_nil
+  end
+end

test/spec_request.rb

@@ -12,6 +12,11 @@ class RackRequestTest < Minitest::Spec
     refute_same req.env, req.dup.env
   end
 
+  it 'can check if something has been set' do
+    req = make_request(Rack::MockRequest.env_for("http://example.com:8080/"))
+    refute req.has_header?("FOO")
+  end
+
   it "can get a key from the env" do
     req = make_request(Rack::MockRequest.env_for("http://example.com:8080/"))
     assert_equal "example.com", req.get_header("SERVER_NAME")
@@ -29,17 +34,6 @@ class RackRequestTest < Minitest::Spec
     assert_equal "bar", req.get_header("FOO")
   end
 
-  it 'can set values in the env' do
-    req = make_request(Rack::MockRequest.env_for("http://example.com:8080/"))
-    req.set_header("FOO", "BAR")
-    assert_equal "BAR", req.get_header("FOO")
-  end
-
-  it 'can check if something has been set' do
-    req = make_request(Rack::MockRequest.env_for("http://example.com:8080/"))
-    refute req.has_header?("FOO")
-  end
-
   it 'can iterate over values' do
     req = make_request(Rack::MockRequest.env_for("http://example.com:8080/"))
     req.set_header 'foo', 'bar'
@@ -50,6 +44,25 @@ class RackRequestTest < Minitest::Spec
     assert_equal 'bar', hash['foo']
   end
 
+  it 'can set values in the env' do
+    req = make_request(Rack::MockRequest.env_for("http://example.com:8080/"))
+    req.set_header("FOO", "BAR")
+    assert_equal "BAR", req.get_header("FOO")
+  end
+
+  it 'can add to multivalued headers in the env' do
+    req = make_request(Rack::MockRequest.env_for('http://example.com:8080/'))
+
+    assert_equal '1', req.add_header('FOO', '1')
+    assert_equal '1', req.get_header('FOO')
+
+    assert_equal '1,2', req.add_header('FOO', '2')
+    assert_equal '1,2', req.get_header('FOO')
+
+    assert_equal '1,2', req.add_header('FOO', nil)
+    assert_equal '1,2', req.get_header('FOO')
+  end
+
   it 'can delete env values' do
     req = make_request(Rack::MockRequest.env_for("http://example.com:8080/"))
     req.set_header 'foo', 'bar'
@@ -1339,8 +1352,8 @@ class DelegateRequest
       include Rack::Request::Helpers
       extend Forwardable
 
-      def_delegators :@req, :get_header, :fetch_header, :delete_header,
-        :set_header, :has_header?, :each_header
+      def_delegators :@req, :has_header?, :get_header, :fetch_header,
+        :each_header, :set_header, :add_header, :delete_header
 
       def_delegators :@req, :[], :[]=, :values_at
 

test/spec_response.rb

@@ -360,3 +360,71 @@ def object_with_each.each
     lambda { res.finish.last.to_ary }.must_raise NoMethodError
   end
 end
+
+describe Rack::Response, 'headers' do
+  before do
+    @response = Rack::Response.new([], 200, { 'Foo' => '1' })
+  end
+
+  it 'has_header?' do
+    lambda { @response.has_header? nil }.must_raise NoMethodError
+
+    @response.has_header?('Foo').must_equal true
+    @response.has_header?('foo').must_equal true
+  end
+
+  it 'get_header' do
+    lambda { @response.get_header nil }.must_raise NoMethodError
+
+    @response.get_header('Foo').must_equal '1'
+    @response.get_header('foo').must_equal '1'
+  end
+
+  it 'set_header' do
+    lambda { @response.set_header nil, '1' }.must_raise NoMethodError
+
+    @response.set_header('Foo', '2').must_equal '2'
+    @response.has_header?('Foo').must_equal true
+    @response.get_header('Foo').must_equal('2')
+
+    @response.set_header('Foo', nil).must_be_nil
+    @response.has_header?('Foo').must_equal true
+    @response.get_header('Foo').must_be_nil
+  end
+
+  it 'add_header' do
+    lambda { @response.add_header nil, '1' }.must_raise NoMethodError
+
+    # Add a value to an existing header
+    @response.add_header('Foo', '2').must_equal '1,2'
+    @response.get_header('Foo').must_equal '1,2'
+
+    # Add nil to an existing header
+    @response.add_header('Foo', nil).must_equal '1,2'
+    @response.get_header('Foo').must_equal '1,2'
+
+    # Add nil to a nonexistent header
+    @response.add_header('Bar', nil).must_be_nil
+    @response.has_header?('Bar').must_equal false
+    @response.get_header('Bar').must_be_nil
+
+    # Add a value to a nonexistent header
+    @response.add_header('Bar', '1').must_equal '1'
+    @response.has_header?('Bar').must_equal true
+    @response.get_header('Bar').must_equal '1'
+  end
+
+  it 'delete_header' do
+    lambda { @response.delete_header nil }.must_raise NoMethodError
+
+    @response.delete_header('Foo').must_equal '1'
+    (!!@response.has_header?('Foo')).must_equal false
+
+    @response.delete_header('Foo').must_be_nil
+    @response.has_header?('Foo').must_equal false
+
+    @response.set_header('Foo', 1)
+    @response.delete_header('foo').must_equal 1
+    @response.has_header?('Foo').must_equal false
+  end
+end