Botbuilder-dotnet: Conversation restart doesn't work

[brandonh-msft]

Channel: Skype
nupkg v: alpha-m3.1

I have a long-running operation my bot kicks off. As such, I send to my endpoint a serialized version of ConversationReference like so:

await _client.PostAsync([my endpoint],
    new StringContent(JsonConvert.SerializeObject(
        new
        {
            callback = new
            {
                url = @"http://mybot.azurewebsites.net/api/callback",
                conversation = TurnContext.GetConversationReference(turn.Activity)
            }
        }),
    Encoding.Default, @"application/json"));

and I've added another controller to my bot like so:

[Route("api/Callback")]
public class CallbackController : Controller
{
    private readonly IConfiguration _config;
    private readonly TelemetryClient _logger = new TelemetryClient();

    private static BotFrameworkAdapter _adapter = null;

    public CallbackController(IConfiguration configuration)
    {
        _config = configuration;

        if (_adapter == null)
        {
            _adapter = new BotFrameworkAdapter(new ConfigurationCredentialProvider(configuration));
        }
    }

For my POST handler, I have the following:

[HttpPost]
public IActionResult Post()
{
    dynamic request = JToken.ReadFrom(new JsonTextReader(new StreamReader(this.Request.Body)));

    var activity = Activity.CreateMessageActivity() as Activity;

    var reference = ((JObject)request.reference).ToObject<ConversationReference>();
    TurnContext.ApplyConversationReference(activity, reference);

    new TurnContext(_adapter, activity)
        .SendActivity(@"All done!");

    return NoContent();
}

but, despite no exceptions in deserializing back in to a ConversationReference and all the properties populated successfully, no message is sent back to my user on Skype.

[brandonh-msft]

I also tried this:

    var reference = ((JObject)request.reference).ToObject<ConversationReference>();
    _adapter.ContinueConversation(reference, HandlerAsync);

    return NoContent();
}

private async Task HandlerAsync(ITurnContext turn)
{
    await turn.SendActivity(@"All done!");
}

with the same results

[brandonh-msft]

Turned on Break on All Exceptions and ran it again (2nd approach in use). There's a nullref exception being thrown in the bowels:

~
System.NullReferenceException
HResult=0x80004003
Message=Object reference not set to an instance of an object.
Source=
StackTrace:
at Microsoft.Bot.Builder.Adapters.BotFrameworkAdapter.d__10.MoveNext()
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at
Microsoft.Bot.Builder.TurnContext.<>c__DisplayClass22_0.<g__ActuallySendStuff|1>d.MoveNext()
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at Microsoft.Bot.Builder.TurnContext.d__26.MoveNext()
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at Microsoft.Bot.Builder.TurnContext.d__22.MoveNext()
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at Microsoft.Bot.Builder.TurnContext.d__21.MoveNext()
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
~

look a lot like this

[drub0y]

