From ab5591bef966d935e1f99ec171fc9581207cdb7d Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Wed, 25 Mar 2020 08:32:56 -0700 Subject: [PATCH 1/7] add alternative implementation of middleware processing the main difference is this implementation aggregates errors from multiple failed listener middleware chains into a single error which captures all of them. the previous implementation is mostly commented out. lint is passing, tests are failing. --- package.json | 2 + src/App.ts | 146 ++++++++++++++++++++++++++++---------- src/errors.ts | 13 ++++ src/middleware/process.ts | 2 +- src/types/middleware.ts | 26 ++++--- 5 files changed, 141 insertions(+), 48 deletions(-) diff --git a/package.json b/package.json index 5ca493640..79f366a39 100644 --- a/package.json +++ b/package.json @@ -46,9 +46,11 @@ "@slack/web-api": "^5.6.0", "@types/express": "^4.16.1", "@types/node": ">=10", + "@types/promise.allsettled": "^1.0.3", "axios": "^0.18.1", "express": "^4.16.4", "please-upgrade-node": "^3.2.0", + "promise.allsettled": "^1.0.2", "raw-body": "^2.3.3", "tsscmp": "^1.0.6" }, diff --git a/src/App.ts b/src/App.ts index cfd7bb497..72d407c47 100644 --- a/src/App.ts +++ b/src/App.ts @@ -22,7 +22,7 @@ import { matchMessage, onlyViewActions, } from './middleware/builtin'; -import { processMiddleware } from './middleware/process'; +// import { processMiddleware } from './middleware/process'; import { ConversationStore, conversationContext, MemoryStore } from './conversation-store'; import { Middleware, @@ -50,8 +50,10 @@ import { CodedError, asCodedError, AppInitializationError, + MultipleListenerError, } from './errors'; -import { MiddlewareContext } from './types/middleware'; +// import { MiddlewareContext } from './types/middleware'; +import promiseAllsettled, { PromiseRejection } from 'promise.allsettled'; const packageJson = require('../package.json'); // tslint:disable-line:no-require-imports no-var-requires /** App initialization options */ @@ -462,16 +464,6 @@ export default class App { }; }; - let listenerArgClient = this.client; - const token = selectToken(context); - if (typeof token !== 'undefined') { - let pool = this.clients[source.teamId]; - if (typeof pool === 'undefined') { - pool = this.clients[source.teamId] = new WebClientPool(); - } - listenerArgClient = pool.getOrCreate(token, this.clientOptions); - } - // Set body and payload (this value will eventually conform to AnyMiddlewareArgs) // NOTE: the following doesn't work because... distributive? // const listenerArgs: Partial = { @@ -482,13 +474,7 @@ export default class App { respond?: RespondFn, /** Ack function might be set below */ ack?: AckFn, - /** The logger for this Bolt app */ - logger?: Logger, - /** WebClient with token */ - client?: WebClient, } = { - logger: this.logger, - client: listenerArgClient, body: bodyArg, payload: (type === IncomingEventType.Event) ? @@ -549,30 +535,85 @@ export default class App { // Events API requests are acknowledged right away, since there's no data expected await ack(); } - const middlewareChain = [...this.middleware]; - - if (this.listeners.length > 0) { - middlewareChain.push(async (ctx) => { - const { next } = ctx; - - await Promise.all(this.listeners.map( - listener => processMiddleware(listener, ctx))); - await next(); - }); + // Get the client arg + let client = this.client; + const token = selectToken(context); + if (typeof token !== 'undefined') { + let pool = this.clients[source.teamId]; + if (typeof pool === 'undefined') { + pool = this.clients[source.teamId] = new WebClientPool(); + } + client = pool.getOrCreate(token, this.clientOptions); } - // Dispatch event through global middleware - try { - await processMiddleware(middlewareChain, { - context, - ...(listenerArgs as MiddlewareContext), - }); - } catch (error) { - return this.handleError(error); - } + // const middlewareChain = [...this.middleware]; + + // if (this.listeners.length > 0) { + // middlewareChain.push(async (ctx) => { + // const { next } = ctx; + + // await Promise.all(this.listeners.map( + // listener => processMiddleware(listener, ctx))); + + // await next(); + // }); + // } + + // // Dispatch event through global middleware + // try { + // await processMiddleware(middlewareChain, { + // context, + // ...(listenerArgs as MiddlewareContext), + // }); + // } catch (error) { + // return this.handleError(error); + // } + + // Dispatch even through the global middleware chain + return processMiddleware( + this.middleware, + listenerArgs as AnyMiddlewareArgs, + context, + client, + this.logger, + async () => { + // Dispatch the event through the listener middleware chains and aggregate their results + // TODO: change the name of this.middleware and this.listeners to help this make more sense + const listenerResults = this.listeners.map(async (origListenerMiddleware) => { + // Copy the array so modifications don't affect the original + const listenerMiddleware = [...origListenerMiddleware]; + + // Don't process the last item in the listenerMiddleware array - it shouldn't get a next fn + const listener = listenerMiddleware.pop(); + + if (listener !== undefined) { + return processMiddleware( + listenerMiddleware, + listenerArgs as AnyMiddlewareArgs, + context, + client, + this.logger, + async context => + // When the listener middleware chain is done processing, call the listener without a next fn + listener({ ...listenerArgs as AnyMiddlewareArgs, context, client, logger: this.logger }), + ); + } + }); + + const settledListenerResults = await promiseAllsettled(listenerResults); + const rejectedListenerResults = + settledListenerResults.filter(lr => lr.status === 'rejected') as PromiseRejection[]; + if (rejectedListenerResults.length === 1) { + throw rejectedListenerResults[0]; + } else if (rejectedListenerResults.length > 1) { + throw new MultipleListenerError(rejectedListenerResults.map(rlr => rlr.reason)); + } + }, + ); } + // TODO: make the following method private if its no longer being used by Receiver /** * Global error handler. The final destination for all errors (hopefully). */ @@ -582,6 +623,37 @@ export default class App { } +async function processMiddleware( + middleware: Middleware[], + initialArgs: AnyMiddlewareArgs, + context: Context, + client: WebClient, + logger: Logger, + betweenPhases?: (context: Context) => Promise, +): Promise { + let middlewareIndex = 0; + + async function invokeCurrentMiddleware(): ReturnType> { + if (middlewareIndex !== middleware.length) { + const result = await middleware[middlewareIndex]({ + next: invokeCurrentMiddleware, + ...initialArgs, + context, + client, + logger, + }); + middlewareIndex += 1; + return result; + } + + if (betweenPhases !== undefined) { + return betweenPhases(context); + } + } + + return invokeCurrentMiddleware(); +} + const tokenUsage = 'Apps used in one workspace should be initialized with a token. Apps used in many workspaces ' + 'should be initialized with a authorize.'; diff --git a/src/errors.ts b/src/errors.ts index f8150e9c6..217c050ff 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -12,6 +12,8 @@ export enum ErrorCode { ReceiverAckTwiceError = 'slack_bolt_receiver_ack_twice_error', ReceiverAuthenticityError = 'slack_bolt_receiver_authenticity_error', + MultipleListenerError = 'slack_bolt_multiple_listener_error', + /** * This value is used to assign to errors that occur inside the framework but do not have a code, to keep interfaces * in terms of CodedError. @@ -68,6 +70,17 @@ export class ReceiverAuthenticityError extends Error implements CodedError { public code = ErrorCode.ReceiverAuthenticityError; } +export class MultipleListenerError extends Error implements CodedError { + public code = ErrorCode.MultipleListenerError; + public originals: Error[]; + + constructor(originals: Error[]) { + super('Multiple errors occurred while handling several listeners. The `originals` property contains an array of each error.'); + + this.originals = originals; + } +} + export class UnknownError extends Error implements CodedError { public code = ErrorCode.UnknownError; public original: Error; diff --git a/src/middleware/process.ts b/src/middleware/process.ts index 025507407..2f3460ec6 100644 --- a/src/middleware/process.ts +++ b/src/middleware/process.ts @@ -1,7 +1,7 @@ import { Middleware, AnyMiddlewareArgs, - MiddlewareContext, ProcessMiddlewareContext, + // MiddlewareContext, ProcessMiddlewareContext, } from '../types'; function composeMiddleware(middleware: Middleware[]): Middleware { diff --git a/src/types/middleware.ts b/src/types/middleware.ts index 63588c76c..4381cb6a9 100644 --- a/src/types/middleware.ts +++ b/src/types/middleware.ts @@ -14,21 +14,27 @@ export type AnyMiddlewareArgs = export interface Context extends StringIndexed { } -export type ProcessMiddlewareContext = Args & { - next?: NextMiddleware, context: Context, - logger: Logger, - client: WebClient, -}; +// export type ProcessMiddlewareContext = Args & { +// next?: NextMiddleware, context: Context, +// logger: Logger, +// client: WebClient, +// }; -export type MiddlewareContext = ProcessMiddlewareContext & { - next: NextMiddleware, -}; +// export type MiddlewareContext = ProcessMiddlewareContext & { +// next: NextMiddleware, +// }; // NOTE: Args should extend AnyMiddlewareArgs, but because of contravariance for function types, including that as a // constraint would mess up the interface of App#event(), App#message(), etc. export interface Middleware { // TODO: is there something nice we can do to get context's property types to flow from one middleware to the next? - (ctx: MiddlewareContext): Promise; + // (ctx: MiddlewareContext): Promise; + (args: Args & { + next?: NextMiddleware, + context: Context, + logger: Logger, + client: WebClient, + }): Promise; } -export type NextMiddleware = () => Promise; +export type NextMiddleware = () => Promise; From 3fc9ba341dc7000dd66d115df5fb4a4ef7942287 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Wed, 25 Mar 2020 09:02:22 -0700 Subject: [PATCH 2/7] removes dead code and moves the new processMiddleware back to the file where the old one was. --- src/App.ts | 57 +-------------------------------- src/middleware/process.ts | 67 +++++++++++++++++---------------------- src/types/middleware.ts | 11 ------- 3 files changed, 30 insertions(+), 105 deletions(-) diff --git a/src/App.ts b/src/App.ts index c7c423414..b42f6a9b9 100644 --- a/src/App.ts +++ b/src/App.ts @@ -23,7 +23,7 @@ import { matchMessage, onlyViewActions, } from './middleware/builtin'; -// import { processMiddleware } from './middleware/process'; +import { processMiddleware } from './middleware/process'; import { ConversationStore, conversationContext, MemoryStore } from './conversation-store'; import { Middleware, @@ -55,7 +55,6 @@ import { AppInitializationError, MultipleListenerError, } from './errors'; -// import { MiddlewareContext } from './types/middleware'; import promiseAllsettled, { PromiseRejection } from 'promise.allsettled'; const packageJson = require('../package.json'); // tslint:disable-line:no-require-imports no-var-requires @@ -593,29 +592,6 @@ export default class App { client = pool.getOrCreate(token, this.clientOptions); } - // const middlewareChain = [...this.middleware]; - - // if (this.listeners.length > 0) { - // middlewareChain.push(async (ctx) => { - // const { next } = ctx; - - // await Promise.all(this.listeners.map( - // listener => processMiddleware(listener, ctx))); - - // await next(); - // }); - // } - - // // Dispatch event through global middleware - // try { - // await processMiddleware(middlewareChain, { - // context, - // ...(listenerArgs as MiddlewareContext), - // }); - // } catch (error) { - // return this.handleError(error); - // } - // Dispatch even through the global middleware chain return processMiddleware( this.middleware, @@ -669,37 +645,6 @@ export default class App { } -async function processMiddleware( - middleware: Middleware[], - initialArgs: AnyMiddlewareArgs, - context: Context, - client: WebClient, - logger: Logger, - betweenPhases?: (context: Context) => Promise, -): Promise { - let middlewareIndex = 0; - - async function invokeCurrentMiddleware(): ReturnType> { - if (middlewareIndex !== middleware.length) { - const result = await middleware[middlewareIndex]({ - next: invokeCurrentMiddleware, - ...initialArgs, - context, - client, - logger, - }); - middlewareIndex += 1; - return result; - } - - if (betweenPhases !== undefined) { - return betweenPhases(context); - } - } - - return invokeCurrentMiddleware(); -} - const tokenUsage = 'Apps used in one workspace should be initialized with a token. Apps used in many workspaces ' + 'should be initialized with a authorize.'; diff --git a/src/middleware/process.ts b/src/middleware/process.ts index 2f3460ec6..eaf4d04cf 100644 --- a/src/middleware/process.ts +++ b/src/middleware/process.ts @@ -1,47 +1,38 @@ import { Middleware, AnyMiddlewareArgs, - // MiddlewareContext, ProcessMiddlewareContext, + Context, } from '../types'; +import { WebClient } from '@slack/web-api'; +import { Logger } from '@slack/logger'; -function composeMiddleware(middleware: Middleware[]): Middleware { - return function (context: ProcessMiddlewareContext): Promise { - // last called middleware # - let index = -1; - - async function dispatch(order: number): Promise { - if (order < index) { - return Promise.reject(new Error('next() called multiple times')); - } - - index = order; - - let fn: Middleware | undefined = middleware[order]; - - if (order === middleware.length) { - fn = context.next; - } - - if (fn === null || fn === undefined) { - return; - } - - context.next = dispatch.bind(null, order + 1); - - return fn((context as MiddlewareContext)); +export async function processMiddleware( + middleware: Middleware[], + initialArgs: AnyMiddlewareArgs, + context: Context, + client: WebClient, + logger: Logger, + betweenPhases?: (context: Context) => Promise, +): Promise { + let middlewareIndex = 0; + + async function invokeCurrentMiddleware(): ReturnType> { + if (middlewareIndex !== middleware.length) { + const result = await middleware[middlewareIndex]({ + next: invokeCurrentMiddleware, + ...initialArgs, + context, + client, + logger, + }); + middlewareIndex += 1; + return result; } - return dispatch(0); - }; -} + if (betweenPhases !== undefined) { + return betweenPhases(context); + } + } -export async function processMiddleware( - middleware: Middleware[], - context: ProcessMiddlewareContext, -): Promise { - return composeMiddleware(middleware)({ - ...context, - next: /* istanbul ignore next: Code can't be reached, noop instead of `null` for typing */ - () => Promise.resolve(), - }); + return invokeCurrentMiddleware(); } diff --git a/src/types/middleware.ts b/src/types/middleware.ts index 2c9254687..3e39e604c 100644 --- a/src/types/middleware.ts +++ b/src/types/middleware.ts @@ -15,21 +15,10 @@ export type AnyMiddlewareArgs = export interface Context extends StringIndexed { } -// export type ProcessMiddlewareContext = Args & { -// next?: NextMiddleware, context: Context, -// logger: Logger, -// client: WebClient, -// }; - -// export type MiddlewareContext = ProcessMiddlewareContext & { -// next: NextMiddleware, -// }; - // NOTE: Args should extend AnyMiddlewareArgs, but because of contravariance for function types, including that as a // constraint would mess up the interface of App#event(), App#message(), etc. export interface Middleware { // TODO: is there something nice we can do to get context's property types to flow from one middleware to the next? - // (ctx: MiddlewareContext): Promise; (args: Args & { next?: NextMiddleware, context: Context, From 5d39aeabcb8e60bfe9694edd4a894b78c2e820b0 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Wed, 25 Mar 2020 09:07:18 -0700 Subject: [PATCH 3/7] it turns out context isn't necessary since its always available through scope closure. might as well remove to keep things tidy. --- src/App.ts | 2 +- src/middleware/process.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/App.ts b/src/App.ts index b42f6a9b9..8e2e2cea9 100644 --- a/src/App.ts +++ b/src/App.ts @@ -616,7 +616,7 @@ export default class App { context, client, this.logger, - async context => + async () => // When the listener middleware chain is done processing, call the listener without a next fn listener({ ...listenerArgs as AnyMiddlewareArgs, context, client, logger: this.logger }), ); diff --git a/src/middleware/process.ts b/src/middleware/process.ts index eaf4d04cf..507e2fa28 100644 --- a/src/middleware/process.ts +++ b/src/middleware/process.ts @@ -12,7 +12,7 @@ export async function processMiddleware( context: Context, client: WebClient, logger: Logger, - betweenPhases?: (context: Context) => Promise, + betweenPhases?: () => Promise, ): Promise { let middlewareIndex = 0; @@ -30,7 +30,7 @@ export async function processMiddleware( } if (betweenPhases !== undefined) { - return betweenPhases(context); + return betweenPhases(); } } From 9a74f9e8eb57711f014d274442e3112cbddcdcaa Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Thu, 26 Mar 2020 14:57:10 -0700 Subject: [PATCH 4/7] Adds tests for multiple listener error behavior, fixes bugs --- src/App.spec.ts | 67 +++++++++++++++++++------ src/App.ts | 90 ++++++++++++++++++---------------- src/conversation-store.spec.ts | 4 +- src/conversation-store.ts | 3 +- src/middleware/builtin.spec.ts | 6 +-- src/middleware/builtin.ts | 39 ++++++++++----- src/middleware/process.ts | 26 +++++----- src/types/middleware.ts | 21 ++++---- 8 files changed, 157 insertions(+), 99 deletions(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index ad04612ac..e499485da 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -5,7 +5,7 @@ import { assert } from 'chai'; import { Override, mergeOverrides, createFakeLogger, delay } from './test-helpers'; import rewiremock from 'rewiremock'; import { ErrorCode, UnknownError } from './errors'; -import { Receiver, ReceiverEvent, SayFn, NextMiddleware } from './types'; +import { Receiver, ReceiverEvent, SayFn, NextFn } from './types'; import { ConversationStore } from './conversation-store'; import { LogLevel } from '@slack/logger'; import App, { ViewConstraints } from './App'; @@ -14,16 +14,17 @@ import { WebClientOptions, WebClient } from '@slack/web-api'; // TODO: swap out rewiremock for proxyquire to see if it saves execution time // Utility functions const noop = (() => Promise.resolve(undefined)); -const noopMiddleware = async ({ next }: { next: NextMiddleware; }) => { await next(); }; +const noopMiddleware = async ({ next }: { next: NextFn; }) => { await next!(); }; const noopAuthorize = (() => Promise.resolve({})); // Dummies (values that have no real behavior but pass through the system opaquely) -function createDummyReceiverEvent(): ReceiverEvent { +function createDummyReceiverEvent(type: string = 'dummy_event_type'): ReceiverEvent { // NOTE: this is a degenerate ReceiverEvent that would successfully pass through the App. it happens to look like a // IncomingEventType.Event return { body: { event: { + type, }, }, ack: noop, @@ -342,11 +343,13 @@ describe('App', () => { // Arrange app.use(fakeFirstMiddleware); app.use(async ({ next }) => { - await next(); - await next(); + await next!(); + await next!(); }); app.use(fakeSecondMiddleware); app.error(fakeErrorHandler); + + // Act await fakeReceiver.sendEvent(dummyReceiverEvent); // Assert @@ -360,7 +363,7 @@ describe('App', () => { await delay(100); changed = true; - await next(); + await next!(); }); await fakeReceiver.sendEvent(dummyReceiverEvent); @@ -374,7 +377,7 @@ describe('App', () => { app.use(async ({ next }) => { try { - await next(); + await next!(); } catch (err) { caughtError = err; } @@ -397,15 +400,17 @@ describe('App', () => { let middlewareCount = 0; /** - * Middleware that, when calls, asserts that it was called in the correct order - * @param orderDown The order it should be called when processing middleware down the stack - * @param orderUp The order it should be called when processing middleware up the stack + * Middleware that, when called, asserts that it was called in the correct order + * @param orderDown The order it should be called when processing middleware down the chain + * @param orderUp The order it should be called when processing middleware up the chain */ - const assertOrderMiddleware = (orderDown: number, orderUp: number) => async ({ next }: any) => { + const assertOrderMiddleware = (orderDown: number, orderUp: number) => async ({ next }: { next?: NextFn }) => { await delay(100); middlewareCount += 1; assert.equal(middlewareCount, orderDown); - await next(); + if (next !== undefined) { + await next(); + } middlewareCount += 1; assert.equal(middlewareCount, orderUp); }; @@ -465,6 +470,36 @@ describe('App', () => { }); }); + describe('listener middleware', () => { + it('should aggregate multiple errors in listeners for the same incoming event', async () => { + // Arrange + const App = await importApp(); // tslint:disable-line:variable-name + const app = new App({ + receiver: fakeReceiver, + authorize: sinon.fake.resolves(dummyAuthorizationResult), + }); + const eventType = 'some_event_type'; + const dummyReceiverEvent = createDummyReceiverEvent(eventType); + app.error(fakeErrorHandler); + const errorsToThrow = [new Error('first listener error'), new Error('second listener error')]; + function createThrowingListener(toBeThrown: Error): () => Promise { + return async () => { throw toBeThrown; }; + } + + // Act + app.event('some_event_type', createThrowingListener(errorsToThrow[0])); + app.event('some_event_type', createThrowingListener(errorsToThrow[1])); + await fakeReceiver.sendEvent(dummyReceiverEvent); + + // Assert + assert(fakeErrorHandler.calledOnce); + const error = fakeErrorHandler.firstCall.args[0]; + assert.instanceOf(error, Error); + assert(error.code === ErrorCode.MultipleListenerError); + assert.sameMembers(error.originals, errorsToThrow); + }); + }); + describe('middleware and listener arguments', () => { let fakeErrorHandler: SinonSpy; const dummyChannelId = 'CHANNEL_ID'; @@ -655,7 +690,7 @@ describe('App', () => { app.use(async ({ next }) => { await ackFn(); - await next(); + await next!(); }); app.shortcut({ callback_id: 'message_action_callback_id' }, async ({ }) => { await shortcutFn(); }); app.shortcut( @@ -799,7 +834,7 @@ describe('App', () => { }); app.use(async ({ logger, body, next }) => { logger.info(body); - await next(); + await next!(); }); app.event('app_home_opened', async ({ logger, event }) => { @@ -847,7 +882,7 @@ describe('App', () => { }); app.use(async ({ logger, body, next }) => { logger.info(body); - await next(); + await next!(); }); app.event('app_home_opened', async ({ logger, event }) => { @@ -913,7 +948,7 @@ describe('App', () => { }); app.use(async ({ client, next }) => { await client.auth.test(); - await next(); + await next!(); }); const clients: WebClient[] = []; app.event('app_home_opened', async ({ client }) => { diff --git a/src/App.ts b/src/App.ts index 8e2e2cea9..a71a920a7 100644 --- a/src/App.ts +++ b/src/App.ts @@ -55,7 +55,7 @@ import { AppInitializationError, MultipleListenerError, } from './errors'; -import promiseAllsettled, { PromiseRejection } from 'promise.allsettled'; +import allSettled = require('promise.allsettled'); // tslint:disable-line:no-require-imports import-name const packageJson = require('../package.json'); // tslint:disable-line:no-require-imports no-var-requires /** App initialization options */ @@ -156,13 +156,13 @@ export default class App { /** Logger */ private logger: Logger; - /** */ + /** Authorize */ private authorize: Authorize; - /** Global middleware */ + /** Global middleware chain */ private middleware: Middleware[]; - /** Listeners (and their middleware) */ + /** Listener middleware chains */ private listeners: Middleware[][]; private errorHandler: ErrorHandler; @@ -593,46 +593,50 @@ export default class App { } // Dispatch even through the global middleware chain - return processMiddleware( - this.middleware, - listenerArgs as AnyMiddlewareArgs, - context, - client, - this.logger, - async () => { - // Dispatch the event through the listener middleware chains and aggregate their results - // TODO: change the name of this.middleware and this.listeners to help this make more sense - const listenerResults = this.listeners.map(async (origListenerMiddleware) => { - // Copy the array so modifications don't affect the original - const listenerMiddleware = [...origListenerMiddleware]; - - // Don't process the last item in the listenerMiddleware array - it shouldn't get a next fn - const listener = listenerMiddleware.pop(); - - if (listener !== undefined) { - return processMiddleware( - listenerMiddleware, - listenerArgs as AnyMiddlewareArgs, - context, - client, - this.logger, - async () => - // When the listener middleware chain is done processing, call the listener without a next fn - listener({ ...listenerArgs as AnyMiddlewareArgs, context, client, logger: this.logger }), - ); + try { + await processMiddleware( + this.middleware, + listenerArgs as AnyMiddlewareArgs, + context, + client, + this.logger, + async () => { + // Dispatch the event through the listener middleware chains and aggregate their results + // TODO: change the name of this.middleware and this.listeners to help this make more sense + const listenerResults = this.listeners.map(async (origListenerMiddleware) => { + // Copy the array so modifications don't affect the original + const listenerMiddleware = [...origListenerMiddleware]; + + // Don't process the last item in the listenerMiddleware array - it shouldn't get a next fn + const listener = listenerMiddleware.pop(); + + if (listener !== undefined) { + return processMiddleware( + listenerMiddleware, + listenerArgs as AnyMiddlewareArgs, + context, + client, + this.logger, + async () => + // When the listener middleware chain is done processing, call the listener without a next fn + listener({ ...listenerArgs as AnyMiddlewareArgs, context, client, logger: this.logger }), + ); + } + }); + + const settledListenerResults = await allSettled(listenerResults); + const rejectedListenerResults = + settledListenerResults.filter(lr => lr.status === 'rejected') as allSettled.PromiseRejection[]; + if (rejectedListenerResults.length === 1) { + throw rejectedListenerResults[0]; + } else if (rejectedListenerResults.length > 1) { + throw new MultipleListenerError(rejectedListenerResults.map(rlr => rlr.reason)); } - }); - - const settledListenerResults = await promiseAllsettled(listenerResults); - const rejectedListenerResults = - settledListenerResults.filter(lr => lr.status === 'rejected') as PromiseRejection[]; - if (rejectedListenerResults.length === 1) { - throw rejectedListenerResults[0]; - } else if (rejectedListenerResults.length > 1) { - throw new MultipleListenerError(rejectedListenerResults.map(rlr => rlr.reason)); - } - }, - ); + }, + ); + } catch (error) { + return this.handleError(error); + } } // TODO: make the following method private if its no longer being used by Receiver diff --git a/src/conversation-store.spec.ts b/src/conversation-store.spec.ts index 09419fee0..40f235ff9 100644 --- a/src/conversation-store.spec.ts +++ b/src/conversation-store.spec.ts @@ -5,7 +5,7 @@ import sinon, { SinonSpy } from 'sinon'; import { Override, createFakeLogger, delay, wrapToResolveOnFirstCall } from './test-helpers'; import rewiremock from 'rewiremock'; import { ConversationStore } from './conversation-store'; -import { AnyMiddlewareArgs, NextMiddleware, Context } from './types'; +import { AnyMiddlewareArgs, NextFn, Context } from './types'; import { WebClient } from '@slack/web-api'; import { Logger } from '@slack/logger'; @@ -193,7 +193,7 @@ describe('MemoryStore', () => { /* Testing Harness */ type MiddlewareArgs = AnyMiddlewareArgs & { - next: NextMiddleware, + next: NextFn, context: Context, logger: Logger, client: WebClient, diff --git a/src/conversation-store.ts b/src/conversation-store.ts index 6cae539cb..0a1454f26 100644 --- a/src/conversation-store.ts +++ b/src/conversation-store.ts @@ -73,7 +73,8 @@ export function conversationContext( .catch(error => logger.debug(error.message)); } else { logger.debug('No conversation ID for incoming event'); - await next(); + // TODO: remove the non-null assertion operator + await next!(); } }; } diff --git a/src/middleware/builtin.spec.ts b/src/middleware/builtin.spec.ts index 9ec486cff..800679230 100644 --- a/src/middleware/builtin.spec.ts +++ b/src/middleware/builtin.spec.ts @@ -7,7 +7,7 @@ import { Override, wrapToResolveOnFirstCall, createFakeLogger } from '../test-he import rewiremock from 'rewiremock'; import { SlackEventMiddlewareArgs, - NextMiddleware, + NextFn, Context, MessageEvent, SlackCommandMiddlewareArgs, @@ -468,7 +468,7 @@ describe('matchCommandName', () => { const logger = createFakeLogger(); const client = new WebClient(undefined, { logger, slackApiUrl: undefined }); - function buildArgs(fakeNext: NextMiddleware): SlackCommandMiddlewareArgs & MiddlewareCommonArgs { + function buildArgs(fakeNext: NextFn): SlackCommandMiddlewareArgs & MiddlewareCommonArgs { const payload: SlashCommand = { ...validCommandPayload }; return { payload, @@ -652,7 +652,7 @@ interface DummyContext { } interface MiddlewareCommonArgs { - next: NextMiddleware; + next: NextFn; context: Context; logger: Logger; client: WebClient; diff --git a/src/middleware/builtin.ts b/src/middleware/builtin.ts index d1feeb05b..655ffa7c3 100644 --- a/src/middleware/builtin.ts +++ b/src/middleware/builtin.ts @@ -34,7 +34,8 @@ export const onlyActions: Middleware } // It matches so we should continue down this middleware listener chain - await next(); + // TODO: remove the non-null assertion operator + await next!(); }; /** @@ -101,7 +106,8 @@ export const onlyViewActions: Middleware { } // If all the previous checks didn't skip this message, then its okay to resume to next - await args.next(); + // TODO: remove the non-null assertion operator + await args.next!(); }; } export function subtype(subtype: string): Middleware> { return async ({ message, next }) => { if (message.subtype === subtype) { - await next(); + // TODO: remove the non-null assertion operator + await next!(); } }; } @@ -335,7 +347,8 @@ export function directMention(): Middleware> return; } - await next(); + // TODO: remove the non-null assertion operator + await next!(); }; } diff --git a/src/middleware/process.ts b/src/middleware/process.ts index 507e2fa28..280aa83d5 100644 --- a/src/middleware/process.ts +++ b/src/middleware/process.ts @@ -12,27 +12,29 @@ export async function processMiddleware( context: Context, client: WebClient, logger: Logger, - betweenPhases?: () => Promise, + last: () => Promise = async () => { return; }, ): Promise { - let middlewareIndex = 0; + let lastCalledMiddlewareIndex = -1; - async function invokeCurrentMiddleware(): ReturnType> { - if (middlewareIndex !== middleware.length) { - const result = await middleware[middlewareIndex]({ - next: invokeCurrentMiddleware, + async function invokeMiddleware(toCallMiddlewareIndex: number): ReturnType> { + if (lastCalledMiddlewareIndex >= toCallMiddlewareIndex) { + // TODO: use a coded error + throw Error('next() called multiple times'); + } + + if (toCallMiddlewareIndex < middleware.length) { + lastCalledMiddlewareIndex = toCallMiddlewareIndex; + return middleware[toCallMiddlewareIndex]({ + next: () => invokeMiddleware(toCallMiddlewareIndex + 1), ...initialArgs, context, client, logger, }); - middlewareIndex += 1; - return result; } - if (betweenPhases !== undefined) { - return betweenPhases(); - } + return last(); } - return invokeCurrentMiddleware(); + return invokeMiddleware(0); } diff --git a/src/types/middleware.ts b/src/types/middleware.ts index 3e39e604c..373f5e227 100644 --- a/src/types/middleware.ts +++ b/src/types/middleware.ts @@ -8,23 +8,26 @@ import { SlackViewMiddlewareArgs } from './view'; import { WebClient } from '@slack/web-api'; import { Logger } from '@slack/logger'; +// TODO: rename this to AnyListenerArgs, and all the constituent types export type AnyMiddlewareArgs = SlackEventMiddlewareArgs | SlackActionMiddlewareArgs | SlackCommandMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs | SlackShortcutMiddlewareArgs; -export interface Context extends StringIndexed { +interface AllMiddlewareArgs { + context: Context; + logger: Logger; + client: WebClient; + // TODO: figure out how to make next non-optional + next?: NextFn; } // NOTE: Args should extend AnyMiddlewareArgs, but because of contravariance for function types, including that as a // constraint would mess up the interface of App#event(), App#message(), etc. export interface Middleware { - // TODO: is there something nice we can do to get context's property types to flow from one middleware to the next? - (args: Args & { - next?: NextMiddleware, - context: Context, - logger: Logger, - client: WebClient, - }): Promise; + (args: Args & AllMiddlewareArgs): Promise; +} + +export interface Context extends StringIndexed { } -export type NextMiddleware = () => Promise; +export type NextFn = () => Promise; From 13cbd883518b46b633fe97cfbf641bc5e0194d55 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Thu, 26 Mar 2020 15:28:09 -0700 Subject: [PATCH 5/7] fix errors in integration tests --- integration-tests/types/action.ts | 10 +++++----- integration-tests/types/shortcut.ts | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/integration-tests/types/action.ts b/integration-tests/types/action.ts index 7b5088345..fb72e673d 100644 --- a/integration-tests/types/action.ts +++ b/integration-tests/types/action.ts @@ -5,27 +5,27 @@ const app = new App({ token: 'TOKEN', signingSecret: 'Signing Secret' }); // calling action method with incorrect an type constraint value should not work // $ExpectError -app.action({ type: 'Something wrong' }, ({ action }) => { - return action; +app.action({ type: 'Something wrong' }, async ({ action }) => { + await Promise.resolve(action); }); // $ExpectType void app.action({ type: 'block_actions' }, async ({ action, // $ExpectType BlockElementAction }) => { - return action; + await Promise.resolve(action); }); // $ExpectType void app.action({ type: 'interactive_message' }, async ({ action, // $ExpectType InteractiveAction }) => { - return action; + await Promise.resolve(action); }); // $ExpectType void app.action({ type: 'dialog_submission' }, async ({ action, // $ExpectType DialogSubmitAction }) => { - return action; + await Promise.resolve(action); }); diff --git a/integration-tests/types/shortcut.ts b/integration-tests/types/shortcut.ts index 0963cb700..73632c7f6 100644 --- a/integration-tests/types/shortcut.ts +++ b/integration-tests/types/shortcut.ts @@ -5,8 +5,8 @@ const app = new App({ token: 'TOKEN', signingSecret: 'Signing Secret' }); // calling shortcut method with incorrect an type constraint value should not work // $ExpectError -app.shortcut({ type: 'Something wrong' }, ({ shortcut }) => { - return shortcut; +app.shortcut({ type: 'Something wrong' }, async ({ shortcut }) => { + await Promise.resolve(shortcut); }); // Shortcut in listener should be - MessageShortcut @@ -14,7 +14,7 @@ app.shortcut({ type: 'Something wrong' }, ({ shortcut }) => { app.shortcut({ type: 'message_action' }, async ({ shortcut, // $ExpectType MessageShortcut }) => { - return shortcut; + await Promise.resolve(shortcut); }); // If shortcut is parameterized with MessageShortcut, shortcut argument in callback should be type MessageShortcut @@ -22,5 +22,5 @@ app.shortcut({ type: 'message_action' }, async ({ app.shortcut({}, async ({ shortcut, // $ExpectType MessageShortcut }) => { - return shortcut; + await Promise.resolve(shortcut); }); From b7b7607bd5f3600fd070450aab3875e448e5953d Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Thu, 26 Mar 2020 16:57:39 -0700 Subject: [PATCH 6/7] increase test coverage, fix bug with single listener errors --- src/App.spec.ts | 34 +++++++++++++++++++++++++++------- src/App.ts | 2 +- src/middleware/process.ts | 2 +- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index 59f568bfc..2bdf98427 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -474,24 +474,44 @@ describe('App', () => { }); describe('listener middleware', () => { - it('should aggregate multiple errors in listeners for the same incoming event', async () => { - // Arrange + let app: App; + const eventType = 'some_event_type'; + const dummyReceiverEvent = createDummyReceiverEvent(eventType); + + beforeEach(async () => { const App = await importApp(); // tslint:disable-line:variable-name - const app = new App({ + app = new App({ receiver: fakeReceiver, authorize: sinon.fake.resolves(dummyAuthorizationResult), }); - const eventType = 'some_event_type'; - const dummyReceiverEvent = createDummyReceiverEvent(eventType); app.error(fakeErrorHandler); + }); + + it('should bubble up errors in listeners to the global error handler', async () => { + // Arrange + const errorToThrow = new Error('listener error'); + + // Act + app.event(eventType, async () => { throw errorToThrow; }); + await fakeReceiver.sendEvent(dummyReceiverEvent); + + // Assert + assert(fakeErrorHandler.calledOnce); + const error = fakeErrorHandler.firstCall.args[0]; + assert.equal(error.code, ErrorCode.UnknownError); + assert.equal(error.original, errorToThrow); + }); + + it('should aggregate multiple errors in listeners for the same incoming event', async () => { + // Arrange const errorsToThrow = [new Error('first listener error'), new Error('second listener error')]; function createThrowingListener(toBeThrown: Error): () => Promise { return async () => { throw toBeThrown; }; } // Act - app.event('some_event_type', createThrowingListener(errorsToThrow[0])); - app.event('some_event_type', createThrowingListener(errorsToThrow[1])); + app.event(eventType, createThrowingListener(errorsToThrow[0])); + app.event(eventType, createThrowingListener(errorsToThrow[1])); await fakeReceiver.sendEvent(dummyReceiverEvent); // Assert diff --git a/src/App.ts b/src/App.ts index a71a920a7..73a1d65e4 100644 --- a/src/App.ts +++ b/src/App.ts @@ -628,7 +628,7 @@ export default class App { const rejectedListenerResults = settledListenerResults.filter(lr => lr.status === 'rejected') as allSettled.PromiseRejection[]; if (rejectedListenerResults.length === 1) { - throw rejectedListenerResults[0]; + throw rejectedListenerResults[0].reason; } else if (rejectedListenerResults.length > 1) { throw new MultipleListenerError(rejectedListenerResults.map(rlr => rlr.reason)); } diff --git a/src/middleware/process.ts b/src/middleware/process.ts index 280aa83d5..3ed778884 100644 --- a/src/middleware/process.ts +++ b/src/middleware/process.ts @@ -12,7 +12,7 @@ export async function processMiddleware( context: Context, client: WebClient, logger: Logger, - last: () => Promise = async () => { return; }, + last: () => Promise, ): Promise { let lastCalledMiddlewareIndex = -1; From a39828fbcb258279ddd40fcd012120746ee9d46a Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Thu, 26 Mar 2020 17:22:57 -0700 Subject: [PATCH 7/7] take a penny, leave a penny: moar coverage --- src/App.spec.ts | 19 +++++++++++++++++++ src/App.ts | 4 ++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/App.spec.ts b/src/App.spec.ts index 2bdf98427..aee109e35 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -1012,6 +1012,25 @@ describe('App', () => { assert.notEqual(clients[0], clients[1]); assert.strictEqual(clients[0], clients[2]); }); + + it('should be to the global app client when authorization doesn\'t produce a token', async () => { + // Arrange + const App = await importApp(); // tslint:disable-line:variable-name + const app = new App({ + receiver: fakeReceiver, + authorize: noopAuthorize, + ignoreSelf: false, + }); + const globalClient = app.client; + + // Act + let clientArg: WebClient | undefined; + app.use(async ({ client }) => { clientArg = client; }); + await fakeReceiver.sendEvent(createDummyReceiverEvent()); + + // Assert + assert.equal(globalClient, clientArg); + }); }); describe('say()', () => { diff --git a/src/App.ts b/src/App.ts index 73a1d65e4..c299e2e1e 100644 --- a/src/App.ts +++ b/src/App.ts @@ -584,9 +584,9 @@ export default class App { // Get the client arg let client = this.client; const token = selectToken(context); - if (typeof token !== 'undefined') { + if (token !== undefined) { let pool = this.clients[source.teamId]; - if (typeof pool === 'undefined') { + if (pool === undefined) { pool = this.clients[source.teamId] = new WebClientPool(); } client = pool.getOrCreate(token, this.clientOptions);