Resolve pending requested changes

Author: samwelkanda

msgraph/core/_graph_client.py

@@ -12,7 +12,7 @@
     'scopes',
 
     # Retry Options
-    'retry_total',
+    'max_retries',
     'retry_backoff_factor',
     'retry_backoff_max',
     'retry_time_limit',

msgraph/core/middleware/retry.py

@@ -13,37 +13,38 @@ class RetryHandler(BaseMiddleware):
     retry policy for all requests
     """
 
-    DEFAULT_TOTAL_RETRIES = 3
-    MAX_TOTAL_RETRIES = 10
+    DEFAULT_MAX_RETRIES = 3
+    MAX_RETRIES = 10
+    DEFAULT_DELAY = 3
+    MAX_DELAY = 180
     DEFAULT_BACKOFF_FACTOR = 0.5
-    DEFAULT_RETRY_TIME_LIMIT = 180
     MAXIMUM_BACKOFF = 120
-    _DEFAULT_RETRY_CODES = {429, 503, 504}
+    _DEFAULT_RETRY_STATUS_CODES = {429, 503, 504}
 
     def __init__(self, **kwargs):
         super().__init__()
-        self.total_retries: int = min(
-            kwargs.pop('retry_total', self.DEFAULT_TOTAL_RETRIES), self.MAX_TOTAL_RETRIES
+        self.max_retries: int = min(
+            kwargs.pop('max_retries', self.DEFAULT_MAX_RETRIES), self.MAX_RETRIES
         )
         self.backoff_factor: float = kwargs.pop('retry_backoff_factor', self.DEFAULT_BACKOFF_FACTOR)
         self.backoff_max: int = kwargs.pop('retry_backoff_max', self.MAXIMUM_BACKOFF)
-        self.timeout: int = kwargs.pop('retry_time_limit', self.DEFAULT_RETRY_TIME_LIMIT)
+        self.timeout: int = kwargs.pop('retry_time_limit', self.MAX_DELAY)
 
         status_codes: [int] = kwargs.pop('retry_on_status_codes', [])
 
-        self._retry_on_status_codes = set(status_codes) | self._DEFAULT_RETRY_CODES
-        self._allowed_methods = frozenset(
+        self._retry_on_status_codes: set = set(status_codes) | self._DEFAULT_RETRY_STATUS_CODES
+        self._allowed_methods: set = frozenset(
             ['HEAD', 'GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS']
         )
-        self._respect_retry_after_header = True
+        self._respect_retry_after_header: bool = True
 
     @classmethod
     def disable_retries(cls):
         """
         Disable retries by setting retry_total to zero.
         retry_total takes precedence over all other counts.
         """
-        return cls(retry_total=0)
+        return cls(max_retries=0)
 
     def get_retry_options(self, middleware_control):
         """
