Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

Fix bug: z.stashFile doesn't pick up filename in Content-Disposition #124

Merged
merged 1 commit into from
Oct 29, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"preversion": "git pull && npm test",
"version": "node bin/bump-dependencies.js && git add package.json",
"postversion": "git push && git push --tags",
"test": "mocha -t 5000 --recursive test",
"test": "mocha -t 10000 --recursive test",
"posttest": "npm run lint",
"plain-test": "mocha -t 5000 --recursive test",
"integration-test": "mocha -t 10000 integration-test",
Expand Down
37 changes: 25 additions & 12 deletions src/tools/create-file-stasher.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,27 +111,36 @@ const createFileStasher = input => {
let newBufferStringStream = response;
if (_.isString(response)) {
newBufferStringStream = response;
} else if (response && response.headers) {
if (response.body && typeof response.body.pipe === 'function') {
} else if (response) {
if (Buffer.isBuffer(response)) {
newBufferStringStream = response;
} else if (Buffer.isBuffer(response.dataBuffer)) {
newBufferStringStream = response.dataBuffer;
} else if (
response.body &&
typeof response.body.pipe === 'function'
) {
newBufferStringStream = response.body;
} else {
newBufferStringStream = response.content;
}
knownLength =
knownLength || response.getHeader('content-length');
const cd = response.getHeader('content-disposition');
if (cd) {
filename =
filename ||
contentDisposition.parse(cd).parameters.filename;

if (response.headers) {
knownLength =
knownLength || response.getHeader('content-length');
const cd = response.getHeader('content-disposition');
if (cd) {
filename =
filename ||
contentDisposition.parse(cd).parameters.filename;
}
}
} else if (Buffer.isBuffer(response)) {
newBufferStringStream = response;
} else {
throw new Error(
'Cannot stash a Promise wrapped file of unknown type.'
);
}

return uploader(
result,
newBufferStringStream,
Expand All @@ -140,9 +149,13 @@ const createFileStasher = input => {
fileContentType
);
};

if (isStreamed) {
maybeResponse.throwForStatus();
return maybeResponse.buffer().then(parseFinalResponse);
return maybeResponse.buffer().then(buffer => {
maybeResponse.dataBuffer = buffer;
return parseFinalResponse(maybeResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

As I'm looking at this diff, it seems that before we were passing parseFinalResponse a buffer object, but not the complete response object. With the change, now we will be giving a complete response object. Am I understanding that correctly? If so, does that have a side effect for other scenarios where we were streaming files?

Mainly concerned with the fact that we'll now be reading off the headers of the response, where as before we didn't touch them. Seems like before and after, we are still leaving the buffer with the response body untouched, so thinking this will be ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you understand it correctly. Without this change, we were always ignoring the response headers if the stream mode is on (z.request({ raw: true })). I'm not sure about the side effect, though. Do you have an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not specifically. I was thinking it might mess up .pip() or .read() if somebody was doing that off the response object, but, we've only been passing in the body this whole time, so it shouldn't matter.

});
} else {
return parseFinalResponse(maybeResponse);
}
Expand Down
28 changes: 28 additions & 0 deletions test/tools/file-stasher.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,32 @@ describe('file upload', () => {
})
.catch(done);
});

it('should get filename from content-disposition', done => {
mocky.mockRpcCall(mocky.fakeSignedPostData);

// Expect to have this part in the request body sent to S3
mocky.mockUpload(
/name="Content-Disposition"\r\n\r\nattachment; filename="an example\.json"/
);

const request = createAppRequestClient(input);
const file = request({
url: 'https://zapier-httpbin.herokuapp.com/response-headers',
params: {
'Content-Disposition': 'inline; filename="an example.json"'
},
raw: true
});
stashFile(file)
.then(url => {
should(url).eql(
`${mocky.fakeSignedPostData.url}${
mocky.fakeSignedPostData.fields.key
}`
);
done();
})
.catch(done);
});
});
4 changes: 2 additions & 2 deletions test/tools/mocky.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ const fakeSignedPostData = {
}
};

const mockUpload = () => {
const mockUpload = bodyMatcher => {
nock('http://s3-fake.zapier.com')
.post('/')
.post('/', bodyMatcher)
.reply(204, '');
};

Expand Down