feat: handle application/x-www-form-urlencoded/Buffer #374
Conversation
google-cla
bot
added
the
cla: yes
src/gaxios.ts
Outdated
| return typeof Buffer !== 'undefined'; | ||
| } | ||
|
|
||
| function hasHeader(options: GaxiosOptions, headerCheck: string) { |
There was a problem hiding this comment.
Support content-type, and Content-Type, and content-Type, when deciding whether to reset header.
src/gaxios.ts
Outdated
| !opts.headers['Content-Type'] || | ||
| !opts.headers['Content-Type'].includes('json') | ||
| opts.headers['Content-Type'] && | ||
| opts.headers['Content-Type'] === 'application/x-www-form-urlencoded' |
There was a problem hiding this comment.
If someone has explicitly asked for application/x-www-form-urlencoded serialize the body as a query strong.
| const body = {hello: '🌎'}; | ||
| const scope = nock(url) | ||
| .matchHeader('Content-Type', 'application/json') | ||
| .post('/', JSON.stringify(body)) | ||
| .matchHeader('Content-Type', 'application/x-www-form-urlencoded') |
There was a problem hiding this comment.
This test was for the old behavior where application/x-www-form-urlencoded got magically rewritten as application/json.
| const pkgJson = JSON.parse(pkg.toString('utf8')); | ||
| const scope = nock(url) | ||
| .matchHeader('content-type', 'application/dicom') | ||
| .post('/', pkgJson) |
There was a problem hiding this comment.
once in transport, the Buffer is properly handled by node-fetch, and becomes a string body.
| const pkg = fs.readFileSync('./package.json'); | ||
| const pkgJson = JSON.parse(pkg.toString('utf8')); | ||
| const scope = nock(url) | ||
| .matchHeader('content-type', 'application/json') |
There was a problem hiding this comment.
Default to application/json if we don't have a hint.
Codecov Report
@@ Coverage Diff @@
## master #374 +/- ##
==========================================
- Coverage 96.31% 96.28% -0.03%
==========================================
Files 6 6
Lines 732 754 +22
Branches 106 114 +8
==========================================
+ Hits 705 726 +21
Misses 27 27
- Partials 0 1 +1
Continue to review full report at Codecov.
|
| const pkgJson = JSON.parse(pkg.toString('utf8')); | ||
| const scope = nock(url) | ||
| .matchHeader('content-type', 'application/dicom') | ||
| .post('/', pkgJson) |
| // Do not attempt to JSON.stringify() a Buffer: | ||
| opts.body = opts.data; | ||
| if (!hasHeader(opts, 'Content-Type')) { | ||
| opts.headers['Content-Type'] = 'application/json'; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
oh, for clarification hasHeader doesn't care about case.
Rather than switching
node-gtokento serializing its body with urlencoding, add support forapplication/x-www-form-urlencodedto gaxios.This PR also corrects our handling of
Buffer(otherwise allowing users to set a content-type header would not have corrected their issue).Refs: googleapis/node-gtoken#362
Refs: googleapis/node-gtoken#363
Refs: googleapis/google-api-nodejs-client#2003