Repository: googleapis/python-storage

Issue:
Blob.metadata not preserved when uploading new file content
`blob.metadata` is not preserved when uploading new file content on an existing Blob instance with `blob.upload_from_string(...)` unless you first reset it with `blob.metadata = blob.metadata`.

I suspect this bug / surprising behaviour is the result of the somewhat awkward mapping of the REST API onto mutable objects. It feels like setting metadata should really be a parameter of the blob upload methods instead of a mutable field on the instance.

#### Environment details

  - OS type and version:

Both locally on my Mac and within Google Cloud Functions

  - Python version: `python --version`

Python 3.8.5

  - pip version: `pip --version`

pip 20.1.1

  - `google-cloud-storage` version: `pip show google-cloud-storage`

google-cloud-storage==1.31.2

#### Steps to reproduce

  See code example.

#### Code example

```python
>>> b = bucket.blob('testme1')
>>> b.metadata = {'test': 'me'}
>>> b.upload_from_string('foo')
>>> b.metadata
{'test': 'me'}
>>> b.upload_from_string('bar')
>>> b.metadata
>>> # Note how b.metadata is not blank

>>> b = bucket.blob('testme2')
>>> b.metadata = {'test': 'me'}
>>> b.upload_from_string('foo')
>>> b.metadata
{'test': 'me'}
>>> b.metadata = b.metadata # <-- workaround
>>> b.upload_from_string('bar')
>>> b.metadata
{'test': 'me'}
>>> # Note how b.metadata is now preserved
```


Discussion:
frankyn: @andrewsg PTAL when you have a moment.
HemangChothani: @tseaver PTAL!

Patch:
diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py
index f63303a..d7303e0 100644
--- a/google/cloud/storage/blob.py
+++ b/google/cloud/storage/blob.py
@@ -1608,6 +1608,8 @@ class Blob(_PropertyMixin):
                 raise ValueError(msg)
 
         transport = self._get_transport(client)
+        if "metadata" in self._properties and "metadata" not in self._changes:
+            self._changes.add("metadata")
         info = self._get_upload_arguments(content_type)
         headers, object_metadata, content_type = info
 
@@ -1775,6 +1777,8 @@ class Blob(_PropertyMixin):
                 chunk_size = _DEFAULT_CHUNKSIZE
 
         transport = self._get_transport(client)
+        if "metadata" in self._properties and "metadata" not in self._changes:
+            self._changes.add("metadata")
         info = self._get_upload_arguments(content_type)
         headers, object_metadata, content_type = info
         if extra_headers is not None:


Test patch:
diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py
index f713861..63f98eb 100644
--- a/tests/unit/test_blob.py
+++ b/tests/unit/test_blob.py
@@ -1828,12 +1828,17 @@ class Test_Blob(unittest.TestCase):
         if_metageneration_not_match=None,
         kms_key_name=None,
         timeout=None,
+        metadata=None,
     ):
         from six.moves.urllib.parse import urlencode
 
         bucket = _Bucket(name="w00t", user_project=user_project)
         blob = self._make_one(u"blob-name", bucket=bucket, kms_key_name=kms_key_name)
         self.assertIsNone(blob.chunk_size)
+        if metadata:
+            self.assertIsNone(blob.metadata)
+            blob._properties["metadata"] = metadata
+            self.assertEqual(len(blob._changes), 0)
 
         # Create mocks to be checked for doing transport.
         transport = self._mock_transport(http_client.OK, {})
@@ -1906,10 +1911,18 @@ class Test_Blob(unittest.TestCase):
 
         upload_url += "?" + urlencode(qs_params)
 
