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.

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

IStatePropertyAccessor can not be injected through DI in 45.state-management sample #1705

Closed
mutanttech opened this issue Aug 14, 2019 · 15 comments
Assignees
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. customer-reported Issue is created by anyone that is not a collaborator in the repository.

Comments

@mutanttech
Copy link

mutanttech commented Aug 14, 2019

Sample information

  1. Sample type: State Management
  2. Sample language: dotnetcore
  3. Sample name: 45.state-management

Describe the bug

The Conversation State and User State are creating property inside bot and using the value.
In case same property needs to be updated for a separate dialog, then will it require Creating the Property Again.
Dont seem to find a way to achieve this using DI.

To Reproduce

Steps to reproduce the behavior:

  1. Try to add a dialog and access the PromptedUserForName property inside it without using CreateProperty method
  2. Try to add IStatePropertyAccessor through DI and it fails -
System.ArgumentException
  HResult=0x80070057
  Message=Cannot instantiate implementation type 'Microsoft.Bot.Builder.IStatePropertyAccessor`1[ChatBot.Classic.App.Models.ConversationStateDataModel]' for service type 'Microsoft.Bot.Builder.IStatePropertyAccessor`1[ChatBot.Classic.App.Models.ConversationStateDataModel]'.
  Source=Microsoft.Extensions.DependencyInjection
  StackTrace:
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.Populate(IEnumerable`1 descriptors)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory..ctor(IEnumerable`1 descriptors)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine..ctor(IEnumerable`1 serviceDescriptors, IServiceProviderEngineCallback callback)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CompiledServiceProviderEngine..ctor(IEnumerable`1 serviceDescriptors, IServiceProviderEngineCallback callback)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider..ctor(IEnumerable`1 serviceDescriptors, ServiceProviderOptions options)
   at Microsoft.Extensions.DependencyInjection.ServiceCollectionContainerBuilderExtensions.BuildServiceProvider(IServiceCollection services, ServiceProviderOptions options)
   at Microsoft.Extensions.DependencyInjection.ServiceCollectionContainerBuilderExtensions.BuildServiceProvider(IServiceCollection services)
   at Microsoft.AspNetCore.Hosting.Internal.WebHost.Initialize()
   at Microsoft.AspNetCore.Hosting.WebHostBuilder.Build()
   at ChatBot.Classic.App.Program.Main(String[] args) in C:\Projects\TPGit\ChatBot\src\ChatBot.Classic.App\Program.cs:line 10

Expected behavior

DI should allow injecting the IStatePropertyAccessor and the same Conversation Property should be accessed and updated from different dialogs.

Screenshots

None

Additional context

Update the sample to include the context of multiple dialogs accessing the state and updating.

@CoHealer CoHealer added Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-reported Issue is created by anyone that is not a collaborator in the repository. labels Aug 14, 2019
@sgellock sgellock added customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. Support labels Aug 15, 2019
@sgellock sgellock assigned CoHealer and unassigned johnataylor Aug 15, 2019
@sgellock
Copy link
Member

@mutanttech I'm asking our support team to help you.

@CoHealer
Copy link

@mdrichardson, please attempt a repro and if found investigate an update for the sample to include the context of multiple dialogs accessing the state and updating.

@mdrichardson
Copy link
Contributor

@mutanttech Can you provide a code sample of what you're attempting to do? It's a little unclear.

@mutanttech
Copy link
Author

@mdrichardson, I am just trying to move this code in Startup.cs
var conversationStateAccessors = _ConversationState.CreateProperty<ConversationStateDataModel>(nameof(ConversationStateDataModel));

And Instead of Injecting ConversationState in the dialog I want to inject - IStatePropertyAccessor

A very simple repro for you would be to add a new dialog in the example, then the PromptedforUserName property should be accessed in the new dialog. The main point would be how you acquire the StateAccessor in Dialog.

Using Below Lines inside a Dialog Step or something else.

var conversationStateAccessors = _ConversationState.CreateProperty<ConversationStateDataModel>(nameof(ConversationStateDataModel)); var conversationData = await conversationStateAccessors.GetAsync(stepContext.Context, () => new ConversationStateDataModel());

@mdrichardson
Copy link
Contributor

@mutanttech Understood. I wasn't able to DI from Startup.cs either. I'm working on a mockup sample to show how to create the accessor once in MyBot.cs and pass it to dialogs as an argument. Will let you know when its done

@mdrichardson
Copy link
Contributor

@mutanttech

I actually won't be writing up a sample; our old samples used to do this (although not through DI).

  1. Take your dialogs out of Startup.cs DIs

  2. Create the accessor in MyBot.cs constructor

var conversationStateAccessors = _ConversationState.CreateProperty<ConversationStateDataModel>(nameof(ConversationStateDataModel));
  1. Add your dialogs in the MyBot.cs constructor and pass in the accessor
Dialogs.Add(new MyDialog(conversationStateAccessors, loggerFactory));
  1. Use the accessor in a dialog
var conversationData = await conversationStateAccessors.GetAsync(stepContext.Context, () => new ConversationStateDataModel());

@mdrichardson
Copy link
Contributor

mdrichardson commented Aug 16, 2019

@sgellock @johnataylor

I don't see that we have a current sample that shows how to address this. Would it be worth writing one up? I'm not sure if there's a better way than my suggestion that uses the newer changes to the SDK like using DI for dialogs and using the DialogExtensions

...or am I just missing something? Like @mutanttech, I tried to use DI but encountered the same error.

@mdrichardson
Copy link
Contributor

mdrichardson commented Aug 16, 2019

@mutanttech I played around with this some more and you actually can use DI. Basic steps:

  1. Create a class container for your accessors. Something like:
using Microsoft.Bot.Builder;
using Microsoft.BotBuilderSamples;

namespace Microsoft.BotBuilderSamples
{
    public class StateAccessors
    {
        public IStatePropertyAccessor<ConversationData> ConversationStateAccessors { get; }
        public IStatePropertyAccessor<UserProfile> UserStateAccessors { get; }

        public StateAccessors(ConversationState conversationState, UserState userState)
        {
            ConversationStateAccessors = conversationState.CreateProperty<ConversationData>(nameof(ConversationData));
            UserStateAccessors = userState.CreateProperty<UserProfile>(nameof(UserProfile));
        }
    }
}
  1. DI your Accessors and Dialogs
services.AddSingleton<StateAccessors>();

services.AddSingleton<RootDialog>();
services.AddSingleton<ChildDialog>();

// Create the bot as a transient. In this case the ASP Controller is expecting an IBot.
services.AddTransient<IBot, StateManagementBot<RootDialog>>();
  1. Have your MyBot.cs call your Root Dialog like the samples do
public class MyBot<T> : ActivityHandler
        where T : Dialog
[...]
public StateManagementBot(ConversationState conversationState, UserState userState, StateAccessors stateAccessors, T dialog)
        {
            _conversationState = conversationState;
            _userState = userState;
            Dialog = dialog;
[...]
await Dialog.RunAsync(turnContext, _conversationState.CreateProperty<DialogState>(nameof(DialogState)), cancellationToken);
  1. Have your Root Dialog constructor take your StateAccessors as an argument along with any Child dialogs and be sure to Add them
public RootDialog(StateAccessors stateAccessors, ChildDialog childDialog)
[...]
AddDialog(childDialog);
  1. Ensure your Child Dialogs take your StateAccessors as an argument
public ChildDialog(StateAccessors stateAccessors)

I think some of our older samples used this method, as well. I'm not sure if this is the best method or not. @johnataylor: If you could chime in on this, that would be great.

@mutanttech
Copy link
Author

@mdrichardson, Thanks for putting up some serious thought here. I stumbled upon few old threads not related to this just yesterday. They were having similar idea. I will wait for @johnataylor comments.

@CoHealer
Copy link

@johnataylor. please let us know if you need anything else on our end, otherwise I think this is a good point for the support team to hand off to you so we can free @mdrichardson up for other issues.

@daveta
Copy link
Contributor

daveta commented Aug 29, 2019

We're still looking at this - @johnataylor will comment..

@fcapallera
Copy link

@mdrichardson

There's this thing I fail to understand. If you want to make your RootDialog receive the StateAccessor object you need to call the RootDialog constructor.

But where is the constructor called in the samples? I only see the RunAsync method with this new BotState property that's created (that I don't know why it is used for either).

I would like to DI the accessors to be able to get the properties in all the dialogs that are called from the RootDialog but I don't know where the RootDialog constructor is called. What am I missing?

@mdrichardson
Copy link
Contributor

mdrichardson commented Aug 30, 2019

@fcapallera I'm far from an expert in Dependency Injection, but my basic understanding is that Dependency Injection in Startup.cs makes a class/service available as an argument anywhere within the project. So, when we use:

services.AddSingleton<RootDialog>();

...it makes RootDialog available anywhere in the bot. When we then use

services.AddTransient<IBot, StateManagementBot<RootDialog>>();

...we're passing the RootDialog to MyBot:

public class MyBot<T> : ActivityHandler
        where T : Dialog
[...]
public StateManagementBot(ConversationState conversationState, UserState userState, StateAccessors stateAccessors, T dialog)

...because we're setting it as T with StateManagementBot<RootDialog> and it then becomes dialog in the constructor parameter, and Dialog with Dialog = dialog;.

We don't need to pass anything to the constructor of RootDialog because its constructor takes these parameters:

public RootDialog(StateAccessors stateAccessors, ChildDialog childDialog)

...and we've also Dependency Injected those with:

services.AddSingleton<StateAccessors>();
services.AddSingleton<ChildDialog>();

@tsuwandy
Copy link
Contributor

tsuwandy commented Sep 9, 2019

@johnataylor, are you still looking into this? Could you update this thread?

@johnataylor
Copy link
Member

Please check out the pattern we have in the samples. For example take a look at Core Bot sample 13.

https://github.com/microsoft/BotBuilder-Samples/blob/master/samples/csharp_dotnetcore/13.core-bot/Startup.cs

Here you can see the BotState objects injected - it is OK to make these singleton because the calls that are made that need to be scoped to the current request are parameterized with TurnContext. This model is a little different than the basic ASP and has had the implication that all the DI objects are in general singleton. (Its OK to have the Bot transient - its used in the transient Controller.)

The pattern we follow is to defer the creation of the accessor until we need it - this seems to hide the inherent noise the most effectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. customer-reported Issue is created by anyone that is not a collaborator in the repository.
Projects
None yet
Development

No branches or pull requests

8 participants