@@ -53,10 +54,7 @@ def get_retry_options(self, middleware_control):
         if middleware_control:
             return {
                 'total':
-                min(
-                    middleware_control.get('retry_total', self.total_retries),
-                    self.MAX_TOTAL_RETRIES
-                ),
+                min(middleware_control.get('max_retries', self.max_retries), self.MAX_RETRIES),
                 'backoff':
                 middleware_control.get('retry_backoff_factor', self.backoff_factor),
                 'max_backoff':
@@ -65,12 +63,12 @@ def get_retry_options(self, middleware_control):
                 middleware_control.get('retry_time_limit', self.timeout),
                 'retry_codes':
                 set(middleware_control.get('retry_on_status_codes', self._retry_on_status_codes))
-                | set(self._DEFAULT_RETRY_CODES),
+                | set(self._DEFAULT_RETRY_STATUS_CODES),
                 'methods':
                 self._allowed_methods,
             }
         return {
-            'total': self.total_retries,
+            'total': self.max_retries,
             'backoff': self.backoff_factor,
             'max_backoff': self.backoff_max,
             'timeout': self.timeout,
@@ -83,26 +81,32 @@ def send(self, request, **kwargs):
         Sends the http request object to the next middleware or retries the request if necessary.
         """
         retry_options = self.get_retry_options(request.context.middleware_control)
-        retry_active = True
-        absolute_time_limit = retry_options['timeout']
+        absolute_time_limit = min(retry_options['timeout'], self.MAX_DELAY)
         response = None
         retry_count = 0
+        retry_valid = True
 
-        while retry_active:
+        while retry_valid:
             try:
                 start_time = time.time()
-                request.headers.update({'Retry-Attempt': '{}'.format(retry_count)})
+                request.headers.update({'retry-attempt': '{}'.format(retry_count)})
                 response = super().send(request, **kwargs)
-                # Check if the request needs to be retried based on the response
+                # Check if the request needs to be retried based on the response method
+                # and status code
                 if self.should_retry(retry_options, response):
-                    retry_active = self.increment_counter(retry_options)
+                    # check that max retries has not been hit
+                    retry_valid = self.check_retry_valid(retry_options, retry_count)
+
                     # Get the delay time between retries
-                    sleep_time = self.get_sleep_time(retry_options, retry_count, response)
-                    if retry_active and sleep_time < absolute_time_limit:
-                        time.sleep(sleep_time)
+                    delay = self.get_delay_time(retry_options, retry_count, response)
+
+                    if retry_valid and delay < absolute_time_limit:
+                        time.sleep(delay)
                         end_time = time.time()
                         absolute_time_limit -= (end_time - start_time)
+                        # increment the count for retries
                         retry_count += 1
+
                         continue
                 break
             except Exception as error:
@@ -142,35 +146,29 @@ def _is_request_payload_buffered(self, response):
             return False
         return True
 
-    def increment_counter(self, retry_options):
+    def check_retry_valid(self, retry_options, retry_count):
         """
-        Increment the retry counters on every valid retry
+        Check that the max retries limit has not been hit
         """
-        if self.retries_exhausted(retry_options):
-            return False
-        retry_options['total'] -= 1
-        return True
-
-    def retries_exhausted(self, retry_options):
-        retries_remaining = retry_options['total']
-        if retries_remaining <= 0:
+        if retry_count < retry_options['total']:
             return True
         return False
 
-    def get_sleep_time(self, retry_options, retry_count, response=None):
+    def get_delay_time(self, retry_options, retry_count, response=None):
         """
-        Get the time in seconds to sleep between retry attempts.
+        Get the time in seconds to delay between retry attempts.
         Respects a retry-after header in the response if provided
         If no retry-after response header, it defaults to exponential backoff
         """
         retry_after = self._get_retry_after(response)
         if retry_after:
             return retry_after
-        return self._get_sleep_time_exp_backoff(retry_options, retry_count)
+        return self._get_delay_time_exp_backoff(retry_options, retry_count)
 
-    def _get_sleep_time_exp_backoff(self, retry_options, retry_count):
+    def _get_delay_time_exp_backoff(self, retry_options, retry_count):
         """
-        Get time to sleep based on exponential backoff value.
+        Get time in seconds to delay between retry attempts based on an exponential
+        backoff value.
         """
         exp_backoff_value = retry_options['backoff'] * +(2**(retry_count - 1))
         backoff_value = exp_backoff_value + (random.randint(0, 1000) / 1000)
@@ -182,7 +180,7 @@ def _get_retry_after(self, response):
         """
         Check if retry-after is specified in the response header and get the value
         """
-        retry_after = response.headers.get("Retry-After")
+        retry_after = response.headers.get("retry-after")
         if retry_after:
             return self._parse_retry_after(retry_after)
         return None

samples/retry_handler_samples.py

@@ -24,7 +24,7 @@ def sample_http_client_with_custom_retry_defaults():
     request using the client."""
 
     client = HTTPClientFactory().create_with_default_middleware(
-        browser_credential, retry_total=5, retry_backoff_factor=0.1, retry_time_limit=60
+        browser_credential, max_retries=5, retry_backoff_factor=0.1, retry_time_limit=60
     )
     result = client.get('/me/messages', scopes=['mail.read'])
     pprint(result.json())
@@ -35,7 +35,7 @@ def sample_graph_client_with_custom_retry_defaults():
     handler. These defaults will be used for every subsequent request using the client unless
     per request options are passed"""
 
-    client = GraphClient(credential=browser_credential, retry_total=2, retry_backoff_factor=0.5)
+    client = GraphClient(credential=browser_credential, max_retries=2, retry_backoff_factor=0.5)
     result = client.get('/me/messages', scopes=['mail.read'])
     pprint(result.json())
 

tests/integration/test_retry_middleware.py

@@ -25,7 +25,7 @@ def test_no_retry_success_response(graph_client):
     )
 
     assert response.status_code == 200
-    assert response.request.headers["Retry-Attempt"] == "0"
+    assert response.request.headers["retry-attempt"] == "0"
 
 
 def test_valid_retry_429(graph_client):
@@ -35,7 +35,7 @@ def test_valid_retry_429(graph_client):
     response = graph_client.get('https://httpbin.org/status/429')
 
     assert response.status_code == 429
-    assert response.request.headers["Retry-Attempt"] == "3"
+    assert response.request.headers["retry-attempt"] == "3"
 
 
 def test_valid_retry_503(graph_client):
@@ -45,7 +45,7 @@ def test_valid_retry_503(graph_client):
     response = graph_client.get('https://httpbin.org/status/503')
 
     assert response.status_code == 503
-    assert response.request.headers["Retry-Attempt"] == "3"
+    assert response.request.headers["retry-attempt"] == "3"
 
 
 def test_valid_retry_504(graph_client):
@@ -55,7 +55,7 @@ def test_valid_retry_504(graph_client):
     response = graph_client.get('https://httpbin.org/status/504')
 
     assert response.status_code == 504
-    assert response.request.headers["Retry-Attempt"] == "3"
+    assert response.request.headers["retry-attempt"] == "3"
 
 
 def test_request_specific_options_override_default(graph_client):
@@ -64,9 +64,9 @@ def test_request_specific_options_override_default(graph_client):
     the default options.
     """
     response_1 = graph_client.get('https://httpbin.org/status/429')
-    response_2 = graph_client.get('https://httpbin.org/status/503', retry_total=2)
+    response_2 = graph_client.get('https://httpbin.org/status/503', max_retries=2)
     response_3 = graph_client.get('https://httpbin.org/status/504')
-    response_4 = graph_client.get('https://httpbin.org/status/429', retry_total=1)
+    response_4 = graph_client.get('https://httpbin.org/status/429', max_retries=1)
 
     assert response_1.status_code == 429
     assert response_1.request.headers["Retry-Attempt"] == "3"
@@ -88,4 +88,4 @@ def test_retries_time_limit(graph_client):
 
     assert response.status_code == 503
     headers = response.request.headers
-    assert headers["Retry-Attempt"] == "0"
+    assert headers["retry-attempt"] == "0"

tests/unit/test_retry_handler.py

@@ -16,30 +16,30 @@ def test_no_config():
     Test that default values are used if no custom confguration is passed
     """
     retry_handler = RetryHandler()
-    assert retry_handler.total_retries == retry_handler.DEFAULT_TOTAL_RETRIES
-    assert retry_handler.timeout == retry_handler.DEFAULT_RETRY_TIME_LIMIT
+    assert retry_handler.max_retries == retry_handler.DEFAULT_MAX_RETRIES
+    assert retry_handler.timeout == retry_handler.MAX_DELAY
     assert retry_handler.backoff_max == retry_handler.MAXIMUM_BACKOFF
     assert retry_handler.backoff_factor == retry_handler.DEFAULT_BACKOFF_FACTOR
     assert retry_handler._allowed_methods == frozenset(
         ['HEAD', 'GET', 'PUT', 'POST', 'PATCH', 'DELETE', 'OPTIONS']
     )
     assert retry_handler._respect_retry_after_header
-    assert retry_handler._retry_on_status_codes == retry_handler._DEFAULT_RETRY_CODES
+    assert retry_handler._retry_on_status_codes == retry_handler._DEFAULT_RETRY_STATUS_CODES
 
 
 def test_custom_config():
     """
     Test that default configuration is overrriden if custom configuration is provided
     """
     retry_handler = RetryHandler(
-        retry_total=10,
+        max_retries=10,
         retry_backoff_factor=0.2,
         retry_backoff_max=200,
         retry_time_limit=100,
         retry_on_status_codes=[502, 503]
     )
 
-    assert retry_handler.total_retries == 10
+    assert retry_handler.max_retries == 10
     assert retry_handler.timeout == 100
     assert retry_handler.backoff_max == 200
     assert retry_handler.backoff_factor == 0.2
@@ -52,10 +52,9 @@ def test_disable_retries():
     """
     retry_handler = RetryHandler()
     retry_handler = retry_handler.disable_retries()
-    assert retry_handler.total_retries == 0
+    assert retry_handler.max_retries == 0
     retry_options = retry_handler.get_retry_options({})
-    assert not retry_handler.increment_counter(retry_options)
-    assert retry_handler.retries_exhausted(retry_options)
+    assert not retry_handler.check_retry_valid(retry_options, 0)
 
 
 @responses.activate
@@ -128,46 +127,24 @@ def test_is_request_payload_buffered_invalid():
     assert not retry_handler._is_request_payload_buffered(response)
 
 
-def test_retries_exhausted():
+def test_check_retry_valid():
     """
-    Test that the retries exhausted method works correctly when total_retries is greater than zero
+    Test that a retry is valid if the maximum number of retries has not been reached
     """
     retry_handler = RetryHandler()
     settings = retry_handler.get_retry_options({})
 
-    assert not retry_handler.retries_exhausted(settings)
+    assert retry_handler.check_retry_valid(settings, 0)
 
 
-def test_retries_exhausted_zero_total():
+def test_check_retry_valid_no_retries():
     """
-    Test that the retries exhausted method works correctly when total retries are set to zero
+    Test that a retry is not valid if maximum number of retries has been reached
     """
-    retry_handler = RetryHandler(retry_total=0)
+    retry_handler = RetryHandler(max_retries=2)
     settings = retry_handler.get_retry_options({})
 
-    assert retry_handler.retries_exhausted(settings)
-
-
-def test_increment_counter():
-    """
-    Test that retry counter is incremented on a valid retry
-    """
-    retry_handler = RetryHandler()
-    settings = retry_handler.get_retry_options({})
-
-    assert retry_handler.increment_counter(settings)
-    assert settings['total'] == retry_handler.DEFAULT_TOTAL_RETRIES - 1
-
-
-def test_increment_counter_invalid_retry():
-    """
-    Test that retry counter is not incremented when a retry is not valid
-    """
-    retry_handler = RetryHandler(retry_total=0)
-    settings = retry_handler.get_retry_options({})
-
-    assert not retry_handler.increment_counter(settings)
-    assert settings['total'] == 0
+    assert not retry_handler.check_retry_valid(settings, 2)
 
 
 @responses.activate
@@ -203,7 +180,7 @@ def test_get_retry_after_http_date():
     """
     timevalue = time() + 120
     http_date = formatdate(timeval=timevalue, localtime=False, usegmt=True)
-    responses.add(responses.GET, BASE_URL, headers={'Retry-After': f'{http_date}'}, status=503)
+    responses.add(responses.GET, BASE_URL, headers={'retry-after': f'{http_date}'}, status=503)
     response = requests.get(BASE_URL)
 
     retry_handler = RetryHandler()