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

Fix #1325 Added support for dynamic custom paths #1785

Merged
merged 5 commits into from
Mar 31, 2023
Merged

Fix #1325 Added support for dynamic custom paths #1785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0c437d9
Added path-to-regexp for dynamic paths
satcheluniverse Mar 27, 2023
2ebf9bd
Added path-to-regexp for dynamic paths
satcheluniverse Mar 27, 2023
892db7b
Exit customRoute forEach when route found
satcheluniverse Mar 28, 2023
e460eae
Remove unneeded ESLint Ignore comment
satcheluniverse Mar 28, 2023
09566d5
Added testing for reverse order
satcheluniverse Mar 30, 2023
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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
"@types/tsscmp": "^1.0.0",
"axios": "^0.27.2",
"express": "^4.16.4",
"path-to-regexp": "^6.2.1",
"please-upgrade-node": "^3.2.0",
"promise.allsettled": "^1.0.2",
"raw-body": "^2.3.3",
Expand Down
116 changes: 112 additions & 4 deletions src/receivers/HTTPReceiver.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { Logger, LogLevel } from '@slack/logger';
import { EventEmitter } from 'events';
import { InstallProvider } from '@slack/oauth';
import { IncomingMessage, ServerResponse } from 'http';
import { match } from 'path-to-regexp';
import { ParamsDictionary } from 'express-serve-static-core';
import { Override, mergeOverrides } from '../test-helpers';
import {
AppInitializationError,
Expand Down Expand Up @@ -485,6 +487,7 @@ describe('HTTPReceiver', function () {
it('should call custom route handler only if request matches route path and method', async function () {
const HTTPReceiver = await importHTTPReceiver();
const customRoutes = [{ path: '/test', method: ['get', 'POST'], handler: sinon.fake() }];
const matchRegex = match(customRoutes[0].path, { decode: decodeURIComponent });
const receiver = new HTTPReceiver({
clientSecret: 'my-client-secret',
signingSecret: 'secret',
Expand All @@ -495,14 +498,17 @@ describe('HTTPReceiver', function () {
const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

fakeReq.url = '/test';
const tempMatch = matchRegex(fakeReq.url);
if (!tempMatch) throw new Error('match failed');
const params : ParamsDictionary = tempMatch.params as ParamsDictionary;

fakeReq.method = 'GET';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith(fakeReq, fakeRes));
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'POST';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith(fakeReq, fakeRes));
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'UNHANDLED_METHOD';
assert.throws(() => receiver.requestListener(fakeReq, fakeRes), HTTPReceiverDeferredRequestError);
Expand All @@ -511,6 +517,7 @@ describe('HTTPReceiver', function () {
it('should call custom route handler only if request matches route path and method, ignoring query params', async function () {
const HTTPReceiver = await importHTTPReceiver();
const customRoutes = [{ path: '/test', method: ['get', 'POST'], handler: sinon.fake() }];
const matchRegex = match(customRoutes[0].path, { decode: decodeURIComponent });
const receiver = new HTTPReceiver({
clientSecret: 'my-client-secret',
signingSecret: 'secret',
Expand All @@ -521,14 +528,115 @@ describe('HTTPReceiver', function () {
const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

fakeReq.url = '/test?hello=world';
const tempMatch = matchRegex('/test');
if (!tempMatch) throw new Error('match failed');
const params : ParamsDictionary = tempMatch.params as ParamsDictionary;

fakeReq.method = 'GET';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith(fakeReq, fakeRes));
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'POST';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith(fakeReq, fakeRes));
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'UNHANDLED_METHOD';
assert.throws(() => receiver.requestListener(fakeReq, fakeRes), HTTPReceiverDeferredRequestError);
});

it('should call custom route handler only if request matches route path and method including params', async function () {
const HTTPReceiver = await importHTTPReceiver();
const customRoutes = [{ path: '/test/:id', method: ['get', 'POST'], handler: sinon.fake() }];
Copy link
Member

@zimeg zimeg Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To verify that the order of these routes is respected by the receiver, we can update these routes to:

const customRoutes = [
  { path: '/test/123', method: ['get', 'POST'], handler: sinon.fake() },
  { path: '/test/:id', method: ['get', 'POST'], handler: sinon.fake() },
];

Then check for the following after making a fake request for /test/123:

assert(customRoutes[1].handler.notCalled);

Another mock request to a different url, say /test/abc, could follow the tests for /test/123 and might include these asserts:

assert(customRoutes[0].handler.notCalled);
assert(customRoutes[1].handler.calledWith({ ...fakeReq, params }, fakeRes));

And with that, I believe this would sufficiently show that the first matchable route in customRoutes is being matched!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am actually already doing something similar in the next it block from your example (line 579) the only difference is that I don't call assert(customRoutes[1].handler.notCalled); because I set the response to sinon.fake.throw('Should not be used.') as the handler which throws an exception. But I have updated that and I also added a second it block to test custom routes in reverse just to make sure that the first path is always called.

const matchRegex = match(customRoutes[0].path, { decode: decodeURIComponent });
const receiver = new HTTPReceiver({
clientSecret: 'my-client-secret',
signingSecret: 'secret',
customRoutes,
});

const fakeReq: IncomingMessage = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

fakeReq.url = '/test/123';
const tempMatch = matchRegex(fakeReq.url);
if (!tempMatch) throw new Error('match failed');
const params : ParamsDictionary = tempMatch.params as ParamsDictionary;

fakeReq.method = 'GET';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'POST';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'UNHANDLED_METHOD';
assert.throws(() => receiver.requestListener(fakeReq, fakeRes), HTTPReceiverDeferredRequestError);
});

it('should call custom route handler only if request matches multiple route paths and method including params', async function () {
const HTTPReceiver = await importHTTPReceiver();
const customRoutes = [
{ path: '/test/123', method: ['get', 'POST'], handler: sinon.fake() },
{ path: '/test/:id', method: ['get', 'POST'], handler: sinon.fake() },
];
const matchRegex = match(customRoutes[0].path, { decode: decodeURIComponent });
const receiver = new HTTPReceiver({
clientSecret: 'my-client-secret',
signingSecret: 'secret',
customRoutes,
});

const fakeReq: IncomingMessage = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

fakeReq.url = '/test/123';
const tempMatch = matchRegex(fakeReq.url);
if (!tempMatch) throw new Error('match failed');
const params : ParamsDictionary = tempMatch.params as ParamsDictionary;

fakeReq.method = 'GET';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));
assert(customRoutes[1].handler.notCalled);

fakeReq.method = 'POST';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'UNHANDLED_METHOD';
assert.throws(() => receiver.requestListener(fakeReq, fakeRes), HTTPReceiverDeferredRequestError);
});

it('should call custom route handler only if request matches multiple route paths and method including params reverse order', async function () {
const HTTPReceiver = await importHTTPReceiver();
const customRoutes = [
{ path: '/test/:id', method: ['get', 'POST'], handler: sinon.fake() },
{ path: '/test/123', method: ['get', 'POST'], handler: sinon.fake() },
];
const matchRegex = match(customRoutes[0].path, { decode: decodeURIComponent });
const receiver = new HTTPReceiver({
clientSecret: 'my-client-secret',
signingSecret: 'secret',
customRoutes,
});

const fakeReq: IncomingMessage = sinon.createStubInstance(IncomingMessage) as IncomingMessage;
const fakeRes: ServerResponse = sinon.createStubInstance(ServerResponse) as unknown as ServerResponse;

fakeReq.url = '/test/123';
const tempMatch = matchRegex(fakeReq.url);
if (!tempMatch) throw new Error('match failed');
const params : ParamsDictionary = tempMatch.params as ParamsDictionary;

fakeReq.method = 'GET';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));
assert(customRoutes[1].handler.notCalled);

fakeReq.method = 'POST';
receiver.requestListener(fakeReq, fakeRes);
assert(customRoutes[0].handler.calledWith({ ...fakeReq, params }, fakeRes));

fakeReq.method = 'UNHANDLED_METHOD';
assert.throws(() => receiver.requestListener(fakeReq, fakeRes), HTTPReceiverDeferredRequestError);
Expand Down
22 changes: 19 additions & 3 deletions src/receivers/HTTPReceiver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import { ListenOptions } from 'net';
import { Logger, ConsoleLogger, LogLevel } from '@slack/logger';
import { InstallProvider, CallbackOptions, InstallProviderOptions, InstallURLOptions, InstallPathOptions } from '@slack/oauth';
import { URL } from 'url';

import { match } from 'path-to-regexp';
import { ParamsDictionary } from 'express-serve-static-core';
import { ParamsIncomingMessage } from './ParamsIncomingMessage';
import { verifyRedirectOpts } from './verify-redirect-opts';
import App from '../App';
import { Receiver, ReceiverEvent } from '../types';
Expand Down Expand Up @@ -392,8 +394,22 @@ export default class HTTPReceiver implements Receiver {

// Handle custom routes
if (Object.keys(this.routes).length) {
const match = this.routes[path] && this.routes[path][method] !== undefined;
if (match) { return this.routes[path][method](req, res); }
// Check if the request matches any of the custom routes
let pathMatch : string | boolean = false;
let params : ParamsDictionary = {};
Object.keys(this.routes).forEach((route) => {
if (pathMatch) return;
const matchRegex = match(route, { decode: decodeURIComponent });
const tempMatch = matchRegex(path);
if (tempMatch) {
pathMatch = route;
params = tempMatch.params as ParamsDictionary;
}
Comment on lines +404 to +407
Copy link
Member

@zimeg zimeg Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We might want to stop checking for matches once one is found to align with the "first come first serve" routing precedence used by Express. As is, attempting to access http://localhost:3000/crayon/blue will return "There are no more blue crayons!".

const app = new App({
  token: "xoxb-token-here",
  signingSecret: "example-signing-secret",
  customRoutes: [{
    path: '/crayon/blue',
    method: ['GET'],
    handler: (req, res) => {
      res.writeHead(200);
      res.end(`How cool! A true blue!`);
    },
  }, {
    path: '/crayon/:color',
    method: ['GET'],
    handler: (req, res) => {
      res.writeHead(200);
      res.end(`There are no more ${req.params.color} crayons!`);
    },
  }, {
    path: '/music/:genre',
    method: ['GET'],
    handler: (req, res) => {
      res.writeHead(200);
      res.end(`Oh? ${req.params.genre}? That slaps!`);
    },
  }, {
    path: '/music/jazz',
    method: ['GET'],
    handler: (req, res) => {
      res.writeHead(200);
      res.end('Play it cool, you cat.');
    },
  }],
});

Copy link
Member

Choose a reason for hiding this comment

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

And if not too much trouble, adding a test with multiple overlapping routes (to verify that this precedence is respected) would be wonderful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a very good point, I missed that. I have gone ahead and added a conditional return in the forEach and a test to make sure the first found route should be returned.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that the conditional is enough to guarantee the ordering and have noticed it's still finicky with the above example 😕 Diving a bit deeper, it looks like the buildReceiverRoutes function is converting the customRoutes array to an object, causing these keys to have an undefined ordering.

Considering that the ReceiverRoutes type is only used in the context of building these routes, I think it's safe make changes to somehow hold these routes in an array to preserve the ordering while building the ReceiverRoutes objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for being so patient with me @E-Zim. Just so I understand, are you thinking it would be better to keep the customRoutes as an array instead of converting it to an object? Or just add customRoutes array to the HTTPReceiver class? Or am I missing the point entirely?

Copy link
Member

Choose a reason for hiding this comment

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

No worries at all! To offer more context, the customRoutes array is converted into ReceiverRoute-like objects in the buildReceiverRoutes function and these objects are used in the HTTPReceiver and SocketModeReceiver classes. For this reason, the customRoutes array has to undergo a conversion and cannot be directly attached to the routes in these Receiver classes.

I would suggest changing the return type of the buildReceiverRoutes function to be a Map from the URL string to an array of a new ReceiverRoute type, with the ReceiverRoute type resembling this object. This Map would preserve the order that entries are inserted, meaning the order of customRoutes can be maintained and relied upon when iterating over the routes in the HTTPReceiver and SocketModeReceiver classes.

Hoping this makes some sense, but apologies if this jumble of words is confusing. Happy to clarify more if needed!

Copy link
Contributor Author

@satcheluniverse satcheluniverse Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the explanation, I believe I did it correctly. But I am happy to revise if this isn't what you were thinking. I tried to make it as simple as possible. Tests pass, but I was not able to replicate the issue with ordering that you described. Would you mind explaining how I could test that so I know for future reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@E-Zim bumping in case the last comment didn't notify you because I forgot to @ you

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay! I have extended the example above to further test this and it appeared that my local bolt-js version was cached to a prior commit. Apologies for not catching this sooner! Removing my node_modules then reinstalling the packages with the latest changes seemed to fix things.

Thank you making the changes in 04ba9e0, but I'm no longer certain that these are required (through testing with the above example). If it is alright, would you be okay reverting this?

In searching for examples to test the supposed undefined object ordering, I found the following from MDN stating that "the order of the array returned by Object.keys() is the same as that provided by a for...in loop", and that the traversal order of a for...in loop "is well-defined and consistent across implementations". This is confirmed to hold true for V8, so it seems like my confusion of caching got in the way of the Javascript standards 😖

I will add another comment about verifying the expected ordering behavior for this PR, but for future reference I don't believe testing an undefined object ordering can be done easily (if it's even relevant to your JS implementation). Sorry again for mistaking this behavior!

});
const urlMatch = pathMatch && this.routes[pathMatch][method] !== undefined;
if (urlMatch && pathMatch) {
return this.routes[pathMatch][method]({ ...req, params } as ParamsIncomingMessage, res);
}
}

// If the request did not match the previous conditions, an error is thrown. The error can be caught by
Expand Down
13 changes: 13 additions & 0 deletions src/receivers/ParamsIncomingMessage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { IncomingMessage } from 'http';
import { ParamsDictionary } from 'express-serve-static-core';

export interface ParamsIncomingMessage extends IncomingMessage {
/**
* **Only valid for requests with path parameters.**
*
* The path parameters of the request. For example, if the request URL is
* `/users/123`, and the route definition is `/users/:id`
* then `request.params` will be `{ id: '123' }`.
*/
params?: ParamsDictionary;
}
Loading