feat: Storage object retention lock #11368

anuragsrivstv · 2023-11-29T19:32:39Z

amanda-tarafa · 2023-11-30T05:30:15Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClientImpl.UploadObject.cs

+
+            if (options?.RetentionMode != null || options?.RetainUntilTime != null)
+            {
+                destination.Retention = new Object.RetentionData();
+                if (options.RetentionMode != null)
+                {
+                    destination.Retention.Mode = options.RetentionMode.ToString();
+                }
+                if (options.RetainUntilTime != null)
+                {
+                    destination.Retention.RetainUntilTimeDateTimeOffset = options.RetainUntilTime;
+                }
+            }


Can't we not do this on the UploadObjectOptions.ModifyMediaUpload method? That's why it's there for.

Yes, we can actually. updated the code.

amanda-tarafa · 2023-11-30T05:32:06Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClientImpl.UpdateObject.cs

+            }
+            else
+            {
+                obj.Retention = null;


Do we need to set null explicitly?

Yes, we need to. This is for the use case where we need to remove retention from an object. So in that case fetched bucket object will already have a retention configure on it, which we need to mark as null and update.

Okay, on thinking more on it, there could be also a use case where user is updating an object and at the same time wants existing retention of the object to be same as it is( not removed ). In that case they may not provide RetentionMode and RetainUntilTime.
To handle such cases as well do you suggest that we do not mark existing retention property of fetched object null here. Introduce a new bool property say options.RemoveRetention and mark retention of existing object as null only if it is set to true ? What do you think ?

amanda-tarafa · 2023-11-30T05:33:07Z

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/StorageClientImpl.UpdateObject.cs

+
+            if (options?.RetentionMode != null || options?.RetainUntilTime != null)
+            {
+                obj.Retention = new Object.RetentionData
+                {
+                    Mode = options.RetentionMode,
+                    RetainUntilTimeDateTimeOffset = options.RetainUntilTime
+                };
+            }
+            else
+            {


We can do all of this on the ModifyRequest method of the options as well.

amanda-tarafa · 2023-11-30T05:34:23Z

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

+        /// <summary>
+        /// When set to true, object retention is enabled for this bucket.
+        /// </summary>
+        public bool EnableObjectRetention { get; set; }


This probably should be, and then we only set it if it has a value. This is to allow for explicitly setting one value or another.

Suggested change

public bool EnableObjectRetention { get; set; }

public bool? EnableObjectRetention { get; set; }

jskeet · 2023-11-30T11:43:13Z

(FYI, I'll wait until @amanda-tarafa is happy, then I'll do a final pass.)

amanda-tarafa

As for your question of how to unset a value vs. how to leave it as is withou explicitly specifying it, that's why we have both update and patch operations.

amanda-tarafa · 2023-11-30T17:28:24Z

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

+            {
+                request.OverrideUnlockedRetention = OverrideUnlockedRetention;
+            }
+            if (RetentionMode != null || RetainUntilTime != null)


Actually, I don't think we need these bits in the optios as calling code is able to build the object itself and set there whatever they need.

We only need OverrideUnlockRetention because that's a request field, and its the request what we build internally for the users.

Makes sense. Updated the code change.

amanda-tarafa · 2023-11-30T17:34:13Z

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

+                upload.Body.Retention = new Object.RetentionData();
+                if (RetentionMode != null)
+                {
+                    upload.Body.Retention.Mode = RetentionMode;
+                }
+                if (RetainUntilTime != null)
+                {
+                    upload.Body.Retention.RetainUntilTimeDateTimeOffset = RetainUntilTime;


Similarly here, we have a pubic overload that acceots the object to upload so users can use that directly to set these.

Yes, updated the code.

amanda-tarafa

Looking better now, still some issues with tests.

amanda-tarafa · 2023-12-01T20:03:09Z

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

@@ -206,5 +264,12 @@ private Object CreateObject(string bucketName, string objectName)
            var bytes = Encoding.UTF8.GetBytes(text);
            return _fixture.Client.UploadObject(bucketName, objectName, "text/plain", new MemoryStream(bytes));
        }
+
+        private Object CreateObject(Object obj)


This one is probably better in the Fixture. Seems like we have one already that's local to another method, but we might be able to easily make it internal.

And ping on this comment?

I'd probably remove the method entirely, and just inline _fixture.Client.UploadObject(obj, new MemoryStream(_fixture.SmallContent)); unless we really need the specific text.

amanda-tarafa · 2023-12-01T20:05:04Z

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

+            CreateObject(newObject);
+            var obj = client.GetObject(bucketName, objectName);
+            Assert.Equal("Unlocked", obj.Retention?.Mode);
+            Assert.Equal(baseDateTime.AddDays(5).Date, obj.Retention.RetainUntilTimeDateTimeOffset.Value.Date);


Or am I missing something?

Suggested change

Assert.Equal(baseDateTime.AddDays(5).Date, obj.Retention.RetainUntilTimeDateTimeOffset.Value.Date);

Assert.Equal(baseDateTime.AddDays(5), obj.Retention.RetainUntilTimeDateTimeOffset.Value);

Exact value comparison fails in milliseconds comparison. Therefore I've compared the dates as the purpose of this test is to verify that we are able to set retention value on the object.

Ah, I see, presumably because the field granularity in Storage is less than .NET DateTimeOffset? If that's the case, then that's fine, but a note would be helpful.

Also, flagging to @jskeet as relevant to the Discovery date/time value handling changes made recently.

I would suggest truncating baseDateTime to the millisecond to start with (with a comment), then performing an exact comparison.

amanda-tarafa · 2023-12-01T20:05:40Z

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

+
+            Assert.Throws<GoogleApiException>(() => client.DeleteObject(bucketName, objectName));
+
+            // Update the retainUntilTime of the object


This should be another test.

It could be, but to set-up that test(to test object retention is updated or not) we will still need to create a bucket and object with retention enabled and set respectively, which is the first part of this integration test. So if the first integration test fails then also both tests will fail( one in Assert and another in setting up test object). So as the tests are not entirely independent I've tested them together. Thoughts ?

amanda-tarafa · 2023-12-01T20:05:53Z

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

+
+            Assert.Throws<GoogleApiException>(() => client.DeleteObject(bucketName, objectName));
+
+            // Remove retention for existing object.


And this should be another test.

amanda-tarafa · 2023-12-01T20:07:11Z

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

+        /// Must be true to remove the retention configuration, reduce its unlocked retention period, or change its
+        /// mode from unlocked to locked.
+        /// </summary>
+        public bool? OverrideUnlockedRetention { get; set; }


You are not unit testing this change specifically.

+1 for this.

amanda-tarafa

@anuragsrivstv I've left a couple more comments, but it's fine if you prefer one of us to address them. Thanks!!

Also, @jskeet this is mostly ready for your review, save for my comments around tests.

amanda-tarafa · 2023-12-05T08:49:52Z

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

+            CreateObject(newObject);
+            var obj = client.GetObject(bucketName, objectName);
+            Assert.Equal("Unlocked", obj.Retention?.Mode);
+            Assert.Equal(baseDateTime.AddDays(5).Date, obj.Retention.RetainUntilTimeDateTimeOffset.Value.Date);


Ah, I see, presumably because the field granularity in Storage is less than .NET DateTimeOffset? If that's the case, then that's fine, but a note would be helpful.

Also, flagging to @jskeet as relevant to the Discovery date/time value handling changes made recently.

amanda-tarafa · 2023-12-05T09:06:29Z

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

@@ -206,5 +264,12 @@ private Object CreateObject(string bucketName, string objectName)
            var bytes = Encoding.UTF8.GetBytes(text);
            return _fixture.Client.UploadObject(bucketName, objectName, "text/plain", new MemoryStream(bytes));
        }
+
+        private Object CreateObject(Object obj)


And ping on this comment?

jskeet · 2023-12-05T09:10:59Z

I'll try to review this in the next couple of days, but there's a lot going on.

jskeet · 2023-12-05T12:42:08Z

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

+                    Mode = "Unlocked",
+                    RetainUntilTimeDateTimeOffset = baseDateTime.AddDays(5),
+                }
+


