feat(storage): support bucket restore in soft delete phase 2 #13894

mahendra-google · 2024-11-28T12:39:02Z

Feature request includes the following:-

Get Bucket Generation
Get a Soft Deleted Bucket (Also soft-delete time and hard-delete time)
List Soft Deleted Buckets
Restore a Soft Deleted Bucket

jskeet

This is not a full code review by any means, but I'm hoping that this little bit will reduce the number of iterations required after Amanda is back from Thanksgiving.

Directory.Packages.props

mahendra-google · 2024-11-28T13:06:58Z

This is not a full code review by any means, but I'm hoping that this little bit will reduce the number of iterations required after Amanda is back from Thanksgiving.

Ok

jskeet · 2024-11-29T10:46:24Z

Unassigning myself from review - I'm on vacation until Tuesday. (I expect Amanda will review before then.)

amanda-tarafa · 2024-12-03T17:59:49Z

I'll review today. @mahendra-google if you can rebase on main to remove conflicts, that'd be great, thanks.

mahendra-google · 2024-12-04T09:28:40Z

I'll review today. @mahendra-google if you can rebase on main to remove conflicts, that'd be great, thanks.

@amanda-tarafa I have removed conflict for Directory.Packages.props file.

mahendra-google · 2024-12-06T05:24:08Z

I'll review today. @mahendra-google if you can rebase on main to remove conflicts, that'd be great, thanks.

@amanda-tarafa I have removed conflict for Directory.Packages.props file.

@amanda-tarafa I have removed project file conflict.

mahendra-google · 2024-12-06T05:34:07Z

@amanda-tarafa Storage Samples requires latest package of Google.Cloud.Storage.V1 having properties of GetBucketOptions , ListBucketOptions, RestoreBucketOptions to work sample as expected. So for samples can I proceed with lastest dlls?

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

amanda-tarafa

You need to add unit tests for all the option classes that you modified. See #12060 for examples.

apis/apis.json

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.csproj

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

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

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

apis/apis.json

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.csproj

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

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

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

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.Tests/RestoreBucketOptionsTest.cs

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

mahendra-google · 2025-01-29T02:36:27Z

You need to add unit tests for all the option classes that you modified. See #12060 for examples.

Unit tests for all the option classes are added that I have modified

…ly.github.com> Co-authored-by: Amanda Tarafa Mas <[email protected]> modified: generator-input/apis.json

…val of package version override

danielduhh · 2025-02-10T17:23:18Z

This is ready for a re-review @amanda-tarafa

amanda-tarafa · 2025-02-11T06:17:27Z

Thanks, I'll do a pass tomorrow.

amanda-tarafa

Almost there, thanks!

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

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.csproj

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

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.Tests/ListBucketsOptionsTest.cs

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

amanda-tarafa · 2025-02-11T23:23:54Z

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

+            foreach (var bucket in actualBuckets)
+            {   // Verify if the bucket is soft-deleted only 
+                Assert.NotNull(bucket.Generation);
+                Assert.NotNull(bucket.SoftDeleteTimeDateTimeOffset);
+                Assert.NotNull(bucket.HardDeleteTimeDateTimeOffset);
+
+                if (bucket.Name == softDeleteBucket.Name)
+                {   // Compare the generation number
+                    Assert.Equal(bucket.Generation, softDeleteBucket.Generation);
+                }
+            }


Please use Assert.All and Assert.Contains instead of a loop here.

@amanda-tarafa Is it not working as expected ? because these changes are approved by My team's silver language leads using the Internal PR review approval process.

There are different ways to achieve the same result, some are clearer than others. Please use Assert.All and Asssert.Contains.

Actually, is not only about clarity, this test is not sufficient as it is. It will pass even if actualBuckets is empty or if it does not contains softDeleteBucket. Using Assert.All and Assert.Contains will make the test sufficient.

@amanda-tarafa If actualBuckets is made empty (actualBuckets = new List<Bucket>();), count will be zero , it will not go inside foreach loop , So it will not pass , I would rather say it will skip assert. Let me know your thoughts on this

@amanda-tarafa On the same lines , if I don't create a soft delete bucket in test itself (which I already created if I remove that piece of code of creation of soft delete bucket ) and hypothetically assuming storage fixture has also not created any soft delete bucket in any other tests , According to me in that case also actualBuckets count will be zero and it will not go inside loop. So it will skip asserts. Does that mean test should fail rather skip ?

