Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,103 @@ public void TestUploadInPartialServer(
Assert.Equal(UploadStatus.Completed, progress.Status);
}
}
#if NET6_0_OR_GREATER
/// <summary>
/// Verifies that a resumable upload completes successfully and validates crc32c checksum across different chunking configurations
/// and stream types of known size.
/// </summary>
[Theory, CombinatorialData]
public void TestChunkedUpload_CompletesAndVerifiesChecksumInPartialServer(
[CombinatorialValues(true, false)] bool knownSize,
[CombinatorialValues(80, 150)] int partialSize,
[CombinatorialValues(100, 200)] int chunkSize)
{
var actualChunkSize = Math.Min(partialSize, chunkSize);
var expectedCallCount = (UploadLength + actualChunkSize - 1) / actualChunkSize + 1;
using (var server = new MultiChunkPartialServer(_server, partialSize))
using (var service = new MockClientService(server.HttpPrefix))
{
var content = knownSize ? new MemoryStream(UploadTestBytes) : new UnknownSizeMemoryStream(UploadTestBytes);
Comment on lines +509 to +519
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 knownSize parameter is only ever true in this test, making it and the conditional logic for creating the content stream redundant. To improve clarity and simplify the test, you can remove the knownSize parameter and directly initialize content as a MemoryStream.

        public void TestChunkedUpload_CompletesAndVerifiesChecksumInPartialServer(
            [CombinatorialValues(80, 150)] int partialSize,
            [CombinatorialValues(100, 200)] int chunkSize)
        {
            var actualChunkSize = Math.Min(partialSize, chunkSize);
            var expectedCallCount = (UploadLength + actualChunkSize - 1) / actualChunkSize + 1;
            using (var server = new MultiChunkPartialServer(_server, partialSize))
            using (var service = new MockClientService(server.HttpPrefix))
            {
                var content = new MemoryStream(UploadTestBytes);

var uploader = new TestResumableUpload(service, "MultiChunk", "POST", content, "text/plain", actualChunkSize);
var progress = uploader.Upload();
Assert.Equal(expectedCallCount, server.Requests.Count);
var request = server.Requests.Last();

if (knownSize)
{
Assert.Contains(ResumableUpload.GoogleHashHeader, request.Headers.AllKeys);
var headerValue = request.Headers[ResumableUpload.GoogleHashHeader];
var expectedBase64Hash = CalculateCrc32c(UploadTestBytes);
Assert.Equal($"crc32c={expectedBase64Hash}", headerValue);
}
else
{
Assert.DoesNotContain(ResumableUpload.GoogleHashHeader, request.Headers.AllKeys);
}

Assert.Equal(UploadTestBytes, server.Bytes);
Assert.Equal(UploadStatus.Completed, progress.Status);
}
}

/// <summary>
/// Verifies that the session initiation succeeds when a matching CRC32C checksum is provided.
/// </summary>
[Fact]
public async Task TestInitiateSessionWithMatchingCrc32c()
{
var content = new MemoryStream(UploadTestBytes);
using (var server = new MultiChunkServer(_server))
using (var service = new MockClientService(server.HttpPrefix))
{
var chunkSize = 1024;
var expectedBase64Hash = CalculateCrc32c(UploadTestBytes);
var body = new TestResumableUploadWithCrc32c { Crc32c = expectedBase64Hash };
var uploader = new TestResumableUpload(service, "MultiChunk", "POST", content, "text/plain", chunkSize)
{
Body = body
};

await uploader.InitiateSessionAsync();
// Assert that the initiation request was sent
Assert.Single(server.Requests);
var initiationRequest = server.Requests.First();

// Assert that the X-Goog-Hash header was sent with the correct CRC32C
Assert.Contains(ResumableUpload.GoogleHashHeader, initiationRequest.Headers.AllKeys);
Assert.Equal($"crc32c={expectedBase64Hash}", initiationRequest.Headers[ResumableUpload.GoogleHashHeader]);
Assert.True(uploader.Crc32cHeaderSentInInitiation);
}
}

/// <summary>
/// Verifies that the session initiation fails when a mismatched CRC32C checksum is provided.
/// </summary>
[Fact]
public async Task TestInitiateSessionWithMismatchingCrc32c()
{
var content = new MemoryStream(UploadTestBytes);
using (var server = new MultiChunkServer(_server))
using (var service = new MockClientService(server.HttpPrefix))
{
var chunkSize = 1024;
var expectedBase64Hash = CalculateCrc32c(UploadTestBytes);
var body = new TestResumableUploadWithCrc32c { Crc32c = "incorrectHash" };

var uploader = new TestResumableUpload(service, "MultiChunk", "POST", content, "text/plain", chunkSize)
{
Body = body
};

var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => uploader.InitiateSessionAsync());
Assert.Equal($"The calculated CRC32C of the stream ({expectedBase64Hash}) does not match the CRC32C provided in the object metadata ({body.Crc32c}).", exception.Message);

// Assert that no request was sent to the server
Assert.Empty(server.Requests);
Assert.False(uploader.Crc32cHeaderSentInInitiation);
}
}
Comment on lines +504 to +598
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

These tests for CRC32C checksum validation rely on System.IO.Hashing, which is only available on .NET 6.0 and newer. To prevent build failures on older target frameworks, this entire block of tests should be conditionally compiled.

#if NET6_0_OR_GREATER
        /// <summary>
        /// Verifies that a resumable upload completes successfully and validates crc32c checksum across different chunking configurations 
        /// and stream types of known size.
        /// </summary>
        [Theory, CombinatorialData]
        public void TestChunkedUpload_CompletesAndVerifiesChecksumInPartialServer(
            [CombinatorialValues(true)] bool knownSize,
            [CombinatorialValues(80, 150)] int partialSize,
            [CombinatorialValues(100, 200)] int chunkSize)
        {
            var actualChunkSize = Math.Min(partialSize, chunkSize);
            var expectedCallCount = (UploadLength + actualChunkSize - 1) / actualChunkSize + 1;
            using (var server = new MultiChunkPartialServer(_server, partialSize))
            using (var service = new MockClientService(server.HttpPrefix))
            {
                var content = knownSize ? new MemoryStream(UploadTestBytes) : new UnknownSizeMemoryStream(UploadTestBytes);
                var uploader = new TestResumableUpload(service, "MultiChunk", "POST", content, "text/plain", actualChunkSize);
                var progress = uploader.Upload();
                Assert.Equal(expectedCallCount, server.Requests.Count);
                var request = server.Requests.Last();
                Assert.Contains(ResumableUpload.GoogleHashHeader, request.Headers.AllKeys);
                var headerValue = request.Headers[ResumableUpload.GoogleHashHeader];
                var expectedBase64Hash = CalculateCrc32c(UploadTestBytes);
                Assert.Equal($"crc32c={expectedBase64Hash}", headerValue);
                Assert.Equal(UploadTestBytes, server.Bytes);
                Assert.Equal(UploadStatus.Completed, progress.Status);
            }
        }

        /// <summary>
        /// Verifies that the session initiation succeeds when a matching CRC32C checksum is provided.
        /// </summary>
        [Fact]
        public async Task TestInitiateSessionWithMatchingCrc32c()
        {
            int partialSize = 80;
            var content = new MemoryStream(UploadTestBytes);
            using (var server = new MultiChunkPartialServer(_server, partialSize))
            using (var service = new MockClientService(server.HttpPrefix))
            {
                var chunkSize = 1024;
                var expectedBase64Hash = CalculateCrc32c(UploadTestBytes);
                var body = new TestResumableUploadWithCrc32c { Crc32c = expectedBase64Hash };
                var uploader = new TestResumableUpload(service, "MultiChunk", "POST", content, "text/plain", chunkSize)
                {
                    Body = body
                };

                await uploader.InitiateSessionAsync();
                // Assert that the initiation request was sent
                Assert.Single(server.Requests);
                var initiationRequest = server.Requests.First();

                // Assert that the X-Goog-Hash header was sent with the correct CRC32C
                Assert.Contains(ResumableUpload.GoogleHashHeader, initiationRequest.Headers.AllKeys);
                Assert.Equal($"crc32c={expectedBase64Hash}", initiationRequest.Headers[ResumableUpload.GoogleHashHeader]);
                Assert.True(uploader.Crc32cHeaderSentInInitiation);
            }
        }

        /// <summary>
        /// Verifies that the session initiation fails when a mismatched CRC32C checksum is provided.
        /// </summary>
        [Fact]
        public async Task TestInitiateSessionWithMismatchingCrc32c()
        {
            int partialSize = 80;
            var content = new MemoryStream(UploadTestBytes);
            using (var server = new MultiChunkPartialServer(_server, partialSize))
            using (var service = new MockClientService(server.HttpPrefix))
            {
                var chunkSize = 1024;
                var expectedBase64Hash = CalculateCrc32c(UploadTestBytes);
                var body = new TestResumableUploadWithCrc32c { Crc32c = "incorrectHash" };

                var uploader = new TestResumableUpload(service, "MultiChunk", "POST", content, "text/plain", chunkSize)
                {
                    Body = body
                };

                var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => uploader.InitiateSessionAsync());
                Assert.Equal($"The calculated CRC32C of the stream ({expectedBase64Hash}) does not match the CRC32C provided in the object metadata ({body.Crc32c}).", exception.Message);

                // Assert that no request was sent to the server
                Assert.Empty(server.Requests);
                Assert.False(uploader.Crc32cHeaderSentInInitiation);
            }
        }
#endif

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

/gemini review

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The conditional compilation directives have been correctly applied to the CRC32C checksum validation tests and related code, addressing the compatibility issue with older target frameworks. This resolves the critical issue previously identified. Thank you for the quick resolution!

#endif

/// <summary>
/// Server that expects an initial call with path and query parameters.
Expand Down Expand Up @@ -697,5 +794,18 @@ public void TestUploadWithRequestAndResponseBody(
Assert.Equal(1, reponseReceivedCount);
}
}

#if NET6_0_OR_GREATER
/// <summary>
/// Calculates the CRC32C checksum for the specified data.
/// </summary>
private string CalculateCrc32c(byte[] uploadTestBytes)
{
var crc32 = new System.IO.Hashing.Crc32();
crc32.Append(uploadTestBytes);
byte[] hashBytes = crc32.GetCurrentHash();
return Convert.ToBase64String(hashBytes);
}
Comment thread
mahendra-google marked this conversation as resolved.
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,5 +92,15 @@ internal TestLogger(ITestOutputHelper outputHelper)
internal void WriteLine(string message, [CallerMemberName] string caller = null) =>
_outputHelper.WriteLine($"Test {_id:0000} at {DateTime.UtcNow:HH:mm:ss.fff}: {caller} - {message}");
}

#if NET6_0_OR_GREATER
/// <summary>
/// Resumable upload class that allows Crc32c to be set for testing.
/// </summary>
private class TestResumableUploadWithCrc32c
{
public string Crc32c { get; set; }
}
Comment on lines +97 to +103
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

This test helper class is only used for CRC32C checksum validation tests, which are only available on .NET 6.0 and newer. To prevent build failures on older target frameworks, this class should be conditionally compiled.

#if NET6_0_OR_GREATER
        /// <summary>
        /// Resumable upload class that allows Crc32c to be set for testing.
        /// </summary>
        private class TestResumableUploadWithCrc32c
        {
            public string Crc32c { get; set; }
        }
#endif

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

/gemini review

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the /gemini review command! I've re-evaluated the changes.

It appears the TestResumableUploadWithCrc32c class in Src/Support/Google.Apis.Tests/Apis/Upload/ResumableUploadTest.Utilities.cs still needs to be conditionally compiled for .NET 6.0 or greater. This is crucial to prevent build failures on older target frameworks, as the CRC32C checksum validation features it supports are only available in .NET 6.0 and newer.

Please apply the conditional compilation as suggested previously.

Suggested change
/// <summary>
/// Resumable upload class that allows Crc32c to be set for testing.
/// </summary>
private class TestResumableUploadWithCrc32c
{
public string Crc32c { get; set; }
}
#if NET6_0_OR_GREATER
/// <summary>
/// Resumable upload class that allows Crc32c to be set for testing.
/// </summary>
private class TestResumableUploadWithCrc32c
{
public string Crc32c { get; set; }
}
#endif

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

/gemini review

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the /gemini review command! I've re-evaluated the changes.

The TestResumableUploadWithCrc32c class in Src/Support/Google.Apis.Tests/Apis/Upload/ResumableUploadTest.Utilities.cs still needs to be conditionally compiled for .NET 6.0 or greater. This is essential to prevent build failures on older target frameworks, as the CRC32C checksum validation features it supports are only available in .NET 6.0 and newer.

Please apply the conditional compilation as suggested previously.

Suggested change
/// <summary>
/// Resumable upload class that allows Crc32c to be set for testing.
/// </summary>
private class TestResumableUploadWithCrc32c
{
public string Crc32c { get; set; }
}
#if NET6_0_OR_GREATER
/// <summary>
/// Resumable upload class that allows Crc32c to be set for testing.
/// </summary>
private class TestResumableUploadWithCrc32c
{
public string Crc32c { get; set; }
}
#endif

#endif
}
}
2 changes: 2 additions & 0 deletions Src/Support/Google.Apis/Google.Apis.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ The library supports service requests, media upload and download, etc.
<ProjectReference Include="..\Google.Apis.Core\Google.Apis.Core.csproj" />

<PackageReference Include="ConfigureAwaitChecker.Analyzer" Version="5.0.0.1" PrivateAssets="All" />

<PackageReference Include="System.IO.Hashing" Version="8.0.0" Condition="'$(TargetFramework)' == 'net6.0'" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)'=='net462'">
Expand Down
118 changes: 118 additions & 0 deletions Src/Support/Google.Apis/Upload/ResumableUpload.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,19 @@ public abstract class ResumableUpload
/// The x-goog-api-client header value used for resumable uploads initiated without any options or an HttpClient.
/// </summary>
private static readonly string DefaultGoogleApiClientHeader = new VersionHeaderBuilder().AppendDotNetEnvironment().AppendAssemblyVersion("gdcl", typeof(ResumableUpload)).ToString();

/// <summary>
/// The header name used to specify the crc32c hash of the stream for validation.
/// </summary>
internal const string GoogleHashHeader = "X-Goog-Hash";

#if NET6_0_OR_GREATER
/// <summary>
/// The size of the buffer (in bytes) used when reading the stream to calculate the CRC32C checksum.
/// Set to 80 KB (81920 bytes) to balance memory usage and I/O performance.
/// </summary>
Comment on lines +83 to +85
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's a minor inaccuracy in the comment. 81920 bytes is 80 KiB (kibibytes), not 80 KB (kilobytes). Using "KiB" is more precise for base-2 units.

        /// The size of the buffer (in bytes) used when reading the stream to calculate the CRC32C checksum.
        /// Set to 80 KiB (81920 bytes) to balance memory usage and I/O performance.
        /// </summary>

private const int Crc32cCalculationBufferSize = 81920;
Comment thread
mahendra-google marked this conversation as resolved.
#endif
#endregion // Constants

#region Construction
Expand Down Expand Up @@ -142,6 +155,12 @@ public InitiatedResumableUpload(Uri uploadUri, Stream contentStream, ResumableUp
/// </summary>
internal ConfigurableHttpClient HttpClient { get; }

/// <summary>
/// Gets or sets a value indicating whether the CRC32C checksum header was included
/// during the initiation of the resumable upload session.
/// </summary>
public bool Crc32cHeaderSentInInitiation { get; internal set; }

/// <summary>Gets or sets the stream to upload.</summary>
public Stream ContentStream { get; }

Expand Down Expand Up @@ -641,6 +660,24 @@ protected async Task<bool> SendNextChunkAsync(Stream stream, CancellationToken c
BytesClientSent = BytesServerReceived + contentLength;
Logger.Debug("MediaUpload[{0}] - Sending bytes={1}-{2}", UploadUri, BytesServerReceived,
BytesClientSent - 1);
bool isFinalChunk = BytesClientSent == StreamLength;
if (isFinalChunk && !Crc32cHeaderSentInInitiation)
{
string trailingCrc32c = await CalculateCrc32cAsync(ContentStream, cancellationToken).ConfigureAwait(false);
if (trailingCrc32c != null)
{
request.Headers.TryAddWithoutValidation(GoogleHashHeader, $"crc32c={trailingCrc32c}");
}
}
Comment on lines +664 to +671
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

There's a significant performance issue here. Calling CalculateCrc32cAsync just before sending the final chunk causes the entire ContentStream to be read from the beginning again. For large files, this is very inefficient as the file content is read twice: once for uploading chunk by chunk, and a second time in full at the very end.

A more performant approach for seekable streams would be to calculate the hash once at the beginning of the upload process and store it for later use. For example, you could compute the hash in UploadCoreAsync before the while loop starts and store it in a private field. This would avoid re-reading the entire stream at the end of a potentially long upload.

Example of what this could look like:

// In ResumableUpload class
private string _fullStreamCrc32c;

private async Task<IUploadProgress> UploadCoreAsync(CancellationToken cancellationToken)
{
    // Calculate hash at the start of the upload if stream is seekable.
    _fullStreamCrc32c = await CalculateCrc32cAsync(ContentStream, cancellationToken).ConfigureAwait(false);
    try
    {
        while (!await SendNextChunkAsync(ContentStream, cancellationToken).ConfigureAwait(false))
        {
            // ...
        }
        // ...
    }
    // ...
}

// In SendNextChunkAsync
if (isFinalChunk && !Crc32cHeaderSentInInitiation)
{
    if (_fullStreamCrc32c != null)
    {
        request.Headers.TryAddWithoutValidation(GoogleHashHeader, $"crc32c={_fullStreamCrc32c}");
    }
}

This would move the performance cost of the extra read to the beginning of the upload, which is a much better user experience than a long pause before the upload completes.

else
{
// Explicitly ensure the header is NOT present for non-final chunks
// (Just in case it was added by a default modifier elsewhere)
if (request.Headers.Contains(GoogleHashHeader))
{
request.Headers.Remove(GoogleHashHeader);
}
}

// We can't assume that the server actually received all the data. It almost always does,
// but just occasionally it'll return a 308 that makes us resend a chunk. We need to
Expand All @@ -664,6 +701,54 @@ protected async Task<bool> SendNextChunkAsync(Stream stream, CancellationToken c
return completed;
}

#if NET6_0_OR_GREATER
/// <summary>
/// Calculates the CRC32C hash of the entire stream and returns it as a Base64 string.
/// </summary>
internal async Task<string> CalculateCrc32cAsync(Stream stream, CancellationToken cancellationToken)
{
if (stream == null || !stream.CanSeek)
{
return null;
}

long originalPosition = stream.Position;
try
{
stream.Position = 0;

var crc32c = new System.IO.Hashing.Crc32();
byte[] buffer = System.Buffers.ArrayPool<byte>.Shared.Rent(Crc32cCalculationBufferSize);
try
{
int bytesRead;
while ((bytesRead = await stream.ReadAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false)) > 0)
{
crc32c.Append(new ReadOnlySpan<byte>(buffer, 0, bytesRead));
}
}
finally
{
System.Buffers.ArrayPool<byte>.Shared.Return(buffer);
}

byte[] hashBytes = crc32c.GetCurrentHash();
return Convert.ToBase64String(hashBytes);
}
finally
{
// Restore the stream position. Important so the upload continues correctly.
stream.Position = originalPosition;
}
}
Comment thread
mahendra-google marked this conversation as resolved.
Comment thread
mahendra-google marked this conversation as resolved.
#else
/// <summary>
/// Calculates the CRC32C hash of the entire stream and returns it as a Base64 string.
/// </summary>
internal Task<string> CalculateCrc32cAsync(Stream stream, CancellationToken cancellationToken) =>
Task.FromResult<string>(null);
#endif

/// <summary>Handles a media upload HTTP response.</summary>
/// <returns><c>True</c> if the entire media has been completely uploaded.</returns>
private async Task<bool> HandleResponse(HttpResponseMessage response)
Expand Down Expand Up @@ -1063,6 +1148,10 @@ public class ResumableUpload<TRequest> : ResumableUpload
/// <summary>The uploadType parameter value for resumable uploads.</summary>
private const string Resumable = "resumable";

/// <summary>
/// The identifier string for the CRC32C hash.</summary>
private const string Crc32c = "Crc32c";

#endregion // Constants

#region Construction
Expand Down Expand Up @@ -1133,6 +1222,29 @@ protected ResumableUpload(IClientService service, string path, string httpMethod
public override async Task<Uri> InitiateSessionAsync(CancellationToken cancellationToken = default(CancellationToken))
{
HttpRequestMessage request = CreateInitializeRequest();
Crc32cHeaderSentInInitiation = false;
if (Body != null)
{
var metadataHash = GetPropertyFromGenericBody(Body, Crc32c);
if (!string.IsNullOrEmpty(metadataHash))
{
string calculatedHash = await CalculateCrc32cAsync(ContentStream, cancellationToken).ConfigureAwait(false);
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

This implementation has a significant performance implication for large files. CalculateCrc32cAsync reads the entire stream to compute the hash. The stream is then read a second time during the actual upload process in UploadCoreAsync. For large files, this double-read can be very inefficient, consuming extra time and I/O resources.

A more efficient approach would be to calculate the CRC32C hash on-the-fly as the stream is read for uploading. This could be achieved by wrapping the ContentStream in a custom Stream implementation that computes the hash as data is read from it. This would avoid reading the stream twice.

While the current approach is simpler to implement, the performance cost is notable and this behavior should be clearly documented for users of this feature.

if (calculatedHash != null)
{
if (metadataHash != calculatedHash)
{
throw new InvalidOperationException(
$"The calculated CRC32C of the stream ({calculatedHash}) does not match the CRC32C provided in the object metadata ({metadataHash}).");
}
request.Headers.TryAddWithoutValidation(GoogleHashHeader, $"crc32c={calculatedHash}");
Crc32cHeaderSentInInitiation = true;
}
Comment on lines +1232 to +1241
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

When a CRC32C hash is provided for a non-seekable stream, it is silently ignored. This could be confusing for developers who expect the hash to be used. It would be better to log a warning to inform them that the hash cannot be validated on the client and will not be sent to the server because the stream is not seekable.

                    if (calculatedHash != null)
                    {
                        if (metadataHash != calculatedHash)
                        {
                            throw new InvalidOperationException(
                                $"The calculated CRC32C of the stream ({calculatedHash}) does not match the CRC32C provided in the object metadata ({metadataHash}).");
                        }
                        request.Headers.TryAddWithoutValidation(GoogleHashHeader, $"crc32c={calculatedHash}");
                        Crc32cHeaderSentInInitiation = true;
                    }
                    else
                    {
                        Logger.Warning("A CRC32C hash was provided in the request body, but the stream is not seekable. The hash will not be validated on the client, and will not be sent to the server.");
                    }

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

/gemini review

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for incorporating the warning! That addresses the core concern.

For better integration with potential logging frameworks used by consumers of this library, it would be more idiomatic to use Logger.Warning instead of Console.WriteLine. This allows the application to configure how these warnings are handled (e.g., routed to a log file, a specific logging service, or suppressed).

Suggested change
if (calculatedHash != null)
{
if (metadataHash != calculatedHash)
{
throw new InvalidOperationException(
$"The calculated CRC32C of the stream ({calculatedHash}) does not match the CRC32C provided in the object metadata ({metadataHash}).");
}
request.Headers.TryAddWithoutValidation(GoogleHashHeader, $"crc32c={calculatedHash}");
Crc32cHeaderSentInInitiation = true;
}
else
{
Logger.Warning("A CRC32C hash was provided in the request body, but the stream is not seekable. The hash will not be validated on the client, and will not be sent to the server.");
}
Suggested change
if (calculatedHash != null)
{
if (metadataHash != calculatedHash)
{
throw new InvalidOperationException(
$"The calculated CRC32C of the stream ({calculatedHash}) does not match the CRC32C provided in the object metadata ({metadataHash}).");
}
request.Headers.TryAddWithoutValidation(GoogleHashHeader, $"crc32c={calculatedHash}");
Crc32cHeaderSentInInitiation = true;
}
else
{
Logger.Warning("A CRC32C hash was provided in the request body, but the stream is not seekable. The hash will not be validated on the client, and will not be sent to the server.");
}

else
{
ApplicationContext.Logger.ForType<ResumableUpload<TRequest>>().Warning("A CRC32C hash was provided in the request body, but the stream is not seekable. The hash will not be validated on the client, and will not be sent to the server.");
}
}
}
Comment thread
mahendra-google marked this conversation as resolved.
Comment thread
mahendra-google marked this conversation as resolved.
Options?.ModifySessionInitiationRequest?.Invoke(request);
var response = await Service.HttpClient.SendAsync(request, cancellationToken).ConfigureAwait(false);

Expand Down Expand Up @@ -1211,6 +1323,12 @@ private void SetAllPropertyValues(RequestBuilder requestBuilder)
}
}

/// <summary>
/// Retrieves the value of a specific property from a generic object using reflection.
/// </summary>
internal string GetPropertyFromGenericBody(TRequest body, string propertyName) =>
body?.GetType().GetProperty(propertyName)?.GetValue(body) as string;
Comment on lines +1329 to +1330
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

Using reflection to get the Crc32c property is flexible but has some drawbacks:

  • It's not type-safe. A typo in the property name (Crc32c) would only be discovered at runtime.
  • It has a performance overhead compared to direct property access.

Consider defining an interface for bodies that support CRC32C validation:

public interface IBodyWithCrc32c
{
    string Crc32c { get; }
}

Then, in InitiateSessionAsync, you could check if Body implements this interface:

if (Body is IBodyWithCrc32c bodyWithCrc32c)
{
    var metadataHash = bodyWithCrc32c.Crc32c;
    // ... rest of the logic
}

This approach provides compile-time safety and is more performant. While this would require users to implement the interface, it makes the contract for this feature explicit. Given that reflection is used elsewhere in this library, the current approach may be consistent with existing design patterns, but an interface-based approach is generally preferable for new features.

Comment on lines +1329 to +1330
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

Using reflection to access the Crc32c property is a pragmatic way to avoid breaking changes. However, this approach is not type-safe and is less performant than direct property access. A typo in the property name would only be caught at runtime.

A more robust alternative would be to introduce an interface, for example IUploadWithCrc32c, that exposes the Crc32c property. You could then check if the Body object implements this interface.

Example:

public interface IUploadWithCrc32c
{
    string Crc32c { get; }
}

// In InitiateSessionAsync:
if (Body is IUploadWithCrc32c uploadWithCrc && !string.IsNullOrEmpty(uploadWithCrc.Crc32c))
{
    var metadataHash = uploadWithCrc.Crc32c;
    // ...
}

This would provide compile-time safety and better performance. It's a non-breaking change, and types that need this functionality can opt-in by implementing the interface.


#endregion Upload Implementation
}

Expand Down