Repository: google/google-api-python-client
Pull request #495 (2018-04-23 18:58:17)

fix socket.timeout retry
resolves https://github.com/google/google-api-python-client/issues/494

thehesiod: ok updated, I think I can change the existing tests as there is no mention of TimeoutError being thrown for sockets, instead even for python2 `socket.timeout` is what it should be: https://docs.python.org/2/library/socket.html#socket.timeout
jonparrott: @thehesiod if you rebase, #496 should fix the test failure.

Patch:
diff --git a/.travis.yml b/.travis.yml
index fbce8fa1e..3826ff64a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,13 +6,41 @@ cache:
 matrix:
   include:
   - python: 2.7
-    env: TOXENV=py27
+    env: TOXENV=py27-oauth2client1
+  - python: 2.7
+    env: TOXENV=py27-oauth2client2
+  - python: 2.7
+    env: TOXENV=py27-oauth2client3
+  - python: 2.7
+    env: TOXENV=py27-oauth2client4
+
+  - python: 3.4
+    env: TOXENV=py34-oauth2client1
+  - python: 3.4
+    env: TOXENV=py34-oauth2client2
+  - python: 3.4
+    env: TOXENV=py34-oauth2client3
   - python: 3.4
-    env: TOXENV=py34
+    env: TOXENV=py34-oauth2client4
+
   - python: 3.5
-    env: TOXENV=py35
+    env: TOXENV=py35-oauth2client1
+  - python: 3.5
+    env: TOXENV=py35-oauth2client2
+  - python: 3.5
+    env: TOXENV=py35-oauth2client3
+  - python: 3.5
+    env: TOXENV=py35-oauth2client4
+
+  - python: 3.6
+    env: TOXENV=py36-oauth2client1
+  - python: 3.6
+    env: TOXENV=py36-oauth2client2
+  - python: 3.6
+    env: TOXENV=py36-oauth2client3
   - python: 3.6
-    env: TOXENV=py36
+    env: TOXENV=py36-oauth2client4
+
 install:
 - pip install tox coveralls
 script:
