Botbuilder-dotnet: DCR: Adapter/Bot initialization is confusing and boundary between middleware and bot is fuzzy

[tomlm]

Feedback from hackathons shows that there is friction between the way the middleware pipeline is hooked up, how it is consumed from ASP.NET and how your bot logic hooks up to it.

Example of the issue

1. Initialization: there is both an adapter and a bot, you have to pass the adapter into the bot, but keep the adapter around so you can push an activity into the adapter which goes to the bot. This really confusing and causes us to describe 2 concepts (bots and adapters) and the complex handshake between them
2. state Some adapters need to keep track of stuff for the entire turn, but since they are not middleware and technically don't know about the bot it creates weird tracking metaphors
3. BotLogic we call the middleware class Bot and have .OnReceive() as a piece of middleware, which pushes people into the direction of thinking of the entire thing as a bot. This blurs the boundary between the bot's logic and stuff which is more environmental like setting up a runtime middleware.

Proposal

This proposal is implemented in pull request #145

1. Rename middleware class from Bot to BotAdapter
   Renaming the class better describes it's functionality, which is to create act as abstraction between the bot and the runtime environment and to provide middleware hooks for running a bot in. It also then creates a mental model where you don't push all sorts of bot logic stuff into the middleware. While you can of course do whatever you want in middleware, the naming makes it a bit more clear that it's about the pipeline, not the bot itself.

2. Combine Adapter and Bot together using inheritance
   Instead of having 2 concepts, merge them together into 1 base class called a BotAdapter. _(I am open to other names, and like BotHost)_ To support a new environment you create a class which is derived from the base BotAdapter class which then implements appropriate methods to adapt to communication with the environment. The base class gives you the standard pipeline implementation.
   All BotAdapters would have at least 4 public methods

3. .Use() for registering middleware using fluent pattern
4. ProcessActivity(callback) which has appropriate parameters for environment but then calls the bot logic

5. CreateConversation() and ContinueConversation() which allows programs to do proactive scenarios
   program to create a new conversation

6. Remove .OnReceive()
   don't use .OnReceive() as middleware, but instead pass bot's logic as a simple (BotContext) callback

Sample

The net result in a ASP.NET Core project is that it looks like this:
Startup.cs
```C#
// define BotAdapter as singleton
services.AddSingleton(serviceProvider =>
{
return new BotFrameworkAdapter(Configuration)
.Use(new BotStateManager(new MemoryStorage()));
});

MessagesController.cs
```C#
    [Route("api/[controller]")]
    public class MessagesController : Controller
    {
        public BotFrameworkAdapter botAdapter = null;

        public MessagesController(BotFrameworkAdapter botAdapter)
        {
             this.botAdapter = botAdapter;
       }

       [HttpPost]
        public async Task<IActionResult> Post([FromBody]Activity activity)
        {
            try
            {
                await botAdapter.ProcessActivity(this.Request.Headers["Authorization"].FirstOrDefault(), 
                    activity, 
                     async (context) =>
                    {
                        if (context.Request.Type == ActivityTypes.Message)
                        {
                              context.Reply($"echo: {context.Request.AsMessageActivity().Text}");
                        }
                     });
                return this.Ok();
            }
            catch (UnauthorizedAccessException)
            {
                return this.Unauthorized();
            }
        }
}

Here's console adapter sample
```C#
class Program
{
static void Main(string[] args)
{
MainAsync(args).GetAwaiter().GetResult();
}

    static async Task MainAsync(string[] args)
    {
        Console.WriteLine("Welcome to the EchoBot.");
        var adapter = new ConsoleAdapter()
                .Use(new BotStateManager(new MemoryStorage()));

        await adapter.ProcessActivity(async (context) =>
        {
            if (context.Request.Type == ActivityTypes.Message)
            {
                context.Reply($"echo: {context.Request.AsMessageActivity().Text}");
            }
        });
    }
}

```

_NOTE: This DCR originally was coded using BotServer as base class. That led to anems like BotFrameworkBotServer and didn't really capture what was going on. Based on feedback from Ccastro I changed this to BotAdapter and everything fell into place._

_Update2: Added console sample_

[sidecus]

I started playing around the new sdk over the past few days. And I found .OnReceive confusing. A natural idea is to register the bot as a singleton service, but it becomes embarrassing where to hook up OnReceive. If it's set as part of the service, it becomes hard to pass request related info to the callback. If it's set in the controller, it creates issue if multiple requests come in at the same time.

So I like the idea a lot to remove OnReceive - it will make things much simpler and more robust.
Thanks @tomlm .

[billba]

I think it would be instructive to show two sets of samples - one for the BotFramework adapter and one for the Console adapter. I'm worried that the design might be too HTTP-centric.

[tomlm]

I updated with console sample

[billba]

I'll ask the dumb-question-from-the-guy-who-hasn't-coded-C#-in-eight-years: does it make sense that those two samples look so different?

What I want - and again this comes from a totally naïve place - is that the "setup bot middleware" code and "bot logic" code would look identical in those two samples, hiding the details of where activities are coming from and going to. I'd like to be able to switch from Console to BotFramework with only relevant code changes.

(This isn't a spurious desire - we have solid feedback that developers like writing and testing bots with Console, switching to BotFramework only when they need to test rich messages.)

[tomlm]

They are identical. Console apps don't use dependency injection or any of that stuff.
Here is the adapter initialized in asp.net using BotframeworkAdapter in startup.cs:
```C#
return new BotFrameworkAdapter(Configuration)
.Use(new BotStateManager(new MemoryStorage()));

Here is the adapter initialized in program.cs
```C#
  var consoleAdapter = new ConsoleAdapter()
                    .Use(new BotStateManager(new MemoryStorage()));

Here is the consumption in asp.net core
``` C#
await botAdapter.ProcessActivity(this.Request.Headers["Authorization"].FirstOrDefault(),
activity,
async (context) =>
{
if (context.Request.Type == ActivityTypes.Message)
{
context.Reply($"echo: {context.Request.AsMessageActivity().Text}");
}
});

here is in the console app:
``` C#
await consoleAdapter.ProcessActivity(async (context) =>
            {
                if (context.Request.Type == ActivityTypes.Message)
                {
                    context.Reply($"echo: {context.Request.AsMessageActivity().Text}");
                }
            });

The differences are because of the environment differences:

• Http Server has to check the authorization of the caller and has the activity from an external HTTP POST, and so has 2 additional parameters
• Console starts listening to console.Stdin, and calls callback on each activity.

I don't really know how to make them more similar...they differ by 2 parameters which only make sense when participating as part of HTTP protocol.

You could in theory use dependency injection, but it would be adding MORE code that nobody does when coding console apps.

[cmayomsft]

I agree that renaming Bot to BotAdapter, combining Adapter and Bot together, and removing OnReceive() feel like good changes. The similarities between console and ASP.NET feel good as well.

But, beyond that, this doesn't feel like the ASP.NET Web API programming model.

In that model, services are passed to the controller through constructor injection, and the controller uses those services to create the response for each request.

In this case, the BotFrameworkAdapter is passed to the controller's constructor, and when the HTTPPost request comes in to api/messages, the response is created by passing a callback to botAdapter.ProcessActivity().

       [HttpPost]
        public async Task<IActionResult> Post([FromBody]Activity activity)
        {
            try
            {
                await botAdapter.ProcessActivity(this.Request.Headers["Authorization"].FirstOrDefault(), 
                    activity, 
                     async (context) =>
                    {
                        if (context.Request.Type == ActivityTypes.Message)
                        {
                              context.Reply($"echo: {context.Request.AsMessageActivity().Text}");
                        }
                     });
                return this.Ok();
            }
            catch (UnauthorizedAccessException)
            {
                return this.Unauthorized();
            }
        }

Of the code above, this code is the code that feels like responding to the request posted to api/messages is this:

                        if (context.Request.Type == ActivityTypes.Message)
                        {
                              context.Reply($"echo: {context.Request.AsMessageActivity().Text}");
                        }

In one of the samples, a BotController was introduced to provide a bridge between the controller and the adapter's concept of a BotContext that represents the request/response of that "turn".

        protected override Task OnReceiveActivity(IBotContext context)
        {
            if (context.Request.Type == ActivityTypes.Message)
            {
                context.Reply($"You said: '{ context.Request.AsMessageActivity().Text }'");

                return Task.CompletedTask;
            }
        }

I realize this code has OnReceiveActivity(), but my point is that this bridges the gap between how Web API Controllers work and the "service" that BotAdapter provides by providing a BotContext for the "turn" that allows the developer to create a response to the request.

That feels like the piece that's missing for me.

[brandonh-msft]

I think to satisfy what @cmayomsft is saying it's straightforward enough to simply not use a lambda. Most sufficiently intelligent bots won't be created in this way, so it's probably best to steer folks to using a method in this place instead of a lambda, and then it becomes a lot like what was seen in the BotController Chris alludes to, going from this:

[HttpPost]
public async Task<IActionResult> Post([FromBody]Activity activity)
{
    try
    {
        await botAdapter.ProcessActivity(this.Request.Headers["Authorization"].FirstOrDefault(), 
            activity, 
             async (context) =>
            {
                if (context.Request.Type == ActivityTypes.Message)
                {
                      context.Reply($"echo: {context.Request.AsMessageActivity().Text}");
                }
             });
        return this.Ok();
    }
    catch (UnauthorizedAccessException)
    {
        return this.Unauthorized();
    }
}

to this:

[HttpPost]
public async Task<IActionResult> Post([FromBody]Activity activity)
{
    try
    {
        await botAdapter.ProcessActivity(this.Request.Headers["Authorization"].FirstOrDefault(), activity, OnReceiveActivity);
        return this.Ok();
    }
    catch (UnauthorizedAccessException)
    {
        return this.Unauthorized();
    }
}

private Task OnReceiveActivity(IBotContext context)
{
    if (context.Request.Type == ActivityTypes.Message)
    {
        context.Reply($"You said: '{ context.Request.AsMessageActivity().Text }'");

        return Task.CompletedTask;
    }
}

while I still dislike the amount of boilerplate needed around ProcessActivity, this is a step in the right direction for making the handling of bot messages when they come in as straightforward as it was in v3 and as it is in node (where the adapter is the endpoint)

[brandonh-msft]

overall I like the feel of what's going on, but the naming is leaving a bad taste.

It seems like BotFrameworkAdapter lends itself toward a lower-level abstraction than what it's being used as here so I think some more thought needs go to in to this. In particular because it's the fact that this particular bot is talking to the Microsoft Bot Framework's Connector Service that necessitates the header requirement (among other things) where other "heads" for bots do not (ie: ConsoleAdapter).

Whatever naming is decided upon, though, it should be consistent across the surfaces. eg: If we go with BotHost, then I'd like to see ConsoleHost, etc.

[sidecus]

@brandonh-msft the two code fragments seem identical to me. The bot owner should have the freedom to decide whether they want to use a lambda, or a named function. It doesn't seem to be something which botframwork itself has to worry about.

Seems to me the key is that the callback HAS to be associated directly with one specific ProcessActivity call - it cannot and shall not be a callback property set in a non-transactional way. Otherwise you end up with security issues since you might have a call back belonging to a one user context responding to a different call.

Maybe a more natural way is to avoid the call back completely - just do necessary processing, return a context from the ProcessActivity, then let the user construct their reply with the context - they should handle returning of the reply (this.Ok(reply)) instead of just this.Ok().

I don't have enough understanding about the botframework code yet so apologies if I am just non-sensing here.

[sidecus]

To be clear, think about this scenario with bot itself as singleton service:
user request1 -> new controller instance1
-> set callback property (callback1) on bot (singleton)
user request2 -> new controller instance2
-> set callback property (callback2) on bot (singleton), which overrides the previous one.
-> callback2 invoked to respond to user request1
-> callback2 invoked to respond to user request2

This is a security drawback which should be avoided by the framework itself.

A lot of the sample codes actually have this issue.

[tomlm]

The Adapter patterns we are showing here are platform agnostic, in that there is no dependency on a given Http framework like ASP.NET/Core

A BotController for ASP.NET/Core which looks like
```C#
public class MessagesController : BotController
{
public MessagesController(BotFrameworkAdapter adapter) : base(adapter)
{
}

public Task<HttpResponseMessage> OnActivity(IBotContext context)   
{
     ....
}

}
```
is easy super easy enough to add, but it does introduce a ASP.NET layer that hides that it's the same framework across all languages. We have in the past taken the tack of not adding layers which hide stuff (which I am rewriting the AlarmBot) so that it is transparent what is going on.

I could entertain the idea of

• Microsoft.Bot.Builder-Core -> Core classes
• Microsoft.Bot.Builder -> Http based classes which are server agnostic (work for HttpListener and Asp.Net and Asp.Net core)
• Microsoft.Bot.Builder.AspNetCore -> Adds ASP.NET Core BotController class
• Microsoft.Bot.Builder.AspNet -> Addds ASP.NET BotController class

[brandonh-msft]

I really like that proposal for nuget pkgs. Not only does it showcase the versatility of the framework simply by showing up, but the implementations will provide a tailored experience. There's also precedent w/ the Microsoft.AspNet/AspNetCore pkgs.

Just need to put emphasis on the pkg descriptions to lead developers to success.

[tomlm]

This has been checked in with https://github.com/Microsoft/botbuilder-dotnet/pull/145