Skip to content

Commit

Permalink
Merge pull request #500 from KharitonOff/fix-issue-307
Browse files Browse the repository at this point in the history 
fix file upload - consider metadata options by creating form data
  • Loading branch information
@aoberoi
aoberoi committed Mar 21, 2018
2 parents 4bdbd79 + 7c89cac commit 68c0874
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 75 deletions.
11 changes: 6 additions & 5 deletions .vscode/settings.json
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"typescript.tsdk": "./node_modules/typescript/lib",
"editor.rulers": [
120
]
}
"typescript.tsdk": "./node_modules/typescript/lib",
"editor.rulers": [
120
],
"editor.tabSize": 2,
}
2 changes: 1 addition & 1 deletion package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

101 changes: 49 additions & 52 deletions src/WebClient.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ describe('WebClient', function () {
});
});

describe('apiCall()', function() {
describe('apiCall()', function () {
beforeEach(function () {
this.client = new WebClient(token, { retryConfig: fastRetriesForTest });
});

describe('when making a successful call', function() {
describe('when making a successful call', function () {
beforeEach(function () {
this.scope = nock('https://slack.com')
.post(/api/)
Expand Down Expand Up @@ -154,9 +154,9 @@ describe('WebClient', function () {
hum: false,
},
})
.then(() => {
scope.done();
});
.then(() => {
scope.done();
});
});

it.skip('should remove undefined or null values from API arguments');
Expand Down Expand Up @@ -206,34 +206,31 @@ describe('WebClient', function () {

// intentially vague about the method name
return this.client.apiCall('upload', {
file: imageBuffer,
filename: 'train.png',
})
file: imageBuffer,
filename: 'train.jpg',
})
.then((parts) => {
assert.lengthOf(parts.files, 1);
const file = parts.files[0];
// TODO: understand why this assertion is failing. already employed the buffer metadata workaround, should
// look into the details about whether that workaround is still required, or why else the `source.on` is not
// defined error would occur, or if Slack just doesn't need a filename for the part
// assert.include(file, { fieldname: 'file', filename: 'train.jpg' });
// NOTE: it seems the file and its filename are emitted as a field in addition to the token, not sure if
// this was happening in the old implementation.
// options were not provided to the form builder
assert.include(file, { fieldname: 'file' });
});
});

// Reactivate this test once we find out if the workaround in the test case before is necessary
it.skip('should log a warning when file is a Buffer and there is no filename', function () {
it('should log a warning when file is a Buffer and there is no filename', function () {
const imageBuffer = fs.readFileSync(path.resolve('test', 'fixtures', 'train.jpg'));
this.capture = new CaptureStdout();
this.capture.startCapture();
const output = [];
const stub = function (level, message) {
output.push([level, message]);
}
const debuggingClient = new WebClient(token, { logLevel: LogLevel.WARN, logger: stub });

// intentially vague about the method name
return this.client.apiCall('upload', {
file: imageBuffer,
})
return debuggingClient.apiCall('upload', {
file: imageBuffer,
})
.then(() => {
const output = this.capture.getCapturedText();
assert.isNotEmpty(output);
const anyLogLineIsLevelWarn = output.reduce((acc, line) => {
return acc || (line.indexOf('warn') !== -1)
Expand All @@ -246,19 +243,19 @@ describe('WebClient', function () {
const imageStream = fs.createReadStream(path.resolve('test', 'fixtures', 'train.jpg'));

return this.client.apiCall('upload', {
file: imageStream,
})
file: imageStream,
})
.then((parts) => {
assert.lengthOf(parts.files, 1);
const file = parts.files[0];
// TODO: understand why this assertion is failing. already employed the buffer metadata workaround, should
// TODO: understand why this assertion was failing. already employed the buffer metadata workaround, should
// look into the details about whether that workaround is still required, or why else the `source.on` is not
// defined error would occur, or if Slack just doesn't need a filename for the part
// assert.include(file, { fieldname: 'file', filename: 'train.jpg' });
assert.include(file, { fieldname: 'file', filename: 'train.jpg' });

// NOTE: it seems the file and its filename are emitted as a field in addition to the token, not sure if
// this was happening in the old implementation.
assert.include(file, { fieldname: 'file' });
// assert.include(file, { fieldname: 'file' });
});
});

Expand All @@ -270,20 +267,20 @@ describe('WebClient', function () {
describe('metadata in the user agent', function () {
it('should set the user agent to contain package metadata', function () {
const scope = nock('https://slack.com', {
reqheaders: {
'User-Agent': (value) => {
const metadata = parseUserAgentIntoMetadata(value)
// NOTE: this assert isn't that strong and doesn't say anything about the values. at this time, there
// isn't a good way to test this without dupicating the logic of the code under test.
assert.containsAllKeys(metadata, ['node', '@slack:client']);
// NOTE: there's an assumption that if there's any keys besides these left at all, its the platform part
delete metadata.node;
delete metadata['@slack:client'];
assert.isNotEmpty(metadata);
return true;
},
reqheaders: {
'User-Agent': (value) => {
const metadata = parseUserAgentIntoMetadata(value)
// NOTE: this assert isn't that strong and doesn't say anything about the values. at this time, there
// isn't a good way to test this without dupicating the logic of the code under test.
assert.containsAllKeys(metadata, ['node', '@slack:client']);
// NOTE: there's an assumption that if there's any keys besides these left at all, its the platform part
delete metadata.node;
delete metadata['@slack:client'];
assert.isNotEmpty(metadata);
return true;
},
})
},
})
.post(/api/)
.reply(200, { ok: true });
return this.client.apiCall('method')
Expand All @@ -296,14 +293,14 @@ describe('WebClient', function () {
const [name, version] = ['appmedataname', 'appmetadataversion'];
addAppMetadata({ name, version });
const scope = nock('https://slack.com', {
reqheaders: {
'User-Agent': (value) => {
const metadata = parseUserAgentIntoMetadata(value)
assert.propertyVal(metadata, name, version);
return true;
},
reqheaders: {
'User-Agent': (value) => {
const metadata = parseUserAgentIntoMetadata(value)
assert.propertyVal(metadata, name, version);
return true;
},
})
},
})
.post(/api/)
.reply(200, { ok: true });
// NOTE: appMetaData is only evalued on client construction, so we cannot use the client already created
Expand Down Expand Up @@ -351,7 +348,7 @@ describe('WebClient', function () {
});
})

describe('has option to change slackApiUrl', function() {
describe('has option to change slackApiUrl', function () {
it('should send requests to an alternative URL', function () {
const alternativeUrl = 'http://12.34.56.78/api/';
const scope = nock(alternativeUrl)
Expand All @@ -364,7 +361,7 @@ describe('WebClient', function () {

describe('has an option to set a custom HTTP agent', function () {
// not confident how to test this. one idea is to use sinon to intercept method calls on the agent.
it.skip('should send a request using the custom agent', function() {
it.skip('should send a request using the custom agent', function () {
const agent = new Agent();
const client = new WebClient(token, { agent });
return client.apiCall('method');
Expand Down Expand Up @@ -410,7 +407,7 @@ describe('WebClient', function () {
// verify that any requests after maxRequestConcurrency were delayed by the responseDelay
const queuedResponses = responses.slice(3);
const minDiff = concurrentResponses[concurrentResponses.length - 1].diff + responseDelay;
queuedResponses.forEach(r => assert.isAbove(r.diff, minDiff));
queuedResponses.forEach(r => assert.isAtLeast(r.diff, minDiff));
});
});

Expand All @@ -423,13 +420,13 @@ describe('WebClient', function () {
assert.lengthOf(responses, 2);

// verify that maxRequestConcurrency requets were all sent concurrently
const concurrentResponses = responses.slice(0, 1); // the first 3 responses
const concurrentResponses = responses.slice(0, 1); // the first response
concurrentResponses.forEach(r => assert.isBelow(r.diff, responseDelay));

// verify that any requests after maxRequestConcurrency were delayed by the responseDelay
const queuedResponses = responses.slice(1);
const queuedResponses = responses.slice(1);// the second response
const minDiff = concurrentResponses[concurrentResponses.length - 1].diff + responseDelay;
queuedResponses.forEach(r => assert.isAbove(r.diff, minDiff));
queuedResponses.forEach(r => { assert.isAtLeast(r.diff, minDiff) });
});
});

Expand Down
34 changes: 18 additions & 16 deletions src/WebClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -517,25 +517,23 @@ export class WebClient extends EventEmitter {
//
// This could also be resolved by making the above condition a type guard for FilesUploadArguments. Let's expore
// this solution when all the arguments in `./methods.ts` are more fully defined.

// @ts-ignore
// if (Buffer.isBuffer((options as WebAPICallOptions).file)) {
// if (!('filename' in options)) {
// this.logger.warn('`file` option is a Buffer, but there is no `filename` option. this upload will likely ' +
// 'fail. add the `filename` option to fix.');
// }
if (Buffer.isBuffer((options as WebAPICallOptions).file)) {
if (!('filename' in options)) {
this.logger.warn('`file` option is a Buffer, but there is no `filename` option. this upload will likely ' +
'fail. add the `filename` option to fix.');
}

// Buffers are sometimes not handled well by the underlying `form-data` package. Adding extra metadata resolves
// that issue. See: https://github.com/slackapi/node-slack-sdk/issues/307#issuecomment-289231737
// @ts-ignore
// options.file = {
// // @ts-ignore
// value: options.file,
// options: {
// // @ts-ignore
// filename: options.filename,
// },
// };
// }
options['file'] = {
value: options['file'],
options: {
filename: options['filename'],
},
};
}
}

const flattened = objectEntries(options)
Expand All @@ -555,6 +553,10 @@ export class WebClient extends EventEmitter {
// a body with binary data should be serialized as multipart/form-data
if (containsBinaryData) {
return flattened.reduce((form, [key, value]) => {
if (key === 'file' && value.value) {
form.append(key, value.value, value.options);
return form;
}
form.append(key, value);
return form;
}, new FormData());
Expand Down Expand Up @@ -658,7 +660,7 @@ interface FormCanBeURLEncoded {
[key: string]: string | number | boolean;
}

interface BodyCanBeFormMultipart extends Readable {}
interface BodyCanBeFormMultipart extends Readable { }

/**
* Determines whether a request body object should be treated as FormData-encodable (Content-Type=multipart/form-data).
Expand Down
2 changes: 1 addition & 1 deletion test/mocha.opts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
--require ts-node/register
--require source-map-support/register
--recursive
--timeout 4000
--timeout 10000

0 comments on commit 68c0874

Please sign in to comment.