Change params hash to stop having side effects.

piotrmurach · piotrmurach · commit 2dfa03b059fb · 2015-02-07T20:26:05.000Z
diff --git a/lib/github_api/params_hash.rb b/lib/github_api/params_hash.rb
@@ -4,18 +4,20 @@
 require 'base64'
 
 module Github
-
   # Class responsible for holding request parameters
   class ParamsHash < DelegateClass(Hash)
     include Normalizer
     include MimeType
 
+    REQUEST_PARAMS = [:accept, :media, :data, :raw, :content_type, :headers]
+
     def initialize(hash)
       super(normalize!(Hash[hash]))
     end
 
     # Create empty hash
     #
+    # @api public
     def self.empty
       new({})
     end
@@ -24,52 +26,55 @@ def self.empty
     #
     #  [.version].param[+json]
     #
+    # @api public
     def media
       parse(delete('media'))
     end
 
-    # Return accept header if present
+    # Get accept header
     #
+    # @api public
     def accept
-      if has_key?('accept')
-        delete('accept')
-      elsif has_key?('media')
-        media
-      else
-        nil
+      if key?('accept') then self['accept']
+      elsif key?('media') then media
+      else nil
       end
     end
 
     # Extract request data from parameters
     #
+    # @api public
     def data
-      if has_key?('data') && !self['data'].nil?
-        return delete('data')
+      if key?('data') && !self['data'].nil?
+        self['data']
       else
-        return to_hash
+        request_params
       end
     end
 
     def encoder
-      if has_key?('encoder') && self['encoder']
-        return delete('encoder')
+      if key?('encoder') && self['encoder']
+        self['encoder']
       else
-        return {}
+        {}
       end
     end
 
-    # Any client configuration options
+    # Configuration options from request
     #
+    # @return [Hash]
+    #
+    # @api public
     def options
-      opts = has_key?('options') ? delete('options') : {}
-      headers = opts.fetch(:headers) { {} }
+      opts    = fetch('options', {})
+      headers = fetch('headers', {})
       if value = accept
         headers[:accept] = value
       end
-      if value = delete('content_type')
-        headers[:content_type] = value
+      if self['content_type']
+        headers[:content_type] = self['content_type']
       end
-      opts[:raw] = has_key?('raw') ? delete('raw') : false
+      opts[:raw]     = key?('raw') ? self['raw'] : false
       opts[:headers] = headers unless headers.empty?
       opts
     end
@@ -79,23 +84,32 @@ def options
     def merge_default(defaults)
       if defaults && !defaults.empty?
         defaults.each do |key, value|
-          self[key] = value unless self.has_key?(key)
+          self[key] = value unless self.key?(key)
         end
       end
       self
     end
 
     # Base64 encode string removing newline characters
     #
+    # @api public
     def strict_encode64(key)
       value = self[key]
       encoded = if Base64.respond_to?(:strict_encode64)
-        Base64.strict_encode64(value)
-      else
-        [value].pack("m0")
-      end
+                  Base64.strict_encode64(value)
+                else
+                  [value].pack('m0')
+                end
       self[key] = encoded.delete("\n\r")
     end
 
+    # Filter out request params
+    #
+    # @api public
+    def request_params
+      to_hash.select do |key, value|
+        !REQUEST_PARAMS.include?(key.to_sym)
+      end
+    end
   end # ParamsHash
 end # Github
diff --git a/spec/github/params_hash_spec.rb b/spec/github/params_hash_spec.rb
@@ -2,74 +2,96 @@
 
 require 'spec_helper'
 
-describe Github::ParamsHash do
+RSpec.describe Github::ParamsHash do
   let(:object) { described_class }
 
-  subject { object.new(hash) }
+  subject(:params) { object.new(hash) }
 
-  context 'converts key to string' do
-    let(:hash) { {:foo => 123 }}
+  context 'when empty' do
+    let(:hash) { {} }
+
+    it { expect(params.options).to eq({:raw => false}) }
+
+    it { expect(params.data).to eq({}) }
 
-    it { expect(subject).to be_a(Github::ParamsHash)}
+    it { expect(params.accept).to eq(nil) }
+  end
+
+  context 'converts key to string' do
+    let(:hash) { {foo: 123 } }
 
-    it { expect(subject['foo']).to eql(123) }
+    it { expect(params['foo']).to eql(123) }
 
-    it { expect(subject.data).to eql({"foo" => 123}) }
+    it { expect(params.data).to eql({"foo" => 123}) }
   end
 
   context 'media' do
-    let(:hash) { {:media => "raw"} }
+    let(:hash) { {media: "raw"} }
 
     it 'parses media key' do
-      expect(subject.media).to eql('application/vnd.github.v3.raw+json')
+      expect(params.media).to eql('application/vnd.github.v3.raw+json')
     end
   end
 
   context 'with accept' do
-    let(:hash) { {:accept => "application/json"} }
+    let(:hash) { {accept: "application/json"} }
 
     it 'overwrites media key' do
-      expect(subject.accept).to eql('application/json')
+      expect(params.accept).to eql('application/json')
+      expect(params.to_hash).to eq({'accept' => 'application/json'})
     end
   end
 
   context 'without accept' do
     let(:hash) { {} }
 
     it 'overwrites media key' do
-      expect(subject.accept).to be_nil
+      expect(params.accept).to be_nil
     end
   end
 
-  context 'extract data' do
-    let(:hash) { {:data => 'foobar'} }
+  context 'when data' do
+    let(:hash) { {data: 'foobar'} }
 
-    it { expect(subject.data).to eql('foobar') }
+    it 'extracts data key' do
+      expect(params.data).to eql('foobar')
+      expect(params.to_hash).to eql({'data' => 'foobar'})
+    end
+  end
+
+  context 'when content_type' do
+    let(:hash) { {content_type: 'application/octet-stream'} }
+
+    it 'extracts content_type key' do
+      expect(params.options[:headers]).to eql(hash)
+    end
   end
 
-  context 'extracts options headers' do
-    let(:hash) { {:content_type => 'application/octet-stream'} }
+  context 'when header' do
+    let(:hash) { {headers: {"X-GitHub-OTP" => "<2FA token>"}} }
 
-    it { expect(subject.options[:headers]).to eql(hash) }
+    it "sets headers" do
+      expect(params.options[:headers]).to eql({"X-GitHub-OTP" => "<2FA token>" })
+    end
   end
 
   context 'merges defaults' do
     let(:hash) { { :homepage => "https://tty.github.io" }}
     let(:defaults) {
-      { "homepage"   => "https://github.com",
-      "private"    => false}
+      { "homepage" => "https://github.com",
+        "private"  => false}
     }
 
     it 'correctly updates values' do
       subject.merge_default(defaults)
-      expect(subject['homepage']).to eql("https://tty.github.io")
-      expect(subject['private']).to be_false
+      expect(params['homepage']).to eql("https://tty.github.io")
+      expect(params['private']).to be_false
     end
   end
 
   context 'strict encode' do
-    let(:hash) { { :content => "puts 'hello ruby'"} }
+    let(:hash) { {content: "puts 'hello ruby'"} }
 
-    it { expect(subject.strict_encode64('content')).to eql('cHV0cyAnaGVsbG8gcnVieSc=')  }
+    it { expect(params.strict_encode64('content')).to eql('cHV0cyAnaGVsbG8gcnVieSc=')  }
   end
 end
