Botbuilder-dotnet: Remove BotFrameworkOptions.State property

[sgellock]

Version

4.1.x

Describe the bug

The BotFrameworkOptions.State property only seems to exist so you can add BotState instances to it in the AddBot configuration callback and then get access to them later in other registrations (e.g. bot accessor pattern). This concept doesn't exist in JS/TS because there the variable containing the BotState instance is simply closed over in other functions. The same thing should be done in the .NET case rather than having added a property.

Additional context

The BotFrameworkOptions::State property seems like a solution to a problem that doesn't even exist. Nothing in the rest of the framework will look at this property and it is only used at start up time by the samples today and seems to be the result of someone not leveraging simple language level features to solve this problem. Instead of adding BotState instances to this property in AddBot and then looking those instances back up in other AddXXX calls later, the developer should simply hoist the variable up into the ConfigureServices scope and then close over it in the various AddXXX callbacks.

The property should be removed to simplify the surface area and all the samples/templates should be updated to use the suggested approach.

Also, what is the impact of fixing this on:

• Docs - do we need to address any doc issues>
• Samples - are there samples implications
• VSIX - anything to cover here

[bug]

[drub0y]

So, this obviously would be a breaking API change and, since we adhere to semver, we should not do that. My recommendation mentioned "removing" it, but really that has to mean _obsoleting_ it.

First, the property should be marked with the ObsoleteAttribute along with an explaination why it's being obsoleted it and what people should do instead. Additionally the property should be marked with the EditorBrowseableAttribute with a value of EditorBrowsableState.Never. This will hide it from intellisense to hide it from people.

Once that's done, it's really about updating all sample code... both in botbuilder-samples and the docs. The pattern to eliminate the use of the State property from the current startup pattern is simply to hoist the ConversationState instance up into the ConfigureServices method itself so it can be used both when calling AddBot and the AddSingleton call for the accessors pattern.

This would look something like the following:

Startup.cs

public void ConfigureServices(IServiceCollection services)
{
    var dataStore = new MemoryStorage();
    var conversationState = new ConversationState(dataStore);

    services.AddBot<EchoBotWithCounter>(options => 
    {
        // Use conversationState here if you needed it
    });

    services.AddSingleton<EchoBotAccessors>(sp =>
        new EchoBotAccessors
        { 
            CounterState = conversationState.CreateProperty<CounterState>(EchoBotAccessors.CounterStateName),
        });
}

Now, I want to be clear that I actually think the entire accessors pattern needs to go to. So, maybe we want to have that discussion at the same time so we don't go rewriting the sample code _N_ times over. There's no reason for this extra "accessors" class overhead. We can just register the IStatePropertyAccessor<T> instances directly and they would be injected automatically into the TBot's constructor which would essentially call for whichever one(s) it wants. This only falls apart if multiple IStatePropertyAccessor<T> of the same T are registered (e.g. two different state properties both of type int or string or Foo) and then what we need is overloads to AddBot<TBot> that adds ability to supply the factory method which would then give explicit control over which IStatePropertyAccessor<T> is chosen.

Thus I would also suggest adding these additional extension method overloads to the integration layer in ServiceCollectionExtensions.cs:

ServiceCollectionExtensions.cs

public static IServiceCollection AddBot<TBot>(this IServiceCollection services, Func<IServiceProvider, TBot> botFactory)
            where TBot : class, IBot

public static IServiceCollection AddBot<TBot>(this IServiceCollection services, Action<BotFrameworkOptions> configure, Func<IServiceProvider, TBot> botFactory)
            where TBot : class, IBot

I have this all of this work already prototyped out and working. If we agree this is a path we want to go down I can polish it up, start the PR process around it and get the documentation written for the new extensions.

[drub0y]

PR in for obsoleting the property; I'll make a separate one for the proposed IServiceCollection extensions.

[johnataylor]

Regarding the obsolete - yes exactly - the assumption was that the implementation of "remove" is technically to mark as "obsolete" as @cleemullins pointed out.

Regarding the overall pattern(s) for adding state to your bot. This is definitely worth more discussion.

We just added a new sample scaleout that demonstrates how to implement an alternatively state solution with different locking semantics from the ground up. Actually the only thing this shares with "default" solution in the framework is the derivation of the conversation key and the fact that you need some type of Accessor implementation to pass a handle on the state into the dialogs. There has been some discussion about simplifying this last step. Anyhow we have a write up of this solution too that covers some of the concepts used eTags.

[micdah]

Maybe the documentation over at https://docs.microsoft.com/en-us/azure/bot-service/bot-builder-tutorial-persist-user-inputs?view=azure-bot-service-4.0&tabs=csharp#create-storage-state-manager-and-state-property-accessor-objects should be updated with the new pattern?

It's confusing, following a guide and encountering obsolete warnings.