Stream support for FileUpload #471

jjsquillante · 2018-06-24T19:50:02Z

Hey all at Stripe!

First, I want to say: from documentation to code, Stripe communicates its design of the API libraries extremely well - thank you very much!

Ok down to the PR, I was implementing Stripe for a recent project, and figured in order to truly understand the Stripe-node library, I might as well dive in to an issue or feature request to get my hands dirty. I noticed issue #401 and decided to give it a go.

This Pull Request allows a user to upload a file by passing a stream. Within the StripeMethod, we now check for a stream, convert it to a buffer, and begin the _request process as normal.

I've added tests to confirm that the new feature is working as designed and did not break any existing functionality.

I'm certainly open to any feedback, so if there's anything you'd like to see different or that can be improved, let me know!

Please reach out if there's anything that I can help further clarify. Thanks!

ob-stripe · 2018-06-25T09:55:06Z

Hi @jjsquillante, thanks for your kind words as well as the contribution! :)

This looks good to me, but going to ask some colleagues with better JS skills than mine to take a look as well.

r? @jlomas-stripe @rattrayalex-stripe

ob-stripe · 2018-06-25T09:56:50Z

lib/StripeMethod.js

+          })
+          .on('end', function() {
+            var dataBuffered = Object.assign({}, data);
+            dataBuffered.file.data = Buffer.concat(bufferArray);


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.

@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 .

jlomas-stripe · 2018-06-25T18:13:52Z

Hey hey. Super-cool. :)

I think there should probably be a test in the flows.spec.js that actually hits the API to confirm this does what it says on the tin. Something like:

  describe('FileUpload', function() {
    it('Allows you to upload a file as a stream', function() {
      var testFilename = path.join(__dirname, 'resources/data/minimal.pdf');
      var f = fs.createReadStream(testFilename);

      return expect(
        stripe.fileUploads.create({
          purpose: 'dispute_evidence',
          file: {
            data: f,
            name: 'minimal.pdf',
            type: 'application/octet-stream',
          },
        }).then(null, function(error) {
          return error;
        })
      ).to.eventually.have.nested.property('size', 739);
    });
  });

More generally, we have a requestDataProcessor function function on the StripeResource - which is only currently used by FileUpload - that feels like a better place for this, but that function is unfortunately synchronous at the moment.

I feel like the right way to do this is to refactor the requestDataProcessor function to be async and then do this stuff right in the FileUpload.js's requestDataProcessor, mostly because it really feels like this is specific to FileUpload...?

jjsquillante · 2018-06-25T18:29:06Z

@jlomas-stripe thanks for the response! totally agree - my initial strategy was to modify requestDataProcessor, but this was certainly the 'path of least resistance', so to speak, due to the synchronous nature you mentioned. It seems to make a lot more sense to confine this code to fileUpload.js. Might as well do it right and not compromise :) - I can look into refactoring the requestDataProcessor to async and everything else that goes along with it, if you'd confirm you'd like to move in that direction?

jlomas-stripe · 2018-06-25T20:29:21Z

You're welcome! :)

cc @ob-stripe / @brandur-stripe / @rattrayalex-stripe on my above suggestion; does that make sense to y'all? What jjsquillante has done here definitely works just fine, but it feels a bit out of place in terms of where it lives, since it only affects one type of API request.

brandur-stripe · 2018-06-26T00:29:11Z

Thanks for the effort here @jjsquillante!

cc @ob-stripe / @brandur-stripe / @rattrayalex-stripe on my above suggestion; does that make sense to y'all? What jjsquillante has done here definitely works just fine, but it feels a bit out of place in terms of where it lives, since it only affects one type of API request.

Yeah, agreed that FileUpload would be a better home for this — putting it in StripeMethod would be okay if we ever wanted to generalize this for some reason, but I don't ever see that happening. Very likely all files and file-like objects will always go the Stripe area specifically through FileUpload (or a direct successor to it).

rattrayalex-stripe · 2018-06-26T00:56:43Z

lib/utils.js

