feat: Support object soft delete #12060

amanda-tarafa · 2024-03-06T08:58:06Z

No description provided.

amanda-tarafa · 2024-03-06T09:01:45Z

do not merge until the RestoreObjectRequest changes (remove the Object body) have been published in Google.Apis.Storage.v1.

jskeet

Just comment nits and an observation about the underlying API.

jskeet · 2024-03-06T10:46:44Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/StorageFixture.cs

+                    Name = name,
+                    Versioning = new Bucket.VersioningData { Enabled = multiVersion },
+                    // The minimum allowed for soft delete is 7 days.
+                    SoftDeletePolicy = softDelete ? new Bucket.SoftDeletePolicyData { RetentionDurationSeconds = 7*24*60*60 } : null,


Nit: spaces between the 7*24*60*60 - or better, work that out earlier:

var sevenDaysInSeconds = (int) TimeSpan.FromDays(7).TotalSeconds;

(Or preferably NodaConstants.SecondsPerWeek, but hey... one day I'll justify a NodaTime dependency ;)

Done, using TimeSpan.

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetObjectOptions.cs

jskeet · 2024-03-06T10:50:54Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetObjectOptions.cs

@@ -30,6 +30,11 @@ public sealed class GetObjectOptions
        /// </summary>
        public long? Generation { get; set; }

+        /// <summary>
+        /// If true, only soft-deleted object versions will be listed. The default is false.


We're not listing, just getting, so this should probably be "only soft-deleted object versions will be retrieved" or similar.

Yep, changing it. I did copied this verbatim from the API field docs.

jskeet · 2024-03-06T10:51:30Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    public bool? CopySourceAcl { get; set; }
+
+    /// <summary>
+    /// The projection of the restored objct to return.


Possibly make it clear that this doesn't affect what's actually restored?

jskeet · 2024-03-06T10:52:45Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// there are no live versions of the object.
+    /// </summary>
+    /// <remarks>
+    /// Note that you specify the generation value when calling the one of the restore object operations.


I think we should reword this remark to clarify the difference between the value of this property and the value specified on the RestoreObject call.

Yes. Done here and elsewhere.

jskeet · 2024-03-06T10:53:16Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// Makes the operation conditional on whether the object's current
+    /// metageneration matches the given value.
+    /// </summary>
+    /// <remarks>


Possibly remove the remark here and below as the metageneration and generation are different?

I've reworded in a similar way as the above, but I'm happy to remove the remarks here entirely.

I think with the change in the remarks, this makes a lot more sense to keep, thanks.

jskeet · 2024-03-06T10:54:09Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.RestoreObject.cs

+    /// </summary>
+    /// <param name="bucket">The name of the bucket containing the object. Must not be null.</param>
+    /// <param name="objectName">The name of the object to restore. Must not be null.</param>
+    /// <param name="generation">The specific revision of the object to restore.</param>


Does Storage use the term "revision"? Maybe we should use "generation" here instead.

The underlying docs read: "Selects a specific revision of this object." and the underlying parameter is named "generation" :(

They may be used interchangeably by Storage? I'm happy yo change to generation if you think it better?

Let's stick with the underlying docs for consistency with them (even if they're inconsistent themselves).

jskeet · 2024-03-06T10:54:36Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.RestoreObject.cs

+    /// <param name="generation">The specific revision of the object to restore.</param>
+    /// <param name="options">Additional options for the restore operation. May be null, in which case appropriate
+    /// defaults will be used.</param>
+    /// <returns>The <see cref="Object"/> representation restored Storage object.</returns>


Add "of the" between "representation" and "restored".

amanda-tarafa

@jskeet all comments addressed in a new commit prefixed review:, PTAL.

amanda-tarafa · 2024-03-06T20:38:31Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/StorageFixture.cs

+                    Name = name,
+                    Versioning = new Bucket.VersioningData { Enabled = multiVersion },
+                    // The minimum allowed for soft delete is 7 days.
+                    SoftDeletePolicy = softDelete ? new Bucket.SoftDeletePolicyData { RetentionDurationSeconds = 7*24*60*60 } : null,


Done, using TimeSpan.

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetObjectOptions.cs

amanda-tarafa · 2024-03-06T21:20:48Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetObjectOptions.cs

@@ -30,6 +30,11 @@ public sealed class GetObjectOptions
        /// </summary>
        public long? Generation { get; set; }

+        /// <summary>
+        /// If true, only soft-deleted object versions will be listed. The default is false.


Yep, changing it. I did copied this verbatim from the API field docs.

amanda-tarafa · 2024-03-06T21:24:04Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    public bool? CopySourceAcl { get; set; }
+
+    /// <summary>
+    /// The projection of the restored objct to return.


amanda-tarafa · 2024-03-06T21:50:58Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// there are no live versions of the object.
+    /// </summary>
+    /// <remarks>
+    /// Note that you specify the generation value when calling the one of the restore object operations.


Yes. Done here and elsewhere.

amanda-tarafa · 2024-03-06T23:05:47Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// Makes the operation conditional on whether the object's current
+    /// metageneration matches the given value.
+    /// </summary>
+    /// <remarks>


I've reworded in a similar way as the above, but I'm happy to remove the remarks here entirely.

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetObjectOptions.cs

amanda-tarafa · 2024-03-06T23:20:28Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.RestoreObject.cs

+    /// </summary>
+    /// <param name="bucket">The name of the bucket containing the object. Must not be null.</param>
+    /// <param name="objectName">The name of the object to restore. Must not be null.</param>
+    /// <param name="generation">The specific revision of the object to restore.</param>


The underlying docs read: "Selects a specific revision of this object." and the underlying parameter is named "generation" :(

They may be used interchangeably by Storage? I'm happy yo change to generation if you think it better?

amanda-tarafa · 2024-03-06T23:21:17Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.RestoreObject.cs

+    /// <param name="generation">The specific revision of the object to restore.</param>
+    /// <param name="options">Additional options for the restore operation. May be null, in which case appropriate
+    /// defaults will be used.</param>
+    /// <returns>The <see cref="Object"/> representation restored Storage object.</returns>


apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetObjectOptions.cs

jskeet · 2024-03-07T07:38:08Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// <remarks>
+    /// Note that the generation value passed to the restore operations (a) specifies the exact generation
+    /// that should be restored, whereas this value (b) is a constraint on the current generation of the object
+    /// for the oepration to succeed. Basically, restore version a if current generation is not b.


oepration => operation

And I'd possibly parenthesize a and b in the final sentence again, to match the previous sentence. Possibly rephrase the final sentence to:

Effectively, this means "restore version (a) if the current generation is not (b)".

(There's something about "basically" which doesn't sit quite right with me...)

All done, here and in the others.

jskeet · 2024-03-07T07:38:34Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// Makes the operation conditional on whether the object's current
+    /// metageneration matches the given value.
+    /// </summary>
+    /// <remarks>


I think with the change in the remarks, this makes a lot more sense to keep, thanks.

jskeet · 2024-03-07T07:39:13Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClient.RestoreObject.cs

+    /// </summary>
+    /// <param name="bucket">The name of the bucket containing the object. Must not be null.</param>
+    /// <param name="objectName">The name of the object to restore. Must not be null.</param>
+    /// <param name="generation">The specific revision of the object to restore.</param>


Let's stick with the underlying docs for consistency with them (even if they're inconsistent themselves).

amanda-tarafa

Latest review addressed in last commit.
And now we wait.

amanda-tarafa · 2024-03-11T19:04:44Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// <remarks>
+    /// Note that the generation value passed to the restore operations (a) specifies the exact generation
+    /// that should be restored, whereas this value (b) is a constraint on the current generation of the object
+    /// for the oepration to succeed. Basically, restore version a if current generation is not b.


All done, here and in the others.

amanda-tarafa · 2024-03-25T16:52:26Z

@jskeet this can now be merged. Last commit are the changes required because of the Discovery based breaking changes. PTAL.

jskeet

A few nits, but that's all.

jskeet · 2024-03-25T17:04:01Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/RestoreObjectTest.cs

@@ -0,0 +1,50 @@
+// Copyright 2015 Google Inc. All Rights Reserved.


jskeet · 2024-03-25T17:05:53Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    public bool? CopySourceAcl { get; set; }
+
+    /// <summary>
+    /// The projection of the restored objct to return. Note the whole object will be restored,


Nit: objct => object

jskeet · 2024-03-25T17:06:13Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// <summary>
+    /// The projection of the restored objct to return. Note the whole object will be restored,
+    /// except for the object's access controls, see <see cref="CopySourceAcl"/> for that. This only affects
+    /// what information is returned when restoration is succesful.


Nit: succesful => successful

Prefixes are available through `PagedEnumerable<Objects, Object>.AsRawResponses()` it's not ideal but it's not awful either. If we get actual user demand for ease of access we can reconsider.

amanda-tarafa

All comments addressed. Will merge on green.

amanda-tarafa · 2024-03-25T17:20:31Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/RestoreObjectTest.cs

@@ -0,0 +1,50 @@
+// Copyright 2015 Google Inc. All Rights Reserved.


amanda-tarafa · 2024-03-25T17:22:11Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    public bool? CopySourceAcl { get; set; }
+
+    /// <summary>
+    /// The projection of the restored objct to return. Note the whole object will be restored,


amanda-tarafa · 2024-03-25T17:22:31Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RestoreObjectOptions.cs

+    /// <summary>
+    /// The projection of the restored objct to return. Note the whole object will be restored,
+    /// except for the object's access controls, see <see cref="CopySourceAcl"/> for that. This only affects
+    /// what information is returned when restoration is succesful.


amanda-tarafa requested a review from jskeet March 6, 2024 08:58

amanda-tarafa requested review from a team as code owners March 6, 2024 08:58

amanda-tarafa added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 6, 2024

jskeet approved these changes Mar 6, 2024

View reviewed changes

amanda-tarafa force-pushed the storage-soft-delete branch from 362515d to 1897eb5 Compare March 6, 2024 23:27

amanda-tarafa commented Mar 6, 2024

View reviewed changes

jskeet reviewed Mar 7, 2024

View reviewed changes

amanda-tarafa force-pushed the storage-soft-delete branch from 1897eb5 to 92e2ae7 Compare March 11, 2024 19:14

amanda-tarafa commented Mar 11, 2024

View reviewed changes

amanda-tarafa force-pushed the storage-soft-delete branch from 92e2ae7 to dfd5e7c Compare March 25, 2024 16:49

amanda-tarafa requested a review from a team as a code owner March 25, 2024 16:49

amanda-tarafa removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 25, 2024

jskeet approved these changes Mar 25, 2024

View reviewed changes

amanda-tarafa added 3 commits March 25, 2024 10:23

chore: Remove TODO regarding how to return prefixes.

88b2bf9

Prefixes are available through `PagedEnumerable<Objects, Object>.AsRawResponses()` it's not ideal but it's not awful either. If we get actual user demand for ease of access we can reconsider.

build: Update Google.Cloud.Storage.V1 Discovery based dependencies

d4a0691

feat: Support object soft delete

2c10a32

amanda-tarafa force-pushed the storage-soft-delete branch from dfd5e7c to 2c10a32 Compare March 25, 2024 17:24

amanda-tarafa commented Mar 25, 2024

View reviewed changes

amanda-tarafa merged commit ae28cb4 into googleapis:main Mar 25, 2024
10 checks passed

amanda-tarafa deleted the storage-soft-delete branch March 25, 2024 17:31

		@@ -0,0 +1,50 @@
		// Copyright 2015 Google Inc. All Rights Reserved.

feat: Support object soft delete #12060

feat: Support object soft delete #12060

Conversation

amanda-tarafa commented Mar 6, 2024

amanda-tarafa commented Mar 6, 2024

jskeet 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

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

amanda-tarafa 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

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa commented Mar 25, 2024

jskeet 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

amanda-tarafa 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