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.

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

Stream support for FileUpload #471

Merged
merged 15 commits into from
Jul 18, 2018
Merged
Show file tree
Hide file tree
Changes from 12 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
10 changes: 10 additions & 0 deletions lib/Error.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,13 @@ _Error.StripeRateLimitError = StripeError.extend({type: 'StripeRateLimitError'})
_Error.StripeConnectionError = StripeError.extend({type: 'StripeConnectionError'});
_Error.StripeSignatureVerificationError = StripeError.extend({type: 'StripeSignatureVerificationError'});
_Error.StripeIdempotencyError = StripeError.extend({type: 'StripeIdempotencyError'});

// Error while processing stream for file upload:
_Error.StreamProcessingError = _Error.extend({
type: 'StreamProcessingError',
populate: function(raw) {
this.type = this.type;
this.message = raw.message;
this.detail = raw.detail;
}
});
55 changes: 39 additions & 16 deletions lib/StripeResource.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,20 @@ StripeResource.prototype = {
}
},

_errorHandlerStream: function(callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Apparently I meant 'further up' 😂)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! - removed and updated StreamProcessingError to FileUploads.

var self = this;
return function(error) {
callback.call(
self,
new Error.StreamProcessingError({
message: 'An error occurred while attempting to process the file for upload.',
detail: error,
}),
null
);
}
},

_defaultHeaders: function(auth, contentLength, apiVersion) {
var userAgentString = 'Stripe/v1 NodeBindings/' + this._stripe.getConstant('PACKAGE_VERSION');

Expand Down Expand Up @@ -241,28 +255,37 @@ StripeResource.prototype = {
var self = this;
var requestData;

if (self.requestDataProcessor) {
requestData = self.requestDataProcessor(method, data, options.headers);
} else {
requestData = utils.stringifyRequestData(data || {});
}
function makeRequestWithData(error, data) {
var apiVersion;
var headers;

var apiVersion = this._stripe.getApiField('version');
if (error) {
var handleError = self._errorHandlerStream(callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

return handleError(error);
}

var headers = self._defaultHeaders(auth, requestData.length, apiVersion);
apiVersion = self._stripe.getApiField('version');
requestData = data;
headers = self._defaultHeaders(auth, requestData.length, apiVersion);

// Grab client-user-agent before making the request:
this._stripe.getClientUserAgent(function(cua) {
headers['X-Stripe-Client-User-Agent'] = cua;
self._stripe.getClientUserAgent(function(cua) {
headers['X-Stripe-Client-User-Agent'] = cua;

if (options.headers) {
Object.assign(headers, options.headers);
}
if (options.headers) {
Object.assign(headers, options.headers);
}

makeRequest();
});
makeRequest(apiVersion, headers);
});
}

if (self.requestDataProcessor) {
self.requestDataProcessor(method, data, options.headers, makeRequestWithData);
} else {
makeRequestWithData(null, utils.stringifyRequestData(data || {}));
}

function makeRequest() {
function makeRequest(apiVersion, headers) {
var timeout = self._stripe.getApiField('timeout');
var isInsecureConnection = self._stripe.getApiField('protocol') == 'http';

Expand Down
29 changes: 27 additions & 2 deletions lib/resources/FileUploads.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

var Buffer = require('safe-buffer').Buffer;
var utils = require('../utils');
var StripeResource = require('../StripeResource');
var stripeMethod = StripeResource.method;
Expand All @@ -9,14 +10,38 @@ module.exports = StripeResource.extend({

overrideHost: 'uploads.stripe.com',

requestDataProcessor: function(method, data, headers) {
requestDataProcessor: function(method, data, headers, callback) {
data = data || {};

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

function getProcessorForSourceType(data) {
var isStream = utils.checkForStream(data);
if (isStream) {
return streamProcessor(multipartDataGenerator);
} else {
var buffer = multipartDataGenerator(method, data, headers);
return callback(null, buffer);
}
}

function streamProcessor (fn) {
var bufferArray = [];
data.file.data.on('data', function(line) {
bufferArray.push(line);
}).on('end', function() {
var bufferData = Object.assign({}, data);
bufferData.file.data = Buffer.concat(bufferArray);
var buffer = fn(method, bufferData, headers);
callback(null, buffer);
}).on('error', function(err) {
callback(err, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated - thanks!

});
}
},

path: 'files',
Expand Down
12 changes: 12 additions & 0 deletions lib/utils.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

var Buffer = require('safe-buffer').Buffer;
var EventEmitter = require('events').EventEmitter;
var qs = require('qs');
var crypto = require('crypto');

Expand Down Expand Up @@ -215,6 +216,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 obj.file.data instanceof EventEmitter;
}
return false;
},
};

function emitWarning(warning) {
Expand Down
23 changes: 22 additions & 1 deletion test/flows.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ var stripe = require('../lib/stripe')(
testUtils.getUserStripeKey(),
'latest'
);

var fs = require('fs');
var path = require('path');
var expect = chai.expect;

var CUSTOMER_DETAILS = {
Expand Down Expand Up @@ -522,4 +523,24 @@ describe('Flows', function() {
}).type).to.equal('StripeInvalidRequestError');
});
});

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);
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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().

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

35 changes: 35 additions & 0 deletions test/resources/FileUploads.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,40 @@ 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);

return stripe.fileUploads.create({
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);

return stripe.fileUploads.create({
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);
});
});
});
});