-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Storage object retention lock #11368
Conversation
a961fcb
to
247b01d
Compare
|
||
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; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we not do this on the UploadObjectOptions.ModifyMediaUpload method? That's why it's there for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can actually. updated the code.
} | ||
else | ||
{ | ||
obj.Retention = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set null explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
|
||
if (options?.RetentionMode != null || options?.RetainUntilTime != null) | ||
{ | ||
obj.Retention = new Object.RetentionData | ||
{ | ||
Mode = options.RetentionMode, | ||
RetainUntilTimeDateTimeOffset = options.RetainUntilTime | ||
}; | ||
} | ||
else | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do all of this on the ModifyRequest method of the options as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/// <summary> | ||
/// When set to true, object retention is enabled for this bucket. | ||
/// </summary> | ||
public bool EnableObjectRetention { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public bool EnableObjectRetention { get; set; } | |
public bool? EnableObjectRetention { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
247b01d
to
b309456
Compare
(FYI, I'll wait until @amanda-tarafa is happy, then I'll do a final pass.) |
b309456
to
1397ed4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
{ | ||
request.OverrideUnlockedRetention = OverrideUnlockedRetention; | ||
} | ||
if (RetentionMode != null || RetainUntilTime != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Updated the code change.
upload.Body.Retention = new Object.RetentionData(); | ||
if (RetentionMode != null) | ||
{ | ||
upload.Body.Retention.Mode = RetentionMode; | ||
} | ||
if (RetainUntilTime != null) | ||
{ | ||
upload.Body.Retention.RetainUntilTimeDateTimeOffset = RetainUntilTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, we have a pubic overload that acceots the object to upload so users can use that directly to set these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, updated the code.
1397ed4
to
118a64c
Compare
118a64c
to
07b8c7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better now, still some issues with tests.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And ping on this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or am I missing something?
Assert.Equal(baseDateTime.AddDays(5).Date, obj.Retention.RetainUntilTimeDateTimeOffset.Value.Date); | |
Assert.Equal(baseDateTime.AddDays(5), obj.Retention.RetainUntilTimeDateTimeOffset.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest truncating baseDateTime
to the millisecond to start with (with a comment), then performing an exact comparison.
|
||
Assert.Throws<GoogleApiException>(() => client.DeleteObject(bucketName, objectName)); | ||
|
||
// Update the retainUntilTime of the object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be another test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
|
||
Assert.Throws<GoogleApiException>(() => client.DeleteObject(bucketName, objectName)); | ||
|
||
// Remove retention for existing object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this should be another test.
/// 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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not unit testing this change specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And ping on this comment?
I'll try to review this in the next couple of days, but there's a lot going on. |
Mode = "Unlocked", | ||
RetainUntilTimeDateTimeOffset = baseDateTime.AddDays(5), | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove blank line.
var baseDateTime = DateTimeOffset.UtcNow; | ||
|
||
string objectName = "object.txt"; | ||
// Create an object, with retention configured.\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove trailing backslash.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest truncating baseDateTime
to the millisecond to start with (with a comment), then performing an exact comparison.
var bucketName = _fixture.GenerateBucketName(); | ||
|
||
// Create a bucket with object retention enabled. | ||
var createBucketoptions = new CreateBucketOptions { EnableObjectRetention = true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens if the code below is tried without object retention enabled? I think that would probably be worth another test.
}; | ||
CreateObject(newObject); | ||
var obj = client.GetObject(bucketName, objectName); | ||
Assert.Equal("Unlocked", obj.Retention?.Mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
var retentionRemovedObj = client.GetObject(bucketName, objectName); | ||
Assert.Null(retentionRemovedObj.Retention); | ||
|
||
client.DeleteObject(bucketName, objectName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// Remove retention for existing object. | ||
updatedObj.Retention = null; | ||
var removeobjectRetentionOption = new UpdateObjectOptions { OverrideUnlockedRetention = true, }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove trailing comma.
/// 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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this.
I'll fetch this locally and make the changes that Amanda and I had requested. |
Fixes #11227.