Cannot write intercepting middleware

[brandonh-msft]

Due to PR #118, it appears we can no longer write middleware that "intercepts" a message from the user (ie: doesn't call await next())

The easiest example is:

public class Interceptor : IMiddleware, IReceiveActivity
{
    public Task ReceiveActivity(IBotContext context, MiddlewareSet.NextDelegate next) => Task.CompletedTask;
}

because the above Middleware implementation doesn't call await next(), the hosting Bot's OnReceive should never be hit; however it will be.

cc @cleemullins @billba @ryanvolum

[ryanvolum]

@brandonh-msft and I have been writing the same middleware in both SDKs.

In the node SDK, this works to intercept messages:

export class TestMiddleware implements Middleware {
    public async receiveActivity(context: BotContext, next: () => Promise<void>): Promise<void> {
    }
}

As expected, not returning next effectively keeps the message from propagating through the rest of the middleware/bot logic, so a bot that uses this middleware is completely unresponsive.

[cleemullins]

You can write Intercepting Middleware that manipulates the incoming Activity. You just can't currently PREVENT OnReceive from being called.

This is by design. The design may be incorrect, and if so we can change it. I'm open to how folks think this should work.

Current behavior is:

1. Middleware is run. If Middleware calls next(), then then goto 1 until we're all done.
2. Once All Receive Middlware is run OnReceive is called.

If I understand correctly you are asserting the following should be true:

• If ALL Receive Middleware is run, and ALL of it calls next(), then (and only then) should OnReceive be called.

Is that accurate?

I'm not clear on what the behavior should be, as I can make a case either way.

[cleemullins]

After a quick consult with @tomlm @Stevenic they both believe (as do you) that the following behavior should be what we have:

• If ALL Receive Middleware is run, and ALL of it calls next(), then (and only then) should OnReceive be called.

[brandonh-msft]

right; and that matches up with what the Node SDK is currently doing as @ryanvolum highlighted.

[RyanDawkins]

By intercept messages do you mean prevent them from being sent to the user? @brandonh-msft

[cleemullins]

@brandonh-msft I'll probably get this taken care of late tonight or tomorrow.

[brandonh-msft]

@RyanDawkins I mean you get a message from the user and your Middleware can do something that usurps the rest of the dialog flow of the bot (ie: everything that would've run as defined in OnReceive)

[ryanvolum]

@RyanDawkins This also means that the message doesn't flow through the following middleware of that pipe. This is to say that if you don't return next in receiveActivity, no other receiveActivity middleware will be called, BUT postActivity middleware will be called.

[RyanDawkins]

One way to prevent postActivity from returning back is to do IList.clear() on the activities parameter. That is how we are stopping messages from going to the user so we can make a list of messages delay as if the user is typing.

[ryanvolum]

That seems fine, though more generally not returning next should stop propagation through that pipe or the onReceive function

[Stevenic]

@RyanDawkins we actually have a 'delay' activity that will let you insert a delay between activities.

@ryanvolum definitely agree that next() should stop propagation. One work around for now would be to look at the responses queue and just no-op your bots OnReceive() handler if there are already responses queued up.

[nrobert]

@Stevenic , about the workaround you mentioned in the previous message: isn't there a problem with the QnAMaker middleware by doing this? As this middleware is setting responses if there is a match in the QnA

[Stevenic]

@nrobert I haven't looked at the QnA Maker middleware implementation but i'm assuming it's currently designed to run on the leading edge of routing which is problematic in general. QnA Maker can be a bit aggressive about answering queries so more often than not you'd want to run it as a fallback if nothing else answers. With our latest middleware design you can do that but it looks like C# is currently missing a key addition that I made to the JS SDK to enable this.

You can code QnA Maker to run on the trailing edge of routing which means it will run after everything else has run but you need a way of knowing if the bot has already replied or not. You could look at the responses queue but that's not super reliable as it can get flushed (and it's going away soon) so on the JS side I added a read-only context.responded property you can check to see if any responses have been sent during the turn (it would be a great PR to add that to the C# side.) Using this flag QnA Maker could conditionally decide to run only when nothing else has sent a message.

[cleemullins]

This is fixed by PR #146.

Tests around intercepting middleware have also been added into the test suite.
Test showing OnReceive slated to run.
Test showing OnReceive running when the Middlware pipeline is empty.
Test #1 showing an Intercepting Middleware.
Test #2 showing an Intercepting Middlware.