diff --git a/googleapiclient/_helpers.py b/googleapiclient/_helpers.py
new file mode 100644
index 000000000..5e8184ba5
--- /dev/null
+++ b/googleapiclient/_helpers.py
@@ -0,0 +1,204 @@
+# Copyright 2015 Google Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Helper functions for commonly used utilities."""
+
+import functools
+import inspect
+import logging
+import warnings
+
+import six
+from six.moves import urllib
+
+
+logger = logging.getLogger(__name__)
+
+POSITIONAL_WARNING = 'WARNING'
+POSITIONAL_EXCEPTION = 'EXCEPTION'
+POSITIONAL_IGNORE = 'IGNORE'
+POSITIONAL_SET = frozenset([POSITIONAL_WARNING, POSITIONAL_EXCEPTION,
+                            POSITIONAL_IGNORE])
+
+positional_parameters_enforcement = POSITIONAL_WARNING
+
+_SYM_LINK_MESSAGE = 'File: {0}: Is a symbolic link.'
+_IS_DIR_MESSAGE = '{0}: Is a directory'
+_MISSING_FILE_MESSAGE = 'Cannot access {0}: No such file or directory'
+
+
+def positional(max_positional_args):
+    """A decorator to declare that only the first N arguments my be positional.
+
+    This decorator makes it easy to support Python 3 style keyword-only
+    parameters. For example, in Python 3 it is possible to write::
+
+        def fn(pos1, *, kwonly1=None, kwonly1=None):
+            ...
+
+    All named parameters after ``*`` must be a keyword::
+
+        fn(10, 'kw1', 'kw2')  # Raises exception.
+        fn(10, kwonly1='kw1')  # Ok.
+
+    Example
+    ^^^^^^^
+
+    To define a function like above, do::
+
+        @positional(1)
+        def fn(pos1, kwonly1=None, kwonly2=None):
+            ...
+
+    If no default value is provided to a keyword argument, it becomes a
+    required keyword argument::
+
+        @positional(0)
+        def fn(required_kw):
+            ...
+
+    This must be called with the keyword parameter::
+
+        fn()  # Raises exception.
+        fn(10)  # Raises exception.
+        fn(required_kw=10)  # Ok.
+
+    When defining instance or class methods always remember to account for
+    ``self`` and ``cls``::
+
+        class MyClass(object):
+
+            @positional(2)
+            def my_method(self, pos1, kwonly1=None):
+                ...
+
+            @classmethod
+            @positional(2)
+            def my_method(cls, pos1, kwonly1=None):
+                ...
+
+    The positional decorator behavior is controlled by
+    ``_helpers.positional_parameters_enforcement``, which may be set to
+    ``POSITIONAL_EXCEPTION``, ``POSITIONAL_WARNING`` or
+    ``POSITIONAL_IGNORE`` to raise an exception, log a warning, or do
+    nothing, respectively, if a declaration is violated.
+
+    Args:
+        max_positional_arguments: Maximum number of positional arguments. All
+                                  parameters after the this index must be
+                                  keyword only.
+
+    Returns:
+        A decorator that prevents using arguments after max_positional_args
+        from being used as positional parameters.
+
+    Raises:
+        TypeError: if a key-word only argument is provided as a positional
+                   parameter, but only if
+                   _helpers.positional_parameters_enforcement is set to
+                   POSITIONAL_EXCEPTION.
+    """
+
+    def positional_decorator(wrapped):
+        @functools.wraps(wrapped)
+        def positional_wrapper(*args, **kwargs):
+            if len(args) > max_positional_args:
+                plural_s = ''
+                if max_positional_args != 1:
+                    plural_s = 's'
+                message = ('{function}() takes at most {args_max} positional '
+                           'argument{plural} ({args_given} given)'.format(
+                               function=wrapped.__name__,
+                               args_max=max_positional_args,
+                               args_given=len(args),
+                               plural=plural_s))
+                if positional_parameters_enforcement == POSITIONAL_EXCEPTION:
+                    raise TypeError(message)
+                elif positional_parameters_enforcement == POSITIONAL_WARNING:
+                    logger.warning(message)
+            return wrapped(*args, **kwargs)
+        return positional_wrapper
+
+    if isinstance(max_positional_args, six.integer_types):
+        return positional_decorator
+    else:
+        args, _, _, defaults = inspect.getargspec(max_positional_args)
+        return positional(len(args) - len(defaults))(max_positional_args)
+
+
+def parse_unique_urlencoded(content):
+    """Parses unique key-value parameters from urlencoded content.
+
+    Args:
+        content: string, URL-encoded key-value pairs.
+
+    Returns:
+        dict, The key-value pairs from ``content``.
+
+    Raises:
+        ValueError: if one of the keys is repeated.
+    """
+    urlencoded_params = urllib.parse.parse_qs(content)
+    params = {}
+    for key, value in six.iteritems(urlencoded_params):
+        if len(value) != 1:
+            msg = ('URL-encoded content contains a repeated value:'
+                   '%s -> %s' % (key, ', '.join(value)))
+            raise ValueError(msg)
+        params[key] = value[0]
+    return params
+
+
+def update_query_params(uri, params):
+    """Updates a URI with new query parameters.
+
+    If a given key from ``params`` is repeated in the ``uri``, then
+    the URI will be considered invalid and an error will occur.
+
+    If the URI is valid, then each value from ``params`` will
+    replace the corresponding value in the query parameters (if
+    it exists).
+
+    Args:
+        uri: string, A valid URI, with potential existing query parameters.
+        params: dict, A dictionary of query parameters.
+
+    Returns:
+        The same URI but with the new query parameters added.
+    """
+    parts = urllib.parse.urlparse(uri)
+    query_params = parse_unique_urlencoded(parts.query)
+    query_params.update(params)
+    new_query = urllib.parse.urlencode(query_params)
+    new_parts = parts._replace(query=new_query)
+    return urllib.parse.urlunparse(new_parts)
+
+
+def _add_query_parameter(url, name, value):
+    """Adds a query parameter to a url.
+
+    Replaces the current value if it already exists in the URL.
+
+    Args:
+        url: string, url to add the query parameter to.
+        name: string, query parameter name.
+        value: string, query parameter value.
+
+    Returns:
+        Updated query parameter. Does not update the url if value is None.
+    """
+    if value is None:
+        return url
+    else:
+        return update_query_params(url, {name: value})
diff --git a/googleapiclient/channel.py b/googleapiclient/channel.py
index a38b4ffbc..0fdb080fe 100644
--- a/googleapiclient/channel.py
+++ b/googleapiclient/channel.py
@@ -61,15 +61,9 @@ import datetime
 import uuid
 
 from googleapiclient import errors
