samples(storage batch operations): add samples and tests for storage batch operations#16
samples(storage batch operations): add samples and tests for storage batch operations#16mahendra-google wants to merge 19 commits intomainfrom
Conversation
|
@gurusai-voleti Please review PR more from SBO feature perspective as feature is already released in terraform. |
mahendra-google
force-pushed
the
sbo
branch
3 times, most recently
from
d8e67fd to
6f4c00a
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of samples and tests for the Storage Batch Operations API. The implementation is solid, but I've identified a few areas for improvement, primarily related to code duplication, test coverage, and general code cleanup. Key suggestions include refactoring a test to cover more scenarios, moving a helper method to a shared fixture to reduce duplication, and simplifying the logic in one of the sample classes. These changes will enhance the maintainability and robustness of the new code.
| var jobTransformationCase = "DeleteObject"; | ||
| var holdState = "EventBasedHoldSet"; | ||
| var jobTransformationObject = new object(); | ||
| string jobType; | ||
|
|
||
| // If the job transformation case is PutObjectHold, we can set the hold state to EventBasedHoldSet or EventBasedHoldUnSet or TemporaryHoldSet or TemporaryHoldUnSet. | ||
| if (jobTransformationCase == "PutObjectHold") | ||
| { | ||
| jobType = $"{jobTransformationCase}{holdState}"; | ||
| } | ||
| // If the job transformation case is other than PutObjectHold, we dont set the hold state. | ||
| else | ||
| { | ||
| jobType = jobTransformationCase; | ||
| } | ||
| // If the job transformation case is RewriteObject, we can set the KmsKey and KmsKeyAsCryptoKeyName. | ||
| if (jobTransformationCase == "RewriteObject") | ||
| { | ||
| _keyRingId = GetEnvironmentVariable("STORAGE_KMS_KEYRING_ID", "This is the Key Ring ID"); | ||
| _cryptoKeyId = GetEnvironmentVariable("STORAGE_KMS_CRYPTOKEY_ID", "This is the Crypto Key ID."); | ||
| _kmsKey = $"projects/{_fixture.ProjectId}/locations/{_fixture.LocationId}/keyRings/{_keyRingId}/cryptoKeys/{_cryptoKeyId}"; | ||
| _cryptoKeyName = CryptoKeyName.FromProjectLocationKeyRingCryptoKey(_fixture.ProjectId, _fixture.LocationId, _keyRingId, _cryptoKeyId); | ||
| RewriteObject rewriteObject = new RewriteObject { KmsKey = _kmsKey, KmsKeyAsCryptoKeyName = _cryptoKeyName }; | ||
| jobTransformationObject = rewriteObject; | ||
|
|
||
| } | ||
| // If the job transformation case is PutMetadata, we can set the CacheControl, ContentDisposition, ContentEncoding, ContentLanguage, ContentType and CustomTime. | ||
| else if (jobTransformationCase == "PutMetadata") | ||
| { | ||
| PutMetadata putMetadata = new PutMetadata | ||
| { | ||
| CacheControl = "no-cache", | ||
| ContentDisposition = "inline", | ||
| ContentEncoding = "gzip", | ||
| ContentLanguage = "en-US", | ||
| ContentType = "text/plain", | ||
| CustomTime = DateTime.UtcNow.ToString("o") | ||
| }; | ||
| jobTransformationObject = putMetadata; | ||
| } |
There was a problem hiding this comment.
The jobTransformationCase variable is hardcoded to "DeleteObject", which means the conditional blocks for other transformation types like "PutObjectHold", "RewriteObject", and "PutMetadata" are never executed. This significantly limits the test's coverage.
To ensure all transformation cases are tested, I recommend converting this test method into an xUnit Theory and using [InlineData] to provide the different job types. This will make the test more robust and comprehensive.
| The samples requires [.NET 8][.NET].That means using | ||
| [Visual Studio 2022](https://www.visualstudio.com/), or the command line. |
There was a problem hiding this comment.
There's a minor grammatical error and a formatting issue here. "requires" should be "require", and a space is needed after the period for better readability.
| The samples requires [.NET 8][.NET].That means using | |
| [Visual Studio 2022](https://www.visualstudio.com/), or the command line. | |
| The samples require [.NET 8][.NET]. That means using | |
| [Visual Studio 2022](https://www.visualstudio.com/), or the command line. |
| public class CancelBatchJobTest | ||
| { | ||
| private readonly StorageFixture _fixture; | ||
| private readonly BucketList.Types.Bucket _bucket = new(); |
There was a problem hiding this comment.
The _bucket field is only assigned and used within the constructor. It can be converted into a local variable within the CancelBatchJobTest constructor to improve encapsulation and clarify its scope.
| public static string CreateBatchJob(LocationName locationName, | ||
| BucketList bucketList, | ||
| string jobId = "12345678910") | ||
| { | ||
| StorageBatchOperationsClient storageBatchClient = StorageBatchOperationsClient.Create(); | ||
| RetryRobot retryHandler = new RetryRobot | ||
| { | ||
| ShouldRetry = ex => ex is NullReferenceException | ||
| }; | ||
|
|
||
| // Creates a batch job with the specified bucket list and delete object settings. | ||
| CreateJobRequest request = new CreateJobRequest | ||
| { | ||
| ParentAsLocationName = locationName, | ||
| JobId = jobId, | ||
| Job = new Job | ||
| { | ||
| BucketList = bucketList, | ||
| DeleteObject = new DeleteObject { PermanentObjectDeletionEnabled = true } | ||
| }, | ||
| RequestId = jobId, | ||
| }; | ||
|
|
||
| Operation<Job, OperationMetadata> response = storageBatchClient.CreateJob(request); | ||
| string operationName = response.Name; | ||
| Operation<Job, OperationMetadata> retrievedResponse = storageBatchClient.PollOnceCreateJob(operationName); | ||
| // Poll once asynchronously. | ||
| Task<Operation<Job, OperationMetadata>> retrievedAsyncResponse = retrievedResponse.PollOnceAsync(); | ||
| string jobName = retryHandler.Eventually(() => retrievedAsyncResponse?.Result?.Metadata?.Job?.Name); | ||
| return jobName; | ||
| } |
There was a problem hiding this comment.
This CreateBatchJob helper method is useful. To promote code reuse and avoid duplication, consider moving it to the StorageFixture class. This would allow other test classes to use it and would improve the consistency of test setup across the suite, as other tests currently use CreateBatchJobSample which has a different behavior (it polls until completion).
| public class CreateBatchJobTest | ||
| { | ||
| private readonly StorageFixture _fixture; | ||
| private readonly BucketList.Types.Bucket _bucket = new(); |
There was a problem hiding this comment.
The _bucket field is only assigned and used within the constructor. It can be converted into a local variable within the CreateBatchJobTest constructor to improve encapsulation and clarify its scope.
| public class DeleteBatchJobTest | ||
| { | ||
| private readonly StorageFixture _fixture; | ||
| private readonly BucketList.Types.Bucket _bucket = new(); |
There was a problem hiding this comment.
The _bucket field is only assigned and used within the constructor. It can be converted into a local variable within the DeleteBatchJobTest constructor to improve encapsulation and clarify its scope.
| public class GetBatchJobTest | ||
| { | ||
| private readonly StorageFixture _fixture; | ||
| private readonly BucketList.Types.Bucket _bucket = new(); |
There was a problem hiding this comment.
The _bucket field is only assigned and used within the constructor. It can be converted into a local variable within the GetBatchJobTest constructor to improve encapsulation and clarify its scope.
| public class ListBatchJobsTest | ||
| { | ||
| private readonly StorageFixture _fixture; | ||
| private readonly BucketList.Types.Bucket _bucket = new(); |
There was a problem hiding this comment.
The _bucket field is only assigned and used within the constructor. It can be converted into a local variable within the ListBatchJobsTest constructor to improve encapsulation and clarify its scope.
| private DeleteObject _deleteObject; | ||
| private PutObjectHold _putObjectHold; | ||
| private Job _job; |
There was a problem hiding this comment.
The fields _deleteObject, _putObjectHold, and _job are only used within the CreateBatchJob method. To make the CreateBatchJobSample class stateless and improve clarity, these should be declared as local variables within the method. This change will also require updating the switch statement to assign to a local job variable.
| switch (jobType) | ||
| { | ||
| case "RewriteObject": | ||
| _job = new Job | ||
| { | ||
| RewriteObject = (RewriteObject) jobTransformationObject, | ||
| BucketList = bucketList | ||
| }; | ||
| break; | ||
| case "PutMetadata": | ||
| _job = new Job | ||
| { | ||
| PutMetadata = (PutMetadata) jobTransformationObject, | ||
| BucketList = bucketList | ||
| }; | ||
| break; | ||
| case "DeleteObject": | ||
| _deleteObject = new DeleteObject { PermanentObjectDeletionEnabled = true }; | ||
|
|
||
| _job = new Job | ||
| { | ||
| DeleteObject = _deleteObject, | ||
| BucketList = bucketList | ||
| }; | ||
| break; | ||
| case "PutObjectHoldEventBasedHoldSet": | ||
| _putObjectHold = new PutObjectHold { EventBasedHold = PutObjectHold.Types.HoldStatus.Set }; | ||
|
|
||
| _job = new Job | ||
| { | ||
| PutObjectHold = _putObjectHold, | ||
| BucketList = bucketList | ||
| }; | ||
| break; | ||
| case "PutObjectHoldEventBasedHoldUnSet": | ||
| _putObjectHold = new PutObjectHold { EventBasedHold = PutObjectHold.Types.HoldStatus.Unset }; | ||
|
|
||
| _job = new Job | ||
| { | ||
| PutObjectHold = _putObjectHold, | ||
| BucketList = bucketList | ||
| }; | ||
| break; | ||
| case "PutObjectHoldTemporaryHoldSet": | ||
| _putObjectHold = new PutObjectHold { TemporaryHold = PutObjectHold.Types.HoldStatus.Set }; | ||
|
|
||
| _job = new Job | ||
| { | ||
| PutObjectHold = _putObjectHold, | ||
| BucketList = bucketList | ||
| }; | ||
| break; | ||
| case "PutObjectHoldTemporaryHoldUnSet": | ||
| _putObjectHold = new PutObjectHold { TemporaryHold = PutObjectHold.Types.HoldStatus.Unset }; | ||
|
|
||
| _job = new Job | ||
| { | ||
| PutObjectHold = _putObjectHold, | ||
| BucketList = bucketList | ||
| }; | ||
| break; | ||
| } | ||
|
|
||
| // Create a job request with the specified location, job ID, and job details. | ||
| CreateJobRequest request = new CreateJobRequest | ||
| { | ||
| ParentAsLocationName = locationName, | ||
| JobId = jobId, | ||
| Job = _job, | ||
| RequestId = jobId, | ||
| }; |
There was a problem hiding this comment.
This switch statement can be refactored to be more concise and reduce code duplication. By creating the Job object once before the switch, you can then just set the appropriate transformation property inside each case. This will make the code cleaner and easier to maintain.
var job = new Job();
switch (jobType)
{
case "RewriteObject":
job.RewriteObject = (RewriteObject) jobTransformationObject;
break;
case "PutMetadata":
job.PutMetadata = (PutMetadata) jobTransformationObject;
break;
case "DeleteObject":
job.DeleteObject = new DeleteObject { PermanentObjectDeletionEnabled = true };
break;
case "PutObjectHoldEventBasedHoldSet":
job.PutObjectHold = new PutObjectHold { EventBasedHold = PutObjectHold.Types.HoldStatus.Set };
break;
case "PutObjectHoldEventBasedHoldUnSet":
job.PutObjectHold = new PutObjectHold { EventBasedHold = PutObjectHold.Types.HoldStatus.Unset };
break;
case "PutObjectHoldTemporaryHoldSet":
job.PutObjectHold = new PutObjectHold { TemporaryHold = PutObjectHold.Types.HoldStatus.Set };
break;
case "PutObjectHoldTemporaryHoldUnSet":
job.PutObjectHold = new PutObjectHold { TemporaryHold = PutObjectHold.Types.HoldStatus.Unset };
break;
}
job.BucketList = bucketList;
// Create a job request with the specified location, job ID, and job details.
CreateJobRequest request = new CreateJobRequest
{
ParentAsLocationName = locationName,
JobId = jobId,
Job = job,
RequestId = jobId,
};Refactor CancelBatchJobTest to remove RetryRobot and handle exceptions.
Removed ProjectReference to testutil.csproj.
…eBatchJobTest Remove unused private fields `_kmsKey`, `_keyRingId`, `_cryptoKeyId`, and `_cryptoKeyName` from CreateBatchJobTest to improve code clarity and maintainability.
…ove job cancellation logic
… CancelBatchJobTest
…easing retry attempts Extended the retry attempts in cancelBatchJobTest from 100 to 1000. This ensures the test accounts for delayed state transitions in the Koko environment while maintaining efficiency via an early exit once the 'Canceled' state is reached.
Updated Google.Cloud.Storage.V1 to 4.14.0 and Google.Cloud.StorageBatchOperations.V1 to 1.0.0-beta05.
…nd update XML docs
3 participants
This PR contains samples and test cases to create, list, get, cancel and delete storage batch job operations.
The region tags used to create samples are as below:-