Skip to content

Commit

Permalink
fix: Include trailing slash when passing empty query parameters.
Browse files Browse the repository at this point in the history
  • Loading branch information
dcr-stripe committed Jul 14, 2022
1 parent 463fb19 commit f425526
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 14 deletions.
34 changes: 23 additions & 11 deletions lib/StripeResource.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,30 +69,42 @@ StripeResource.prototype = {
validateRequest: null,

createFullPath(commandPath, urlData) {
return this._joinUrlParts([
this.basePath(urlData),
this.path(urlData),
typeof commandPath == 'function' ? commandPath(urlData) : commandPath,
]);
const urlParts = [this.basePath(urlData), this.path(urlData)];

if (typeof commandPath === 'function') {
const computedCommandPath = commandPath(urlData);
// If we have no actual command path, we just omit it to avoid adding a
// trailing slash. This is important for top-level listing requests, which
// do not have a command path.
if (computedCommandPath) {
urlParts.push(computedCommandPath);
}
} else {
urlParts.push(commandPath);
}

return this._joinUrlParts(urlParts);
},

// Creates a relative resource path with symbols left in (unlike
// createFullPath which takes some data to replace them with). For example it
// might produce: /invoices/{id}
createResourcePathWithSymbols(pathWithSymbols) {
return `/${this._joinUrlParts([this.resourcePath, pathWithSymbols || ''])}`;
// If there is no path beyond the resource path, we want to produce just
// /<resource path> rather than /<resource path>/.
if (pathWithSymbols) {
return `/${this._joinUrlParts([this.resourcePath, pathWithSymbols])}`;
} else {
return `/${this.resourcePath}`;
}
},

_joinUrlParts(parts) {
// Replace any accidentally doubled up slashes. This previously used
// path.join, which would do this as well. Unfortunately we need to do this
// as the functions for creating paths are technically part of the public
// interface and so we need to preserve backwards compatibility.
const path = parts.join('/').replace(/\/{2,}/g, '/');

// If the path ends with a /, we preserve the behavior of path.join and
// strip off the trailing / (eg. /v1/customers/ -> /v1/customers).
return path.endsWith('/') ? path.slice(0, -1) : path;
return parts.join('/').replace(/\/{2,}/g, '/');
},

// DEPRECATED: Here for backcompat in case users relied on this.
Expand Down
1 change: 1 addition & 0 deletions lib/makeRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ function getRequestOpts(self, requestArgs, spec, overrideData) {
const requestPath = isUsingFullPath
? commandPath(urlData)
: self.createFullPath(commandPath, urlData);

const headers = Object.assign(options.headers, spec.headers);

if (spec.validator) {
Expand Down
79 changes: 76 additions & 3 deletions test/StripeResource.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,19 @@ const {

describe('StripeResource', () => {
describe('createResourcePathWithSymbols', () => {
it('Generates a path', () => {
it('Generates a path when there is a symbol', () => {
stripe.invoices.create({});
const path = stripe.invoices.createResourcePathWithSymbols('{id}');
expect(path).to.equal('/invoices/{id}');
});

it('Generates a path when there is nothing beyond the resource path', () => {
stripe.invoices.create({});
const path = stripe.invoices.createResourcePathWithSymbols('');
// This explicitly shouldn't have a trailing slash.
expect(path).to.equal('/invoices');
});

it('Handles accidental double slashes', () => {
stripe.invoices.create({});
const path = stripe.invoices.createResourcePathWithSymbols('/{id}');
Expand All @@ -36,9 +43,9 @@ describe('StripeResource', () => {
});

describe('_joinUrlParts', () => {
it('handles trailing empty values correctly', () => {
it('includes trailing empty values', () => {
const path = stripe.invoices._joinUrlParts(['a', '']);
expect(path).to.equal('a');
expect(path).to.equal('a/');
});

it('joins parts', () => {
Expand Down Expand Up @@ -132,6 +139,72 @@ describe('StripeResource', () => {
});
});

it('handles .. as a query param', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
.get('/v1/customers/..', '')
.reply(200, '{}');

realStripe.customers.retrieve('..', (err, response) => {
done(err);
scope.done();
});
});

it('handles empty string as a query param', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note this should always have a trailing space to avoid calling the
// top level list endpoint (/v1/customers) and returning all customers.
.get('/v1/customers/', '')
.reply(200, '{}');

realStripe.customers.retrieve('', (err, response) => {
done(err);
scope.done();
});
});

it('handles empty string as a query param for namespaced resources', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note this should always have a trailing space to avoid calling the
// top level list endpoint (/v1/customers) and returning all customers.
.get('/v1/checkout/sessions/', '')
.reply(200, '{}');

realStripe.checkout.sessions.retrieve('', (err, response) => {
done(err);
scope.done();
});
});

it('handles empty string as a query param for nested resources', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note this should always have a trailing space to avoid calling the
// top level list endpoint (/v1/customers) and returning all customers.
.get('/v1/customers/cus_123/balance_transactions/', '')
.reply(200, '{}');

realStripe.customers.retrieveBalanceTransaction(
'cus_123',
'',
(err, response) => {
done(err);
scope.done();
}
);
});

it('does not include trailing slash for endpoints without query parameters', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
// Note that no trailing slash is present.
.get('/v1/customers', '')
.reply(200, '{}');

realStripe.customers.list((err, response) => {
done(err);
scope.done();
});
});

it('works correctly with undefined optional arguments', (done) => {
const scope = nock(`https://${stripe.getConstant('DEFAULT_HOST')}`)
.get('/v1/accounts/acct_123')
Expand Down

0 comments on commit f425526

Please sign in to comment.