Skip to content

samples(Storage): Update sample and test for compose object with delete source objects#23

Open
mahendra-google wants to merge 1 commit into
mainfrom
compose-delete-source-objects
Open

samples(Storage): Update sample and test for compose object with delete source objects#23
mahendra-google wants to merge 1 commit into
mainfrom
compose-delete-source-objects

Conversation

@mahendra-google

@mahendra-google mahendra-google commented Jun 9, 2026

Copy link
Copy Markdown
Owner

This PR contains updation of the sample and test for ComposeObject with the introduction of boolean deleteSourceObjects parameter.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new sample ComposeObjectWithDeleteSourceObjects and its corresponding tests to demonstrate composing objects while deleting the source files. It also updates package dependencies in the project file. However, the standard ComposeObject sample was incorrectly modified to also delete source objects. Feedback recommends reverting the changes to ComposeObject.cs, updating its tests to assert that source files still exist, and fixing a typo in the filename of the new test file (ComposeObjectWithDeleteSourceObjects'Test.cs).

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +41 to +47
DeleteSourceObjects = true,
SourceObjects = sourceObjects,
Destination = new Google.Apis.Storage.v1.Data.Object { ContentType = "text/plain" }
}, bucketName, targetObjectName).Execute();

Console.WriteLine($"New composite file {targetObjectName} was created in bucket {bucketName}" +
$" by combining {firstObjectName} and {secondObjectName}.");
$" by combining {firstObjectName} and {secondObjectName} and the source objects were deleted.");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The basic ComposeObject sample should not delete the source objects. Since you have introduced a new dedicated sample ComposeObjectWithDeleteSourceObjects for this behavior, please revert the changes in this file so that ComposeObject remains a demonstration of the standard compose operation (which keeps the source objects).

            SourceObjects = sourceObjects,
            Destination = new Google.Apis.Storage.v1.Data.Object { ContentType = "text/plain" }
        }, bucketName, targetObjectName).Execute();

        Console.WriteLine($"New composite file {targetObjectName} was created in bucket {bucketName}" +
            $" by combining {firstObjectName} and {secondObjectName}.");

Comment on lines +46 to +48
var files = listFilesSample.ListFiles(_fixture.BucketNameGeneric);
Assert.DoesNotContain(files, c => c.Name == firstObject);
Assert.DoesNotContain(files, c => c.Name == secondObject);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Since the basic ComposeObject sample does not delete the source objects, we should assert that the source files still exist in the bucket after composition.

        var files = listFilesSample.ListFiles(_fixture.BucketNameGeneric);
        Assert.Contains(files, c => c.Name == firstObject);
        Assert.Contains(files, c => c.Name == secondObject);

using Xunit;

[Collection(nameof(StorageFixture))]
public class ComposeObjectWithDeleteSourceObjectsTest

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a typo in the filename of this test file: ComposeObjectWithDeleteSourceObjects'Test.cs contains an unexpected single quote ('). Please rename the file to ComposeObjectWithDeleteSourceObjectsTest.cs.

@mahendra-google

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new sample, ComposeObjectWithDeleteSourceObjects, and its corresponding test to demonstrate composing objects while deleting the source files. It also updates project dependencies. However, the PR incorrectly modifies the standard ComposeObject sample to also delete source objects, which alters its expected behavior and makes the new sample redundant. Feedback has been provided to revert this change in the standard sample and update its test to assert that the source files are preserved.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +41 to +47
DeleteSourceObjects = true,
SourceObjects = sourceObjects,
Destination = new Google.Apis.Storage.v1.Data.Object { ContentType = "text/plain" }
}, bucketName, targetObjectName).Execute();

Console.WriteLine($"New composite file {targetObjectName} was created in bucket {bucketName}" +
$" by combining {firstObjectName} and {secondObjectName}.");
$" by combining {firstObjectName} and {secondObjectName} and the source objects were deleted.");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The standard 'ComposeObject' sample should not delete the source objects by default. A new dedicated sample 'ComposeObjectWithDeleteSourceObjects' has been added specifically for this feature. Modifying the standard sample to delete source objects changes its expected behavior and makes the new sample redundant.

            SourceObjects = sourceObjects,
            Destination = new Google.Apis.Storage.v1.Data.Object { ContentType = \"text/plain\" }
        }, bucketName, targetObjectName).Execute();

        Console.WriteLine($\"New composite file {targetObjectName} was created in bucket {bucketName}\" +
            $\" by combining {firstObjectName} and {secondObjectName}.\");

Comment on lines +46 to +48
var files = listFilesSample.ListFiles(_fixture.BucketNameGeneric);
Assert.DoesNotContain(files, c => c.Name == firstObject);
Assert.DoesNotContain(files, c => c.Name == secondObject);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Since the standard 'ComposeObject' sample should not delete the source objects, the test should assert that the source files still exist in the bucket after composition.

        var files = listFilesSample.ListFiles(_fixture.BucketNameGeneric);
        Assert.Contains(files, c => c.Name == firstObject);
        Assert.Contains(files, c => c.Name == secondObject);

@mahendra-google mahendra-google changed the title samples(Storage): Add sample and test for compose objects with delete source objects samples(Storage): Add sample and test for compose object with delete source objects Jun 9, 2026
@mahendra-google

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the ComposeObject sample to support deleting source objects after composition by adding a deleteSourceObjects parameter and updating the underlying ComposeRequest. It also updates the project dependencies and modifies the tests to verify this behavior using a parameterized theory. Feedback suggests materializing the PagedEnumerable returned by ListFiles to a list in the tests to prevent redundant API calls during multiple assertions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

composeObjectSample.ComposeObject(_fixture.BucketNameGeneric, firstObject, secondObject, _fixture.Collect(targetObject));
composeObjectSample.ComposeObject(_fixture.BucketNameGeneric, firstObject, secondObject, _fixture.Collect(targetObject), shouldDeleteSources);

var files = listFilesSample.ListFiles(_fixture.BucketNameGeneric);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The ListFiles method returns a PagedEnumerable, which is lazily evaluated. Because it is not materialized, each subsequent Assert call (e.g., Assert.DoesNotContain or Assert.Contains) will re-iterate the enumerable, triggering redundant API requests to Google Cloud Storage. Materializing the sequence to a list using System.Linq.Enumerable.ToList avoids these duplicate network calls and improves test performance.

        var files = System.Linq.Enumerable.ToList(listFilesSample.ListFiles(_fixture.BucketNameGeneric));

…introduction of the new boolean flag delete source objects

@thiyaguk09 thiyaguk09 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@mahendra-google mahendra-google changed the title samples(Storage): Add sample and test for compose object with delete source objects samples(Storage): Update sample and test for compose object with delete source objects Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants