Add missing generation arguments for bucket and blob methods. by jmcarp · Pull Request #7023 · googleapis/google-cloud-python

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.

Sign in to view

        return self.bucket.user_project

+    @property
+    def query_params(self):


Sign in to view

+        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()

Sign in to view

+        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

+            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

+            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

+            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

+
        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

+        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

+
        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

@@ -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

+        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

 # 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

        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

+        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

+        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

Comment thread 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


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

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

Conversation

jmcarp commented Dec 25, 2018

Uh oh!

tseaver commented Jan 3, 2019

Uh oh!

jmcarp commented Jan 4, 2019

Uh oh!

jmcarp commented Jan 12, 2019

Uh oh!

frankyn commented Jan 12, 2019

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

jmcarp commented Jan 14, 2019

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

jmcarp commented Jan 18, 2019

Uh oh!

jmcarp commented Jan 26, 2019

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmcarp commented Feb 12, 2019

Uh oh!

frankyn commented Feb 12, 2019

Uh oh!

tseaver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants