docs(storage): add gaxios migration guide - #8340
thiyaguk09 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the library to version 8, bumping the minimum Node.js requirement to 18 and replacing the legacy request stack with a new StorageTransport layer based on gaxios and native Web APIs. The review identifies several critical issues in the new transport implementation, including a module-level global variable causing race conditions for project IDs, incorrect URL resolution logic, and broken interceptor support. Further feedback points out that per-object interceptors are no longer being applied, identifies fragile link extraction logic in GitHub scripts, and notes potential bugs related to unawaited async calls and synchronous project ID usage before it is fetched.
I am having trouble creating individual review comments. Click here to see my feedback.
handwritten/storage/src/storage-transport.ts (90)
The use of a module-level global variable projectId to store state across requests is a critical bug. In a concurrent environment, multiple Storage instances or concurrent requests will overwrite this shared variable, leading to race conditions where a request for one project might use the ID of another. This state must be encapsulated within the StorageTransport instance.
handwritten/storage/src/storage-transport.ts (192)
The URL construction logic uses string concatenation (${this.baseUrl}${pathUri}) instead of the native URL resolution rules. As noted in MIGRATION.md, a leading slash in pathUri should resolve relative to the host root, stripping segments like /storage/v1 from the baseUrl. The current implementation will incorrectly produce URLs with duplicated segments (e.g., .../storage/v1/storage/v1/...) because many paths in the library now include the full API prefix.
url = new URL(pathUri, this.baseUrl);
handwritten/storage/conformance-test/conformanceCommon.ts (63)
This TODO effectively disables the methodMap initialization by using an empty object. This will cause conformance tests to skip all scenarios or fail silently, as the mapping between JSON test methods and library methods is lost.
Object.entries(jsonToNodeApiMapping),
handwritten/storage/src/nodejs-common/service-object.ts (448-464)
The new implementation of getMetadata (and similarly setMetadata and delete) fails to pass this.interceptors to this.storageTransport.makeRequest. In previous versions, ServiceObject would merge parent and local interceptors. This change breaks the functionality of per-object interceptors (e.g., interceptors added specifically to a File or Bucket instance).
handwritten/storage/src/storage-transport.ts (134-139)
There are two issues here: 1) Clearing and adding interceptors on a shared this.gaxiosInstance is not thread-safe for concurrent requests. 2) The actual request is performed via this.authClient.request, which uses its own internal transporter and ignores the interceptors added to this.gaxiosInstance. This effectively breaks all interceptor support in the library.
handwritten/storage/.github/scripts/close-invalid-link.cjs (45)
The logic for extracting the reproduction link is extremely fragile as it relies on a hardcoded line index (18). If a user adds extra lines or if the issue template is modified, this script will fail to find the link or pick up incorrect data. It is better to search the entire issue body for a GitHub/Gist URL following the expected section header.
handwritten/storage/src/bucket.ts (2081)
If this.storage.projectId is not provided in the constructor and has not yet been fetched via an internal request (which triggers the async getProjectId in StorageTransport), this topic string will be incorrectly constructed as projects/undefined/topics/.... Since this is a synchronous assignment, it should account for the possibility of the project ID being unavailable.
handwritten/storage/src/file.ts (1478)
The CopyCallback expects three arguments: (err, file, apiResponse). Calling it with only the error argument may cause issues for consumers expecting the full signature, especially if they are using promisified versions of the library that rely on specific argument positions.
.catch(err => callback!(err, null, null));
handwritten/storage/src/resumable-upload.ts (1362)
The call to attemptDelayedRetry is not awaited. Since attemptDelayedRetry is an async function that can call startUploading (also async), any rejection within that chain will result in an unhandled promise rejection because the caller (onResponse) has already returned.
await this.attemptDelayedRetry(resp);eacb087 to
0c58a9a
Compare
e3288e3 to
1fba172
Compare
c0a6220 to
0f2652b
Compare
2c0e9c8 to
696a337
Compare
696a337 to
fe055f7
Compare
3217d07 to
0d373cf
Compare
b1a54b0 to
f41a664
Compare
05532de to
9e95e7a
Compare
00dcb72 to
644cae2
Compare
8edeb26 to
5421117
Compare
5421117 to
d8b93e8
Compare
cbfd51b to
f84761a
Compare
3eb2445 to
d749fc2
Compare
35de221 to
daa5c53
Compare
f9023b1 to
a148670
Compare
…nd fix storage tests/lint
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