Oh you're doing proactive, there's a known bug (#314) there right now that's fixed in #461.

[drub0y]

I've decoupled this fix from the other work in #461, now the PR we're waiting for is #468.

[drub0y]

@brandonh-msft #468 was merged in earlier this morning. There are no packages published for it, but if you build locally and reference the local packages you can try out the fix and see if it's working for you now. At bare minimum I know that specific NullReferenceException will be gone. I was also able to get a contrived test scenario working locally against the emulator in which I was able to send a proactive message back using ContinueConversation from a ConversationReference much like you laid out above. I would definitely suggest using the direct _adapter.ContinueConversation approach though, it doesn't make sense for you to spin up an activity + TurnContext for this scenario.

[brandonh-msft]

Doesn't fix the problem. I built the latest master source, published the packages to a VSTS instance, and pulled them down. I use them, ship the bot to Azure, and get a proactive message. At the point where I do turn.SendActivity() it blows up with:

~~~
Microsoft.Bot.Connector.Authentication.MicrosoftAppCredentials.OAuthException
HResult=0x80131500
Message=Unauthorized
Source=
StackTrace:
at Microsoft.Bot.Connector.Authentication.MicrosoftAppCredentials.d__30.MoveNext() in D:\Github\Microsoft\botbuilder-dotnet\libraries\Microsoft.Bot.Connector\Authentication\MicrosoftAppCredentials.cs:line 203
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()

Inner Exception 1:
HttpRequestException: Response status code does not indicate success: 401 (Unauthorized).
~~~

mind you this is the same bot instance and I'm injecting the same IConfiguration and using it to build the _botAdapter in the controller that receives the proactive message. So the credentials _are_ valid.

Problem appears to lie in TurnContext.cs or something feeding it. It's pulling the wrong credentials:

The appid shown there is my bot's "name" in the chat, not the appid from my configuration vars. and, as you can see, the password is null.

This code on line 120 of BotFrameworkAdapter.cs is the reason for the bad id:

        public override Task ContinueConversation(ConversationReference reference, Func<ITurnContext, Task> callback) =>
            ContinueConversation(reference.Bot.Id, reference, callback);

the ID being passed in is the ID of the bot. The ID that you want there is the botAppId (as in the appid from its portal registration).
This erroneous appId is then used to look up the claim later in GetAppCredentialsAsync and ends up finding nothing and creating a [botid,null] pair as the creds.

The good news is I solved this by simply using the other overload like so:

_adapter.ContinueConversation(_config[@"MicrosoftAppId"], reference, HandlerAsync);

which likely means the problematic one needs to be removed from the API completely as it doesn't appear to be possible to infer the appid from just a conversationReference object & a callback.

[cleemullins]

@brandonh-msft Thanks for the bug report, and the detailed analyis. I'll look into removing the problematic overload completly. I want to sync with @Stevenic to make sure we mirror that behavior in Node.

[cleemullins]

Fixed / Merged in PR #483

[drub0y]

Removing the overload does not seem like the correct solution to this and has now regressed #314. The default overload of BotAdapter::ContinueConversation needs to be overridden in BotFrameworkAdapter or else it won't have a chance to inject the services into the TurnContext that it relies on later. So, while I understand I used the wrong value in the implementation (bot id vs bot _app_ id), we need to overload it to support the scenario from #314 where the caller is in the midst of a turn and decides they want to do something proactive:

Imagine MyBot.cs...

public Task OnTurn(ITurnContext turnContext)
{
    // received some activity that triggers some proactive conversation
    await turnContext.Adapter.ContinueConversation(
        someConversationReference,
        continuedTurnContext =>
        {
             await continuedTurnContext.SendActivity("hello from proactive");
        });
}

Let's consider what's happening here: a dev has written a bot that's supposedly agnostic of the adapter it's hosted in and thus, by design, TurnContext::Adapter is of type BotAdapter. Given that, the only overload the developer would ever have available to them would be the base overload that _does not_ take a botAppId.

Now, they could obviously cast the Adapter to BotFramworkAdapter and then call the overload with botAppId, but, for this specific scenario, I would argue that in the case above we could assume that whatever "app" was in place in the original turn is the one they want to use to make the proactive calls and that would be a reasonable default the majority of the time not requiring people to go that extra step further. I can definitely solve that problem and make it work based on the "current" app identity, but I'd love some confirmation of my thinking here before submitting a new PR. If this _doesn't_ make sense, then it's possible the entire abstraction is flawed and needs to be revisited.

Ok, so... that's not even @brandonh-msft's scenario, right? In his case he actually is working with the BotFrameworkAdapter directly, so he absolutely could call the overload directly, though he does have to discover this himself. The fact is, I don't actually think he (or any developer) needs to be doing this themselves. This is what PR #461 is about enabling. It would allow devs to submit event activities from "the outside" to which the bot can could react and it already allows passing the botAppId via an HTTP header or on the querystring and the developer would not even need to be concerned with the adapter instance at all.

[johnataylor]

The adapter / bot has been heavily refactored a couple of times since this thread was active.
We think you should be able to achieve what you want to with the current factoring. Possibly there are some patterns that will emerge and we will want to first class in the core framework. However, for this version we are locking down.