+        blob_data = b'{"name": "blob-name"}\r\n'
+        if metadata:
+            blob_data = (
+                b'{"name": "blob-name", "metadata": '
+                + json.dumps(metadata).encode("utf-8")
+                + b"}\r\n"
+            )
+            self.assertEqual(blob._changes, set(["metadata"]))
         payload = (
             b"--==0==\r\n"
             + b"content-type: application/json; charset=UTF-8\r\n\r\n"
-            + b'{"name": "blob-name"}\r\n'
+            + blob_data
             + b"--==0==\r\n"
             + b"content-type: application/xml\r\n\r\n"
             + data_read
@@ -1974,6 +1987,10 @@ class Test_Blob(unittest.TestCase):
             mock_get_boundary, if_generation_not_match=4, if_metageneration_not_match=4
         )
 
+    @mock.patch(u"google.resumable_media._upload.get_boundary", return_value=b"==0==")
+    def test__do_multipart_upload_with_metadata(self, mock_get_boundary):
+        self._do_multipart_success(mock_get_boundary, metadata={"test": "test"})
+
     def test__do_multipart_upload_bad_size(self):
         blob = self._make_one(u"blob-name", bucket=None)
 
@@ -2006,6 +2023,7 @@ class Test_Blob(unittest.TestCase):
         blob_chunk_size=786432,
         kms_key_name=None,
         timeout=None,
+        metadata=None,
     ):
         from six.moves.urllib.parse import urlencode
         from google.resumable_media.requests import ResumableUpload
@@ -2013,7 +2031,12 @@ class Test_Blob(unittest.TestCase):
 
         bucket = _Bucket(name="whammy", user_project=user_project)
         blob = self._make_one(u"blob-name", bucket=bucket, kms_key_name=kms_key_name)
-        blob.metadata = {"rook": "takes knight"}
+        if metadata:
+            self.assertIsNone(blob.metadata)
+            blob._properties["metadata"] = metadata
+            self.assertEqual(len(blob._changes), 0)
+        else:
+            blob.metadata = {"rook": "takes knight"}
         blob.chunk_size = blob_chunk_size
         if blob_chunk_size is not None:
             self.assertIsNotNone(blob.chunk_size)
@@ -2022,8 +2045,11 @@ class Test_Blob(unittest.TestCase):
 
         # Need to make sure **same** dict is used because ``json.dumps()``
         # will depend on the hash order.
-        object_metadata = blob._get_writable_metadata()
-        blob._get_writable_metadata = mock.Mock(return_value=object_metadata, spec=[])
+        if not metadata:
+            object_metadata = blob._get_writable_metadata()
+            blob._get_writable_metadata = mock.Mock(
+                return_value=object_metadata, spec=[]
+            )
 
         # Create mocks to be checked for doing transport.
         resumable_url = "http://test.invalid?upload_id=hey-you"
@@ -2107,6 +2133,8 @@ class Test_Blob(unittest.TestCase):
             self.assertNotEqual(blob.chunk_size, chunk_size)
             self.assertEqual(upload._chunk_size, chunk_size)
         self.assertIs(upload._stream, stream)
+        if metadata:
+            self.assertEqual(blob._changes, set(["metadata"]))
         if size is None:
             self.assertIsNone(upload._total_bytes)
         else:
@@ -2125,8 +2153,11 @@ class Test_Blob(unittest.TestCase):
         # Make sure we never read from the stream.
         self.assertEqual(stream.tell(), 0)
 
-        # Check the mocks.
-        blob._get_writable_metadata.assert_called_once_with()
+        if metadata:
+            object_metadata = {"name": u"blob-name", "metadata": metadata}
+        else:
+            # Check the mocks.
+            blob._get_writable_metadata.assert_called_once_with()
         payload = json.dumps(object_metadata).encode("utf-8")
         expected_headers = {
             "content-type": "application/json; charset=UTF-8",
@@ -2144,6 +2175,9 @@ class Test_Blob(unittest.TestCase):
             timeout=expected_timeout,
         )
 
+    def test__initiate_resumable_upload_with_metadata(self):
+        self._initiate_resumable_helper(metadata={"test": "test"})
+
     def test__initiate_resumable_upload_with_custom_timeout(self):
         self._initiate_resumable_helper(timeout=9.58)