A test that executes no asserts, assuming there are no exceptions (none in this case), is a passing test, it's not a skipped test. A passing test indicates that the feature has been tested and everything is working as expected, whereas a skipped test indicates that nothing was tested. This test is wrong because it may pass even when nothing is tested.

Explicitly, if there is a bug in production code that has the effect of the list returning empty (even when you have just created a soft deleted bucket), this test will pass, when it shouldn't have. That's because this test is saying:

Create a soft deleted bucket (correct, we guarantee there's at least one soft deleted bucket so we can test).

Fetch soft deleted buckets (the feature we want to test).

If there happens to be elements in the list, make sure they are all soft deleted. (hmm, if the feature we want to test is broken so that the list is empty, then we really don't test anything and we don't find out the feature is broken)

If the recently created bucket happens to be on the list, make sure all its fields have the expected values and that it is indeed soft deleted. (hmm, if the feature we want to test is broken so that the list is empty, then we really don't test anything and we don't find out the feature is broken)

The key being the "if it happens" where a test needs to make certain "it happened", no ifs and no buts.

What this test needs to do is:

Create a soft deleted bucket

Fetch soft deleted buckets

Make sure the recently created bucket is on the list, and all its fields have the expected values.

Make sure all the elements on the list are indeed soft deleted. Apart from the test own bucket, these may be buckets created by the fixture or by other tests, we don't care if there are none of these, or many, because we don't care about the order in which tests execute. The only thing that we care is that all tests in the list are soft deleted because that's the feature we are testing.

See those two last points describing the correct test, how they very naturally transalate to:

Assert.Contains recently created bucket with the right fields.

Assert.All are soft deletede buckets

You can certainly express the same semantic with a loop and some checks, but, it's way less naturally readable plus, errors like the one currently in this test are harder to spot and harder to explain, as this very thread demonstrates. Using the right assert statements makes the tests robust and correct more easily, plus way clearer.

Please fix.

Thanks for the elaborated reply it clears some of my doubts about the test as well especially the production bug scenario.

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

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.Tests/GetBucketOptionsTest.cs

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

Co-authored-by: Amanda Tarafa Mas <[email protected]>

…BucketOptionsTest.cs Co-authored-by: Amanda Tarafa Mas <[email protected]>

…Options.cs Co-authored-by: Amanda Tarafa Mas <[email protected]>

…tBucketsOptionsTest.cs Co-authored-by: Amanda Tarafa Mas <[email protected]>

amanda-tarafa · 2025-02-12T18:28:21Z

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

+            foreach (var bucket in actualBuckets)
+            {   // Verify if the bucket is soft-deleted only 
+                Assert.NotNull(bucket.Generation);
+                Assert.NotNull(bucket.SoftDeleteTimeDateTimeOffset);
+                Assert.NotNull(bucket.HardDeleteTimeDateTimeOffset);
+
+                if (bucket.Name == softDeleteBucket.Name)
+                {   // Compare the generation number
+                    Assert.Equal(bucket.Generation, softDeleteBucket.Generation);
+                }
+            }


There are different ways to achieve the same result, some are clearer than others. Please use Assert.All and Asssert.Contains.

amanda-tarafa · 2025-02-12T21:40:41Z

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

+            foreach (var bucket in actualBuckets)
+            {   // Verify if the bucket is soft-deleted only 
+                Assert.NotNull(bucket.Generation);
+                Assert.NotNull(bucket.SoftDeleteTimeDateTimeOffset);
+                Assert.NotNull(bucket.HardDeleteTimeDateTimeOffset);
+
+                if (bucket.Name == softDeleteBucket.Name)
+                {   // Compare the generation number
+                    Assert.Equal(bucket.Generation, softDeleteBucket.Generation);
+                }
+            }


Actually, is not only about clarity, this test is not sufficient as it is. It will pass even if actualBuckets is empty or if it does not contains softDeleteBucket. Using Assert.All and Assert.Contains will make the test sufficient.

amanda-tarafa · 2025-02-14T22:11:41Z

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

+            foreach (var bucket in actualBuckets)
+            {   // Verify if the bucket is soft-deleted only 
+                Assert.NotNull(bucket.Generation);
+                Assert.NotNull(bucket.SoftDeleteTimeDateTimeOffset);
+                Assert.NotNull(bucket.HardDeleteTimeDateTimeOffset);
+
+                if (bucket.Name == softDeleteBucket.Name)
+                {   // Compare the generation number
+                    Assert.Equal(bucket.Generation, softDeleteBucket.Generation);
+                }
+            }


A test that executes no asserts, assuming there are no exceptions (none in this case), is a passing test, it's not a skipped test. A passing test indicates that the feature has been tested and everything is working as expected, whereas a skipped test indicates that nothing was tested. This test is wrong because it may pass even when nothing is tested.

Explicitly, if there is a bug in production code that has the effect of the list returning empty (even when you have just created a soft deleted bucket), this test will pass, when it shouldn't have. That's because this test is saying:

Create a soft deleted bucket (correct, we guarantee there's at least one soft deleted bucket so we can test).

Fetch soft deleted buckets (the feature we want to test).

If there happens to be elements in the list, make sure they are all soft deleted. (hmm, if the feature we want to test is broken so that the list is empty, then we really don't test anything and we don't find out the feature is broken)

If the recently created bucket happens to be on the list, make sure all its fields have the expected values and that it is indeed soft deleted. (hmm, if the feature we want to test is broken so that the list is empty, then we really don't test anything and we don't find out the feature is broken)

The key being the "if it happens" where a test needs to make certain "it happened", no ifs and no buts.

What this test needs to do is:

Create a soft deleted bucket

Fetch soft deleted buckets

Make sure the recently created bucket is on the list, and all its fields have the expected values.

Make sure all the elements on the list are indeed soft deleted. Apart from the test own bucket, these may be buckets created by the fixture or by other tests, we don't care if there are none of these, or many, because we don't care about the order in which tests execute. The only thing that we care is that all tests in the list are soft deleted because that's the feature we are testing.

See those two last points describing the correct test, how they very naturally transalate to:

Assert.Contains recently created bucket with the right fields.

Assert.All are soft deletede buckets

You can certainly express the same semantic with a loop and some checks, but, it's way less naturally readable plus, errors like the one currently in this test are harder to spot and harder to explain, as this very thread demonstrates. Using the right assert statements makes the tests robust and correct more easily, plus way clearer.

Please fix.

amanda-tarafa · 2025-02-18T22:52:44Z

@mahendra-google Do let me know here when this is ready for review again. No rush on my part, but just know I won't review until you have said so here. Thanks!

mahendra-google · 2025-02-19T02:26:57Z

@mahendra-google Do let me know here when this is ready for review again. No rush on my part, but just know I won't review until you have said so here. Thanks!

@amanda-tarafa Yes right , I will leave a comment for re-review in PR itself like @danielduhh left , apart from clicking on re-review icon post completion of internal PR review from my team as a part of process. Thanks!

amanda-tarafa · 2025-02-19T06:37:39Z

@amanda-tarafa Yes right , I will leave a comment for re-review in PR itself like @danielduhh left , apart from clicking on re-review icon post completion of internal PR review from my team as a part of process. Thanks!

That's perfect, thanks!

mahendra-google requested review from a team as code owners November 28, 2024 12:39

product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Nov 28, 2024

jskeet reviewed Nov 28, 2024

View reviewed changes

Directory.Packages.props Outdated Show resolved Hide resolved

mahendra-google requested a review from jskeet November 29, 2024 10:36

amanda-tarafa self-assigned this Dec 3, 2024

jskeet reviewed Dec 11, 2024

View reviewed changes

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetBucketOptions.cs Outdated Show resolved Hide resolved

amanda-tarafa requested changes Dec 11, 2024

View reviewed changes

mahendra-google requested a review from amanda-tarafa January 2, 2025 12:45

amanda-tarafa requested changes Jan 8, 2025

View reviewed changes

amanda-tarafa requested changes Jan 10, 2025

View reviewed changes

mahendra-google force-pushed the restore_bucket_soft_delete_phase_2 branch from cb1650a to db71efc Compare January 28, 2025 06:01

mahendra-google added 7 commits January 29, 2025 10:35

Co-authored-by: Amanda Tarafa Mas <[email protected]…

12c0d70

…ly.github.com> Co-authored-by: Amanda Tarafa Mas <[email protected]> modified: generator-input/apis.json

file-scoped namespace and generate bucket name method added

39a1f21

Adding all tests in single test , addition of generatebucket name method

38fd06f

adding a blank line between one property and the next

dd02b37

List Soft Deleted Bucket test case changes

0258eb2

Removal of encryption key support for restore bucket request and remo…

9a5bfe7

…val of package version override

Recommended changes as per internal PR review

7efe472

mahendra-google force-pushed the restore_bucket_soft_delete_phase_2 branch from 0957b16 to 7efe472 Compare January 29, 2025 10:43

mahendra-google requested a review from amanda-tarafa January 29, 2025 10:46

mahendra-google requested review from jskeet and JesseLovelace February 6, 2025 01:28

amanda-tarafa requested changes Feb 11, 2025

View reviewed changes

mahendra-google and others added 9 commits February 12, 2025 09:14

named arguments added while calling create bucket method

595e6ff

Co-authored-by: Amanda Tarafa Mas <[email protected]>

Update apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.Tests/Get…

ddd62d1

…BucketOptionsTest.cs Co-authored-by: Amanda Tarafa Mas <[email protected]>

Update apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/GetBucket…

229e343

…Options.cs Co-authored-by: Amanda Tarafa Mas <[email protected]>

Update apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.Tests/Lis…

beb036c

…tBucketsOptionsTest.cs Co-authored-by: Amanda Tarafa Mas <[email protected]>

restore white space in Google.Cloud.Storage.V1.csproj

517b827

Update GetBucketOptionsTest.cs

8b89da6

Update ListBucketsTest.cs

56639b4

Update GetBucketTest.cs

ec7aab9

encryption key removed from RestoreBucketOptions

25d3356

amanda-tarafa requested changes Feb 12, 2025

View reviewed changes

amanda-tarafa requested changes Feb 14, 2025

View reviewed changes

mahendra-google added 4 commits February 17, 2025 19:18

Initial commit for Assert.Contains and Assert.All in list bucket test

1e026b1

listbucket linters changes

678d101

restore bucket linter changes

f7853af

getbucket linter changes

620c980

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): support bucket restore in soft delete phase 2 #13894

feat(storage): support bucket restore in soft delete phase 2 #13894

mahendra-google commented Nov 28, 2024

jskeet left a comment

mahendra-google commented Nov 28, 2024

jskeet commented Nov 29, 2024

amanda-tarafa commented Dec 3, 2024

mahendra-google commented Dec 4, 2024

mahendra-google commented Dec 6, 2024

mahendra-google commented Dec 6, 2024 •

edited

Loading

amanda-tarafa left a comment

mahendra-google commented Jan 29, 2025

danielduhh commented Feb 10, 2025

amanda-tarafa commented Feb 11, 2025

amanda-tarafa left a comment

amanda-tarafa Feb 11, 2025

mahendra-google Feb 12, 2025 •

edited

Loading

amanda-tarafa Feb 12, 2025

amanda-tarafa Feb 12, 2025

mahendra-google Feb 14, 2025 •

edited

Loading

mahendra-google Feb 14, 2025

amanda-tarafa Feb 14, 2025

mahendra-google Feb 15, 2025

amanda-tarafa Feb 12, 2025

amanda-tarafa Feb 12, 2025

amanda-tarafa Feb 14, 2025

amanda-tarafa commented Feb 18, 2025

mahendra-google commented Feb 19, 2025

amanda-tarafa commented Feb 19, 2025

feat(storage): support bucket restore in soft delete phase 2 #13894

Are you sure you want to change the base?

feat(storage): support bucket restore in soft delete phase 2 #13894

Conversation

mahendra-google commented Nov 28, 2024

jskeet left a comment

Choose a reason for hiding this comment

mahendra-google commented Nov 28, 2024

jskeet commented Nov 29, 2024

amanda-tarafa commented Dec 3, 2024

mahendra-google commented Dec 4, 2024

mahendra-google commented Dec 6, 2024

mahendra-google commented Dec 6, 2024 • edited Loading

amanda-tarafa left a comment

Choose a reason for hiding this comment

mahendra-google commented Jan 29, 2025

danielduhh commented Feb 10, 2025

amanda-tarafa commented Feb 11, 2025

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahendra-google Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahendra-google Feb 14, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa commented Feb 18, 2025

mahendra-google commented Feb 19, 2025

amanda-tarafa commented Feb 19, 2025

mahendra-google commented Dec 6, 2024 •

edited

Loading

mahendra-google Feb 12, 2025 •

edited

Loading

mahendra-google Feb 14, 2025 •

edited

Loading