Repository navigation
fix(gaxios): honor maxContentLength and maxRedirects instance defaults - #9778
danieljbruce wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates Gaxios to check opts instead of options for maxContentLength and maxRedirects, allowing instance defaults to be correctly honored. A test case was added to verify that maxContentLength is enforced from instance defaults. The reviewer suggested adding a corresponding test case for maxRedirects to ensure both options are fully covered and protected against future regressions.
| const defaultScope = nock(url).get('/').reply(200, body); | ||
| const client = new Gaxios({maxContentLength: 5}); | ||
| await assert.rejects( | ||
| client.request({url}), | ||
| (err: Error) => err instanceof GaxiosError && /limit/.test(err.message), | ||
| ); | ||
| defaultScope.done(); | ||
| }); |
There was a problem hiding this comment.
The pull request description and title mention honoring both maxContentLength and maxRedirects instance defaults. However, the added unit test only covers maxContentLength. Adding a test case for maxRedirects ensures both options are fully verified and protected against future regressions.
| const defaultScope = nock(url).get('/').reply(200, body); | |
| const client = new Gaxios({maxContentLength: 5}); | |
| await assert.rejects( | |
| client.request({url}), | |
| (err: Error) => err instanceof GaxiosError && /limit/.test(err.message), | |
| ); | |
| defaultScope.done(); | |
| }); | |
| const defaultScope = nock(url).get('/').reply(200, body); | |
| const client = new Gaxios({maxContentLength: 5}); | |
| await assert.rejects( | |
| client.request({url}), | |
| (err: Error) => err instanceof GaxiosError && /limit/.test(err.message), | |
| ); | |
| defaultScope.done(); | |
| }); | |
| it('should enforce maxRedirects from instance defaults', async () => { | |
| const redirectScope = nock(url) | |
| .get('/') | |
| .reply(302, undefined, {Location: '/redirect'}); | |
| const client = new Gaxios({maxRedirects: 0}); | |
| await assert.rejects( | |
| client.request({url}), | |
| (err: Error) => err instanceof GaxiosError, | |
| ); | |
| redirectScope.done(); | |
| }); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request ensures that Gaxios instance defaults for maxContentLength and maxRedirects are correctly respected by checking opts instead of options. It also adds a test case verifying the maxContentLength default behavior. The feedback suggests extending the test coverage to also verify the maxRedirects default behavior.
| const client = new Gaxios({maxContentLength: 5}); | ||
| await assert.rejects( | ||
| client.request({url}), | ||
| (err: Error) => err instanceof GaxiosError && /limit/.test(err.message), | ||
| ); | ||
| defaultScope.done(); |
There was a problem hiding this comment.
The pull request fixes both maxContentLength and maxRedirects instance defaults, but the added test only covers maxContentLength. It would be great to also add a test case verifying that maxRedirects instance defaults are correctly honored and enforced.
const defaultScope = nock(url).get('/').reply(200, body);
const client = new Gaxios({maxContentLength: 5});
await assert.rejects(
client.request({url}),
(err: Error) => err instanceof GaxiosError && /limit/.test(err.message),
);
defaultScope.done();
const redirectScope = nock(url)
.get('/')
.reply(302, undefined, {Location: '/redirect'})
.get('/redirect')
.reply(200, 'done');
const redirectClient = new Gaxios({maxRedirects: 0});
await assert.rejects(
redirectClient.request({url}),
(err: Error) => err instanceof GaxiosError,
);
redirectScope.done();…9773) ## Description Enforces `maxContentLength` on headerless buffered responses under Bun. ## Impact Prevents oversized buffered responses from bypassing `maxContentLength` limits on Bun. ## Changes - Enforce `maxSize` (`rawInit.size` / `rawInit.maxContentLength`) inside `readBuffer()` in `Gaxios.#createBunFetch()` so buffered (`json`, `text`, `arraybuffer`, `blob`) responses reject with `content size at <url> over limit: <maxSize>` when exceeded even without a `Content-Length` header. ### Stack 1. **#9773** — `fix(gaxios): enforce maxContentLength on headerless responses in Bun` (`fix/gaxios-bun-max-content-length` → `main`) *(this PR)* 2. #9777 — `fix(gaxios): enforce maxContentLength on stream responses in Bun` (`fix/gaxios-bun-max-content-length-stream` → `fix/gaxios-bun-max-content-length`) 3. #9778 — `fix(gaxios): honor maxContentLength and maxRedirects instance defaults` (`fix/gaxios-instance-defaults` → `fix/gaxios-bun-max-content-length-stream`) 4. #9774 — `fix(gaxios): forward https.Agent TLS options and agent factories in Bun` (`fix/gaxios-bun-agent-tls` → `fix/gaxios-instance-defaults`) ## Testing - Added unit tests in `core/packages/gaxios/test/test.getch.ts` covering `maxContentLength` rejection on headerless responses across `json`, `text`, `arraybuffer`, and `blob` response types. - **Steps to Reproduce (in terminal)**: Run `cd core/packages/gaxios && pnpm run compile && JS_RUNTIME=bun MOCHA_PARALLEL=false bun --bun ../../../bin/run-test.cjs --no-c8 build/esm/test/test.getch.js -g "maxContentLength"`. ## Alternatives - Checking `maxContentLength` only when a `Content-Length` header is present in `getResponseData()` was rejected because chunked/headerless responses silently bypass response size limits under Bun. - Not merging this PR was rejected because `node-fetch` enforces `size` on headerless responses on Node.js.
Description
Honors
maxContentLengthandmaxRedirectsinstance defaults inGaxios.#prepareRequest.Impact
Ensures
new Gaxios({maxContentLength, maxRedirects})instance defaults populateopts.sizeandopts.follow.Changes
opts.maxContentLengthandopts.maxRedirectsfrom the mergedoptsobject inGaxios.#prepareRequestinstead of the raw per-requestoptionsargument.Stack
fix(gaxios): enforce maxContentLength on headerless responses in Bun(fix/gaxios-bun-max-content-length→main)fix(gaxios): enforce maxContentLength on stream responses in Bun(fix/gaxios-bun-max-content-length-stream→fix/gaxios-bun-max-content-length)fix(gaxios): honor maxContentLength and maxRedirects instance defaults(fix/gaxios-instance-defaults→fix/gaxios-bun-max-content-length-stream) (this PR)fix(gaxios): forward https.Agent TLS options and agent factories in Bun(fix/gaxios-bun-agent-tls→fix/gaxios-instance-defaults)Testing
core/packages/gaxios/test/test.getch.tsverifying thatnew Gaxios({maxContentLength: 5})enforces the limit onclient.request({url}).cd core/packages/gaxios && pnpm run compile && JS_RUNTIME=bun MOCHA_PARALLEL=false bun --bun ../../../bin/run-test.cjs --no-c8 build/esm/test/test.getch.js -g "instance defaults".Alternatives
maxContentLengthandmaxRedirectson everyrequest()call was rejected becausenew Gaxios(defaults)is documented to apply default options across all requests on the instance.maxContentLengthandmaxRedirectsdefaults are otherwise ignored when populatingopts.sizeandopts.follow.