Nit: remove blank line.

jskeet · 2023-12-05T12:42:16Z

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

+            var baseDateTime = DateTimeOffset.UtcNow;
+
+            string objectName = "object.txt";
+            // Create an object, with retention configured.\


Nit: remove trailing backslash.

jskeet · 2023-12-05T12:44:13Z

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

+            CreateObject(newObject);
+            var obj = client.GetObject(bucketName, objectName);
+            Assert.Equal("Unlocked", obj.Retention?.Mode);
+            Assert.Equal(baseDateTime.AddDays(5).Date, obj.Retention.RetainUntilTimeDateTimeOffset.Value.Date);


I would suggest truncating baseDateTime to the millisecond to start with (with a comment), then performing an exact comparison.

jskeet · 2023-12-05T12:44:46Z

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

+            var bucketName = _fixture.GenerateBucketName();
+
+            // Create a bucket with object retention enabled.
+            var createBucketoptions = new CreateBucketOptions { EnableObjectRetention = true };


So what happens if the code below is tried without object retention enabled? I think that would probably be worth another test.

jskeet · 2023-12-05T12:46:21Z

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

+            };
+            CreateObject(newObject);
+            var obj = client.GetObject(bucketName, objectName);
+            Assert.Equal("Unlocked", obj.Retention?.Mode);


I'd remove the ? here - that way we can tell the difference between obj.Retention being null (NRE) and obj.Retention.Mode being null (assertion failure).

jskeet · 2023-12-05T12:47:41Z

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