+from googleapiclient import _helpers as util
 import six
 
-# Oauth2client < 3 has the positional helper in 'util', >= 3 has it
-# in '_helpers'.
-try:
-  from oauth2client import util
-except ImportError:
-  from oauth2client import _helpers as util
-
 
 # The unix time epoch starts at midnight 1970.
 EPOCH = datetime.datetime.utcfromtimestamp(0)
diff --git a/googleapiclient/discovery.py b/googleapiclient/discovery.py
index 5c8624902..163f3c95b 100644
--- a/googleapiclient/discovery.py
+++ b/googleapiclient/discovery.py
@@ -73,14 +73,8 @@ from googleapiclient.model import MediaModel
 from googleapiclient.model import RawModel
 from googleapiclient.schema import Schemas
 
-# Oauth2client < 3 has the positional helper in 'util', >= 3 has it
-# in '_helpers'.
-try:
-  from oauth2client.util import _add_query_parameter
-  from oauth2client.util import positional
-except ImportError:
-  from oauth2client._helpers import _add_query_parameter
-  from oauth2client._helpers import positional
+from googleapiclient._helpers import _add_query_parameter
+from googleapiclient._helpers import positional
 
 
 # The client library requires a version of httplib2 that supports RETRIES.
@@ -138,7 +132,7 @@ def fix_method_name(name):
     name: string, method name.
 
   Returns:
