Don't modify the params hash passed to Client.new

CHANGELOG.md

@@ -1,4 +1,5 @@
 # Changelog
+ * [Unreleased] Stop `Client.new` from modifying the params hash passed in, and accept frozen hashes
  * [Unreleased] Add native Extract API support via the `extract` method
  * [Unreleased] Apply the `timeout` option to persistent connections
  * [Unreleased] Add Image API support

lib/serpapi/client.rb

@@ -74,22 +74,20 @@ def initialize(params = {})
       raise SerpApiError, 'params cannot be nil' if params.nil?
       raise SerpApiError, "params must be hash, not: #{params.class}" unless params.instance_of?(Hash)
 
+      # work on a copy so the caller's hash is never modified
+      # (dup, unlike clone, also returns an unfrozen copy of a frozen hash)
+      config = params.dup
+
       # store client HTTP request timeout
-      @timeout = params[:timeout] || 120
+      @timeout = config.delete(:timeout) || 120
       @timeout.freeze
 
       # enable HTTP persistent mode
-      @persistent = true
-      @persistent = params[:persistent] if params.key?(:persistent)
+      @persistent = config.key?(:persistent) ? config.delete(:persistent) : true
       @persistent.freeze
 
-      # delete this client only configuration keys
-      %i[timeout persistent].each do |option|
-        params.delete(option) if params.key?(option)
-      end
-
       # set default query parameters
-      @params = params.clone || {}
+      @params = config
 
       # track ruby library as a client for statistic purpose
       @params[:source] = 'serpapi-ruby:' << SerpApi::VERSION

spec/serpapi/client/constructor_spec.rb

@@ -0,0 +1,51 @@
+require 'spec_helper'
+
+# These specs only build clients, so they make no HTTP request to serpapi.com.
+describe 'SerpApi::Client constructor' do
+  it 'does not modify the params hash passed by the caller' do
+    config = { engine: 'google', api_key: 'test_key', timeout: 10, persistent: false }
+    original = config.dup
+
+    SerpApi::Client.new(config)
+
+    expect(config).to eq(original)
+  end
+
+  it 'gives the same settings to every client built from a reused config hash' do
+    config = { engine: 'google', api_key: 'test_key', timeout: 10, persistent: false }
+
+    first = SerpApi::Client.new(config)
+    second = SerpApi::Client.new(config)
+
+    expect(second.timeout).to eq(first.timeout)
+    expect(second.timeout).to eq(10)
+    expect(second.persistent).to be false
+    expect(second.socket).to be_nil
+  end
+
+  it 'accepts a frozen params hash' do
+    config = { engine: 'google', api_key: 'test_key', timeout: 10, persistent: false }.freeze
+
+    client = SerpApi::Client.new(config)
+
+    expect(client.timeout).to eq(10)
+    expect(client.persistent).to be false
+    expect(client.engine).to eq('google')
+  end
+
+  it 'keeps timeout and persistent out of the query parameters' do
+    client = SerpApi::Client.new(engine: 'google', api_key: 'test_key', timeout: 10, persistent: false)
+
+    expect(client.params).not_to have_key(:timeout)
+    expect(client.params).not_to have_key(:persistent)
+  end
+
+  it 'defaults to a 120 second timeout and persistent mode' do
+    client = SerpApi::Client.new(engine: 'google', api_key: 'test_key')
+
+    expect(client.timeout).to eq(120)
+    expect(client.persistent).to be true
+    expect(client.socket).not_to be_nil
+    client.close
+  end
+end