Botbuilder-dotnet: BotState clear should clear any durable state

[johnataylor]

In the C# implementation the bot state is replaced (with a new CachedBotState) rather than cleared and so the cache state will not be saved that looks like a bug.

However the new DI changes (as of merged in yesterday) for the Integration layer seems to makes plugging the state in _more awkward_ not less. The state is needed to set up the options _and_ to create the bot. But the options are set in a callback that doesn't get handed the DI service provider. This needs more work.

And note the TurnState collection is next to useless because the object it contains is private and this has caused confusion. To add to this confusion the TurnContext _is_ handed to the error handler. So despite the "god object" nature of the TurnContext it still manages to not have all the objects you actually need.

The JavaScript looks OK in terms of the Clear implementation. But the fact that everything is public isn't really making things simpler longer term. The samples need to just Clear (with the await.)

[johnataylor]

@drub0y can you comment. I think you had a pattern in mind to at least get rid of the accessors pattern in the samples. Anyhow, in the mean time I have been experimenting with the botFactory and the options callbacks - the problem is I need the state in both. So the question is where is the best place to create the state now.

@sgellock see comments regarding JavaScript. You should just be able to call Clear and it should work. I need to do a little more testing before we update samples across the board.

[drub0y]

In the C# implementation the bot state is replaced (with a new CachedBotState) rather than cleared and so the cache state will not be saved that looks like a bug.

Yes, that is what we observed as well. So, I want to be restate what I _believe_ is being proposed here to make sure we're on the same page:

You're proposing that a call to BotState::ClearStateAsync should result in loading any latent state entries in that scope, walk through each entry and clear each _value_ such that a subsequent call to SaveAllChanges will end up deleting those values from the backing store.

That right? If so, that would be a breaking change from the existing behavior which simply clears any state that has been loaded from the cache such that any read that might follow would cause the latest state to be read from the backing store again, thuse getting fresh values. The only way to actually remove state from the backing store is an explicit call to BotState::DeletePropertyValueAsync or, more directly, IStatePropertyAccessor<T>::DeleteAsync.

Let me say that I support both behaviors, but perhaps the current behavior of BotState::ClearStateAsync should be moved to a new method BotState::ClearCachedState (non-async... it doesn't need to be) and then the new implementation of actually deleting all state in the scope could then be moved into BotState::ClearStateAsync.

Again, this would technically qualify as a breaking change, albeit internal, so this should be considered strongly, but I would think it's the best thing to do and I think it's reasonable to do in a minor version.

However the new DI changes (as of merged in yesterday) for the Integration layer seems to makes plugging the state in more awkward not less. The state is needed to set up the options and to create the bot. But the options are set in a callback that doesn't get handed the DI service provider. This needs more work.

And note the TurnState collection is next to useless because the object it contains is private and this has caused confusion. To add to this confusion the TurnContext is handed to the error handler. So despite the "god object" nature of the TurnContext it still manages to not have all the objects you actually need.

I only somewhat follow this, but it seems tangential to the topic at hand (clearing vs resetting state), right?

However, yes, as far as the whole XXXAccessors pattern and DI registration, I plan to PR you guys a cleaner version of what that could look like now that we have the new factory methods and we need to decide on a solution to #1164. So maybe we can just table this part of the conversation for now and resume it on that PR once I get it in?

[johnataylor]

Yes @drub0y two topics somewhat conflated here but related.

1. The behavior of BotState. Just to revisiting the goal: @vishwacsena @sgellock @Stevenic intended the "clear" method to mean "clear all state both cache _and_ durable" where the "durable" aspect is deferred until the execution of the autosavemiddleware. The aim is to call this method from the default implementation of the OnTurnError handler. So I think the C# just clears the cache. And I think the JavaScript does that and clears on save. (I'm verifying this morning.) Agreed this is change of behavior _but its a bug in the C#_ and the method was not intended to apply to the cache only. Though i guess its not ridiculous to add an extra method if we really thought it necessary.

2. The DI in the integration layer is still quite awkward. _We should work out want the clean pattern is_. It's OK if that involves adding more overloads or modifying the last one you added which we haven't shipped yet. It would be great to flesh out asap how we want Startup.cs to look.

Having said that, we are seeing that a bunch of people will just go with building a regular MVC application and, naturally, we want to also make sure things are good for them. The minimum that means is keeping HTTP hosting dependencies out of the Adapter which we are doing. Beyond that we not providing much help, other than showing that it is simple and easy to go that way if that's what fits your needs.

[drub0y]

Ok, so I agree we need to iterate a bit and arrive at something more palatable for 2. Let's continue to follow up on that separately and keep this focused on 1. Fair?

For 1, do we agree that separate methods are needed so that you can explicitly clear the cache (i.e. the functionality there today), independent of actually clearing actual persisted values? If so here's a PR that does that: #1169

[sgellock]

Once we land on an approach and agree on behavior, we need to ensure we're doing the same thing correctly on JS. If so, then can @johnataylor open an issue in the js repo?

[johnataylor]

@drub0y yes absolutely, lets focus on 1 for starters.

OK actually drilled into this a little further the situation is really not too bad. And we think the behavior is definitely a bug, it is not intended nor expected behavior, according to @stevenic who designed this.

Basically in the following:

                    options.OnTurnError = async (turnContext, exception) =>
                    {
                        await conversationState.ClearStateAsync(turnContext);
                        await conversationState.SaveChangesAsync(turnContext, true);
                    };

The problem is that you _must_ add the true (forced) to the SaveChanges in this scenario. This makes no sense as clearing the state surely must be interpreted as a change if there was actually something there. The bug is that this isn't the case but only because of the slightly careless way it's implemented.

The good news is the behavior is actually the same in both JavaScript and C#. That was a surprise because we thought the JavaScript was good, but I just tested this and it isn't.

_So the plan is to fix the bug._ We can discuss the need for additional methods as a DCR. In fact @stevenic is promising to write a DCR to simplify the overall behavior but it would be a change in the desired behavior too.

It is also worth taking into account the implication demonstrated in sample 42 that its pretty straight forward to build your own state solution all up if you happen to have sightly different requirements.

[Stevenic]

So throwing out my 2 cents... I don't know that we need separate methods for clearing the cache versus clearing the backing object... In general we'd like the cache to be hidden from the caller and if anything I'd re-consider even having the force flag.

As I was digging into this I realized that the bigger issue here is that we don't expose any direct way letting the developer delete the states backing storage object so as you call clearStateAsync() everywhere you're just leaving empty objects in your store. I've submitted a DCR which proposes that we add a BotState.deleteAsync() method and that this is what we start recommending to developers that they call.

[johnataylor]

fix for the current clear is checked in now. @stevenic DCR to add a delete is still active and we are hoping to get that in for 4.2.