samples(Storage): Add samples and tests for ObjectContext#21
samples(Storage): Add samples and tests for ObjectContext#21mahendra-google wants to merge 2 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Google Cloud Storage client library samples by introducing the capability to manage object contexts. It provides a new sample and associated test for setting custom context data on storage objects, enabling users to attach key-payload pairs for identification and metadata purposes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces a new sample and corresponding test for setting object contexts in Google Cloud Storage. The review identifies an issue where the test currently verifies only the return value of the sample method, rather than confirming the actual update of the object's contexts in Cloud Storage by retrieving the object. Additionally, it suggests an improvement for the sample method to return the Object instance provided by the UpdateObject call, ensuring the most accurate state from the service.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new sample and an associated test for setting object contexts in Google Cloud Storage. The SetObjectContexts.cs file provides a SetObjectContextsSample class with a method to update an object's contexts using StorageClient.UpdateObject, while SetObjectContextsTest.cs adds an xUnit test to verify this functionality, including handling Unicode characters in context keys and values. Feedback includes improving the sample's robustness by providing a default non-null Object.ContextsData if contextsData is null, making the SetObjectContexts method and potentially its class static due to their stateless nature, and generalizing the console output message for accuracy.
| public Google.Apis.Storage.v1.Data.Object.ContextsData SetObjectContexts( | ||
| string bucketName = "your-unique-bucket-name", | ||
| string objectName = "your-object-name", | ||
| Google.Apis.Storage.v1.Data.Object.ContextsData contextsData = null) |
There was a problem hiding this comment.
The default value null for the contextsData parameter is problematic for a sample demonstrating how to "set" contexts. If the sample is run without arguments, obj.Contexts will be null, which typically means the Contexts field will not be updated in a PATCH request. This contradicts the sample's description of "update or apply new context data".
To ensure the sample always demonstrates setting contexts, even when run standalone, contextsData should either be explicitly provided or a default non-null Object.ContextsData should be created if contextsData is null.
Google.Apis.Storage.v1.Data.Object.ContextsData contextsData = null)
{
// Provide a default example context if none is provided for standalone execution
contextsData ??= new Google.Apis.Storage.v1.Data.Object.ContextsData
{
Custom = new Dictionary<string, ObjectCustomContextPayload>
{
{ "exampleKey", new ObjectCustomContextPayload { Value = "exampleValue" } }
}
};| [Fact] | ||
| public void SetObjectContexts() | ||
| { | ||
| SetObjectContextsSample contextsSample = new SetObjectContextsSample(); |
There was a problem hiding this comment.
The SetObjectContexts method in SetObjectContextsSample does not rely on any instance state. It creates a StorageClient internally and performs an operation. Consider making the SetObjectContexts method static in SetObjectContexts.cs to reflect its stateless nature and avoid unnecessary object instantiation in tests and other sample calls.
SetObjectContextsSample.SetObjectContexts(bucketName, objectName, contextsData);| using Google.Cloud.Storage.V1; | ||
| using System; | ||
|
|
||
| public class SetObjectContextsSample |
There was a problem hiding this comment.
The SetObjectContexts method within this class does not utilize any instance members. To improve clarity and avoid unnecessary object instantiation, consider making both the class SetObjectContextsSample and the SetObjectContexts method static.
public static class SetObjectContextsSample| }; | ||
| var updatedObject = storage.UpdateObject(obj); | ||
| Console.WriteLine($"Updated custom contexts data for the object {objectName}"); | ||
| return updatedObject.Contexts; |
There was a problem hiding this comment.
The console message currently specifies "custom contexts data". However, the contextsData parameter description mentions "User-defined or system-defined object contexts". To be more accurate and general, the message should reflect the possibility of both types of contexts.
Console.WriteLine($"Updated contexts data for the object {objectName}");
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new C# samples and XUnit tests for managing object context data in Google Cloud Storage, specifically for getting and setting custom contexts. Feedback includes addressing a potential NullReferenceException in GetObjectContexts.cs if contextsData.Custom is null, and removing ineffective Contexts property assignments during initial object uploads in both GetObjectContextsTest.cs and SetObjectContextsTest.cs to improve code clarity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new functionality for managing object contexts in Google Cloud Storage. It adds two new sample methods: GetObjectContexts to retrieve custom context data associated with an object, and SetObjectContexts to update an object with new custom context data. Corresponding XUnit tests, GetObjectContextsTest and SetObjectContextsTest, have also been added to validate this functionality. I have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new C# samples and corresponding tests for managing object contexts in Google Cloud Storage. The GetObjectContexts and SetObjectContexts samples demonstrate how to retrieve and set custom context data for objects. Review feedback suggests improving resource management by explicitly disposing MemoryStream instances in tests using using statements, and by reusing StorageClient instances across multiple operations in the samples to reduce overhead.
| string bucketName = "your-unique-bucket-name", | ||
| string objectName = "your-object-name") | ||
| { | ||
| var storage = StorageClient.Create(); |
There was a problem hiding this comment.
Creating a new StorageClient instance with StorageClient.Create() for each operation can introduce unnecessary overhead. For better efficiency and resource management in a production application, it's generally recommended to reuse a single StorageClient instance across multiple calls, perhaps by making it a static field or injecting it.
| string objectName = "your-object-name", | ||
| Google.Apis.Storage.v1.Data.Object.ContextsData contextsData = null) | ||
| { | ||
| var storage = StorageClient.Create(); |
There was a problem hiding this comment.
Creating a new StorageClient instance with StorageClient.Create() for each operation can introduce unnecessary overhead. For better efficiency and resource management in a production application, it's generally recommended to reuse a single StorageClient instance across multiple calls, perhaps by making it a static field or injecting it.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and unit tests for managing object contexts in Google Cloud Storage, including setting, retrieving, and filtering objects by context. It also updates the Google.Cloud.Storage.V1 package to version 4.15.0. The review feedback identifies potential NullReferenceException risks in the sample code when accessing context properties and suggests refactoring duplicated setup logic across the new test files to improve maintainability.
| string contextKey = "A\u00F1\u03A9\U0001F680"; | ||
| string contextValue = "Ab\u00F1\u03A9\U0001F680"; | ||
|
|
||
| var custom = new Dictionary<string, ObjectCustomContextPayload> | ||
| { | ||
| { contextKey, new ObjectCustomContextPayload { Value = contextValue } } | ||
| }; | ||
|
|
||
| var bucketName = _fixture.GenerateBucketName(); | ||
| var objectName = _fixture.GenerateName(); | ||
| var contextsData = new Object.ContextsData { Custom = custom }; | ||
| _fixture.CreateBucket(bucketName, multiVersion: false, softDelete: true, registerForDeletion: true); | ||
|
|
||
| var destination = new Object | ||
| { | ||
| Bucket = bucketName, | ||
| Name = objectName | ||
| }; | ||
| var content = _fixture.GenerateName(); | ||
| byte[] byteArray = Encoding.UTF8.GetBytes(content); | ||
| using (MemoryStream source = new MemoryStream(byteArray)) | ||
| { | ||
| _fixture.Client.UploadObject(destination, source); | ||
| } |
There was a problem hiding this comment.
There's a significant amount of duplicated code for setting up the test environment (creating bucket, object, and context data) across GetObjectContextsTest.cs, ListObjectsWithContextFilterTest.cs, and SetObjectContextsTest.cs. To improve maintainability and reduce boilerplate, consider extracting this setup logic into a shared helper method. This could be a private method within each test class or a method in a shared test utility class.
There was a problem hiding this comment.
@gemini-code-assist I feel that each test should have a different test bucket, test object and test context data. Separate helper methods are already created in StorageFixture for creating test buckets and test objects. In addition to that helper methods are created in such a way that all created resources are getting disposed of at the end of each individual tests.
There was a problem hiding this comment.
I understand your point regarding the need for unique test resources to ensure test isolation. However, the current approach results in significant code duplication across these three test files. You can maintain test isolation while reducing boilerplate by creating a shared helper method in StorageFixture that handles the common setup (creating the bucket, object, and context data) and returns the necessary identifiers. This would keep the individual test methods clean and focused on their specific assertions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds sample code and tests for setting, getting, and filtering Google Cloud Storage objects using metadata contexts, and updates the Google.Cloud.Storage.V1 dependency to version 4.15.0. The review feedback suggests refactoring duplicated test setup logic into a helper method and correcting a typo in the documentation for the context filter parameter.
| var destination = new Object | ||
| { | ||
| Bucket = bucketName, | ||
| Name = objectName | ||
| }; | ||
| var content = _fixture.GenerateName(); | ||
| byte[] byteArray = Encoding.UTF8.GetBytes(content); | ||
| using (MemoryStream source = new MemoryStream(byteArray)) | ||
| { | ||
| _fixture.Client.UploadObject(destination, source); | ||
| } |
There was a problem hiding this comment.
This block of code for creating and uploading a test object is duplicated in ListObjectsWithContextFilterTest.cs and SetObjectContextsTest.cs. To improve maintainability and reduce code duplication, consider extracting this logic into a helper method within the StorageFixture class.
For example, you could add a method like this to StorageFixture:
public Google.Apis.Storage.v1.Data.Object UploadTestObject(string bucketName, string objectName)
{
var destination = new Google.Apis.Storage.v1.Data.Object
{
Bucket = bucketName,
Name = objectName
};
var content = GenerateName();
byte[] byteArray = Encoding.UTF8.GetBytes(content);
using (MemoryStream source = new MemoryStream(byteArray))
{
return Client.UploadObject(destination, source);
}
}Then, you could replace these lines in the test with a single call:
_fixture.UploadTestObject(bucketName, objectName);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds sample code and tests for Google Cloud Storage object contexts and updates the Google.Cloud.Storage.V1 package. Feedback includes fixing potential NullReferenceExceptions when accessing context data, optimizing object listing performance, and removing unused using directives in test files.
| if (contextsData == null) | ||
| { | ||
| Console.WriteLine($"No Context Data found for the Object {objectName}"); | ||
| return contextsData; | ||
| } | ||
|
|
||
| Console.WriteLine($"Context Data for the Object {objectName} is as follows:"); | ||
| foreach (var (key, contextPayload) in contextsData.Custom) | ||
| { | ||
| Console.WriteLine($"Context Key: {key}, Context Value: {contextPayload.Value}"); | ||
| } | ||
| return contextsData; |
There was a problem hiding this comment.
The current implementation checks if contextsData is null, but does not handle the case where contextsData.Custom is null or empty. This could lead to a NullReferenceException if contextsData exists but Custom does not, or confusing output if Custom is empty. It's safer to check if contextsData.Custom is null or has no elements before attempting to iterate.
if (contextsData?.Custom is null || contextsData.Custom.Count == 0)
{
Console.WriteLine($"No Context Data found for the Object {objectName}");
return contextsData;
}
Console.WriteLine($"Context Data for the Object {objectName} is as follows:");
foreach (var (key, contextPayload) in contextsData.Custom)
{
Console.WriteLine($"Context Key: {key}, Context Value: {contextPayload.Value}");
}
return contextsData;| Console.WriteLine($"Context Data for the Object {obj.Name} is as follows:"); | ||
|
|
||
| foreach (var (key, contextPayload) in obj.Contexts.Custom) | ||
| { | ||
| Console.WriteLine($"Context Key: {key}, Context Value: {contextPayload.Value}"); | ||
| } |
There was a problem hiding this comment.
The code accesses obj.Contexts.Custom without checking if obj.Contexts or obj.Contexts.Custom might be null. If an object in the list does not have context data, this will cause a NullReferenceException. You should add a check to ensure obj.Contexts.Custom is not null and not empty before iterating over it.
if (obj.Contexts?.Custom != null && obj.Contexts.Custom.Count > 0)
{
Console.WriteLine($"Context Data for the Object {obj.Name} is as follows:");
foreach (var (key, contextPayload) in obj.Contexts.Custom)
{
Console.WriteLine($"Context Key: {key}, Context Value: {contextPayload.Value}");
}
}| using System.IO; | ||
| using System.Text; |
There was a problem hiding this comment.
The using directives for System.IO and System.Text are not used in this file. They can be removed to improve code cleanliness.
| using System.IO; | ||
| using System.Text; |
There was a problem hiding this comment.
The using directives for System.IO and System.Text are not used in this file. They can be removed to improve code cleanliness.
| using System.IO; | ||
| using System.Text; |
There was a problem hiding this comment.
The using directives for System.IO and System.Text are not used in this file. They can be removed to improve code cleanliness.
| string filter = "contexts.\"KEY\"=\"VALUE\"") | ||
| { | ||
| var storage = StorageClient.Create(); | ||
| var objects = storage.ListObjects(bucketName, prefix: null, new ListObjectsOptions { Filter = filter }).ToList(); |
There was a problem hiding this comment.
Calling ToList() on the result of ListObjects materializes the entire collection in memory. This can be inefficient if the bucket contains a large number of objects. It's better to iterate over the PagedEnumerable<Object> returned by ListObjects directly, which fetches objects in pages as needed.
var objects = storage.ListObjects(bucketName, prefix: null, new ListObjectsOptions { Filter = filter });
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new sample code and unit tests for managing object contexts in Google Cloud Storage, specifically for setting, retrieving, and filtering objects. It also updates the Google.Cloud.Storage.V1 package version to 4.15.0. The review feedback suggests optimizing the API calls in the samples by explicitly specifying the required fields in the request options to improve performance and ensure the presence of the contexts property in the response.
| string objectName = "your-object-name") | ||
| { | ||
| var storage = StorageClient.Create(); | ||
| var obj = storage.GetObject(bucketName, objectName); |
There was a problem hiding this comment.
For efficiency, it's better to request only the fields you need. Since this sample only uses the Contexts property of the object, you can specify that using GetObjectOptions. This avoids fetching the entire object metadata.
var obj = storage.GetObject(bucketName, objectName, new GetObjectOptions { Fields = "contexts" });| Name = objectName, | ||
| Contexts = contextsData | ||
| }; | ||
| var updatedObject = storage.UpdateObject(obj); |
There was a problem hiding this comment.
For better performance, you can specify that only the Contexts field should be returned in the response object. This reduces the amount of data transferred from the service.
var updatedObject = storage.UpdateObject(obj, new UpdateObjectOptions { Fields = "contexts" });
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples for managing and filtering object contexts in Google Cloud Storage, including updates to the project dependencies. The review identified potential NullReferenceExceptions when accessing context data in both the retrieval and listing samples, and provided specific code suggestions to safely handle null or empty context collections.
mahendra-google
requested review from
gurusai-voleti,
shubhangi-google and
thiyaguk09
mahendra-google
force-pushed
the
object-context-samples
branch
from
6386527 to
4e50755
Compare
1 participant
This PR adds three new samples and tests for
Object Contexts.storage_get_object_contexts: Retrieves the ContextsData for a specific object.storage_list_object_contexts: Demonstrates how to iterate through and display the context metadata associated with an object.storage_set_object_contexts: llustrates how to update or apply new context data to an existing object. Note :- We need a new version ofGoogle.Cloud.Storage.V1Nuget Package which containsFilteroption ofListObjectOptionsso that sample and test forList Objects with Context Datawill not fail on Kokoro server. As of now I have updatedGoogle.Cloud.Storage.V1to 4.15.0 in the project file which is yet to be released.