feat(storage): setting idempotency token header - #27338
shubhangi-google wants to merge 19 commits into
Conversation
There was a problem hiding this comment.
AI review: there are two issues.
-
Global Default Scope Leak (Critical Issue):
By settingRequestOptions.default.add_idempotency_token_header = trueingoogle/apis/options.rb, this PR enables the GCS-specificX-Goog-Gcs-Idempotency-Tokenheader globally for all 400+ Google API clients usinggoogle-apis-core(e.g., YouTube, Drive, Compute Engine, Calendar, Bigtable). This violates service abstraction boundaries and adds a 36-byte UUID overhead and confusing headers to every non-GCS request.
Fix: Defaultadd_idempotency_token_headertofalseingoogle-apis-coreand explicitly enable it in thegoogle-apis-storage_v1client generation or configuration. -
Redundant Memoization:
Inset_idempotency_token_header, the@idempotency_token ||= SecureRandom.uuidmemoization is functionally dead code because of the early return on the line right before it:
return if header.any? { |k, _| k.to_s.downcase == 'x-goog-gcs-idempotency-token' }
@idempotency_token ||= SecureRandom.uuidBecause the header Hash is mutated in place and retained across retries of the ApiCommand instance, the first network attempt injects the header into header. On subsequent retries, the loop evaluates return if header.any? { ... }, successfully finding the original key, and returning early. Therefore, @idempotency_token is never evaluated or used again across retries.
Fix: You can simply do header['X-Goog-Gcs-Idempotency-Token'] = SecureRandom.uuid, as caching the UUID as an instance variable is unnecessary.
|
|
||
| def invocation_id_header | ||
| "gccl-invocation-id/#{SecureRandom.uuid}" | ||
| @invocation_id ||= SecureRandom.uuid |
There was a problem hiding this comment.
Why are we making this an instance variable?
| .join(' ') | ||
| .split | ||
| .find_all { |s| s !~ %r{^gl-ruby/|^gdcl/} } | ||
| .find_all { |s| s !~ %r{^gl-ruby/|^gdcl/|^gccl-invocation-id/} } |
There was a problem hiding this comment.
Why are we adding invocation_id header regex here?
|
|
||
| def set_idempotency_token_header | ||
| return if options&.header&.any? { |k, _| k.to_s.downcase == 'x-goog-gcs-idempotency-token' } | ||
| return if header.any? { |k, _| k.to_s.downcase == 'x-goog-gcs-idempotency-token' } |
There was a problem hiding this comment.
Can you please help me understand how header is populated?
| RequestOptions.default.quota_project = nil | ||
| RequestOptions.default.add_invocation_id_header = false | ||
| RequestOptions.default.upload_chunk_size = 100 * 1024 * 1024 # 100 MB | ||
| RequestOptions.default.add_idempotency_token_header = false |
There was a problem hiding this comment.
Is adding idempotency token going to be optional?
I thought it was going to be added by default.
| it 'should not accumulate duplicate gccl-invocation-id clauses when prepare! is called multiple times' do | ||
| command.options.add_invocation_id_header = true | ||
| command.prepare! | ||
| first_header = command.header['X-Goog-Api-Client'] |
There was a problem hiding this comment.
The test talks about checking duplicity of gccl-invocation-id while over here we're checking X-Goog-Api-Client .
Is this intentional?
| command.options.add_invocation_id_header = true | ||
| command.prepare! | ||
| expect(command.header["X-Goog-Api-Client"]).to include("gccl-invocation-id") | ||
| expect(command.header["X-Goog-Api-Client"]).to match(/gccl-invocation-id\/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/) |
There was a problem hiding this comment.
Same thing here. We're setting X-Goog-Api-Client to match with gccl-invocation-id value.
| command.options.header = { 'x-goog-gcs-idempotency-token' => 'my-custom-token' } | ||
| command.prepare! | ||
| expect(command.header['x-goog-gcs-idempotency-token']).to eql 'my-custom-token' | ||
| expect(command.header['X-Goog-Gcs-Idempotency-Token']).to be_nil |
There was a problem hiding this comment.
Why would it be nil here?
This pull request introduces support for sending an idempotency token header (X-Goog-Gcs-Idempotency-Token) in API commands, enabled by default via the new add_idempotency_token_header option.
linked storage library Pr : googleapis/google-cloud-ruby#34956