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.

Sign up for GitHub

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

Jump to bottom

Stream support for FileUpload #471

Merged
merged 15 commits into from
Jul 18, 2018
Merged

Stream support for FileUpload #471

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a58cb35
Stream support for FileUpload.
jjsquillante Jun 24, 2018
5dc9afd
Stream support for FileUpload.
jjsquillante Jun 24, 2018
d99d077
Stream support for FileUploads.js. + test.
jjsquillante Jul 1, 2018
54a79a6
Merge remote-tracking branch 'upstream/master'
jjsquillante Jul 1, 2018
27c0121
Add safe-buffer module to FileUploads.
jjsquillante Jul 1, 2018
87328dd
Add back safe-buffer module to utils.
jjsquillante Jul 1, 2018
f109d07
Update promise chain indentation to match Stripe patterns.
jjsquillante Jul 1, 2018
8764b7d
Update async mocha tests to prevent false positive.
jjsquillante Jul 5, 2018
183d644
Add error handler for stream processing.
jjsquillante Jul 8, 2018
05a84c4
Remove unnecessary callback.
jjsquillante Jul 8, 2018
5acf777
Simplify _request and requestDataProcessor for stream with additional…
jjsquillante Jul 8, 2018
c039eeb
Merge remote-tracking branch 'upstream/master'
jjsquillante Jul 8, 2018
aa90cec
Flow to callback if method is not a POST.
jjsquillante Jul 15, 2018
a1ebecc
Handle and generate stream error within fileUpload.js.
jjsquillante Jul 15, 2018
d963055
Tests for synchronous file upload and stream error.
jjsquillante Jul 16, 2018
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
17 changes: 16 additions & 1 deletion lib/StripeMethod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,22 @@ function stripeMethod(spec) {
}
}

self._request(requestMethod, requestPath, data, opts.auth, options, requestCallback);
var isStream = utils.checkForStream(data);

if (isStream) {
var bufferArray = [];
data.file.data
.on('data', function(line) {
bufferArray.push(line);
})
.on('end', function() {
var dataBuffered = Object.assign({}, data);
dataBuffered.file.data = Buffer.concat(bufferArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is fine, but would you mind adding var Buffer = require('safe-buffer').Buffer; at the top of the file? That way if we add more code that uses Buffer in this file it will use the safe version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ob-stripe thanks for the quick response and catching my oversight on safe-buffer! I noticed safe-buffer in the MultipartDataGenerator.js file, but totally forgot about requiring it here. I'll update the code asap and keep a look out for any other feedback from @jlomas-stripe @rattrayalex-stripe .

self._request(requestMethod, requestPath, dataBuffered, opts.auth, options, requestCallback);
});
} else {
self._request(requestMethod, requestPath, data, opts.auth, options, requestCallback);
}
}).bind(this)), callback);
};
}
Expand Down
11 changes: 11 additions & 0 deletions lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,17 @@ var utils = module.exports = {

return obj;
},

/**
* Determine if file data is a derivative of EventEmitter class.
* https://nodejs.org/api/events.html#events_events
*/
checkForStream: function (obj) {
if (obj.file && obj.file.data) {
return typeof obj.file.data.on === 'function' && typeof obj.file.data.emit === 'function';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do an instanceof check? This is probably fine too, just curious.

Copy link
Contributor Author

@jjsquillante jjsquillante Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@rattrayalex-stripe good question! An instanceof check would have been more explicit, but I'm not aware of require('events').EventEmitter available in the utils file. In retrospect, I was trying to implement the feature with just minimal additions to the existing code. When it comes to collaboration, though, it makes sense to be more explicit - I should have probably just required the events module! Thanks for the feedback. I plan to refactor the requestDataProcessor function to async, and if I need this check, I'll implement via instanceof . Let me know if you have any other feedback - thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks! That reasoning makes sense. A duck-type check as you have here may make more sense; let's see how it looks after the refactor. Cheers

}
return false;
},
};

function emitWarning(warning) {
Expand Down
37 changes: 37 additions & 0 deletions test/resources/FileUploads.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,42 @@ describe('File Uploads Resource', function() {
expect(stripe.LAST_REQUEST).to.deep.property('url', '/v1/files');
expect(stripe.LAST_REQUEST).to.deep.property('auth', TEST_AUTH_KEY);
});

it('Streams a file and sends the correct file upload request', function() {
var testFilename = path.join(__dirname, 'data/minimal.pdf');
var f = fs.createReadStream(testFilename);

stripe.fileUploads.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to return this so Mocha knows it's async otherwise it will always pass (because it won't wait for the result in the Promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, my fault! nice catch @jlomas-stripe - I just ran it through the debugger with an intentional fail and noticed exactly what you were talking about. I'll update this asap.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries. This is a super-insidious mocha thing. Not including a done callback and then doing something async used to catch me out unfortunately often in the good ol' days. 😂

purpose: 'dispute_evidence',
file: {
data: f,
name: 'minimal.pdf',
type: 'application/octet-stream',
},
})
.then(function() {
expect(stripe.LAST_REQUEST).to.deep.property('method', 'POST');
expect(stripe.LAST_REQUEST).to.deep.property('url', '/v1/files');
});
});

it('Streams a file and sends the correct file upload request [with specified auth]', function() {
var testFilename = path.join(__dirname, 'data/minimal.pdf');
var f = fs.createReadStream(testFilename);

stripe.fileUploads.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: you need to return this so Mocha knows it's async otherwise it will always pass (because it won't wait for the result in the Promise.

purpose: 'dispute_evidence',
file: {
data: f,
name: 'minimal.pdf',
type: 'application/octet-stream',
},
}, TEST_AUTH_KEY)
.then(function() {
expect(stripe.LAST_REQUEST).to.deep.property('method', 'POST');
expect(stripe.LAST_REQUEST).to.deep.property('url', '/v1/files');
expect(stripe.LAST_REQUEST).to.deep.property('auth', TEST_AUTH_KEY);
});
});
});
});