Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: handle application/x-www-form-urlencoded/Buffer #374

Merged
merged 3 commits into from
Mar 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions src/gaxios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,19 @@ function hasFetch() {
return hasWindow() && !!window.fetch;
}

function hasBuffer() {
return typeof Buffer !== 'undefined';
}

function hasHeader(options: GaxiosOptions, headerCheck: string) {
Copy link
Contributor Author

@bcoe bcoe Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support content-type, and Content-Type, and content-Type, when deciding whether to reset header.

headerCheck = headerCheck.toLowerCase();
for (let header of Object.keys(options?.headers || {})) {
header = header.toLowerCase();
if (header === headerCheck) return true;
}
return false;
}

let HttpsProxyAgent: any;

function loadProxy() {
Expand Down Expand Up @@ -219,21 +232,25 @@ export class Gaxios {
if (opts.data) {
if (isStream.readable(opts.data)) {
opts.body = opts.data;
} else if (hasBuffer() && Buffer.isBuffer(opts.data)) {
// Do not attempt to JSON.stringify() a Buffer:
opts.body = opts.data;
if (!hasHeader(opts, 'Content-Type')) {
opts.headers['Content-Type'] = 'application/json';
JustinBeckwith marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@JustinBeckwith JustinBeckwith Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also should be getHeader to ensure case safety. I am starting to wonder if we should lowercase all header names in validateOptions, and then always use lowercase throughout this code. Eh? (don't feel like you need to go that far right now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be, because we already know it doesn't have the header Content-Type, or content-type. So, we're just picking one when we choose it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, for clarification hasHeader doesn't care about case.

}
} else if (typeof opts.data === 'object') {
opts.body = JSON.stringify(opts.data);
// Allow the user to specifiy their own content type,
// such as application/json-patch+json; for historical reasons this
// content type must currently be a json type, as we are relying on
// application/x-www-form-urlencoded (which is incompatible with
// upstream GCP APIs) being rewritten to application/json.
//
// TODO: refactor upstream dependencies to stop relying on this
// side-effect.
// If www-form-urlencoded content type has been set, but data is
// provided as an object, serialize the content using querystring:
if (
!opts.headers['Content-Type'] ||
!opts.headers['Content-Type'].includes('json')
opts.headers['Content-Type'] &&
bcoe marked this conversation as resolved.
Show resolved Hide resolved
opts.headers['Content-Type'] === 'application/x-www-form-urlencoded'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone has explicitly asked for application/x-www-form-urlencoded serialize the body as a query strong.

bcoe marked this conversation as resolved.
Show resolved Hide resolved
) {
opts.headers['Content-Type'] = 'application/json';
opts.body = opts.paramsSerializer(opts.data);
bcoe marked this conversation as resolved.
Show resolved Hide resolved
} else {
if (!hasHeader(opts, 'Content-Type')) {
opts.headers['Content-Type'] = 'application/json';
}
opts.body = JSON.stringify(opts.data);
}
} else {
opts.body = opts.data;
Expand Down
39 changes: 36 additions & 3 deletions test/test.getch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,11 +449,11 @@ describe('🎏 data handling', () => {
assert.deepStrictEqual(res.data, {});
});

it('replaces application/x-www-form-urlencoded with application/json', async () => {
it('application/x-www-form-urlencoded with object data should stringify with qs', async () => {
bcoe marked this conversation as resolved.
Show resolved Hide resolved
const body = {hello: '🌎'};
const scope = nock(url)
.matchHeader('Content-Type', 'application/json')
.post('/', JSON.stringify(body))
.matchHeader('Content-Type', 'application/x-www-form-urlencoded')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was for the old behavior where application/x-www-form-urlencoded got magically rewritten as application/json.

.post('/', qs.stringify(body))
.reply(200, {});
const res = await request({
url,
Expand Down Expand Up @@ -530,4 +530,37 @@ describe('🍂 defaults & instances', () => {
scope.done();
assert.deepStrictEqual(res.data, body);
});

it('should allow buffer to be posted', async () => {
const pkg = fs.readFileSync('./package.json');
const pkgJson = JSON.parse(pkg.toString('utf8'));
const scope = nock(url)
.matchHeader('content-type', 'application/dicom')
.post('/', pkgJson)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once in transport, the Buffer is properly handled by node-fetch, and becomes a string body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sick

.reply(200, {});
const res = await request({
url,
method: 'POST',
data: pkg,
headers: {'content-type': 'application/dicom'},
});
scope.done();
assert.deepStrictEqual(res.data, {});
});

it('should set content-type to application/json by default, for buffer', async () => {
const pkg = fs.readFileSync('./package.json');
const pkgJson = JSON.parse(pkg.toString('utf8'));
const scope = nock(url)
.matchHeader('content-type', 'application/json')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default to application/json if we don't have a hint.

.post('/', pkgJson)
.reply(200, {});
const res = await request({
url,
method: 'POST',
data: pkg,
});
scope.done();
assert.deepStrictEqual(res.data, {});
});
});