feat(bigquery-driver): Upgrade @google-cloud/storage to 8 - #11905
Conversation
github-actions
Bot
added
driver:bigquery
|
Claude finished @ovr's task in 3m 55s —— View job Review of #11905 — approve with 2 low-severity notes (0 high, 0 medium, 2 low)Full review
Compatibility checkAll three usages are compatible with v8:
None of these are on v8's removal list, and Version consistency is right: The Findings
Both posted inline: #discussion_r4027928769, #discussion_r4027930536. One correction to the inline note on finding 1: Not worth an inline comment
Worth confirming out-of-band since I had no registry access here: that Review threadsThe thread listing returned 0 of my own threads (no unresolved threads at all on this PR), so none resolved and none re-affirmed. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "@cubejs-backend/shared": "1.7.39", | ||
| "@google-cloud/bigquery": "^7.7.0", | ||
| "@google-cloud/storage": "^7.13.0", | ||
| "@google-cloud/storage": "^8.1.0", |
There was a problem hiding this comment.
Verification gap on this major bump: nothing that runs in CI exercises the v8 GCS data path.
The two call sites that matter are bucket.getFiles(...) / file.getSignedUrl(...) in packages/cubejs-bigquery-driver/src/BigQueryDriver.ts:373-375 and the same pair in packages/cubejs-base-driver/src/storage-fs/gcs.fs.ts:39-43. Against the real SDK, the only coverage is new Storage(undefined) in packages/cubejs-base-driver/test/unit/storage-fs.test.ts:78, which just asserts the constructor doesn't throw — it never touches getFiles or getSignedUrl. The BigQuery driver's own tests are integration-only and need GCP credentials, so they don't run here either.
So a v8 behaviour change in either method's options shape or return tuple would land green. Worth adding a unit test that mocks Storage and asserts the getFiles/getSignedUrl call shapes against v8's types, or running the BigQuery export-bucket integration test once manually and noting the result on this PR.
There was a problem hiding this comment.
One addition to this, since it changes how much the green build is worth: tsc passing gives no signal on the option shape either.
BigQueryDriver.ts:163 calls new Storage(this.options), and this.options is the full BigQuery options object built at BigQueryDriver.ts:126-153 — scopes, keyFilename, exportBucket, exportBucketCsvEscapeSymbol, pollTimeout, pollMaxInterval, location, userAgent, plus a spread of user ...config. None of the last six are StorageOptions keys. TypeScript only applies excess-property checks to object literals, so passing a variable means every one of them is accepted silently. A clean tsc is not evidence that storage 8 tolerates them.
So the cheapest thing that would cover the upgrade here is not the getFiles/getSignedUrl mock, it is the constructor smoke test base-driver already has at test/unit/storage-fs.test.ts:74 (new Storage(undefined)), applied to the shape this driver actually passes:
expect(() => new Storage({
scopes: ["https://www.googleapis.com/auth/bigquery", "https://www.googleapis.com/auth/drive"],
projectId: "p",
exportBucket: "b",
pollTimeout: 600000,
userAgent: "CubeDev_Cube/1.7.39",
} as any)).not.toThrow();That needs a runnable unit path in this package first — package.json:18 is "test": "yarn integration" → jest --verbose dist/test, so a *.unit.test.ts added here compiles but is never invoked without GCP credentials.
|
Claude finished @ovr's task in 5m 54s —— View job Approve with two things to settle — 0 high, 2 mediumFull review
VerdictThe lockfile is clean and the PR body's claims hold up where I could check them. Two medium findings, both about what the upgrade is not covered by rather than about the upgrade being wrong. What checks outThe API-compatibility claim is correct for the actual call sites. Both consumers use a narrow surface:
None of those signatures moved in 7 → 8. The dedupe analysis is right, and there are no orphans. I checked every old transitive entry the diff leaves behind, and each still has a live consumer via
So the remaining duplicates genuinely straddle majors, as stated. The base-driver's 17/17 is a real signal, not an incidental one. No lockfile drift elsewhere. The other four lockfiles ( Findings
What I could not verifyThis runner has no Review threadsThe listing returned 2 of my own threads. Resolved 1 ( |
| "@cubejs-backend/shared": "1.7.39", | ||
| "@google-cloud/bigquery": "^7.7.0", | ||
| "@google-cloud/storage": "^7.13.0", | ||
| "@google-cloud/storage": "^8.1.0", |
There was a problem hiding this comment.
Storage 8 puts an ESM-only node-fetch on the GCS code path, which engines: { "node": ">=20.0.0" } (package.json:12, same in base-driver package.json:12) does not guarantee is loadable.
The new transitive chain in this diff:
@google-cloud/storage@8.1.0
-> retry-request@^9.0.1 (yarn.lock:19568)
-> teeny-request@^11.0.0 (yarn.lock:21046)
-> teeny-request@^11.0.1
-> node-fetch@^3.3.2 -> 3.3.2 (yarn.lock:16746)
node-fetch@3 is "type": "module" with no CJS entry. Both of these packages compile to CommonJS ("main": "dist/src/index.js", base-driver package.json:14), so reaching it goes through require(). require() of ESM only works from Node 20.19 / 22.12 onward — below that it throws ERR_REQUIRE_ESM. The declared floor is >=20.0.0, which admits 20.0 through 20.18.
This is new to this path. Storage 7.13.0's chain was retry-request@^7.0.0 → teeny-request@^9.0.0 → node-fetch@^2.6.9, all CJS, and storage 8 still pins gaxios@^6.0.2 (→ gaxios@6.6.0, node-fetch@^2.6.9, yarn.lock:13131), so gaxios is not what changed. gaxios@7 in the lockfile does depend on node-fetch 3 already, but nothing in the storage/bigquery chain resolves to it — teeny-request 11 is the first ESM-only dependency the GCS calls in BigQueryDriver.unload() and extractFilesFromGCS() can reach.
The published Docker images are unaffected — packages/cubejs-docker/latest.Dockerfile:1 is node:24.18.0-trixie-slim. The exposure is a user who npm i @cubejs-backend/server on a Node 20.x older than 20.19 and configures an export bucket; they get a load-time crash rather than a resolution error at install.
Two things to settle before merge:
- Whether
teeny-request@11.0.1is itself ESM (in which case therequire()is unavoidable) or CJS with a dynamicimport()of node-fetch (in which case it degrades gracefully). Worth confirming from the installednode_modules/teeny-request/package.json— I couldn't check it here, this runner has nonode_modulesand no registry access. - If it is a hard
require(), raise the floor to">=20.19.0"in both manifests so the incompatibility surfaces at install time. A silentERR_REQUIRE_ESMon the pre-aggregation export path is a much worse failure mode than anEBADENGINE.
There was a problem hiding this comment.
Correction on the floor number, from this round: @google-cloud/storage@8 declares engines.node: ">=22" itself, so ">=20.19.0" is not high enough — the required floor in both packages/cubejs-bigquery-driver/package.json:12 and packages/cubejs-base-driver/package.json:12 is ">=22". And the failure is not only the runtime ERR_REQUIRE_ESM described above: Yarn 1 classic hard-errors on an engine mismatch between a package and its dependency, so a self-hosted yarn install on Node 20 breaks at install time. Both manifests still say ">=20.0.0", so this thread stays open; the fix should bump the declared floor to >=22 in this PR rather than defer it.
`@google-cloud/storage ^7.13.0` -> `^8.1.0` in `@cubejs-backend/bigquery-driver` and `@cubejs-backend/base-driver`. Both use only `Storage` (and `Bucket` in the BigQuery driver), unchanged across 7 -> 8. This also clears 1 Dependabot alert on the root lockfile: `@google-cloud/storage@7.13.0` pinned `fast-xml-parser ^4.4.1`, capping it below the 5.7.0 that GHSA-jr48-27g9-4h5h needs; 8.1.0 depends on `^5.3.4`, which resolves to 5.11.1. The only remaining copies are 5.7.2 and 5.11.1. Verified: `yarn install --frozen-lockfile` is a no-op, `yarn tsc`, and `yarn workspace @cubejs-backend/base-driver unit` (17/17). The BigQuery driver has integration tests only, which need GCP credentials. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d/storage 8 `fast-xml-parser@5.11.1` (new, via `@google-cloud/storage@8.1.0`) requires `strnum ^2.4.2`, which landed alongside the `strnum ^2.2.3` that the pinned `fast-xml-parser@5.7.2` already required. 2.4.2 satisfies `^2.2.3`, so the `^2.2.3` spec key moves onto the 2.4.2 entry and the 2.2.3 entry drops. Key merged onto the surviving entry rather than deleted and re-resolved, so yarn picks nothing new. Verified by a name -> version-set diff of the lockfile: 0 new packages, 0 added versions, `strnum@2.2.3` removed, multi-version packages 390 -> 389. `yarn install --frozen-lockfile` passes and leaves the lock unmodified; a non-frozen `yarn install` is a no-op (nothing to re-resolve or prune). The other duplicates this upgrade brought in are not collapsible: `@google-cloud/paginator` 5/7, `projectify` 4/6, `promisify` 4/6, `retry-request` 7/9, `teeny-request` 9/11 and `@nodable/entities` 2/3 all straddle majors, held down by the `@google-cloud/bigquery@7.7.0` subtree and `@aws-sdk/xml-builder`; `fast-xml-parser@5.7.2` is an exact pin from `@aws-sdk/xml-builder`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….11.1 Adding `@google-cloud/storage@8`'s `fast-xml-parser ^5.3.4` made yarn re-resolve the whole 5.x group to 5.11.1, even though the 5.7.2 already in the tree satisfies `^5.3.4`. That split 5.x into two copies — `@aws-sdk/xml-builder`'s exact `5.7.2` pin cannot follow the bump — and pulled in three packages that were not in the lockfile before: `@nodable/entities@3.0.0`, `is-unsafe@2.0.2`, `anynum@1.0.1`. Merges `^5.0.7`, `^5.3.4` and `^5.4.1` back onto the 5.7.2 entry and drops the 5.11.1 one; a non-frozen `yarn install` then prunes the three orphans. 5.7.2 is still above the 5.7.0 that GHSA-jr48-27g9-4h5h requires, so the alert stays closed. This supersedes the `strnum` collapse in 111df0d, which is reverted here: that duplicate only existed because 5.11.1 required `strnum ^2.4.2`. With 5.11.1 gone, `strnum` is back to a single 2.2.3 on its own. Net effect on the lockfile versus master, by name -> version-set diff: | package | master | before | after | | --- | --- | --- | --- | | `fast-xml-parser` | 4.5.6, 5.7.2 | 5.7.2, 5.11.1 | 5.7.2 | | `strnum` | 1.0.5, 2.2.3 | 2.4.2 | 2.2.3 | | `@nodable/entities` | 2.1.0 | 2.1.0, 3.0.0 | 2.1.0 | | `is-unsafe` | - | 2.0.2 | - | | `anynum` | - | 1.0.1 | - | 0 new packages against master. `yarn install --frozen-lockfile` passes and leaves the lock unmodified, `tsc` on the BigQuery driver is clean, and `yarn workspace @cubejs-backend/base-driver unit` is 17/17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
38c13de to
80aade9
Compare
|
Claude finished @ovr's task in 4m 20s —— View job Review: dependency-only bump, no blocking defect — 1 medium (re-affirmed), 1 low (new inline comment)Full review
Review threadsThe listing returned 2 threads first-authored by me, both on
One new inline comment this round ( API surfaceThe used surface checks out against the call sites: Caveat on the stated verification method: a typings diff cannot show a changed runtime default (signing version, retry/timeout defaults live in code, not types), so "only an optional CoverageBetter than the PR body implies. Note that LockfileThe name-level claim holds: no new package names, and
Not flaggedNo security surface: the change adds no code, and |
| strnum "^1.0.5" | ||
|
|
||
| fast-xml-parser@^5.0.7, fast-xml-parser@^5.4.1: | ||
| fast-xml-parser@5.7.2, fast-xml-parser@^5.0.7, fast-xml-parser@^5.3.4, fast-xml-parser@^5.4.1: |
There was a problem hiding this comment.
This dedup is hand-merged and nothing enforces it, so it will come undone on the next lockfile touch.
^5.3.4 was moved onto this entry by hand — commit 80aade9 (fix(deps): Keep fast-xml-parser on 5.7.2 instead of re-resolving to 5.11.1) deleted the separate fast-xml-parser@^4.4.1 → 4.5.6 block and folded the range in here alongside 5.7.2, ^5.0.7, ^5.4.1. That is valid — 5.7.2 satisfies ^5.3.4 — but it is a lockfile-only fact. ^5.3.4 comes from a transitive dependent (yarn.lock:3334), ^5.0.7 from another (yarn.lock:1195), and ^5.4.1 from a third (yarn.lock:20081); none of those ranges appear in any package.json in the repo, and the root resolutions block (package.json:68-74 — es5-ext, lodash, @types/node, @types/ramda, thrift) has no fast-xml-parser key.
So the only thing keeping all four ranges on one resolution is that this hunk currently says so. Any yarn upgrade that revisits the entry, or a dependabot bump to one of the three dependents, is free to re-resolve ^5.3.4 (and the others) to the newest matching publish — 5.11.1 — and re-split the entry into the two blocks that 80aade9 just collapsed. The regression is silent: dedup is not something CI asserts, so it lands green and the duplicate copy reappears.
Making it durable is a one-line addition to the root resolutions:
"fast-xml-parser": "5.7.2",That pins every range — ^5.0.7, ^5.3.4, ^5.4.1 — to the single 5.7.2 entry regardless of what the transitive dependents ask for next, which is the invariant 80aade9 was reaching for. Worth a short comment on the entry too, noting why it is pinned rather than floating, so a future bump does not read it as stale and lift it.
Same reasoning applies to 8554df3 (Collapse the duplicate strnum introduced by @google-cloud/storage 8) if that collapse was also achieved by editing the lockfile rather than by a resolution.
There was a problem hiding this comment.
Not doing this — an exact resolutions pin on this package would cause a worse and quieter failure than the duplicate it prevents.
1. It would silently override a vendor pin that actually moves. @aws-sdk/xml-builder pins fast-xml-parser exactly, and that pin is not static:
@aws-sdk/xml-builder |
pins |
|---|---|
| 3.972.0 | 5.2.5 |
| 3.972.21 (current) | 5.7.2 |
| 3.972.30 | 5.7.3 |
| 3.972.40 | (dependency dropped) |
A root "fast-xml-parser": "5.7.2" would have forced 5.7.3 back down to 5.7.2 on the very next aws-sdk bump, against what the vendor deliberately chose — and because CI runs --frozen-lockfile, nothing would flag it.
2. It would block security patches on a package that just had one. The whole reason this PR exists is GHSA-jr48-27g9-4h5h (fast-xml-parser < 5.7.0). Exact-pinning the package Dependabot most recently filed an alert against means the next advisory needs someone to remember to edit resolutions by hand. Every existing entry in that block is there for a deliberate reason (es5-ext protestware avoidance, @types/node unification, thrift); none is a dedupe.
3. The premise overstates the risk. "Nothing enforces it" is true of every one of the ~3000 entries in yarn.lock — the lockfile is the enforcement, and this repo runs yarn install --frozen-lockfile in 15 places across 6 workflows, so nothing re-resolves in CI. The only thing that can re-split the entry is a deliberate dependency bump, which is a reviewed PR with the lockfile diff visible.
Same answer for 8554df3: that strnum collapse was reverted in 80aade9 anyway, since it only existed because the re-resolved 5.11.1 required strnum ^2.4.2.
If this drifts on a future bump, the fix is the same key-merge, applied then, with the version-set diff to prove it — not a standing pin on a CVE-prone parser.
…l-vm-modules
`@google-cloud/storage@8` pulls in `teeny-request@11`, which reaches node-fetch
through a dynamic import:
```js
const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args));
```
node-fetch 3 is ESM-only, so under Jest's CJS VM the import callback is never
installed and the first HTTP call throws before it reaches the network:
```
BigQueryDriver > unload
TypeError: A dynamic import callback was invoked without --experimental-vm-modules
```
`NODE_OPTIONS=--experimental-vm-modules` is already how athena, base-driver,
clickhouse, databricks-jdbc, prestodb and testing-drivers handle the same
problem; this applies it to the remaining driver `integration*` scripts so the
next dependency that goes ESM-only doesn't reopen this one suite at a time.
| package | scripts |
| --- | --- |
| bigquery | `integration:bigquery` |
| crate | `integration:crate` |
| dremio | `integration:dremio` |
| druid | `integration`, `integration:druid` |
| duckdb | `integration:duckdb` |
| firebolt | `integration:firebolt` |
| materialize | `integration:materialize` |
| mongobi | `integration`, `integration:mongobi` |
| mysql | `integration:mysql` |
| mysql-aurora-serverless | `integration` |
| pinot | `integration:pinot` |
| postgres | `integration:postgres` |
| questdb | `integration:questdb` |
| trino | `integration`, `integration:trino` |
| vertica | `integration:vertica` |
Reproduced the failure against the real `teeny-request@11` in a throwaway Jest
suite and confirmed the flag fixes it (fails without, passes with). `yarn
lint:npm` passes and every `packages/*-driver/package.json` still parses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
driver:mongodb
|
Claude finished @ovr's task in 4m 51s —— View job Dependency upgrade looks mechanically sound — 0 high, 2 medium, 2 low. 1 new inline comment; 3 prior threads re-affirmed, 0 resolved. Full review
What holds upThe
The Findings1. Declared Node floor is now wrong — medium. 2. 3. "0 new packages" is true by name, not by version — low, informational. Five resolutions are new to the tree: 4. The upgraded path is unverified against a live bucket — medium. Review threadsThe listing returned 3 threads first-authored by
Re-affirmed 3, resolved 0, 1 new inline comment posted. |
| "test": "yarn integration", | ||
| "integration": "npm run integration:crate", | ||
| "integration:crate": "jest --verbose dist/test", | ||
| "integration:crate": "NODE_OPTIONS=--experimental-vm-modules jest --verbose dist/test", |
There was a problem hiding this comment.
The flag went onto the integration* scripts but not the sibling unit scripts in the same packages, so the commit's stated goal — "so the next dependency that goes ESM-only doesn't reopen this one suite at a time" — is only half met.
Still running bare jest:
packages/cubejs-crate-driver/package.json:36packages/cubejs-dremio-driver/package.json:19packages/cubejs-duckdb-driver/package.json:33packages/cubejs-pinot-driver/package.json:33packages/cubejs-trino-driver/package.json:33packages/cubejs-mysql-aurora-serverless-driver/package.json:18
Nothing is broken today — teeny-request@11's import('node-fetch') only fires on an actual HTTP call, and these unit suites mock the connection. But push.yml:137 runs lerna run unit across every package, and BaseDriver.ts:703 already reaches GCS through await import('./storage-fs/gcs.fs.js'), so the first unit test that exercises an unload path fails with the same A dynamic import callback was invoked without --experimental-vm-modules.
Two options: add the flag to those six unit scripts for parity with clickhouse/athena/prestodb (which already have it on both), or set NODE_OPTIONS once at the workflow level in push.yml and drop it from the 29 individual scripts. The per-script approach is what the repo already does, so parity is the smaller change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11905 +/- ##
=======================================
Coverage 60.67% 60.67%
=======================================
Files 240 240
Lines 19297 19297
Branches 3914 3914
=======================================
Hits 11709 11709
Misses 7040 7040
Partials 548 548
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Check List
Description of Changes Made (if issue reference is not provided)
Bumps
@google-cloud/storagefrom^7.13.0to^8.1.0in@cubejs-backend/bigquery-driverand@cubejs-backend/base-driver; both use onlyStorage(plusBucketin the BigQuery driver), and thegetFiles/getSignedUrl/bucket.filesurface they touch is unchanged between 7.13.0 and 8.1.0 (verified by diffing the shipped typings — only an optionalGetFilesOptions.filterwas added, and the default signing version is stillv2). This clears one Dependabot alert on the root lockfile, since 7.13.0 pinnedfast-xml-parser ^4.4.1and capped it below the 5.7.0 that GHSA-jr48-27g9-4h5h requires. A follow-up commit keepsfast-xml-parseron the 5.7.2 already in the tree rather than letting the new^5.3.4spec re-resolve the whole 5.x group to 5.11.1, which would have split it into two copies and added three transitives — a name → version-set diff confirms 0 new packages against master, withfast-xml-parsergoing 4.5.6 + 5.7.2 → a single 5.7.2 andstrnum1.0.5 + 2.2.3 → a single 2.2.3. Verified thatyarn install --frozen-lockfileis a no-op,tscon the BigQuery driver is clean, andyarn workspace @cubejs-backend/base-driver unitis 17/17; the BigQuery driver has integration tests only, which need GCP credentials.Note for reviewers:
@google-cloud/storageraisedengines.nodefrom>=14to>=22in v8, while allpackages/*declare>=20.0.0. Nothing breaks at runtime (Docker images are node 24, CI runs 24/26), but the declared floor is worth revisiting separately.🤖 Generated with Claude Code