Merge branch 'dougforpres-auth-retry'

Author: sqrrrl

README.md

@@ -123,6 +123,8 @@ in the credentials. Detailed instructions on how to enable delegation for your d
 
 The API client can automatically retry requests for recoverable errors. To enable retries, set the `client.retries` property to
 the number of additional attempts. To avoid flooding servers, retries invovle a 1 second delay that increases on each subsequent retry.
+In the case of authentication token expiry, the API client will attempt to refresh the token and retry the failed operation - this
+is a specific exception to the retry rules.
 
 The default value for retries is 0, but will be enabled by default in future releases.
 

lib/google/api_client.rb

@@ -118,6 +118,7 @@ def initialize(options={})
       self.key = options[:key]
       self.user_ip = options[:user_ip]
       self.retries = options.fetch(:retries) { 0 }
+      self.expired_auth_retry = options.fetch(:expired_auth_retry) { true }
       @discovery_uris = {}
       @discovery_documents = {}
       @discovered_apis = {}
@@ -255,6 +256,13 @@ def authorization=(new_authorization)
     #  Number of retries
     attr_accessor :retries
 
+    ##
+    # Whether or not an expired auth token should be re-acquired
+    # (and the operation retried) regardless of retries setting
+    # @return [Boolean]
+    #  Auto retry on auth expiry
+    attr_accessor :expired_auth_retry
+
     ##
     # Returns the URI for the directory document.
     #
@@ -613,28 +621,40 @@ def execute!(*params)
       request.authorization = options[:authorization] || self.authorization unless options[:authenticated] == false
 
       tries = 1 + (options[:retries] || self.retries)
+      attempt = 0
 
       Retriable.retriable :tries => tries, 
                           :on => [TransmissionError],
-                          :on_retry => client_error_handler(request.authorization),
+                          :on_retry => client_error_handler,
                           :interval => lambda {|attempts| (2 ** attempts) + rand} do
-        result = request.send(connection, true)
-
-        case result.status
-          when 200...300
-            result
-          when 301, 302, 303, 307
-            request = generate_request(request.to_hash.merge({
-              :uri => result.headers['location'],
-              :api_method => nil
-            }))
-            raise RedirectError.new(result.headers['location'], result)
-          when 400...500
-            raise ClientError.new(result.error_message || "A client error has occurred", result)
-          when 500...600
-            raise ServerError.new(result.error_message || "A server error has occurred", result)
-          else
-            raise TransmissionError.new(result.error_message || "A transmission error has occurred", result)
+        attempt += 1
+
+        # This 2nd level retriable only catches auth errors, and supports 1 retry, which allows
+        # auth to be re-attempted without having to retry all sorts of other failures like
+        # NotFound, etc
+        Retriable.retriable :tries => ((expired_auth_retry || tries > 1) && attempt == 1) ? 2 : 1, 
+                            :on => [AuthorizationError],
+                            :on_retry => authorization_error_handler(request.authorization) do
+          result = request.send(connection, true)
+
+          case result.status
+            when 200...300
+              result
+            when 301, 302, 303, 307
+              request = generate_request(request.to_hash.merge({
+                :uri => result.headers['location'],
+                :api_method => nil
+              }))
+              raise RedirectError.new(result.headers['location'], result)
+            when 401
+              raise AuthorizationError.new(result.error_message || 'Invalid/Expired Authentication', result)
+            when 400, 402...500
+              raise ClientError.new(result.error_message || "A client error has occurred", result)
+            when 500...600
+              raise ServerError.new(result.error_message || "A server error has occurred", result)
+            else
+              raise TransmissionError.new(result.error_message || "A transmission error has occurred", result)
+          end
         end
       end
     end
@@ -682,18 +702,17 @@ def resolve_uri(template, mapping={})
 
 
     ##
-    # Returns on proc for special processing of retries as not all client errors
-    # are recoverable. Only 401s should be retried and only if the credentials
-    # are refreshable
+    # Returns on proc for special processing of retries for authorization errors
+    # Only 401s should be retried and only if the credentials are refreshable
     #
     # @param [#fetch_access_token!] authorization
     #   OAuth 2 credentials
-    # @return [Proc] 
-    def client_error_handler(authorization)  
+    # @return [Proc]
+    def authorization_error_handler(authorization)
       can_refresh = authorization.respond_to?(:refresh_token) && auto_refresh_token 
       Proc.new do |exception, tries|
-        next unless exception.kind_of?(ClientError)
-        if exception.result.status == 401 && can_refresh && tries == 1
+        next unless exception.kind_of?(AuthorizationError)
+        if can_refresh
           begin
             logger.debug("Attempting refresh of access token & retry of request")
             authorization.fetch_access_token!
@@ -705,6 +724,17 @@ def client_error_handler(authorization)
       end
     end
 
+    ##
+    # Returns on proc for special processing of retries as not all client errors
+    # are recoverable. Only 401s should be retried (via authorization_error_handler)
+    #
+    # @return [Proc]
+    def client_error_handler
+      Proc.new do |exception, tries|
+        raise exception if exception.kind_of?(ClientError)
+      end
+    end
+
   end
 
 end

lib/google/api_client/errors.rb

@@ -43,6 +43,11 @@ class ValidationError < StandardError
     class ClientError < TransmissionError
     end
 
+    ##
+    # A 401 HTTP error occurred.
+    class AuthorizationError < ClientError
+    end
+
     ##
     # A 5xx class HTTP error occurred.
     class ServerError < TransmissionError

spec/google/api_client_spec.rb

@@ -200,7 +200,7 @@
       )
     end
 
-    it 'should refresh tokens on 401 tokens' do
+    it 'should refresh tokens on 401 errors' do
       client.authorization.access_token = '12345'
       expect(client.authorization).to receive(:fetch_access_token!)
 
@@ -290,4 +290,63 @@
     end
 
   end
+
+  describe 'when retries disabled and expired_auth_retry on (default)' do
+    before do
+      client.retries = 0
+    end
+
+    after do
+      @connection.verify
+    end
+
+    it 'should refresh tokens on 401 errors' do
+      client.authorization.access_token = '12345'
+      expect(client.authorization).to receive(:fetch_access_token!)
+
+      @connection = stub_connection do |stub|
+        stub.get('/foo') do |env|
+          [401, {}, '{}']
+        end
+        stub.get('/foo') do |env|
+          [200, {}, '{}']
+        end
+      end
+
+      client.execute(
+        :uri => 'https://www.gogole.com/foo',
+        :connection => @connection
+      )
+    end
+
+  end
+
+  describe 'when retries disabled and expired_auth_retry off' do
+    before do
+      client.retries = 0
+      client.expired_auth_retry = false
+    end
+
+    it 'should not refresh tokens on 401 errors' do
+      client.authorization.access_token = '12345'
+      expect(client.authorization).not_to receive(:fetch_access_token!)
+
+      @connection = stub_connection do |stub|
+        stub.get('/foo') do |env|
+          [401, {}, '{}']
+        end
+        stub.get('/foo') do |env|
+          [200, {}, '{}']
+        end
+      end
+
+      resp = client.execute(
+        :uri => 'https://www.gogole.com/foo',
+        :connection => @connection
+      )
+
+      expect(resp.response.status).to be == 401
+    end
+
+  end
 end