@@ -206,5 +264,12 @@ private Object CreateObject(string bucketName, string objectName)
            var bytes = Encoding.UTF8.GetBytes(text);
            return _fixture.Client.UploadObject(bucketName, objectName, "text/plain", new MemoryStream(bytes));
        }
+
+        private Object CreateObject(Object obj)


I'd probably remove the method entirely, and just inline _fixture.Client.UploadObject(obj, new MemoryStream(_fixture.SmallContent)); unless we really need the specific text.

jskeet · 2023-12-05T12:48:30Z

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

+            var retentionRemovedObj = client.GetObject(bucketName, objectName);
+            Assert.Null(retentionRemovedObj.Retention);
+
+            client.DeleteObject(bucketName, objectName);


I'd add a comment here to indicate that this is really part of the test, not just clean-up: once we've removed the lock, we should be able to delete the object.

jskeet · 2023-12-05T12:48:37Z

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

+
+            // Remove retention for existing object.
+            updatedObj.Retention = null;
+            var removeobjectRetentionOption = new UpdateObjectOptions { OverrideUnlockedRetention = true, };


Nit: remove trailing comma.

jskeet · 2023-12-05T12:50:27Z

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

+        /// Must be true to remove the retention configuration, reduce its unlocked retention period, or change its
+        /// mode from unlocked to locked.
+        /// </summary>
+        public bool? OverrideUnlockedRetention { get; set; }


+1 for this.

jskeet · 2024-01-02T12:16:32Z

I'll fetch this locally and make the changes that Amanda and I had requested.

Closes googleapis#11227 Closes googleapis#11368

Closes #11227 Closes #11368

anuragsrivstv requested review from a team as code owners November 29, 2023 19:32

anuragsrivstv force-pushed the gcs-object-lock branch from a961fcb to 247b01d Compare November 29, 2023 19:35

anuragsrivstv changed the title ~~feat: Object Retention Lock~~ feat: Storage object retention lock Nov 29, 2023

amanda-tarafa requested changes Nov 30, 2023

View reviewed changes

anuragsrivstv force-pushed the gcs-object-lock branch from 247b01d to b309456 Compare November 30, 2023 09:41

anuragsrivstv force-pushed the gcs-object-lock branch from b309456 to 1397ed4 Compare November 30, 2023 12:25

anuragsrivstv requested a review from amanda-tarafa November 30, 2023 16:59

amanda-tarafa requested changes Nov 30, 2023

View reviewed changes

anuragsrivstv force-pushed the gcs-object-lock branch from 1397ed4 to 118a64c Compare December 1, 2023 05:46

anuragsrivstv requested a review from amanda-tarafa December 1, 2023 05:48

feat: Storage object retention lock

07b8c7b

anuragsrivstv force-pushed the gcs-object-lock branch from 118a64c to 07b8c7b Compare December 1, 2023 07:45

amanda-tarafa requested changes Dec 1, 2023

View reviewed changes

amanda-tarafa reviewed Dec 5, 2023

View reviewed changes

jskeet reviewed Dec 5, 2023

View reviewed changes

jskeet self-assigned this Jan 2, 2024

amanda-tarafa assigned amanda-tarafa and unassigned jskeet Feb 14, 2024

amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this pull request Mar 5, 2024

feat: Support object retention

6d20fd3

Closes googleapis#11227 Closes googleapis#11368

amanda-tarafa mentioned this pull request Mar 5, 2024

feat: Support object retention #12043

Merged

amanda-tarafa closed this in #12043 Mar 5, 2024

amanda-tarafa added a commit that referenced this pull request Mar 5, 2024

feat: Support object retention

6d73d8e

Closes #11227 Closes #11368

	public bool EnableObjectRetention { get; set; }
	public bool? EnableObjectRetention { get; set; }

	Assert.Equal(baseDateTime.AddDays(5).Date, obj.Retention.RetainUntilTimeDateTimeOffset.Value.Date);
	Assert.Equal(baseDateTime.AddDays(5), obj.Retention.RetainUntilTimeDateTimeOffset.Value);


		Assert.Throws<GoogleApiException>(() => client.DeleteObject(bucketName, objectName));

		// Update the retainUntilTime of the object


		Assert.Throws<GoogleApiException>(() => client.DeleteObject(bucketName, objectName));

		// Remove retention for existing object.

feat: Storage object retention lock #11368

feat: Storage object retention lock #11368

Conversation

anuragsrivstv commented Nov 29, 2023 • edited by jskeet Loading

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

anuragsrivstv Nov 30, 2023 • edited Loading

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

jskeet commented Nov 30, 2023

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

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

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

jskeet commented Dec 5, 2023

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

jskeet commented Jan 2, 2024

anuragsrivstv commented Nov 29, 2023 •

edited by jskeet

Loading

anuragsrivstv Nov 30, 2023 •

edited

Loading