fix file upload - consider metadata options by creating form data #500

KharitonOff · 2018-03-14T10:12:47Z

Summary

fix file upload issue, now you can upload buffer stream as expected:

let file = fs.readFileSync('../file.png')
slackClient.files.upload({ channels, file, filename: 'file.png' })

as well as proposed fix in #307:

let file = fs.readFileSync('../file.png')
slackClient.files.upload({ channels, file: {value: file, options:{filename: 'file.png'}}, filename: 'file.png' })

related to #307

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

…lated to slackapi#307

CLAassistant · 2018-03-14T10:12:55Z

All committers have signed the CLA.

codecov · 2018-03-14T13:22:31Z

Codecov Report

Merging #500 into master will decrease coverage by 0.37%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #500      +/-   ##
=========================================
- Coverage   80.08%   79.7%   -0.38%     
=========================================
  Files           6       6              
  Lines         241     271      +30     
  Branches       37      42       +5     
=========================================
+ Hits          193     216      +23     
- Misses         30      38       +8     
+ Partials       18      17       -1

Impacted Files	Coverage Δ
src/WebClient.ts	`81.88% <100%> (+3.25%)`	⬆️
src/util.ts	`67.56% <100%> (ø)`	⬆️
src/IncomingWebhook.ts	`60% <0%> (-13.34%)`	⬇️
src/errors.ts	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c49ba78...7c89cac. Read the comment docs.

Allows WebClient token to be undefined

…ypes exports method argument types

…ixes slackapi#481

removes pjson and directly requires package.json

more tightly specifies OAuthAccessArguments and OAuthTokenArguments

aoberoi · 2018-03-14T22:56:19Z

src/WebClient.spec.js

@@ -390,7 +400,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));


was this actually causing a failure? i think your change is more technically correct, but i'm wondering if you actually got a test run where the timers were so perfect that r.diff and minDiff were actually equal.

I can confirm that I've run into the issue of getting a test run where the timers matched up and a false positive was thrown--just once, but it has happened.

yes, I added this fix to the PR, because the build of my first commit broke accidentally

aoberoi · 2018-03-14T22:57:44Z

src/WebClient.spec.js

          const minDiff = concurrentResponses[concurrentResponses.length - 1].diff + responseDelay;
-          queuedResponses.forEach(r => assert.isAbove(r.diff, minDiff));
+          queuedResponses.forEach(r => {
+            assert.isAtLeast(r.diff, minDiff)


i think this could fit in a single line rather than a block.

aoberoi · 2018-03-14T23:04:09Z

src/WebClient.spec.js

+        // intentially vague about the method name
+        return this.client.apiCall('upload', {
+          file: { value: imageBuffer, options: { filename: 'train.png' } },
+          filename: 'train.png',


i was trying to avoid this usage all together, because it seems redundant to have to specify the filename twice. can you see any advantage to doing it this way?

The main advantage I see is the current implementation state of all the folks who used the old style. I don't know whether there is a need for someone to provide further attributes in the options field (e.g. contentType or knownLength)

i also want to make sure the implementation accounts for those who used the old style, but i prefer that the path we take to get there is not to redo the same hack, but to smooth out the API so that you don't need to redundantly specify the filename.

i wouldn't call it a hack if there was legitimately a use case or need for using the { value, options } format for file. i don't see any benefit from exposing contentType and/or knownLength at this time, since that only has the potential to force to incorrect values and cause the upload to fail (but I'm happy to be corrected if there is a real use case).

maybe we can channel this effort towards investigating why the following code didn't behave as we want:

node-slack-sdk/src/WebClient.ts

Lines 530 to 537 in a206c3e

// options.file = {

// // @ts-ignore

// value: options.file,

// options: {

// // @ts-ignore

// filename: options.filename,

// },

// };

AFAICT it should result in the same thing...

also, have you tried this implementation to upload actual files? i'm not 100% confident that the nock expectations are correct.

Sorry, but I don't really understand your comment... this code snippet, you mentioned above works now as well. Off course I have tried it before I've opened this PR. And the problem with it, why it didn't work before was in the implementation of

node-slack-sdk/src/WebClient.ts

Lines 556 to 561 in a206c3e

if (containsBinaryData) {

return flattened.reduce((form, [key, value]) => {

form.append(key, value);

return form;

}, new FormData());

}

I've added this part in order to provide the attribute options to the form builder.

if (key === 'file' && value.value) { form.append(key, value.value, value.options); return form; }

https://github.com/KharitonOff/node-slack-sdk/blob/45c77022a81cd2631a5a2bd3ee0dd29e401f51d7/src/WebClient.ts#L554-L563

This is actually the main goal of this PR and all other things we talked here about, were done with intention not to break the current state. As I mentioned in the PR description, you can upload files now with no need of additional/redundant information

let file = fs.readFileSync('../file.png') slackClient.files.upload({ channels, file, filename: 'file.png' })

Should i remove the 'backup' solution to restrict the file upload to only one way?

@KharitonOff thanks so much for your clarification. i missed that diff amidst the rest of the changes but i now see where the issue was before.

yes, i'd prefer removing the backup solution so that we don't unnecessarily expand the API contract. if you can do that, i'm happy to land this PR!

aoberoi · 2018-03-14T23:04:48Z

src/WebClient.spec.js

+            assert.lengthOf(parts.files, 1);
+            const file = parts.files[0];
+            // options were not provided to the form builder
+            assert.include(file, { fieldname: 'file' });


still not asserting that the filename field exists, and removed the comment that would remind us to improve the tests so that it is asserted. why?

sorry, I've overseen this assertion statement and felt that the fix solved the problem pointed in the comment. I'll bring your TODO and assertion back.

aoberoi · 2018-03-14T23:05:32Z

src/WebClient.spec.js

-            // 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.
            assert.include(file, { fieldname: 'file' });


still not asserting that the filename field exists.

aoberoi · 2018-03-14T23:07:11Z

@KharitonOff thanks for your contribution! can you help clarify by answering my comments above?

v4.0.1

…lated to slackapi#307

KharitonOff · 2018-03-15T15:18:15Z

I've changed your test again in order to fix the build. I hope it is ok for you.
The last travis build failed because of timeout. This seems to be an occasional fail:
build for node v9 succeeds:

build for node v8 fails:

https://travis-ci.org/slackapi/node-slack-sdk/builds/353822357

aoberoi · 2018-03-15T20:29:30Z

test/typescript/test.ts

@@ -4,7 +4,7 @@ import { check, checkDirectory } from 'typings-tester';
 describe('typescript typings tests', () => {
  it('should allow WebClient to work without a token', () => {
    check([`${__dirname}/sources/webclient-no-token.ts`], `${__dirname}/tsconfig-strict.json`);
-  });


maybe the best place to override this value is in test/mocha.opts, so that it doesn't just apply to this one case. i don't see anything unique to this case that should make it run longer than the other. its my guess that Travis generally has unpredictable speed and we should make the ceiling high enough to accomodate.

Agree! I didn't find this setting before, that's why I added it only to this test.

aoberoi · 2018-03-20T01:02:47Z

@KharitonOff its a small world. I just noticed that you work on CLA Assistant (which we use) and you helped review an implementation for a feature i requested, way back when (cla-assistant/cla-assistant#246). 🙌

KharitonOff · 2018-03-20T13:53:16Z

@aoberoi yes, indeed, glad to meet you again! It is nice to see that you are using CLA Assistant and it was very interesting to have a contributer role and sign your CLA 😄

aoberoi · 2018-03-21T20:02:21Z

src/WebClient.spec.js

-            // 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 to self: should the assertion with filename work now?

aoberoi · 2018-03-21T20:04:15Z

src/WebClient.spec.js

            // 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


note to self: the following assertion is probably redundant now

aoberoi · 2018-03-21T20:21:53Z

This is so great!! Extra thanks for cleaning up the style issues and some incorrect behavior in the existing tests.

I'm going to follow up with a PR that addresses the "note to self" comments above.

Follow up on #500

KharitonOff · 2018-03-22T09:08:16Z

Cool! Thanks for merging it 👍
I hope you'll release it soon, our 🤖 is eagerly waiting for the fix 😃

aoberoi and others added 7 commits March 13, 2018 19:30

update node/npm and reinstall packages

e58136b

add unit tests for WebClient without a token

5feaa6b

adds typings-tester, and failing test for WebClient without token

e6580bb

lintstaged ignores test typescript

70b2d74

allow WebClient token to be undefined, fixes slackapi#482

99db715

allow integration test cases to take up to 4s before timing out

93d8350

fix file upload - consider metadata options by creating form data, re…

73dea90

…lated to slackapi#307

fix unregular build failure

8e4d4ee

aoberoi added 9 commits March 14, 2018 11:25

remove nyc from integration tests, it broke codecov

9d0801c

Merge pull request slackapi#499 from aoberoi/webclient-no-token

805dd6f

Allows WebClient token to be undefined

exports method argument types, fixes slackapi#483

5ce84bb

Merge pull request slackapi#502 from aoberoi/export-method-argument-t…

dee4b03

…ypes exports method argument types

more tightly specifies OAuthAccessArguments and OAuthTokenArguments, f…

7f8e5b7

…ixes slackapi#481

removes pjson and directly requires package.json, fixes slackapi#478

8e4da08

Merge pull request slackapi#504 from aoberoi/remove-pjson

945439a

removes pjson and directly requires package.json

update for boolean flag quirk

501b0af

Merge pull request slackapi#503 from aoberoi/oauth-argument-types

6902c4e

more tightly specifies OAuthAccessArguments and OAuthTokenArguments

aoberoi reviewed Mar 14, 2018

View reviewed changes

aoberoi and others added 5 commits March 14, 2018 16:34

v4.0.1

8ce3b61

Merge pull request slackapi#506 from slackapi/rel-v4.0.1

a206c3e

v4.0.1

add assertion for filename

e96b059

put forEach statement in one line

ba661a7

fix file upload - consider metadata options by creating form data, re…

45c7702

…lated to slackapi#307

KharitonOff added 6 commits March 15, 2018 14:13

fix unregular build failure

c1d65b5

add assertion for filename

bb58541

put forEach statement in one line

e7bbd38

resolve merge conflict

f5596ba

resolve merge conflict

b0a7d94

increase test timeout in order to avoid unexpected build failure

1899050

aoberoi reviewed Mar 15, 2018

View reviewed changes

increase mocha tests timeout to 10 sec

6de72b3

remove check for options in file attribute, remove unit test

7c89cac

aoberoi reviewed Mar 21, 2018

View reviewed changes

aoberoi approved these changes Mar 21, 2018

View reviewed changes

aoberoi merged commit 68c0874 into slackapi:master Mar 21, 2018

aoberoi mentioned this pull request Mar 21, 2018

Follow up on #500 #516

Merged

2 tasks

aoberoi added a commit that referenced this pull request Mar 21, 2018

Merge pull request #516 from aoberoi/KharitonOff-fix-issue-307

e46bab9

Follow up on #500

aoberoi mentioned this pull request Mar 22, 2018

Enable WebClient to send binary data #513

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix file upload - consider metadata options by creating form data #500

fix file upload - consider metadata options by creating form data #500

KharitonOff commented Mar 14, 2018

CLAassistant commented Mar 14, 2018 •

edited

codecov bot commented Mar 14, 2018 •

edited

aoberoi Mar 14, 2018

clavin Mar 15, 2018

KharitonOff Mar 15, 2018

aoberoi Mar 14, 2018

aoberoi Mar 14, 2018

KharitonOff Mar 15, 2018

aoberoi Mar 15, 2018

KharitonOff Mar 16, 2018

aoberoi Mar 20, 2018

aoberoi Mar 14, 2018

KharitonOff Mar 15, 2018

aoberoi Mar 14, 2018

aoberoi commented Mar 14, 2018

KharitonOff commented Mar 15, 2018

aoberoi Mar 15, 2018

KharitonOff Mar 16, 2018 •

edited

aoberoi commented Mar 20, 2018

KharitonOff commented Mar 20, 2018

aoberoi Mar 21, 2018

aoberoi Mar 21, 2018

aoberoi commented Mar 21, 2018

KharitonOff commented Mar 22, 2018

	// options.file = {
	// // @ts-ignore
	// value: options.file,
	// options: {
	// // @ts-ignore
	// filename: options.filename,
	// },
	// };

	if (containsBinaryData) {
	return flattened.reduce((form, [key, value]) => {
	form.append(key, value);
	return form;
	}, new FormData());
	}

fix file upload - consider metadata options by creating form data #500

fix file upload - consider metadata options by creating form data #500

Conversation

KharitonOff commented Mar 14, 2018

Summary

Requirements (place an x in each [ ])

CLAassistant commented Mar 14, 2018 • edited

codecov bot commented Mar 14, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoberoi commented Mar 14, 2018

KharitonOff commented Mar 15, 2018

Choose a reason for hiding this comment

KharitonOff Mar 16, 2018 • edited

Choose a reason for hiding this comment

aoberoi commented Mar 20, 2018

KharitonOff commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoberoi commented Mar 21, 2018

KharitonOff commented Mar 22, 2018

Requirements (place an `x` in each `[ ]`)

CLAassistant commented Mar 14, 2018 •

edited

codecov bot commented Mar 14, 2018 •

edited

KharitonOff Mar 16, 2018 •

edited