Add missing generation arguments for bucket and blob methods. #7023

jmcarp · 2018-12-25T17:35:43Z

Allow setting generation for getting and deleting blobs, and respect generation for rewriting.

tseaver · 2019-01-03T22:49:57Z

What is implemented here looks OK, but seems like it might not be comprehensive enough (e.g., should Blob.reload / Blob.exists / Blob.delete support generations?).

@frankyn PTAL.

jmcarp · 2019-01-04T01:53:33Z

Thanks @tseaver. I took a stab at adding generations to mixed-in methods like reload and update. WDYT?

jmcarp · 2019-01-12T00:34:22Z

Ping @tseaver @frankyn when you have time.

frankyn · 2019-01-12T00:36:29Z

Thanks for the ping. Looking over it now.

frankyn

I did a first pass. Let's follow-up next week. Thank you for your patience.

storage/google/cloud/storage/blob.py

@@ -257,6 +257,16 @@ def user_project(self):
        """
        return self.bucket.user_project

+    @property
+    def query_params(self):


storage/google/cloud/storage/bucket.py

+        if self.user_project is not None:
+            params["userProject"] = self.user_project
+        return params
+
    def blob(self, blob_name, chunk_size=None, encryption_key=None, kms_key_name=None):
        """Factory constructor for blob object.


@frankyn

…ructor. h/t @frankyn

jmcarp · 2019-01-14T04:22:01Z

Thanks @frankyn. Updated the PR.

frankyn

