Merge pull request #170 from pusher/refactor_client

Handle nil cluster, minor refactor

Author: Jon Elverkilde

lib/pusher/client.rb

@@ -1,5 +1,4 @@
 require 'base64'
-
 require 'pusher-signature'
 
 module Pusher
@@ -10,6 +9,11 @@ class Client
                 :keep_alive_timeout
 
     ## CONFIGURATION ##
+    DEFAULT_CONNECT_TIMEOUT = 5
+    DEFAULT_SEND_TIMEOUT = 5
+    DEFAULT_RECEIVE_TIMEOUT = 5
+    DEFAULT_KEEP_ALIVE_TIMEOUT = 30
+    DEFAULT_CLUSTER = "mt1"
 
     # Loads the configuration from an url in the environment
     def self.from_env(key = 'PUSHER_URL')
@@ -25,43 +29,31 @@ def self.from_url(url)
     end
 
     def initialize(options = {})
-      default_options = {
-        :scheme => 'http',
-        :port => 80,
-      }
+      @scheme = "http"
+      @port = options[:port] || 80
 
       if options[:use_tls] || options[:encrypted]
-        default_options[:scheme] = "https"
-        default_options[:port] = 443
+        @scheme = "https"
+        @port = options[:port] || 443
       end
 
-      merged_options = default_options.merge(options)
-
-      if options.has_key?(:host)
-        merged_options[:host] = options[:host]
-      elsif options.has_key?(:cluster)
-        merged_options[:host] = "api-#{options[:cluster]}.pusher.com"
-      else
-        merged_options[:host] = "api.pusherapp.com"
-      end
+      @app_id = options[:app_id]
+      @key = options[:key]
+      @secret = options[:secret]
 
-      @scheme, @host, @port, @app_id, @key, @secret =
-        merged_options.values_at(
-          :scheme, :host, :port, :app_id, :key, :secret
-        )
+      @host = options[:host]
+      @host ||= "api-#{options[:cluster]}.pusher.com" unless options[:cluster].nil? || options[:cluster].empty?
+      @host ||= "api-#{DEFAULT_CLUSTER}.pusher.com"
 
-      if options.has_key?(:encryption_master_key_base64)
-        @encryption_master_key =
-          Base64.strict_decode64(options[:encryption_master_key_base64])
-      end
+      @encryption_master_key = Base64.strict_decode64(options[:encryption_master_key_base64]) if options[:encryption_master_key_base64]
 
       @http_proxy = options[:http_proxy]
 
       # Default timeouts
-      @connect_timeout = 5
-      @send_timeout = 5
-      @receive_timeout = 5
-      @keep_alive_timeout = 30
+      @connect_timeout = DEFAULT_CONNECT_TIMEOUT
+      @send_timeout = DEFAULT_SEND_TIMEOUT
+      @receive_timeout = DEFAULT_RECEIVE_TIMEOUT
+      @keep_alive_timeout = DEFAULT_KEEP_ALIVE_TIMEOUT
     end
 
     # @private Returns the authentication token for the client
@@ -75,10 +67,10 @@ def authentication_token
     def url(path = nil)
       raise ConfigurationError, :app_id unless @app_id
       URI::Generic.build({
-        :scheme => @scheme,
-        :host => @host,
-        :port => @port,
-        :path => "/apps/#{@app_id}#{path}"
+        scheme: @scheme,
+        host: @host,
+        port: @port,
+        path: "/apps/#{@app_id}#{path}"
       })
     end
 
@@ -102,13 +94,12 @@ def http_proxy=(http_proxy)
       @http_proxy = http_proxy
       uri = URI.parse(http_proxy)
       @proxy = {
-        :scheme => uri.scheme,
-        :host => uri.host,
-        :port => uri.port,
-        :user => uri.user,
-        :password => uri.password
+        scheme: uri.scheme,
+        host: uri.host,
+        port: uri.port,
+        user: uri.user,
+        password: uri.password
       }
-      @http_proxy
     end
 
     # Configure whether Pusher API calls should be made over SSL
@@ -128,6 +119,8 @@ def encrypted?
     end
 
     def cluster=(cluster)
+      cluster = DEFAULT_CLUSTER if cluster.nil? || cluster.empty?
+
       @host = "api-#{cluster}.pusher.com"
     end
 
@@ -360,9 +353,9 @@ def authenticate(channel_name, socket_id, custom_data = nil)
 
     # @private Construct a net/http http client
     def sync_http_client
-      @client ||= begin
-        require 'httpclient'
+      require 'httpclient'
 
+      @client ||= begin
         HTTPClient.new(@http_proxy).tap do |c|
           c.connect_timeout = @connect_timeout
           c.send_timeout = @send_timeout
@@ -381,14 +374,14 @@ def em_http_client(uri)
         require 'em-http' unless defined?(EventMachine::HttpRequest)
 
         connection_opts = {
-          :connect_timeout => @connect_timeout,
-          :inactivity_timeout => @receive_timeout,
+          connect_timeout: @connect_timeout,
+          inactivity_timeout: @receive_timeout,
         }
 
         if defined?(@proxy)
           proxy_opts = {
-            :host => @proxy[:host],
-            :port => @proxy[:port]
+            host: @proxy[:host],
+            port: @proxy[:port]
           }
           if @proxy[:user]
             proxy_opts[:authorization] = [@proxy[:user], @proxy[:password]]