+  */
+  checkForStream: function (obj) {
+    if (obj.file && obj.file.data) {
+      return typeof obj.file.data.on === 'function' && typeof obj.file.data.emit === 'function';


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

@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!

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

rattrayalex-stripe · 2018-06-26T00:58:14Z

Nice, thanks @jjsquillante ! Other than a +1 to @jlomas-stripe 's suggestions, this looks good to me.

jjsquillante · 2018-06-26T14:39:44Z

@ob-stripe @brandur-stripe @jlomas-stripe @rattrayalex-stripe thanks for all of the feedback. I appreciate you all being open to my contribution and I have enjoyed working within Stripe's library. Sounds like the best strategy is to close this PR and I'll resolve to refactor the requestDataProcessor function to be async. sound good?

jlomas-stripe · 2018-06-26T18:44:11Z

@jjsquillante You're very welcome, and I'm glad you like the library!

Since this PR represents the reason/inspiration for the refactor, and since the tests for streaming FileUpload will exercise that refactor anyways, it seems reasonable to me to just stick with this PR.

jjsquillante · 2018-06-27T00:08:09Z

@jlomas-stripe ok - sounds good. So to confirm, I'll leave this PR open and will update once the refactor of requestDataProcessor function to async is complete.

jlomas-stripe · 2018-06-27T16:05:56Z

Sure, or just make all the changes in here and re-title - whatever you prefer!

jjsquillante · 2018-07-01T21:40:37Z

Hey all - @jlomas-stripe @rattrayalex-stripe @ob-stripe
I've refactored the code and added the test @jlomas-stripe provided to flows.spec.js. Let me know if there's anything you'd like to see different. Thanks!

jlomas-stripe

Cool. :) There are a couple of changes in the tests that need made and a few other comments. Hopefully @rattrayalex-stripe or @ob-stripe can take a peek as well and provide their thoughts!

jlomas-stripe · 2018-07-04T00:29:32Z

test/resources/FileUploads.spec.js

+      var testFilename = path.join(__dirname, 'data/minimal.pdf');
+      var f = fs.createReadStream(testFilename);
+
+      stripe.fileUploads.create({


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.

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.

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. 😂

jlomas-stripe · 2018-07-04T00:29:46Z

test/resources/FileUploads.spec.js

+      var testFilename = path.join(__dirname, 'data/minimal.pdf');
+      var f = fs.createReadStream(testFilename);
+
+      stripe.fileUploads.create({


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.

jlomas-stripe · 2018-07-04T16:06:03Z

lib/StripeResource.js

    var requestData;

    if (self.requestDataProcessor) {
-      requestData = self.requestDataProcessor(method, data, options.headers);
+      requestProcessor = self.requestDataProcessor(method, data);


Since this now gets a function, rather than just returning the data, it might be clearer if it were called something like requestDataProcessorFn or getRequestDataProcessorFn I think?

The variable also might be a bit clearer if it were named processRequestData since that's what it's going to to.

thanks for the feedback. I'll update the variable names to be more clear.

jlomas-stripe · 2018-07-04T16:17:33Z

lib/MultipartDataGenerator.js

@@ -39,7 +39,10 @@ function multipartDataGenerator(method, data, headers) {
  }
  push('--' + segno + '--');

-  return buffer;
+  if (!callback) {


This function doesn't have any async pieces; does it really need the callback in here?

you're correct - definitely does not need to have a callback in there. I'll update. thanks @jlomas-stripe

jlomas-stripe · 2018-07-04T16:20:34Z

lib/StripeResource.js

+    if (requestProcessor) {
+      requestProcessor(method, data, options.headers, function(err, buffer) {
+        if (err) {
+          var handleError = self._errorHandler({}, callback);


I think this one is probably better handled with a different kind of error, rather than overloading the connection error, since this isn't a connection (or other Stripe) error; (i presume) it'll be something related to the Buffer itself, so we should probably expose it as such.

ok, cool - makes sense. Looking back now, it was kind of a 'cop out' to overload the connection error. I'll resolve to update and expose the error as such.

jlomas-stripe · 2018-07-04T16:25:59Z

lib/resources/FileUploads.js

    } else {
      return utils.stringifyRequestData(data);
    }
+
+    function determineSource(data) {


With the approach you've got here, this function does more than just determineSource - it getProcessorForSourceType - so it probably should be renamed so it's clearer as to what you're getting back.

sounds good, i'll rename and update!

jlomas-stripe · 2018-07-04T16:26:54Z

lib/resources/FileUploads.js

@@ -9,14 +10,45 @@ module.exports = StripeResource.extend({

  overrideHost: 'uploads.stripe.com',

-  requestDataProcessor: function(method, data, headers) {
+  requestDataProcessor: function(method, data) {


This got a bit more complicated than I expected - I imagined something very similar to what you'd done previously, except that the entire requestDataProcessor function would just get an additional callback parameter that was always used (either sync or async), and the main code flow would either hit some 'default' function with callback, or the one specified here.

@jlomas-stripe Definitely a bit more complicated but I believe I vetted the scenario you've mentioned and the nature of streams posed a problem. If I'm understanding your comment (correct me if I'm wrong), the gist is: requestDataProcessor would send the buffered multipart/form-data back to _request via the additional callback - which ever way handled (ie. data processed via stream or already buffered data). From there _request can proceed as normal.

The trouble I encountered is that when the stream is set by attaching an event listener .on('data'), the code is handled via process.nextTick() which runs the callback after the rest of the code and before the event loop. If I remember correctly, this solution didn't wait for data to be passed back to the callback, but instead proceed with data as undefined and continue through _request without the buffered data. The way I figured, returns a function in both scenarios so we can wait for the stream to finish processing. This will guarantee the buffered data exists when the rest of _request is processed by invoking the setHeadersAndMakeRequest within the callback from either scenario. My apologies if that was long-winded! 🙂

If that logic may be flawed or there's a better way to go about this - I'm fine with scrapping this solution for the sake of clarity and going back to the drawing board. Let me know!

This is sort of what I was thinking from the _request perspective:

_request: function (method, path, data, auth, options, callback) { var requestData; function makeRequestWithData(error, data) { if (error) { /* something */ } requestData = data; /* some stuff */ this._stripe.getClientUserAgent(function(cua) { /* some stuff */ makeRequest(); }); } if (self.requestDataProcessor) { self.requestDataProcessor(method, data, options.headers, makeRequestWithData); } else { makeRequestWithData(null, utils.stringifyRequestData(data || {})); } }

(Note, this is just what I was thinking; I haven't tested it so I may be out to lunch 😂)

I.e., just adding one more function call to 'asyncify' the flow from 'process the data' to 'make the request'. This doesn't even need to be done async (the else is a synchronous function call here), but it at least allows for it.

I think adding that one function call, plus adding a callback argument to requestDataProcessor, is enough to make this handle both cases.

And then rather than returning a function from requestDataProcessor, you'd just hit the callback that was provided to it when it's done, in stream.on('end'), as you're doing now.

If you were ending up with data being undefined, maybe the callback was firing early/more than once, or something was returning a value when you wanted it to be 'returned' via the callback instead...?

@jlomas-stripe ohhh no I misunderstood! All good, yea that'll definitely work inside _request - I have some free time later and can put it to action. thanks for further explaining!

@jlomas-stripe just tested it and worked perfectly. I appreciate the help simplifying. I'll update the PR with the changes requested in the review.

rattrayalex-stripe · 2018-07-05T16:04:59Z

Thanks – I'll defer to @ob-stripe on this one

… callback.

jjsquillante · 2018-07-08T20:36:15Z

hey all - I've made the requested updates from the review. Let me know if there's any other changes required or something I may have forgot! Thanks

ob-stripe · 2018-07-12T06:56:13Z

@jlomas-stripe Mind doing another review? Cheers :)

jlomas-stripe · 2018-07-12T16:36:07Z

@ob-stripe / @jjsquillante Yup, for sure; sorry for the delay there. WIll peek it this morning. 👍

jlomas-stripe

Looks fantastic - thanks again for taking this on! A couple of comments inline, but this is nearly there. :)

jlomas-stripe · 2018-07-12T18:26:23Z

lib/resources/FileUploads.js

    data = data || {};

    if (method === 'POST') {
-      return multipartDataGenerator(method, data, headers);
+      return getProcessorForSourceType(data);
    } else {
      return utils.stringifyRequestData(data);


This needs to make use of the callback flow here, so it's probably return callback(null, utils.stringifyRequestData(data));.

oh wow, great catch - my fault - I totally missed that! Thanks

jlomas-stripe · 2018-07-12T18:29:13Z

lib/StripeResource.js


-    var apiVersion = this._stripe.getApiField('version');
+      if (error) {
+        var handleError = self._errorHandlerStream(callback);


I'd rather see this FileUpload-specific error generation/handling pushed to FileUploads.js, and have this just be more generic here so it can handle future cases, i.e. just:

if (error) { return callback(error); }

At this point in the code, we don't (or shouldn't) really care what kind of error this is, we should just expose it in the callback.

jlomas-stripe · 2018-07-12T18:30:22Z

lib/StripeResource.js

@@ -212,6 +212,20 @@ StripeResource.prototype = {
    }
  },

+  _errorHandlerStream: function(callback) {


Per comment further down, I think the StreamProcessingError should be created (and wrap the incoming error) in FileUploads.js, since it's so specific to FileUploads.

(Apparently I meant 'further up' 😂)

good point! - removed and updated StreamProcessingError to FileUploads.

jlomas-stripe · 2018-07-12T18:31:38Z

lib/resources/FileUploads.js

+        var buffer = fn(method, bufferData, headers);
+        callback(null, buffer);
+      }).on('error', function(err) {
+        callback(err, null);


Per comments above, this is probably the right place to new up the StreamProcessorError.

updated - thanks!

jlomas-stripe · 2018-07-12T18:37:28Z

test/flows.spec.js

+        })
+      ).to.eventually.have.nested.property('size', 739);
+    });
+  });
 });


We don't actually test that the error comes back as expected; this should do it:

it('Surfaces stream errors correctly', function(done) { var mockedStream = new stream.Readable(); mockedStream._read = function() {}; var fakeError = new Error('I am a fake error'); process.nextTick(function() { mockedStream.emit('error', fakeError); }); stripe.fileUploads.create({ purpose: 'dispute_evidence', file: { data: mockedStream, name: 'minimal.pdf', type: 'application/octet-stream', }, }).catch(function(error) { expect(error.message).to.equal('An error occurred while attempting to process the file for upload.'); expect(error.detail).to.equal(fakeError); done(); }); });

We should also test that you can still provide a (non-Stream) file, which is just the same as what you've already got except with fs.readFileSync() instead of fs.createReadStream().

(I tried doing this in a promisey way but ~~I don't trust promises in Mocha~~ I couldn't get it to work. ¯\_(ツ)_/¯ :) )

added both tests (for synchronous file uploads and testing for the stream error to return properly) - thanks!

jjsquillante · 2018-07-12T18:50:44Z

@jlomas-stripe awesome, thanks for reviewing again! I'll look over the changes requested and revise based on your comments.

jlomas-stripe · 2018-07-12T19:12:34Z

Awesome - you're welcome! And I'll try to be more timely in my next review. :)

jjsquillante · 2018-07-16T00:31:52Z

@jlomas-stripe updated the code and revised based on your comments - let me know if there's anything that needs to be changed! thanks

jlomas-stripe · 2018-07-16T16:46:34Z

@jjsquillante Looks fantastic - thank you! :)

@ob-stripe / @rattrayalex-stripe Can one of y'all please take a rip through this and make sure it all makes sense to you as well? Thanks!

jlomas-stripe · 2018-07-16T21:10:02Z

Hold on - I didn't actually approve this, just the changes - I'm not sure how that happened. Weird.

I'd really like @ob-stripe or @rattrayalex-stripe to just have a quick peek first.

ob-stripe

Looks good to me. @rattrayalex-stripe, mind taking a final 👀?

rattrayalex-stripe

LGTM! Thanks @jjsquillante

jjsquillante · 2018-07-17T19:05:26Z

@jlomas-stripe @ob-stripe @rattrayalex-stripe @brandur-stripe
Thanks for taking the time to work with me on this PR - I've enjoyed collaborating with you all!

lopesc · 2018-07-17T19:17:36Z

Unsubscribe. On 15 Jul 2018 21:55, James Squillante <notifications@github.com> wrote: @jjsquillante commented on this pull request.

________________________________ In lib/resources/FileUploads.js<#471 (comment)>:

data = data || {};

if (method === 'POST') { - return multipartDataGenerator(method, data, headers); + return getProcessorForSourceType(data); } else { return utils.stringifyRequestData(data); oh wow, great catch - my fault - I totally missed that! Thanks — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#471 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AHSs70LZo4XoY6ag7r3MTN0_InlNYI_lks5uG6w_gaJpZM4U1Rn->.

jlomas-stripe · 2018-07-17T22:04:36Z

No problemo - thanks for taking this on! :)

@ob-stripe can you merge && release? Or ... can ... I? 😂

ob-stripe · 2018-07-18T11:39:04Z

I'll take :)

ob-stripe · 2018-07-18T11:41:24Z

Released as 6.3.0.

Stream support for FileUpload.

a58cb35

jjsquillante changed the title ~~Stream support for FileUpload #401~~ Stream support for FileUpload Jun 24, 2018

ob-stripe reviewed Jun 25, 2018

View reviewed changes

rattrayalex-stripe reviewed Jun 26, 2018

View reviewed changes

Stream support for FileUpload.

5dc9afd

jjsquillante added 5 commits July 1, 2018 13:40

Stream support for FileUploads.js. + test.

d99d077

Merge remote-tracking branch 'upstream/master'

54a79a6

Add safe-buffer module to FileUploads.

27c0121

Add back safe-buffer module to utils.

87328dd

Update promise chain indentation to match Stripe patterns.

f109d07

jlomas-stripe suggested changes Jul 4, 2018

View reviewed changes

jjsquillante added 5 commits July 5, 2018 18:55

Update async mocha tests to prevent false positive.

8764b7d

Add error handler for stream processing.

183d644

Remove unnecessary callback.

05a84c4

Simplify _request and requestDataProcessor for stream with additional…

5acf777

… callback.

Merge remote-tracking branch 'upstream/master'

c039eeb

jlomas-stripe suggested changes Jul 12, 2018

View reviewed changes

jjsquillante added 3 commits July 15, 2018 15:57

Flow to callback if method is not a POST.

aa90cec

Handle and generate stream error within fileUpload.js.

a1ebecc

Tests for synchronous file upload and stream error.

d963055

jlomas-stripe approved these changes Jul 16, 2018

View reviewed changes

stripe-ci added the approved label Jul 16, 2018

remi-stripe removed the approved label Jul 16, 2018

rattrayalex-stripe assigned ob-stripe Jul 16, 2018

ob-stripe approved these changes Jul 17, 2018

View reviewed changes

stripe-ci added the approved label Jul 17, 2018

rattrayalex-stripe approved these changes Jul 17, 2018

View reviewed changes

ob-stripe merged commit d597f14 into stripe:master Jul 18, 2018

ob-stripe mentioned this pull request Jul 18, 2018

Feature Request - FileUpload should start supporting streams #401

Closed

Stream support for FileUpload #471

Stream support for FileUpload #471

Conversation

jjsquillante commented Jun 24, 2018

ob-stripe commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlomas-stripe commented Jun 25, 2018 • edited by ob-stripe Loading

jjsquillante commented Jun 25, 2018 • edited Loading

jlomas-stripe commented Jun 25, 2018

brandur-stripe commented Jun 26, 2018

Choose a reason for hiding this comment

jjsquillante Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex-stripe commented Jun 26, 2018

jjsquillante commented Jun 26, 2018

jlomas-stripe commented Jun 26, 2018

jjsquillante commented Jun 27, 2018

jlomas-stripe commented Jun 27, 2018

jjsquillante commented Jul 1, 2018

jlomas-stripe left a 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

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

jjsquillante Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

jlomas-stripe Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex-stripe commented Jul 5, 2018

jjsquillante commented Jul 8, 2018

ob-stripe commented Jul 12, 2018

jlomas-stripe commented Jul 12, 2018

jlomas-stripe left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsquillante commented Jul 12, 2018

jlomas-stripe commented Jul 12, 2018

jjsquillante commented Jul 16, 2018 • edited Loading

jlomas-stripe commented Jul 16, 2018

jlomas-stripe commented Jul 16, 2018

ob-stripe left a comment • edited Loading

Choose a reason for hiding this comment

rattrayalex-stripe left a comment

Choose a reason for hiding this comment

jjsquillante commented Jul 17, 2018

lopesc commented Jul 17, 2018 via email

jlomas-stripe commented Jul 17, 2018

ob-stripe commented Jul 18, 2018

ob-stripe commented Jul 18, 2018

jlomas-stripe commented Jun 25, 2018 •

edited by ob-stripe

Loading

jjsquillante commented Jun 25, 2018 •

edited

Loading

jjsquillante Jun 26, 2018 •

edited

Loading

jjsquillante Jul 6, 2018 •

edited

Loading

jlomas-stripe Jul 6, 2018 •

edited

Loading

jjsquillante commented Jul 16, 2018 •

edited

Loading

ob-stripe left a comment •

edited

Loading