-    The name with a '_' prefixed if the name is a reserved word.
+    The name with an '_' appended if the name is a reserved word.
   """
   if keyword.iskeyword(name) or name in RESERVED_WORDS:
     return name + '_'
diff --git a/googleapiclient/errors.py b/googleapiclient/errors.py
index bab14189f..8c4795c4c 100644
--- a/googleapiclient/errors.py
+++ b/googleapiclient/errors.py
@@ -23,12 +23,7 @@ __author__ = 'jcgregorio@google.com (Joe Gregorio)'
 
 import json
 
-# Oauth2client < 3 has the positional helper in 'util', >= 3 has it
-# in '_helpers'.
-try:
-  from oauth2client import util
-except ImportError:
-  from oauth2client import _helpers as util
+from googleapiclient import _helpers as util
 
 
 class Error(Exception):
diff --git a/googleapiclient/http.py b/googleapiclient/http.py
index 17c3714f7..221b1c2b5 100644
--- a/googleapiclient/http.py
+++ b/googleapiclient/http.py
@@ -55,12 +55,7 @@ from email.mime.multipart import MIMEMultipart
 from email.mime.nonmultipart import MIMENonMultipart
 from email.parser import FeedParser
 
-# Oauth2client < 3 has the positional helper in 'util', >= 3 has it
-# in '_helpers'.
-try:
-  from oauth2client import util
-except ImportError:
-  from oauth2client import _helpers as util
+from googleapiclient import _helpers as util
 
 from googleapiclient import _auth
 from googleapiclient.errors import BatchError
@@ -168,10 +163,14 @@ def _retry_request(http, num_retries, req_type, sleep, rand, uri, method, *args,
     # Retry on SSL errors and socket timeout errors.
     except _ssl_SSLError as ssl_error:
       exception = ssl_error
+    except socket.timeout as socket_timeout:
+      # It's important that this be before socket.error as it's a subclass
+      # socket.timeout has no errorcode
+      exception = socket_timeout
     except socket.error as socket_error:
       # errno's contents differ by platform, so we have to match by name.
-      if socket.errno.errorcode.get(socket_error.errno) not in (
-          'WSAETIMEDOUT', 'ETIMEDOUT', 'EPIPE', 'ECONNABORTED', ):
+      if socket.errno.errorcode.get(socket_error.errno) not in {
+        'WSAETIMEDOUT', 'ETIMEDOUT', 'EPIPE', 'ECONNABORTED'}:
         raise
       exception = socket_error
 
diff --git a/googleapiclient/sample_tools.py b/googleapiclient/sample_tools.py
index 5ed632dab..21fede3ec 100644
--- a/googleapiclient/sample_tools.py
+++ b/googleapiclient/sample_tools.py
@@ -27,9 +27,13 @@ import os
 
 from googleapiclient import discovery
 from googleapiclient.http import build_http
-from oauth2client import client
-from oauth2client import file
-from oauth2client import tools
+
+try:
+    from oauth2client import client
+    from oauth2client import file
+    from oauth2client import tools
+except ImportError:
+    raise ImportError('googleapiclient.sample_tools requires oauth2client. Please install oauth2client and try again.')
 
 
 def init(argv, name, version, doc, filename, scope=None, parents=[], discovery_filename=None):
diff --git a/googleapiclient/schema.py b/googleapiclient/schema.py
index 160d388d8..10d4a1b59 100644
--- a/googleapiclient/schema.py
+++ b/googleapiclient/schema.py
@@ -65,12 +65,7 @@ __author__ = 'jcgregorio@google.com (Joe Gregorio)'
 
 import copy
 
-# Oauth2client < 3 has the positional helper in 'util', >= 3 has it
-# in '_helpers'.
-try:
-  from oauth2client import util
-except ImportError:
-  from oauth2client import _helpers as util
+from googleapiclient import _helpers as util
 
 
 class Schemas(object):


Test patch:
diff --git a/tests/test__helpers.py b/tests/test__helpers.py
new file mode 100644
index 000000000..e33ea7153
--- /dev/null
+++ b/tests/test__helpers.py
@@ -0,0 +1,161 @@
+# Copyright 2015 Google Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unit tests for googleapiclient._helpers."""
+
+import unittest
+
+import mock
+
+import six
+from six.moves import urllib
+
+from googleapiclient import _helpers
+
+
+class PositionalTests(unittest.TestCase):
+
+    def test_usage(self):
+        _helpers.positional_parameters_enforcement = (
+            _helpers.POSITIONAL_EXCEPTION)
+
+        # 1 positional arg, 1 keyword-only arg.
+        @_helpers.positional(1)
+        def function(pos, kwonly=None):
+            return True
+
+        self.assertTrue(function(1))
+        self.assertTrue(function(1, kwonly=2))
+        with self.assertRaises(TypeError):
+            function(1, 2)
+
+        # No positional, but a required keyword arg.
+        @_helpers.positional(0)
+        def function2(required_kw):
+            return True
+
+        self.assertTrue(function2(required_kw=1))
+        with self.assertRaises(TypeError):
+            function2(1)
+
+        # Unspecified positional, should automatically figure out 1 positional
+        # 1 keyword-only (same as first case above).
+        @_helpers.positional
+        def function3(pos, kwonly=None):
+            return True
+
+        self.assertTrue(function3(1))
+        self.assertTrue(function3(1, kwonly=2))
+        with self.assertRaises(TypeError):
+            function3(1, 2)
+
+    @mock.patch('googleapiclient._helpers.logger')
+    def test_enforcement_warning(self, mock_logger):
+        _helpers.positional_parameters_enforcement = (
+            _helpers.POSITIONAL_WARNING)
+
+        @_helpers.positional(1)
+        def function(pos, kwonly=None):
+            return True
+
+        self.assertTrue(function(1, 2))
+        self.assertTrue(mock_logger.warning.called)
+
+    @mock.patch('googleapiclient._helpers.logger')
+    def test_enforcement_ignore(self, mock_logger):
+        _helpers.positional_parameters_enforcement = _helpers.POSITIONAL_IGNORE
+
+        @_helpers.positional(1)
+        def function(pos, kwonly=None):
+            return True
+
+        self.assertTrue(function(1, 2))
+        self.assertFalse(mock_logger.warning.called)
+
+
+class AddQueryParameterTests(unittest.TestCase):
+
+    def test__add_query_parameter(self):
+        self.assertEqual(
+            _helpers._add_query_parameter('/action', 'a', None),
+            '/action')
+        self.assertEqual(
+            _helpers._add_query_parameter('/action', 'a', 'b'),
+            '/action?a=b')
+        self.assertEqual(
+            _helpers._add_query_parameter('/action?a=b', 'a', 'c'),
+            '/action?a=c')
+        # Order is non-deterministic.
+        self.assertIn(
+            _helpers._add_query_parameter('/action?a=b', 'c', 'd'),
+            ['/action?a=b&c=d', '/action?c=d&a=b'])
+        self.assertEqual(
+            _helpers._add_query_parameter('/action', 'a', ' ='),
+            '/action?a=+%3D')
+
+
+def assertUrisEqual(testcase, expected, actual):
+    """Test that URIs are the same, up to reordering of query parameters."""
+    expected = urllib.parse.urlparse(expected)
+    actual = urllib.parse.urlparse(actual)
+    testcase.assertEqual(expected.scheme, actual.scheme)
+    testcase.assertEqual(expected.netloc, actual.netloc)
+    testcase.assertEqual(expected.path, actual.path)
+    testcase.assertEqual(expected.params, actual.params)
+    testcase.assertEqual(expected.fragment, actual.fragment)
+    expected_query = urllib.parse.parse_qs(expected.query)
+    actual_query = urllib.parse.parse_qs(actual.query)
+    for name in expected_query.keys():
+        testcase.assertEqual(expected_query[name], actual_query[name])
+    for name in actual_query.keys():
+        testcase.assertEqual(expected_query[name], actual_query[name])
+
+
+class Test_update_query_params(unittest.TestCase):
+
+    def test_update_query_params_no_params(self):
+        uri = 'http://www.google.com'
+        updated = _helpers.update_query_params(uri, {'a': 'b'})
+        self.assertEqual(updated, uri + '?a=b')
+
+    def test_update_query_params_existing_params(self):
+        uri = 'http://www.google.com?x=y'
+        updated = _helpers.update_query_params(uri, {'a': 'b', 'c': 'd&'})
+        hardcoded_update = uri + '&a=b&c=d%26'
+        assertUrisEqual(self, updated, hardcoded_update)
+
+    def test_update_query_params_replace_param(self):
+        base_uri = 'http://www.google.com'
+        uri = base_uri + '?x=a'
+        updated = _helpers.update_query_params(uri, {'x': 'b', 'y': 'c'})
+        hardcoded_update = base_uri + '?x=b&y=c'
+        assertUrisEqual(self, updated, hardcoded_update)
+
+    def test_update_query_params_repeated_params(self):
+        uri = 'http://www.google.com?x=a&x=b'
+        with self.assertRaises(ValueError):
+            _helpers.update_query_params(uri, {'a': 'c'})
+
+
+class Test_parse_unique_urlencoded(unittest.TestCase):
+
+    def test_without_repeats(self):
+        content = 'a=b&c=d'
+        result = _helpers.parse_unique_urlencoded(content)
+        self.assertEqual(result, {'a': 'b', 'c': 'd'})
+
+    def test_with_repeats(self):
+        content = 'a=b&a=d'
+        with self.assertRaises(ValueError):
+            _helpers.parse_unique_urlencoded(content)
diff --git a/tests/test_discovery.py b/tests/test_discovery.py
index 94027e3de..01c67ef66 100644
--- a/tests/test_discovery.py
+++ b/tests/test_discovery.py
@@ -78,10 +78,7 @@ from googleapiclient.model import JsonModel
 from oauth2client import GOOGLE_TOKEN_URI
 from oauth2client.client import OAuth2Credentials, GoogleCredentials
 
-try:
-  from oauth2client import util
-except ImportError:
-  from oauth2client import _helpers as util
+from googleapiclient import _helpers as util
 
 import uritemplate
 
diff --git a/tests/test_http.py b/tests/test_http.py
index f33740144..f6beff712 100644
--- a/tests/test_http.py
+++ b/tests/test_http.py
@@ -125,24 +125,26 @@ class HttpMockWithErrors(object):
       return httplib2.Response(self.success_json), self.success_data
     else:
       self.num_errors -= 1
-      if self.num_errors == 1:
+      if self.num_errors == 1:  # initial == 2
         raise ssl.SSLError()
-      else:
-        if PY3:
-          ex = TimeoutError()
-        else:
-          ex = socket.error()
+      else:  # initial != 2
         if self.num_errors == 2:
-          #first try a broken pipe error (#218)
+          # first try a broken pipe error (#218)
+          ex = socket.error()
           ex.errno = socket.errno.EPIPE
         else:
           # Initialize the timeout error code to the platform's error code.
           try:
             # For Windows:
+            ex = socket.error()
             ex.errno = socket.errno.WSAETIMEDOUT
           except AttributeError:
             # For Linux/Mac:
-            ex.errno = socket.errno.ETIMEDOUT
+            if PY3:
+              ex = socket.timeout()
+            else:
+              ex = socket.error()
+              ex.errno = socket.errno.ETIMEDOUT
         # Now raise the correct error.
         raise ex