refactor: add / use 'Client._put_resource' method (#441)

Toward #38.

Author: tseaver

packages/google-cloud-storage/google/cloud/storage/_helpers.py

@@ -416,14 +416,13 @@ def update(
             if_metageneration_not_match=if_metageneration_not_match,
         )
 
-        api_response = client._connection.api_request(
-            method="PUT",
-            path=self.path,
-            data=self._properties,
+        api_response = client._put_resource(
+            self.path,
+            self._properties,
             query_params=query_params,
-            _target_object=self,
             timeout=timeout,
             retry=retry,
+            _target_object=self,
         )
         self._set_properties(api_response)
 

packages/google-cloud-storage/google/cloud/storage/blob.py

@@ -2898,16 +2898,16 @@ def set_iam_policy(
         if self.user_project is not None:
             query_params["userProject"] = self.user_project
 
+        path = "{}/iam".format(self.path)
         resource = policy.to_api_repr()
         resource["resourceId"] = self.path
-        info = client._connection.api_request(
-            method="PUT",
-            path="%s/iam" % (self.path,),
+        info = client._put_resource(
+            path,
+            resource,
             query_params=query_params,
-            data=resource,
-            _target_object=None,
             timeout=timeout,
             retry=retry,
+            _target_object=None,
         )
         return Policy.from_api_repr(info)
 

packages/google-cloud-storage/google/cloud/storage/bucket.py

@@ -2943,17 +2943,19 @@ def set_iam_policy(
         if self.user_project is not None:
             query_params["userProject"] = self.user_project
 
+        path = "{}/iam".format(self.path)
         resource = policy.to_api_repr()
         resource["resourceId"] = self.path
-        info = client._connection.api_request(
-            method="PUT",
-            path="%s/iam" % (self.path,),
+
+        info = client._put_resource(
+            path,
+            resource,
             query_params=query_params,
-            data=resource,
-            _target_object=None,
             timeout=timeout,
             retry=retry,
+            _target_object=None,
         )
+
         return Policy.from_api_repr(info)
 
     def test_iam_permissions(

packages/google-cloud-storage/google/cloud/storage/client.py

@@ -457,6 +457,77 @@ def _patch_resource(
             _target_object=_target_object,
         )
 
+    def _put_resource(
+        self,
+        path,
+        data,
+        query_params=None,
+        headers=None,
+        timeout=_DEFAULT_TIMEOUT,
+        retry=DEFAULT_RETRY,
+        _target_object=None,
+    ):
+        """Helper for bucket / blob methods making API 'PUT' calls.
+
+        Args:
+            path str:
+                The path of the resource to fetch.
+
+            data dict:
+                The data to be patched.
+
+            query_params Optional[dict]:
+                HTTP query parameters to be passed
+
+            headers Optional[dict]:
+                HTTP headers to be passed
+
+            timeout (Optional[Union[float, Tuple[float, float]]]):
+                The amount of time, in seconds, to wait for the server response.
+
+                Can also be passed as a tuple (connect_timeout, read_timeout).
+                See :meth:`requests.Session.request` documentation for details.
+
+            retry (Optional[Union[google.api_core.retry.Retry, google.cloud.storage.retry.ConditionalRetryPolicy]]):
+                How to retry the RPC. A None value will disable retries.
+                A google.api_core.retry.Retry value will enable retries, and the object will
+                define retriable response codes and errors and configure backoff and timeout options.
+
+                A google.cloud.storage.retry.ConditionalRetryPolicy value wraps a Retry object and
+                activates it only if certain conditions are met. This class exists to provide safe defaults
+                for RPC calls that are not technically safe to retry normally (due to potential data
+                duplication or other side-effects) but become safe to retry if a condition such as
+                if_metageneration_match is set.
+
+                See the retry.py source code and docstrings in this package (google.cloud.storage.retry) for
+                information on retry types and how to configure them.
+
+            _target_object (Union[ \
+                :class:`~google.cloud.storage.bucket.Bucket`, \
+                :class:`~google.cloud.storage.bucket.blob`, \
+            ]):
+                Object to which future data is to be applied -- only relevant
+                in the context of a batch.
+
+        Returns:
+            dict
+                The JSON resource fetched
+
+        Raises:
+            google.cloud.exceptions.NotFound
+                If the bucket is not found.
+        """
+        return self._connection.api_request(
+            method="PUT",
+            path=path,
+            data=data,
+            query_params=query_params,
+            headers=headers,
+            timeout=timeout,
+            retry=retry,
+            _target_object=_target_object,
+        )
+
     def get_bucket(
         self,
         bucket_or_name,

packages/google-cloud-storage/google/cloud/storage/hmac_key.py

@@ -298,13 +298,8 @@ def update(self, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY_IF_ETAG_IN_JSON):
             qs_params["userProject"] = self.user_project
 
         payload = {"state": self.state}
-        self._properties = self._client._connection.api_request(
-            method="PUT",
-            path=self.path,
-            data=payload,
-            query_params=qs_params,
-            timeout=timeout,
-            retry=retry,
+        self._properties = self._client._put_resource(
+            self.path, payload, query_params=qs_params, timeout=timeout, retry=retry,
         )
 
     def delete(self, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY):

packages/google-cloud-storage/tests/unit/test__helpers.py

@@ -349,82 +349,100 @@ def test_patch_w_user_project_w_explicit_client(self):
             _target_object=derived,
         )
 
-    def test_update(self):
-        connection = _Connection({"foo": "Foo"})
-        client = _Client(connection)
-        derived = self._derivedClass("/path")()
+    def test_update_w_defaults(self):
+        path = "/path"
+        api_response = {"foo": "Foo"}
+        derived = self._derivedClass(path)()
         # Make sure changes is non-empty, so we can observe a change.
-        BAR = object()
-        BAZ = object()
-        derived._properties = {"bar": BAR, "baz": BAZ}
+        bar = object()
+        baz = object()
+        expected_data = derived._properties = {"bar": bar, "baz": baz}
         derived._changes = set(["bar"])  # Update sends 'baz' anyway.
-        derived.update(client=client, timeout=42)
-        self.assertEqual(derived._properties, {"foo": "Foo"})
-        kw = connection._requested
-        self.assertEqual(len(kw), 1)
-        self.assertEqual(kw[0]["method"], "PUT")
-        self.assertEqual(kw[0]["path"], "/path")
-        self.assertEqual(kw[0]["query_params"], {"projection": "full"})
-        self.assertEqual(kw[0]["data"], {"bar": BAR, "baz": BAZ})
-        self.assertEqual(kw[0]["timeout"], 42)
-        self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED)
-        # Make sure changes get reset by patch().
+        client = derived.client = mock.Mock(spec=["_put_resource"])
+        client._put_resource.return_value = api_response
+
+        derived.update()
+
+        self.assertEqual(derived._properties, api_response)
+        # Make sure changes get reset by update().
         self.assertEqual(derived._changes, set())
 
-    def test_update_with_metageneration_not_match(self):
-        GENERATION_NUMBER = 6
+        expected_query_params = {"projection": "full"}
+        client._put_resource.assert_called_once_with(
+            path,
+            expected_data,
+            query_params=expected_query_params,
+            timeout=self._get_default_timeout(),
+            retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
+            _target_object=derived,
+        )
 
-        connection = _Connection({"foo": "Foo"})
-        client = _Client(connection)
-        derived = self._derivedClass("/path")()
+    def test_update_with_metageneration_not_match_w_timeout_w_retry(self):
+        path = "/path"
+        generation_number = 6
+        api_response = {"foo": "Foo"}
+        derived = self._derivedClass(path)()
         # Make sure changes is non-empty, so we can observe a change.
-        BAR = object()
-        BAZ = object()
-        derived._properties = {"bar": BAR, "baz": BAZ}
+        bar = object()
+        baz = object()
+        expected_data = derived._properties = {"bar": bar, "baz": baz}
         derived._changes = set(["bar"])  # Update sends 'baz' anyway.
+        client = derived.client = mock.Mock(spec=["_put_resource"])
+        client._put_resource.return_value = api_response
+        timeout = 42
+
         derived.update(
-            client=client, timeout=42, if_metageneration_not_match=GENERATION_NUMBER
+            if_metageneration_not_match=generation_number, timeout=timeout,
         )
+
         self.assertEqual(derived._properties, {"foo": "Foo"})
-        kw = connection._requested
-        self.assertEqual(len(kw), 1)
-        self.assertEqual(kw[0]["method"], "PUT")
-        self.assertEqual(kw[0]["path"], "/path")
-        self.assertEqual(
-            kw[0]["query_params"],
-            {"projection": "full", "ifMetagenerationNotMatch": GENERATION_NUMBER},
-        )
-        self.assertEqual(kw[0]["data"], {"bar": BAR, "baz": BAZ})
-        self.assertEqual(kw[0]["timeout"], 42)
-        self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED)
         # Make sure changes get reset by patch().
         self.assertEqual(derived._changes, set())
 
-    def test_update_w_user_project(self):
+        expected_query_params = {
+            "projection": "full",
+            "ifMetagenerationNotMatch": generation_number,
+        }
+        client._put_resource.assert_called_once_with(
+            path,
+            expected_data,
+            query_params=expected_query_params,
+            timeout=timeout,
+            retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
+            _target_object=derived,
+        )
+
+    def test_update_w_user_project_w_retry_w_explicit_client(self):
         user_project = "user-project-123"
-        connection = _Connection({"foo": "Foo"})
-        client = _Client(connection)
-        derived = self._derivedClass("/path", user_project)()
+        path = "/path"
+        api_response = {"foo": "Foo"}
+        derived = self._derivedClass(path, user_project)()
         # Make sure changes is non-empty, so we can observe a change.
-        BAR = object()
-        BAZ = object()
-        derived._properties = {"bar": BAR, "baz": BAZ}
+        bar = object()
+        baz = object()
+        expected_data = derived._properties = {"bar": bar, "baz": baz}
         derived._changes = set(["bar"])  # Update sends 'baz' anyway.
-        derived.update(client=client)
-        self.assertEqual(derived._properties, {"foo": "Foo"})
-        kw = connection._requested
-        self.assertEqual(len(kw), 1)
-        self.assertEqual(kw[0]["method"], "PUT")
-        self.assertEqual(kw[0]["path"], "/path")
-        self.assertEqual(
-            kw[0]["query_params"], {"projection": "full", "userProject": user_project}
-        )
-        self.assertEqual(kw[0]["data"], {"bar": BAR, "baz": BAZ})
-        self.assertEqual(kw[0]["timeout"], self._get_default_timeout())
-        self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED)
+        client = mock.Mock(spec=["_put_resource"])
+        client._put_resource.return_value = api_response
+        retry = mock.Mock(spec=[])
+
+        derived.update(client=client, retry=retry)
         # Make sure changes get reset by patch().
         self.assertEqual(derived._changes, set())
 
+        expected_query_params = {
+            "projection": "full",
+            "userProject": user_project,
+        }
+        client._put_resource.assert_called_once_with(
+            path,
+            expected_data,
+            query_params=expected_query_params,
+            timeout=self._get_default_timeout(),
+            retry=retry,
+            _target_object=derived,
+        )
+
 
 class Test__scalar_property(unittest.TestCase):
     def _call_fut(self, fieldName):
@@ -575,17 +593,6 @@ def test_hostname_and_scheme(self):
         self.assertEqual(self._call_fut(host=HOST, scheme=SCHEME), EXPECTED_URL)
 
 
-class _Connection(object):
-    def __init__(self, *responses):
-        self._responses = responses
-        self._requested = []
-
-    def api_request(self, **kw):
-        self._requested.append(kw)
-        response, self._responses = self._responses[0], self._responses[1:]
-        return response
-
-
 class _MD5Hash(object):
     def __init__(self, digest_val):
         self.digest_val = digest_val
@@ -617,8 +624,3 @@ def __init__(self):
     def b64encode(self, value):
         self._called_b64encode.append(value)
         return value
-
-
-class _Client(object):
-    def __init__(self, connection):
-        self._connection = connection