Some overdue cleanup

Configuration gets a timeout and added to request setup.
The timeout matches Net HTTP defaults so should be backwards compat.
Configuration uses accessor to set (no poking at internals).
Default headers made reasonable, no placeholder values.
Request sets basic_auth via options like everything (httparty is weird).
Adds a User-Agent for the client.
Fix passed in options (a reference) being mutated by Request.

Author: mark-cooper

lib/collectionspace/client/client.rb

@@ -7,6 +7,8 @@ class Client
 
     attr_reader :config
 
+    NAME = "CollectionSpaceClient"
+
     def initialize(config = Configuration.new)
       unless config.is_a? CollectionSpace::Configuration
         raise CollectionSpace::ArgumentError, "Invalid configuration object"

lib/collectionspace/client/configuration.rb

@@ -10,18 +10,19 @@ class Configuration
       page_size: 25,
       include_deleted: false,
       throttle: 0,
+      timeout: 60,
       verbose: false,
       verify_ssl: true
     }.freeze
 
-    attr_accessor :base_uri, :username, :password, :page_size, :include_deleted, :throttle, :verbose, :verify_ssl
+    attr_accessor :base_uri, :username, :password, :page_size, :include_deleted,
+      :throttle, :timeout, :verbose, :verify_ssl
 
     def initialize(settings = {})
-      settings = DEFAULTS.merge(settings)
-      settings.each do |property, value|
+      DEFAULTS.merge(settings).each do |property, value|
         next unless DEFAULTS.key?(property)
 
-        instance_variable_set(:"@#{property}", value)
+        send(:"#{property}=", value)
       end
     end
   end

lib/collectionspace/client/request.rb

@@ -7,38 +7,22 @@ class Request
 
     attr_reader :config, :headers, :method, :path, :options
 
-    def default_headers(method = :get)
-      headers = {
-        delete: {},
-        get: {},
-        post: {
-          "Content-Type" => "application/xml",
-          "Content-Length" => "nnnn"
-        },
-        put: {
-          "Content-Type" => "application/xml",
-          "Content-Length" => "nnnn"
-        }
-      }
-      headers[method]
-    end
+    DEFAULT_HEADERS = {"Content-Type" => "application/xml"}.freeze
 
     def initialize(config, method = "GET", path = "", options = {})
       @config = config
       @method = method.downcase.to_sym
-      @path = path.gsub(%r{^/}, "")
+      @path = path.gsub(%r{^/+}, "")
+
+      @options = options.dup
+      @options[:basic_auth] = {username: config.username, password: config.password}
 
-      @auth = {
-        username: config.username,
-        password: config.password
-      }
+      @options[:headers] = DEFAULT_HEADERS.merge(@options.fetch(:headers, {}))
+      @options[:headers]["User-Agent"] = "#{Client::NAME}/#{Client::VERSION}"
 
-      headers = default_headers(@method).merge(options.fetch(:headers, {}))
-      @options = options
-      @options[:basic_auth] = @auth
-      @options[:headers] = headers
+      @options[:query] = @options.fetch(:query, {})
+      @options[:timeout] = config.timeout
       @options[:verify] = config.verify_ssl
-      @options[:query] = options.fetch(:query, {})
 
       self.class.base_uri config.base_uri
       self.class.debug_output $stdout if config.verbose

spec/collectionspace/configuration_spec.rb

@@ -13,6 +13,7 @@
     expect(config.page_size).to eq 25
     expect(config.include_deleted).to eq false
     expect(config.throttle).to eq 0
+    expect(config.timeout).to eq 60
     expect(config.verbose).to eq false
     expect(config.verify_ssl).to eq true
   end

spec/collectionspace/request_spec.rb

@@ -17,6 +17,13 @@
     }
   end
 
+  it "does not mutate the caller's options hash" do
+    opts = {query: {foo: "bar"}, headers: {"X-Custom" => "1"}}
+    caller_opts = Marshal.load(Marshal.dump(opts))
+    CollectionSpace::Request.new(default_config, "GET", "collectionobjects", opts)
+    expect(opts).to eq(caller_opts)
+  end
+
   it "can create a collectionobject" do
     VCR.use_cassette("request_collectionobjects_create") do
       response = client.post("collectionobjects", post_payload)