Please add system tests for this feature as well in (https://github.com/googleapis/google-cloud-python/blob/master/storage/tests/system.py) with respect to a generation:

blob.update()
blob.patch()
blob.delete()
bucket.get_object()

storage/tests/unit/test_bucket.py

+        connection = _Connection({"name": BLOB_NAME})
+        client = _Client(connection)
+        bucket = self._make_one(name=NAME)
+        blob = bucket.get_blob(BLOB_NAME, client=client, generation=GENERATION)


jmcarp · 2019-01-18T05:18:49Z

Updated. Looks like we already exercise the Blob constructor with generation in https://github.com/jmcarp/google-cloud-python/blob/blob-generations/storage/tests/unit/test_blob.py#L104-L109.

jmcarp · 2019-01-26T03:58:34Z

Ping @frankyn when you have time; ready for another look.

frankyn

Overall, lgtm. Just had two last comments then I'll approve. @tseaver do you see anything else?

frankyn · 2019-01-31T21:57:55Z

storage/tests/system.py

+            blob0.content_language = "en"
+            blob0.patch()
+            self.assertEqual(blob0.content_language, "en")
+            self.assertIsNone(blob1.content_language)


Is this required? Seems unnecessary based on the previous line.

We're patching the language of the 0th generation of the blob, so we expect the metadata of the 1st generation to still be None.

ahhh I missed that it's blob1 and not blob0 thank you.

frankyn · 2019-01-31T21:58:14Z

storage/tests/system.py

+            blob0.metadata = metadata
+            blob0.update()
+            self.assertEqual(blob0.metadata, metadata)
+            self.assertIsNone(blob1.metadata)


Same here, doesn't look necessary if metadata for sure is not None.

jmcarp · 2019-02-12T23:35:16Z

Anything else I should do on this @frankyn @JustinBeckwith?

frankyn · 2019-02-12T23:51:03Z

@crwilcox I'm good with the merge wdyt?

tseaver

@jmcarp Thanks very much for the patch, and apologies for taking so long to review it.

tseaver · 2019-02-20T17:19:20Z

storage/google/cloud/storage/blob.py

@@ -387,6 +406,9 @@ def exists(self, client=None):
        # minimize the returned payload.
        query_params = {"fields": "name"}

+        if self.generation:
+            query_params["generation"] = self.generation
+
        if self.user_project is not None:
            query_params["userProject"] = self.user_project


Seems like this block should use the _query_params property, e.g.:

# minimize the returned payload. query_params = self._query_params query_params["fields"] = "name"

tseaver · 2019-02-20T17:21:11Z

storage/google/cloud/storage/blob.py

@@ -1485,6 +1509,9 @@ def rewrite(self, source, token=None, client=None):
        if token:
            query_params["rewriteToken"] = token

+        if source.generation:
+            query_params["sourceGeneration"] = source.generation
+
        if self.user_project is not None:
            query_params["userProject"] = self.user_project



Likewise here:

query_params = self._query_params if token: query_params["rewriteToken"] = token if source.generation: query_params["sourceGeneration"] = source.generation if self.kms_key_name is not None: query_params["destinationKmsKeyName"] = self.kms_key_name

tseaver · 2019-02-20T17:23:26Z

storage/google/cloud/storage/_helpers.py

+        params = {}
+        if self.user_project is not None:
+            params["userProject"] = self.user_project
+        return params


This method is shadowed by both Blob and Bucket: let's make it virtual, e.g.:

@property def _query_params(self): """Default query parameters.""" raise NotImplementedError

That way we don't have to add test coverage for it, either.

Looking at how it is used in reload, I've changed my mind: lets just get rid of the duplicate version in Bucket.

tseaver · 2019-02-20T17:29:06Z

storage/google/cloud/storage/bucket.py

+
+        if generation is not None:
+            query_params["generation"] = generation
+
        blob = Blob(
            bucket=self, name=blob_name, encryption_key=encryption_key, **kwargs
        )


I'd rather create the blob with the expected generation here, and then use its _query_params property, e.g.:

client = self._require_client(client) blob = Blob( bucket=self, name=blob_name, encryption_key=encryption_key, generation=generation, **kwargs ) query_params = blob._query_params try: headers = _get_encryption_headers(encryption_key) response = client._connection.api_request( method="GET", path=blob.path, query_params=query_params, headers=headers, _target_object=blob, )

Now that I look at it more closely, I'm not sure why this method doesn't just call blob.reload().

tseaver · 2019-02-20T17:33:01Z

storage/google/cloud/storage/bucket.py

@@ -825,6 +859,9 @@ def delete_blob(self, blob_name, client=None):
        client = self._require_client(client)
        query_params = {}


Use self._query_params here as well, e.g.:

query_params = self._query_params if generation is not None: query_params["generation"] = generation blob_path = Blob.path_helper(self.path, blob_name)

tseaver · 2019-02-20T17:41:11Z

storage/google/cloud/storage/bucket.py

+        params = {}
+        if self.user_project is not None:
+            params["userProject"] = self.user_project
+        return params


Please add explicit test coverage for the branches in this property.

Per above, let's just delete this version of the property, which duplicates the one in the base class.

tseaver · 2019-02-20T17:42:49Z

storage/tests/system.py

@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

+import io


Import will be unused if you switch to blob.upload_from_string below.

tseaver · 2019-02-20T17:46:29Z

storage/tests/unit/test_blob.py

@@ -576,7 +583,7 @@ def test_delete(self):
        bucket._blobs[BLOB_NAME] = 1
        blob.delete()
        self.assertFalse(blob.exists())
-        self.assertEqual(bucket._deleted, [(BLOB_NAME, None)])
+        self.assertEqual(bucket._deleted, [(BLOB_NAME, None, blob.generation)])


Please duplicate this test, once with an explicit generation, and make this assertion be clear for both cases (None for the case where the blob is created without a generation).

tseaver · 2019-02-20T17:48:07Z

storage/tests/unit/test_blob.py

+        client = _Client(connection)
+        source_bucket = _Bucket(client=client)
+        source_blob = self._make_one(SOURCE_BLOB, bucket=source_bucket)
+        source_blob._set_properties({"generation": SOURCE_GENERATION})


Please pass the generation to _make_one, e.g.:

source_blob = self._make_one(SOURCE_BLOB, bucket=source_bucket, generation=GENERATION)

tseaver · 2019-02-20T17:53:22Z

storage/tests/unit/test_bucket.py

+        NAME = "name"
+        BLOB_NAME = "blob-name"
+        GENERATION = 1512565576797178
+        connection = _Connection({"name": BLOB_NAME})


For clarity, add the generation to the properties here, and then assert that it is read in correctly below:

connection = _Connection({"name": BLOB_NAME, generation=GENERATION})

Add missing generation arguments for bucket and blob methods.

81849bd

jmcarp requested a review from crwilcox as a code owner December 25, 2018 17:35

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 25, 2018

tseaver added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: storage Issues related to the Cloud Storage API. labels Jan 3, 2019

tseaver requested a review from frankyn January 3, 2019 22:49

Add generic query parameters to storage property helper.

231e8b4

frankyn suggested changes Jan 12, 2019

View reviewed changes

Privatize storage query param helper; accept generation in blob const…

8bb96d0

…ructor. h/t @frankyn

frankyn suggested changes Jan 15, 2019

View reviewed changes

storage/tests/unit/test_bucket.py

connection = _Connection({"name": BLOB_NAME})

client = _Client(connection)

bucket = self._make_one(name=NAME)

blob = bucket.get_blob(BLOB_NAME, client=client, generation=GENERATION)

This comment was marked as spam.

Sign in to view

Add system tests for blob generations.

31174f6

jmcarp force-pushed the blob-generations branch from 2aa77a3 to 31174f6 Compare January 18, 2019 05:08

frankyn suggested changes Jan 31, 2019

View reviewed changes

frankyn approved these changes Feb 4, 2019

View reviewed changes

JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019

yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Feb 7, 2019

tseaver suggested changes Feb 20, 2019

View reviewed changes

tseaver added needs work This is a pull request that needs a little love. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed 🚨 This issue needs some love. labels Feb 20, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2019

tseaver mentioned this pull request Feb 25, 2019

Storage: add generation arguments (from #7023). #7444

Merged

tseaver closed this in #7444 Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing generation arguments for bucket and blob methods. #7023

Add missing generation arguments for bucket and blob methods. #7023

jmcarp commented Dec 25, 2018

tseaver commented Jan 3, 2019

jmcarp commented Jan 4, 2019

jmcarp commented Jan 12, 2019

frankyn commented Jan 12, 2019

frankyn left a comment

This comment was marked as spam.

This comment was marked as spam.

jmcarp commented Jan 14, 2019

frankyn left a comment

This comment was marked as spam.

jmcarp commented Jan 18, 2019

jmcarp commented Jan 26, 2019

frankyn left a comment

frankyn Jan 31, 2019

jmcarp Feb 3, 2019

frankyn Feb 4, 2019

frankyn Jan 31, 2019

jmcarp commented Feb 12, 2019

frankyn commented Feb 12, 2019

tseaver left a comment

tseaver Feb 20, 2019

tseaver Feb 20, 2019

tseaver Feb 20, 2019

tseaver Feb 25, 2019

tseaver Feb 20, 2019

tseaver Feb 20, 2019

tseaver Feb 20, 2019

tseaver Feb 25, 2019

tseaver Feb 20, 2019

tseaver Feb 20, 2019

tseaver Feb 20, 2019

tseaver Feb 20, 2019

		@@ -825,6 +859,9 @@ def delete_blob(self, blob_name, client=None):
		client = self._require_client(client)
		query_params = {}

Add missing generation arguments for bucket and blob methods. #7023

Add missing generation arguments for bucket and blob methods. #7023

Conversation

jmcarp commented Dec 25, 2018

tseaver commented Jan 3, 2019

jmcarp commented Jan 4, 2019

jmcarp commented Jan 12, 2019

frankyn commented Jan 12, 2019

frankyn left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

jmcarp commented Jan 14, 2019

frankyn left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

jmcarp commented Jan 18, 2019

jmcarp commented Jan 26, 2019

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarp commented Feb 12, 2019

frankyn commented Feb 12, 2019

tseaver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment