feat(Storage): Enable full object checksum validation for resumable uploads - #17
mahendra-google wants to merge 32 commits into
Conversation
![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 updates the upload validation mechanism in the Google Cloud Storage client by deprecating client-side validation (UploadValidationMode.ThrowOnly and UploadValidationMode.DeleteAndThrow) in favor of server-side validation (UploadValidationMode.RejectAndThrow). It introduces a custom HashingStream to calculate and inject the CRC32C hash on the fly. The review feedback highlights two main issues: first, HashingStream accesses _stream.Position directly, which will fail with a NotSupportedException on non-seekable streams; second, there is a redundant and unused local variable calculatedHash declared in the CustomMediaUpload constructor.
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.
| internal sealed class HashingStream : Stream | ||
| { | ||
| private readonly Stream _stream; | ||
| private readonly Crc32c _hasher; | ||
| private long _maxPositionHashed = 0; | ||
|
|
||
| public HashingStream(Stream stream) | ||
| { | ||
| _stream = stream; | ||
| _hasher = new Crc32c(); | ||
| } | ||
|
|
||
| public override int Read(byte[] buffer, int offset, int count) | ||
| { | ||
| long startingPos = _stream.Position; | ||
| int bytesRead = _stream.Read(buffer, offset, count); | ||
| ProcessBytes(buffer, offset, bytesRead, startingPos); | ||
| return bytesRead; | ||
| } | ||
|
|
||
| public override async Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) | ||
| { | ||
| long startingPos = _stream.Position; | ||
| int bytesRead = await _stream.ReadAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false); | ||
| ProcessBytes(buffer, offset, bytesRead, startingPos); | ||
| return bytesRead; | ||
| } | ||
|
|
||
| private void ProcessBytes(byte[] buffer, int offset, int bytesRead, long startingPos) | ||
| { | ||
| if (bytesRead <= 0) return; | ||
|
|
||
| // Only hash bytes that are beyond the furthest point we've already hashed. | ||
| // This handles the rewind and re-read scenario during retries. | ||
| if (startingPos + bytesRead > _maxPositionHashed) | ||
| { | ||
| long newBytesStart = Math.Max(startingPos, _maxPositionHashed); | ||
| int actuallyNewCount = (int) ((startingPos + bytesRead) - newBytesStart); | ||
| int bufferOffset = offset + (int) (newBytesStart - startingPos); | ||
|
|
||
| _hasher.UpdateHash(buffer, bufferOffset, actuallyNewCount); | ||
| _maxPositionHashed = startingPos + bytesRead; | ||
| } | ||
| } | ||
|
|
||
| public override long Position | ||
| { | ||
| get => _stream.Position; | ||
| set => _stream.Position = value; | ||
| } | ||
|
|
||
| public override long Seek(long offset, SeekOrigin origin) => _stream.Seek(offset, origin); | ||
| public string GetBase64Hash() => Convert.ToBase64String(_hasher.GetHash()); | ||
| public override bool CanRead => _stream.CanRead; | ||
| public override bool CanSeek => _stream.CanSeek; | ||
| public override bool CanWrite => _stream.CanWrite; | ||
| public override long Length => _stream.Length; | ||
| public override void Flush() => _stream.Flush(); | ||
| public override void SetLength(long value) => _stream.SetLength(value); | ||
| public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count); | ||
| } |
There was a problem hiding this comment.
Calling _stream.Position directly in Read and ReadAsync will throw a NotSupportedException if the underlying stream is non-seekable (e.g., NetworkStream, GZipStream, or streamed HTTP responses). Since UploadValidationMode.RejectAndThrow is now the default, this will cause uploads from non-seekable streams to fail.\n\nTo support non-seekable streams, we should track the stream's position internally within HashingStream and only delegate to _stream.Position or _stream.Seek when the stream supports seeking.
internal sealed class HashingStream : Stream\n {\n private readonly Stream _stream;\n private readonly Crc32c _hasher;\n private long _maxPositionHashed = 0;\n private long _position;\n\n public HashingStream(Stream stream)\n {\n _stream = stream;\n _hasher = new Crc32c();\n _position = stream.CanSeek ? stream.Position : 0;\n }\n\n public override int Read(byte[] buffer, int offset, int count)\n {\n long startingPos = _position;\n int bytesRead = _stream.Read(buffer, offset, count);\n ProcessBytes(buffer, offset, bytesRead, startingPos);\n _position = startingPos + bytesRead;\n return bytesRead;\n }\n\n public override async Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)\n {\n long startingPos = _position;\n int bytesRead = await _stream.ReadAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false);\n ProcessBytes(buffer, offset, bytesRead, startingPos);\n _position = startingPos + bytesRead;\n return bytesRead;\n }\n\n private void ProcessBytes(byte[] buffer, int offset, int bytesRead, long startingPos)\n {\n if (bytesRead <= 0) return;\n\n // Only hash bytes that are beyond the furthest point we've already hashed.\n // This handles the rewind and re-read scenario during retries.\n if (startingPos + bytesRead > _maxPositionHashed)\n {\n long newBytesStart = Math.Max(startingPos, _maxPositionHashed);\n int actuallyNewCount = (int) ((startingPos + bytesRead) - newBytesStart);\n int bufferOffset = offset + (int) (newBytesStart - startingPos);\n\n _hasher.UpdateHash(buffer, bufferOffset, actuallyNewCount);\n _maxPositionHashed = startingPos + bytesRead;\n }\n }\n\n public override long Position\n {\n get => _position;\n set\n {\n _stream.Position = value;\n _position = value;\n }\n }\n\n public override long Seek(long offset, SeekOrigin origin)\n {\n long newPosition = _stream.Seek(offset, origin);\n _position = newPosition;\n return newPosition;\n }\n\n public string GetBase64Hash() => Convert.ToBase64String(_hasher.GetHash());\n public override bool CanRead => _stream.CanRead;\n public override bool CanSeek => _stream.CanSeek;\n public override bool CanWrite => _stream.CanWrite;\n public override long Length => _stream.Length;\n public override void Flush() => _stream.Flush();\n public override void SetLength(long value) => _stream.SetLength(value);\n public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count);\n }| _hashingStream = ContentStream as HashingStream; | ||
| var calculatedHash = _hashingStream.GetBase64Hash(); |
There was a problem hiding this comment.
The local variable calculatedHash declared on line 42 is redundant and unused because another local variable with the same name is declared inside the LastRequestExecuting lambda on line 45. Additionally, calling GetBase64Hash() at construction time is unnecessary as no data has been read from the stream yet. This line should be removed.
_hashingStream = ContentStream as HashingStream;
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the upload validation mechanism from client-side checks (which threw UploadValidationException and deleted invalid objects) to server-side validation using the new RejectAndThrow mode, which rejects invalid uploads with a 400 Bad Request. To support this, a custom HashingStream is introduced to calculate the CRC32C hash on the fly and inject it into the x-goog-hash header. The review feedback highlights critical issues in HashingStream where accessing _stream.Position will throw a NotSupportedException on non-seekable streams, and where seeking forward could result in gaps and an incorrect hash. It is recommended to track gaps, avoid sending the hash header if the hash is incomplete, and clean up unused variables in the new unit tests to prevent compiler warnings or errors.
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.
| internal sealed class HashingStream : Stream | ||
| { | ||
| private readonly Stream _stream; | ||
| private readonly Crc32c _hasher; | ||
| private long _maxPositionHashed = 0; | ||
|
|
||
| public HashingStream(Stream stream) | ||
| { | ||
| _stream = stream; | ||
| _hasher = new Crc32c(); | ||
| } | ||
|
|
||
| public override int Read(byte[] buffer, int offset, int count) | ||
| { | ||
| long startingPos = _stream.Position; | ||
| int bytesRead = _stream.Read(buffer, offset, count); | ||
| ProcessBytes(buffer, offset, bytesRead, startingPos); | ||
| return bytesRead; | ||
| } | ||
|
|
||
| public override async Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) | ||
| { | ||
| long startingPos = _stream.Position; | ||
| int bytesRead = await _stream.ReadAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false); | ||
| ProcessBytes(buffer, offset, bytesRead, startingPos); | ||
| return bytesRead; | ||
| } | ||
|
|
||
| private void ProcessBytes(byte[] buffer, int offset, int bytesRead, long startingPos) | ||
| { | ||
| if (bytesRead <= 0) return; | ||
|
|
||
| // Only hash bytes that are beyond the furthest point we've already hashed. | ||
| // This handles the rewind and re-read scenario during retries. | ||
| if (startingPos + bytesRead > _maxPositionHashed) | ||
| { | ||
| long newBytesStart = Math.Max(startingPos, _maxPositionHashed); | ||
| int actuallyNewCount = (int) ((startingPos + bytesRead) - newBytesStart); | ||
| int bufferOffset = offset + (int) (newBytesStart - startingPos); | ||
|
|
||
| _hasher.UpdateHash(buffer, bufferOffset, actuallyNewCount); | ||
| _maxPositionHashed = startingPos + bytesRead; | ||
| } | ||
| } | ||
|
|
||
| public override long Position | ||
| { | ||
| get => _stream.Position; | ||
| set => _stream.Position = value; | ||
| } | ||
|
|
||
| public override long Seek(long offset, SeekOrigin origin) => _stream.Seek(offset, origin); | ||
| public string GetBase64Hash() => Convert.ToBase64String(_hasher.GetHash()); | ||
| public override bool CanRead => _stream.CanRead; | ||
| public override bool CanSeek => _stream.CanSeek; | ||
| public override bool CanWrite => _stream.CanWrite; | ||
| public override long Length => _stream.Length; | ||
| public override void Flush() => _stream.Flush(); | ||
| public override void SetLength(long value) => _stream.SetLength(value); | ||
| public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count); | ||
| } |
There was a problem hiding this comment.
The current implementation of HashingStream unconditionally accesses _stream.Position in Read and ReadAsync. For non-seekable streams (such as NetworkStream or GZipStream), accessing Position throws a NotSupportedException. Since UploadValidationMode.RejectAndThrow is now enabled by default, this will cause all uploads of non-seekable streams to fail immediately.
Additionally, if the stream is seeked forward or starts at a non-zero position (e.g., when resuming an upload), there will be gaps in the hashed bytes, resulting in an incorrect final hash and a 400 Bad Request from the server. We should track if there are any gaps and expose an IsHashComplete property so we can avoid sending the hash header in those cases.
internal sealed class HashingStream : Stream
{
private readonly Stream _stream;
private readonly Crc32c _hasher;
private long _maxPositionHashed = 0;
private long _position = 0;
private bool _hasGaps = false;
public HashingStream(Stream stream)
{
_stream = stream;
_hasher = new Crc32c();
}
public bool IsHashComplete => !_hasGaps;
public override int Read(byte[] buffer, int offset, int count)
{
long startingPos = _stream.CanSeek ? _stream.Position : _position;
int bytesRead = _stream.Read(buffer, offset, count);
ProcessBytes(buffer, offset, bytesRead, startingPos);
if (!_stream.CanSeek)
{
_position += bytesRead;
}
return bytesRead;
}
public override async Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
{
long startingPos = _stream.CanSeek ? _stream.Position : _position;
int bytesRead = await _stream.ReadAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false);
ProcessBytes(buffer, offset, bytesRead, startingPos);
if (!_stream.CanSeek)
{
_position += bytesRead;
}
return bytesRead;
}
private void ProcessBytes(byte[] buffer, int offset, int bytesRead, long startingPos)
{
if (bytesRead <= 0) return;
if (startingPos > _maxPositionHashed)
{
_hasGaps = true;
}
if (startingPos + bytesRead > _maxPositionHashed)
{
long newBytesStart = Math.Max(startingPos, _maxPositionHashed);
int actuallyNewCount = (int) ((startingPos + bytesRead) - newBytesStart);
int bufferOffset = offset + (int) (newBytesStart - startingPos);
_hasher.UpdateHash(buffer, bufferOffset, actuallyNewCount);
_maxPositionHashed = startingPos + bytesRead;
}
}
public override long Position
{
get => _stream.CanSeek ? _stream.Position : _position;
set
{
if (_stream.CanSeek)
{
_stream.Position = value;
}
else
{
throw new NotSupportedException();
}
}
}
public override long Seek(long offset, SeekOrigin origin)
{
if (_stream.CanSeek)
{
return _stream.Seek(offset, origin);
}
throw new NotSupportedException();
}
public string GetBase64Hash() => Convert.ToBase64String(_hasher.GetHash());
public override bool CanRead => _stream.CanRead;
public override bool CanSeek => _stream.CanSeek;
public override bool CanWrite => _stream.CanWrite;
public override long Length => _stream.Length;
public override void Flush() => _stream.Flush();
public override void SetLength(long value) => _stream.SetLength(value);
public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count);
}| _hashingStream = ContentStream as HashingStream; | ||
| LastRequestExecuting += (HttpRequestMessage request) => | ||
| { | ||
| var calculatedHash = _hashingStream.GetBase64Hash(); | ||
| if (!request.Headers.Contains(GoogleHashHeader)) | ||
| { | ||
| request.Headers.Add(GoogleHashHeader, $"crc32c={calculatedHash}"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
To prevent sending an incomplete or incorrect hash when the upload is resumed or has gaps (e.g., when seeking forward), we should only add the x-goog-hash header if the hash is complete. We can check _hashingStream.IsHashComplete before calculating and adding the header.
_hashingStream = ContentStream as HashingStream;
LastRequestExecuting += (HttpRequestMessage request) =>
{
if (_hashingStream != null && _hashingStream.IsHashComplete)
{
var calculatedHash = _hashingStream.GetBase64Hash();
if (!request.Headers.Contains(GoogleHashHeader))
{
request.Headers.Add(GoogleHashHeader, $"crc32c={calculatedHash}");
}
}
};| hashingStream.Read(buffer, 0, 10); | ||
| var hashAfterPartial = hashingStream.GetBase64Hash(); |
There was a problem hiding this comment.
The variable hashAfterPartial is assigned but never used. In projects with <TreatWarningsAsErrors> enabled, this will cause a compiler error (CS0219). Since it is not needed for the test, it should be removed.
hashingStream.Read(buffer, 0, 10);| hashingStream.Read(buffer, 0, 10); | ||
| var hashAfterPartial = hashingStream.GetBase64Hash(); |
There was a problem hiding this comment.
The variable hashAfterPartial is assigned but never used. In projects with <TreatWarningsAsErrors> enabled, this will cause a compiler error (CS0219). Since it is not needed for the test, it should be removed.
hashingStream.Read(buffer, 0, 10);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces server-side hash validation for uploads by adding the RejectAndThrow validation mode and deprecating the older client-side validation modes (ThrowOnly, DeleteAndThrow) along with UploadValidationException. It implements a new internal HashingStream to calculate the CRC32C hash on the fly and attach it to the request headers. The review feedback suggests three improvements to CustomMediaUpload.cs: ensuring consistent resolution of the validation mode when options are null, caching the calculated hash in HashingStream to guarantee idempotency during retries, and overriding Dispose in HashingStream to prevent potential resource leaks of the underlying stream.
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.
| Stream stream, string contentType) | ||
| : base(service, body, bucket, stream, contentType) | ||
| Stream stream, string contentType, UploadObjectOptions options) | ||
| : base(service, body, bucket, options?.UploadValidationMode != UploadValidationMode.None ? new HashingStream(stream) : stream, contentType) |
There was a problem hiding this comment.
Using options?.UploadValidationMode != UploadValidationMode.None in the base constructor call can lead to inconsistency if UploadObjectOptions.DefaultValidationMode is ever changed in the future (e.g., to UploadValidationMode.None).
If options is null, options?.UploadValidationMode is null, so options?.UploadValidationMode != UploadValidationMode.None evaluates to true, and a HashingStream is created. However, in the constructor body, validationMode is resolved using options?.UploadValidationMode ?? UploadObjectOptions.DefaultValidationMode. If the default mode were None, validationMode would be None, and the LastRequestExecuting handler would not be registered, leaving the HashingStream unused.
To prevent this inconsistency, resolve the validation mode consistently using the default value in both places.
: base(service, body, bucket, (options?.UploadValidationMode ?? UploadObjectOptions.DefaultValidationMode) != UploadValidationMode.None ? new HashingStream(stream) : stream, contentType)| } | ||
| throw new NotSupportedException(); | ||
| } | ||
| public string GetBase64Hash() => Convert.ToBase64String(_hasher.GetHash()); |
There was a problem hiding this comment.
Depending on the implementation of Crc32c, calling GetHash() might be non-idempotent (e.g., it might reset the internal state of the hash algorithm). If the final request is retried, LastRequestExecuting will be called again, which would result in a second call to GetBase64Hash(). If GetHash() resets the state, this second call would return an incorrect hash or throw an exception.
Caching the calculated hash ensures that subsequent calls to GetBase64Hash() are safe, idempotent, and avoid redundant computation.
private string _calculatedHash;
public string GetBase64Hash() => _calculatedHash ??= Convert.ToBase64String(_hasher.GetHash());| public override long Length => _stream.Length; | ||
| public override void Flush() => _stream.Flush(); | ||
| public override void SetLength(long value) => _stream.SetLength(value); | ||
| public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count); |
There was a problem hiding this comment.
Since HashingStream is a wrapper stream, it should propagate disposal to the underlying _stream. Currently, if HashingStream is disposed (e.g., by ResumableUpload when DisposeStream is true), the underlying stream will not be disposed, potentially leading to resource leaks.
public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count);
protected override void Dispose(bool disposing)
{
if (disposing)
{
_stream.Dispose();
}
base.Dispose(disposing);
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the upload validation mechanism from client-side checks to server-side validation using the new RejectAndThrow mode, deprecating older validation modes and the UploadValidationException. It introduces a HashingStream to calculate and append the CRC32C hash header during upload. The review feedback suggests improving the null-handling logic when initializing the base stream in CustomMediaUpload and adding an extra safety check to IsHashComplete to verify that seekable streams are fully read.
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.
| Stream stream, string contentType) | ||
| : base(service, body, bucket, stream, contentType) | ||
| Stream stream, string contentType, UploadObjectOptions options) | ||
| : base(service, body, bucket, options?.UploadValidationMode != UploadValidationMode.None ? new HashingStream(stream) : stream, contentType) |
There was a problem hiding this comment.
Using options?.UploadValidationMode != UploadValidationMode.None can be less readable because if options is null, the expression evaluates to true due to nullable comparison rules. It is clearer and more consistent with the constructor body to explicitly fall back to UploadObjectOptions.DefaultValidationMode.
: base(service, body, bucket, (options?.UploadValidationMode ?? UploadObjectOptions.DefaultValidationMode) != UploadValidationMode.None ? new HashingStream(stream) : stream, contentType)| _hasher = new Crc32c(); | ||
| } | ||
|
|
||
| public bool IsHashComplete => !_hasGaps; |
There was a problem hiding this comment.
If the underlying stream is seekable, we can perform an extra safety check to ensure that the stream has been fully read up to its length before declaring the hash complete. This prevents potential false positives if the upload was aborted or finalized prematurely.
public bool IsHashComplete => !_hasGaps && (!_stream.CanSeek || _maxPositionHashed == _stream.Length);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the upload validation mechanism in Google Cloud Storage V1 by deprecating client-side validation (such as DeleteAndThrow and ThrowOnly) in favor of server-side validation via the new RejectAndThrow mode, which returns a GoogleApiException with a 400 BadRequest status code. It introduces a custom HashingStream to calculate CRC32C hashes on the fly and inject them into the x-goog-hash header. The review feedback highlights a bug in HashingStream where a seekable stream starting at a non-zero position incorrectly disables validation, and suggests adding a corresponding unit test. Additionally, the reviewer recommends strengthening the integration tests by verifying that rejected objects are indeed not created on the server.
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.
| public HashingStream(Stream stream) | ||
| { | ||
| _stream = stream; | ||
| _hasher = new Crc32c(); | ||
| } |
There was a problem hiding this comment.
If the source stream is seekable and its initial position is not 0 (for example, if a BOM or header was already read, or if uploading a specific segment of a stream), startingPos on the first read will be greater than _maxPositionHashed (which is initialized to 0). This will incorrectly set _hasGaps to true and prevent the x-goog-hash header from being sent, silently disabling validation.
To fix this, initialize _maxPositionHashed to the stream's current position if it is seekable.
public HashingStream(Stream stream)
{
_stream = stream;
_hasher = new Crc32c();
_maxPositionHashed = _stream.CanSeek ? _stream.Position : 0;
}| public void HashingStream_ShouldHandleRetries_WhenSeekingBackwardsToIntermediatePoint() | ||
| { | ||
| var data = Encoding.UTF8.GetBytes("The quick brown fox jumps over the lazy dog"); | ||
| var baseStream = new MemoryStream(data); | ||
| var hashingStream = new CustomMediaUpload.HashingStream(baseStream); | ||
| var buffer = new byte[data.Length]; | ||
|
|
||
| hashingStream.Read(buffer, 0, 10); | ||
|
|
||
| // Simulate the Retry logic: Seek back to the intermediate point. | ||
| hashingStream.Position = 5; | ||
|
|
||
| hashingStream.Read(buffer, 0, data.Length); | ||
| var finalHash = hashingStream.GetBase64Hash(); | ||
|
|
||
| var expectedHasher = new Crc32c(); | ||
| expectedHasher.UpdateHash(data, 0, data.Length); | ||
| var expectedHash = Convert.ToBase64String(expectedHasher.GetHash()); | ||
| Assert.Equal(expectedHash, finalHash); | ||
| } |
There was a problem hiding this comment.
Add a unit test to verify that HashingStream correctly handles streams with a non-zero initial position, ensuring that IsHashComplete is evaluated as true and the hash is computed correctly.
[Fact]
public void HashingStream_ShouldHandleRetries_WhenSeekingBackwardsToIntermediatePoint()
{
var data = Encoding.UTF8.GetBytes("The quick brown fox jumps over the lazy dog");
var baseStream = new MemoryStream(data);
var hashingStream = new CustomMediaUpload.HashingStream(baseStream);
var buffer = new byte[data.Length];
hashingStream.Read(buffer, 0, 10);
// Simulate the Retry logic: Seek back to the intermediate point.
hashingStream.Position = 5;
hashingStream.Read(buffer, 0, data.Length);
var finalHash = hashingStream.GetBase64Hash();
var expectedHasher = new Crc32c();
expectedHasher.UpdateHash(data, 0, data.Length);
var expectedHash = Convert.ToBase64String(expectedHasher.GetHash());
Assert.Equal(expectedHash, finalHash);
}
[Fact]
public void HashingStream_ShouldHandleNonZeroInitialPosition()
{
var data = Encoding.UTF8.GetBytes("The quick brown fox jumps over the lazy dog");
var baseStream = new MemoryStream(data);
baseStream.Position = 10;
var hashingStream = new CustomMediaUpload.HashingStream(baseStream);
var buffer = new byte[data.Length];
hashingStream.Read(buffer, 0, data.Length);
var finalHash = hashingStream.GetBase64Hash();
var expectedHasher = new Crc32c();
expectedHasher.UpdateHash(data, 10, data.Length - 10);
var expectedHash = Convert.ToBase64String(expectedHasher.GetHash());
Assert.Equal(expectedHash, finalHash);
Assert.True(hashingStream.IsHashComplete);
}| var exception = Assert.Throws<GoogleApiException>(() => client.UploadObject(bucket, name, null, stream, options)); | ||
| Assert.Equal(HttpStatusCode.BadRequest, exception.HttpStatusCode); |
There was a problem hiding this comment.
To fully verify the RejectAndThrow behavior, assert that the object was indeed not created on the server by attempting to retrieve it and expecting a 404 Not Found response.
var exception = Assert.Throws<GoogleApiException>(() => client.UploadObject(bucket, name, null, stream, options));
Assert.Equal(HttpStatusCode.BadRequest, exception.HttpStatusCode);
var notFound = Assert.Throws<GoogleApiException>(() => _fixture.Client.GetObject(bucket, name));
Assert.Equal(HttpStatusCode.NotFound, notFound.HttpStatusCode);| var exception = await Assert.ThrowsAsync<GoogleApiException>(() => client.UploadObjectAsync(bucket, name, null, stream, options)); | ||
| Assert.Equal(HttpStatusCode.BadRequest, exception.HttpStatusCode); |
There was a problem hiding this comment.
To fully verify the RejectAndThrow behavior asynchronously, assert that the object was indeed not created on the server by attempting to retrieve it and expecting a 404 Not Found response.
var exception = await Assert.ThrowsAsync<GoogleApiException>(() => client.UploadObjectAsync(bucket, name, null, stream, options));
Assert.Equal(HttpStatusCode.BadRequest, exception.HttpStatusCode);
var notFound = await Assert.ThrowsAsync<GoogleApiException>(() => _fixture.Client.GetObjectAsync(bucket, name));
Assert.Equal(HttpStatusCode.NotFound, notFound.HttpStatusCode);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions upload validation from client-side (which previously involved deleting mismatched objects after upload) to server-side validation using the x-goog-hash header. It deprecates UploadValidationException along with the ThrowOnly and DeleteAndThrow validation modes, introducing RejectAndThrow as the new default. To support this, a custom HashingStream is introduced to calculate the CRC32C hash on the fly, including support for stream rewinds during retries. Feedback on these changes includes: (1) refining the x-goog-hash header check to specifically look for an existing crc32c entry rather than the header's presence alone, as multiple hashes can coexist; (2) tracking EOF for non-seekable streams in HashingStream to guarantee the hash is fully complete; and (3) caching the calculated hash in GetBase64Hash() to prevent redundant computations and potential state resets.
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.
| if (!request.Headers.Contains(GoogleHashHeader)) | ||
| { | ||
| request.Headers.Add(GoogleHashHeader, $"crc32c={calculatedHash}"); | ||
| } |
There was a problem hiding this comment.
Checking request.Headers.Contains(GoogleHashHeader) will prevent adding the CRC32C checksum if another hash (such as MD5) is already present in the x-goog-hash header. GCS supports multiple x-goog-hash headers (e.g., one for MD5 and one for CRC32C). We should check specifically if a crc32c entry is already present, and if not, append the calculated CRC32C hash.
bool hasCrc32c = false;
if (request.Headers.TryGetValues(GoogleHashHeader, out var values))
{
foreach (var value in values)
{
if (value.Contains("crc32c"))
{
hasCrc32c = true;
break;
}
}
}
if (!hasCrc32c)
{
request.Headers.TryAddWithoutValidation(GoogleHashHeader, $"crc32c={calculatedHash}");
}| _hasher = new Crc32c(); | ||
| } | ||
|
|
||
| public bool IsHashComplete => !_hasGaps && (!_stream.CanSeek || _maxPositionHashed == _stream.Length); |
There was a problem hiding this comment.
For non-seekable streams, IsHashComplete currently returns true as long as there are no gaps, even if the stream has not been fully read to the end. While LastRequestExecuting is typically called at the end of the upload, tracking whether the stream has actually reached EOF (by checking if Read/ReadAsync returned 0) would make this check much more robust and prevent sending a partial hash if the upload is finalized prematurely or aborted.
Consider tracking _reachedEof when Read or ReadAsync returns 0:
private bool _reachedEof = false;
public override int Read(byte[] buffer, int offset, int count)
{
long startingPos = _stream.CanSeek ? _stream.Position : _position;
int bytesRead = _stream.Read(buffer, offset, count);
if (bytesRead == 0)
{
_reachedEof = true;
}
ProcessBytes(buffer, offset, bytesRead, startingPos);
if (!_stream.CanSeek)
{
_position += bytesRead;
}
return bytesRead;
}
public bool IsHashComplete => !_hasGaps && (_stream.CanSeek ? _maxPositionHashed == _stream.Length : _reachedEof);| throw new NotSupportedException(); | ||
| } | ||
|
|
||
| public string GetBase64Hash() => Convert.ToBase64String(_hasher.GetHash()); |
There was a problem hiding this comment.
If GetBase64Hash() is called multiple times (e.g., during retries of the final request), caching the calculated hash avoids re-computing it and prevents any potential issues if the underlying _hasher.GetHash() is not idempotent or resets state.
private string _calculatedHash;
public string GetBase64Hash() => _calculatedHash ??= Convert.ToBase64String(_hasher.GetHash());
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request shifts upload validation from client-side to server-side by introducing the RejectAndThrow validation mode and deprecating the older ThrowOnly and DeleteAndThrow modes. A custom HashingStream is introduced to calculate the CRC32C hash on the fly and append it to the x-goog-hash header, allowing the server to reject mismatched uploads before creation. Review feedback recommends adding enum validation for UploadValidationMode, throwing an exception if the hashing stream fails to initialize to prevent silent validation bypass, and using null-safe navigation when checking header values to avoid potential null reference exceptions.
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.
| Stream stream, string contentType, UploadObjectOptions options) | ||
| : base(service, body, bucket, (options?.UploadValidationMode ?? UploadObjectOptions.DefaultValidationMode) != UploadValidationMode.None ? new HashingStream(stream) : stream, contentType) | ||
| { | ||
| var validationMode = options?.UploadValidationMode ?? UploadObjectOptions.DefaultValidationMode; |
There was a problem hiding this comment.
The validation of the UploadValidationMode enum value was removed from UploadHelper but is not performed here. To prevent unexpected behavior if an undefined integer value is cast to the enum, we should validate it using GaxPreconditions.CheckEnumValue.
var validationMode = options?.UploadValidationMode ?? UploadObjectOptions.DefaultValidationMode;
Google.Api.Gax.GaxPreconditions.CheckEnumValue(validationMode, nameof(UploadValidationMode));| var validationMode = options?.UploadValidationMode ?? UploadObjectOptions.DefaultValidationMode; | ||
| if (validationMode != UploadValidationMode.None) | ||
| { | ||
| _hashingStream = ContentStream as HashingStream; |
There was a problem hiding this comment.
If ContentStream is wrapped or modified by the base class (or in future versions of the Google API Client Library), ContentStream as HashingStream will return null, and checksum validation will be silently skipped without any error. To prevent this silent failure, we should throw an exception if _hashingStream is null when validation is enabled.
_hashingStream = ContentStream as HashingStream;
if (_hashingStream == null)
{
throw new InvalidOperationException("Failed to initialize hashing stream for upload validation.");
}| { | ||
| foreach (var value in values) | ||
| { | ||
| if (value.Contains("crc32c")) |
There was a problem hiding this comment.
To prevent a potential NullReferenceException if any header value is null, use null-safe navigation when checking if the header value contains "crc32c".
if (value?.Contains("crc32c") == true)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the upload validation mechanism from client-side validation to server-side validation using the new RejectAndThrow mode. It introduces a HashingStream to calculate the CRC32C hash on the fly and append it as a header for server-side verification, while deprecating UploadValidationException and older validation modes. Feedback suggests an optimization in HashingStream.ProcessBytes to return early if gaps are already detected, avoiding unnecessary hashing.
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.
| private void ProcessBytes(byte[] buffer, int offset, int bytesRead, long startingPos) | ||
| { | ||
| if (bytesRead <= 0) return; | ||
|
|
||
| if (startingPos > _maxPositionHashed) | ||
| { | ||
| _hasGaps = true; | ||
| } | ||
|
|
||
| // Only hash bytes that are beyond the furthest point we've already hashed. | ||
| // This handles the rewind and re-read scenario during retries. | ||
| if (startingPos + bytesRead > _maxPositionHashed) | ||
| { | ||
| long newBytesStart = Math.Max(startingPos, _maxPositionHashed); | ||
| int actuallyNewCount = (int) ((startingPos + bytesRead) - newBytesStart); | ||
| int bufferOffset = offset + (int) (newBytesStart - startingPos); | ||
|
|
||
| _hasher.UpdateHash(buffer, bufferOffset, actuallyNewCount); | ||
| _maxPositionHashed = startingPos + bytesRead; | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimization Opportunity
If _hasGaps is already true, the hash is incomplete and will never be used. We can optimize ProcessBytes by returning early when _hasGaps is true or when a new gap is detected. This avoids unnecessary hashing and calculations for the remainder of the stream.
private void ProcessBytes(byte[] buffer, int offset, int bytesRead, long startingPos)
{
if (bytesRead <= 0 || _hasGaps) return;
if (startingPos > _maxPositionHashed)
{
_hasGaps = true;
return;
}
// Only hash bytes that are beyond the furthest point we've already hashed.
// This handles the rewind and re-read scenario during retries.
if (startingPos + bytesRead > _maxPositionHashed)
{
long newBytesStart = Math.Max(startingPos, _maxPositionHashed);
int actuallyNewCount = (int) ((startingPos + bytesRead) - newBytesStart);
int bufferOffset = offset + (int) (newBytesStart - startingPos);
_hasher.UpdateHash(buffer, bufferOffset, actuallyNewCount);
_maxPositionHashed = startingPos + bytesRead;
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions object upload validation from client-side checks to server-side validation using the x-goog-hash header. It introduces a new HashingStream to calculate the CRC32C hash on the fly, deprecates old validation modes (ThrowOnly, DeleteAndThrow) along with UploadValidationException, and adds the new RejectAndThrow validation mode. A critical issue was identified in HashingStream where initializing _maxPositionHashed to 0 instead of the stream's initial position (when seekable) causes checksum validation to be silently disabled if the stream starts at a non-zero position.
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.
| public HashingStream(Stream stream) | ||
| { | ||
| _stream = stream; | ||
| _hasher = new Crc32c(); | ||
| } |
There was a problem hiding this comment.
If a seekable stream is passed to the uploader with a non-zero initial position (for example, if the stream was partially read or if only a portion of the stream is being uploaded), _maxPositionHashed is initialized to 0 while the first read starts at stream.Position (e.g., 100). This causes startingPos > _maxPositionHashed to evaluate to true on the very first read, setting _hasGaps = true and silently disabling the checksum validation.
Initializing _maxPositionHashed to the stream's initial position when CanSeek is true resolves this issue and ensures checksum validation works correctly for partially read streams.
public HashingStream(Stream stream)
{
_stream = stream;
_hasher = new Crc32c();
_maxPositionHashed = stream.CanSeek ? stream.Position : 0;
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the upload validation mechanism to server-side validation by introducing the RejectAndThrow validation mode and deprecating the client-side ThrowOnly and DeleteAndThrow modes. It implements a custom HashingStream to calculate CRC32C hashes on the fly during upload, allowing the server to reject mismatched uploads. The review feedback is highly valuable and identifies critical improvements: ensuring Read and ReadAsync do not incorrectly flag EOF when count is zero, making the "crc32c" header check case-insensitive, and properly enforcing the read-only contract of HashingStream by disabling write operations.
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.
| if (bytesRead == 0) | ||
| { | ||
| _reachedEof = true; | ||
| } |
There was a problem hiding this comment.
If count is 0, _stream.Read will return 0 even if the end of the stream has not been reached. This incorrectly sets _reachedEof to true. To prevent this, only set _reachedEof to true when count > 0 and bytesRead == 0.
if (count > 0 && bytesRead == 0)
{
_reachedEof = true;
}| if (bytesRead == 0) | ||
| { | ||
| _reachedEof = true; | ||
| } |
There was a problem hiding this comment.
If count is 0, _stream.ReadAsync will return 0 even if the end of the stream has not been reached. This incorrectly sets _reachedEof to true. To prevent this, only set _reachedEof to true when count > 0 and bytesRead == 0.
if (count > 0 && bytesRead == 0)
{
_reachedEof = true;
}| if (value?.Contains("crc32c") == true) | ||
| { | ||
| hasCrc32c = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
The check value?.Contains("crc32c") is case-sensitive. To ensure robustness against potential case variations in header values, use a case-insensitive comparison like IndexOf with StringComparison.OrdinalIgnoreCase.
if (value?.IndexOf("crc32c", StringComparison.OrdinalIgnoreCase) >= 0)
{
hasCrc32c = true;
break;
}| public override bool CanWrite => _stream.CanWrite; | ||
| public override long Length => _stream.Length; | ||
| public override void Flush() => _stream.Flush(); | ||
| public override void SetLength(long value) => _stream.SetLength(value); | ||
| public override void Write(byte[] buffer, int offset, int count) => _stream.Write(buffer, offset, count); |
There was a problem hiding this comment.
Since HashingStream is a read-only wrapper stream designed solely for hashing read bytes, it should explicitly enforce the read-only contract by returning false for CanWrite and throwing NotSupportedException for write operations (Write, SetLength).
public override bool CanWrite => false;
public override long Length => _stream.Length;
public override void Flush() => _stream.Flush();
public override void SetLength(long value) => throw new NotSupportedException();
public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException();
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Google Cloud Storage upload validation mechanism by introducing 'RejectAndThrow' as the new default validation mode. This change leverages server-side validation to reject invalid uploads before object creation, rendering the previous client-side 'ThrowOnly' and 'DeleteAndThrow' validation modes obsolete. The changes include the addition of a 'HashingStream' to support hash calculation during uploads, updates to 'CustomMediaUpload' to include the 'x-goog-hash' header, and deprecation of the 'UploadValidationException' class. Corresponding unit tests have been updated to reflect these changes, and new tests have been added to verify the 'HashingStream' behavior under various retry and resume scenarios.
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.
75a97e4 to
b60ecce
Compare
8160401 to
4b0d135
Compare
- Add integration tests to verify ArgumentException propagation through Google.Apis ResumableUpload when resuming an upload from the intermediate offset.
…ry in pipeline-state.json - Assign value 5.0.0 to the nextVersion propery so that next storage release will be major release
…76.0.4250 in apis.json. - Removed Google.Apis 1.76 version dependency from Google.Cloud.Storage.V1 project file - Regenerated project file using generateprojects.sh
- Throw an argument exception as soon as gap is found while resuming upload from an intermeditae offset - Maintain -position value up to date in the CustomMediaUpload - Add static method GetWrappedSourceStream to wrap source stream based on upload validation mode - Modify upload object integration tests - Create an unit test file for CustomMediaUpload test. - Modify UploadObjectOptionsTest file.
…e offset retry. Removed test for CustomMediaUpload with intermediate offset retry.
docs: Fix for some documentation cross-references feat: Added `DaiSessionService` feat: Added `startTime`, `archived`, and `publisherFloorExempt` fields to `PrivateAuctionDeal` PiperOrigin-RevId: 981266310 Source-Link: googleapis/googleapis@859b02b
feat: add streaming_mode and effective_streaming_mode to the Gateway resource docs: describe defaultHostname by what it serves rather than by a fixed hostname template PiperOrigin-RevId: 981915566 Source-Link: googleapis/googleapis@e5830f2
feat: Add support for ExtensionBinding resource in NetworkServices API PiperOrigin-RevId: 981699957 Source-Link: googleapis/googleapis@1d4ae20
feat: add Templates functionality to parametermanager API feat: add Tagging support on Parameters feat: add checksum support on Parameters docs: update documentation for ListLocations and View enum PiperOrigin-RevId: 982650924 Source-Link: googleapis/googleapis@5e89775
feat: add ClientInfo message to DeviceSession message PiperOrigin-RevId: 981998285 Source-Link: googleapis/googleapis@5f61cd5
### New features - Added `DaiSessionService` - Added `startTime`, `archived`, and `publisherFloorExempt` fields to `PrivateAuctionDeal` ### Documentation improvements - Fix for some documentation cross-references Librarian-Release-Library: Google.Ads.AdManager.V1 Librarian-Release-Version: 1.0.0-beta12 Librarian-Release-ID: release-20260917T182808Z
### New features - Add streaming_mode and effective_streaming_mode to the Gateway resource ### Documentation improvements - Describe defaultHostname by what it serves rather than by a fixed hostname template - Rewrap field comments to the publishing tool's line width, with no change of meaning - Reference google.longrunning.Operation.error explicitly in OperationMetadata so the link resolves Librarian-Release-Library: Google.Cloud.ApiGateway.V1 Librarian-Release-Version: 2.6.0 Librarian-Release-ID: release-20260917T182808Z
### New features - Add Templates functionality to parametermanager API - Add Tagging support on Parameters - Add checksum support on Parameters ### Documentation improvements - Update documentation for ListLocations and View enum Librarian-Release-Library: Google.Cloud.ParameterManager.V1 Librarian-Release-Version: 1.1.0 Librarian-Release-ID: release-20260917T182808Z
### New features - Add idempotency header (googleapis#15778) Librarian-Release-Library: Google.Cloud.Storage.V1 Librarian-Release-Version: 4.16.0 Librarian-Release-ID: release-20260917T182808Z
### New features - Add ClientInfo message to DeviceSession message Librarian-Release-Library: Google.Cloud.DeviceStreaming.V1 Librarian-Release-Version: 1.1.0 Librarian-Release-ID: release-20260917T182808Z
…ersion 1.0.0-beta01 ### New features - Initial generation for Google.Shopping.Merchant.LoyaltyCustomers.V1 Librarian-Release-Library: Google.Shopping.Merchant.LoyaltyCustomers.V1 Librarian-Release-Version: 1.0.0-beta01 Librarian-Release-ID: release-20260917T182808Z
…rom Google.Cloud.Storage.V1 file
…Cloud.Storage.V1 file
5 participants
This PR transitions upload object checksum integrity validation from client-side to the server-side.
Previously,
CRC32Cvalidation occurred after the upload is completed, requiring the client to delete the object (usingDeleteAndThrowupload validation mode) or leave a corrupted object in the bucket (usingThrowOnlyupload validation mode) upon hash mismatches. By leveraging the newLastRequestExecutingevent ingoogle-api-dotnet-clientcore library, this implementation calculatesCRC32Cincrementally during streaming and injects thex-goog-hash: crc32c=...header on the final chunk. If a checksum mismatch occurs, the server rejects the upload with an HTTP400 Bad Request, ensuring invalid objects are never created in the bucket.