spec/channel_spec.rb

@@ -29,7 +29,7 @@ def stub_post_to_raise(e)
   describe '#trigger!' do
     it "should use @client.trigger internally" do
       expect(@client).to receive(:trigger)
-      @channel.trigger('new_event', 'Some data')
+      @channel.trigger!('new_event', 'Some data')
     end
   end
 
@@ -39,16 +39,14 @@ def stub_post_to_raise(e)
 
       expect(Pusher.logger).to receive(:error).with("Exception from WebMock (HTTPClient::BadResponseError) (Pusher::HTTPError)")
       expect(Pusher.logger).to receive(:debug) #backtrace
-      channel = Pusher::Channel.new(@client.url, 'test_channel', @client)
-      channel.trigger('new_event', 'Some data')
+      @channel.trigger('new_event', 'Some data')
     end
 
     it "should log failure if Pusher returns an error response" do
       stub_post 401, "some signature info"
       expect(Pusher.logger).to receive(:error).with("some signature info (Pusher::AuthenticationError)")
       expect(Pusher.logger).to receive(:debug) #backtrace
-      channel = Pusher::Channel.new(@client.url, 'test_channel', @client)
-      channel.trigger('new_event', 'Some data')
+      @channel.trigger('new_event', 'Some data')
     end
   end
 

spec/client_spec.rb

@@ -17,7 +17,7 @@
 
     describe 'default configuration' do
       it 'should be preconfigured for api host' do
-        expect(@client.host).to eq('api.pusherapp.com')
+        expect(@client.host).to eq('api-mt1.pusher.com')
       end
 
       it 'should be preconfigured for port 80' do
@@ -79,6 +79,16 @@
         expect(@client.host).to eq('api-eu.pusher.com')
       end
 
+      it 'should handle nil gracefully' do
+        @client.cluster = nil
+        expect(@client.host).to eq('api-mt1.pusher.com')
+      end
+
+      it 'should handle empty string' do
+        @client.cluster = ""
+        expect(@client.host).to eq('api-mt1.pusher.com')
+      end
+
       it 'should be overridden by host if it comes after' do
         @client.cluster = 'eu'
         @client.host = 'api.staging.pusher.com'
@@ -518,7 +528,7 @@
       [:get, :post].each do |verb|
         describe "##{verb}" do
           before :each do
-            @url_regexp = %r{api.pusherapp.com}
+            @url_regexp = %r{api-mt1.pusher.com}
             stub_request(verb, @url_regexp).
               to_return(:status => 200, :body => "{}")
           end
@@ -527,13 +537,13 @@
 
           it "should use http by default" do
             call_api
-            expect(WebMock).to have_requested(verb, %r{http://api.pusherapp.com/apps/20/path})
+            expect(WebMock).to have_requested(verb, %r{http://api-mt1.pusher.com/apps/20/path})
           end
 
           it "should use https if configured" do
             @client.encrypted = true
             call_api
-            expect(WebMock).to have_requested(verb, %r{https://api.pusherapp.com})
+            expect(WebMock).to have_requested(verb, %r{https://api-mt1.pusher.com})
           end
 
           it "should format the respose hash with symbols at first level" do
@@ -598,7 +608,7 @@
         [[:get, :get_async], [:post, :post_async]].each do |verb, method|
           describe "##{method}" do
             before :each do
-              @url_regexp = %r{api.pusherapp.com}
+              @url_regexp = %r{api-mt1.pusher.com}
               stub_request(verb, @url_regexp).
                 to_return(:status => 200, :body => "{}")
             end
@@ -614,13 +624,13 @@
 
             it "should use http by default" do
               call_api
-              expect(WebMock).to have_requested(verb, %r{http://api.pusherapp.com/apps/20/path})
+              expect(WebMock).to have_requested(verb, %r{http://api-mt1.pusher.com/apps/20/path})
             end
 
             it "should use https if configured" do
               @client.encrypted = true
               call_api
-              expect(WebMock).to have_requested(verb, %r{https://api.pusherapp.com})
+              expect(WebMock).to have_requested(verb, %r{https://api-mt1.pusher.com})
             end
 
             # Note that the raw httpclient connection object is returned and
@@ -640,7 +650,7 @@
         [[:get, :get_async], [:post, :post_async]].each do |verb, method|
           describe "##{method}" do
             before :each do
-              @url_regexp = %r{api.pusherapp.com}
+              @url_regexp = %r{api-mt1.pusher.com}
               stub_request(verb, @url_regexp).
                 to_return(:status => 200, :body => "{}")
             end
@@ -650,7 +660,7 @@
             it "should use http by default" do
               EM.run {
                 call_api.callback {
-                  expect(WebMock).to have_requested(verb, %r{http://api.pusherapp.com/apps/20/path})
+                  expect(WebMock).to have_requested(verb, %r{http://api-mt1.pusher.com/apps/20/path})
                   EM.stop
                 }
               }
@@ -660,7 +670,7 @@
               EM.run {
                 @client.encrypted = true
                 call_api.callback {
-                  expect(WebMock).to have_requested(verb, %r{https://api.pusherapp.com})
+                  expect(WebMock).to have_requested(verb, %r{https://api-mt1.pusher.com})
                   EM.stop
                 }
               }