Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

Using TranscriptLoggerMiddleware breaks Bot on Azure Infrastructure #139

Merged

Conversation

matthiasblaesing
Copy link
Contributor

The TranscriptLoggerMiddleware works on the Emulator, but fails when run
on Azure Infrastructure. This is caused by the invalid modification of
the incoming Activity. While outgoing activities are cloned before they
are modified, the incoming activity is modified in place.

The modifications then create invalid data leading to this JSON fragment
for reply activities:

{
    // ...
    "recipient":{
        "properties":{
            "role":{}
        },
        "id":"8d51e790-8bd3-4450-af3d-1741838d1354",
        "role":{}
    },
    // ...
}

The role key in the properties subobject should not be there and while
the second role entry should indeed be set, it should be set to a
string, not an object. This results in a http 400 status from the bot
framework:

{
  "error": {
    "code": "BadSyntax",
    "message": "Invalid or missing Activity in request"
  }
}

While adding the role information to the from property of the activity
is convenient, it should not modify the incoming activity.

To fix this, the incoming activity is cloned before it is modified, as
the outgoing activity also lacks the role information, the update of the
role attribute is applied to both types. In addition the correct
atttribute is updated for the role information.

The TranscriptLoggerMiddleware works on the Emulator, but fails when run
on Azure Infrastructure. This is caused by the invalid modification of
the incoming Activity. While outgoing activities are cloned before they
are modified, the incoming activity is modified in place.

The modifications then create invalid data leading to this JSON fragment
for reply activities:

{
    // ...
    "recipient":{
        "properties":{
            "role":{}
        },
        "id":"8d51e790-8bd3-4450-af3d-1741838d1354",
        "role":{}
    },
    // ...
}

The role key in the properties subobject should not be there and while
the second role entry should indeed be set, it should be set to a
string, not an object. This results in a http 400 status from the bot
framework:

{
  "error": {
    "code": "BadSyntax",
    "message": "Invalid or missing Activity in request"
  }
}

While adding the role information to the from property of the activity
is convenient, it should not modify the incoming activity.

To fix this, the incoming activity is cloned before it is modified, as
the outgoing activity also lacks the role information, the update of the
role attribute is applied to both types. In addition the correct
atttribute is updated for the role information.
@tracyboehrer
Copy link
Member

@matthiasblaesing Thanks Matthias. I'll need to research this a bit before I can merge. Since it was ported from one of our other Bot SDK's, we need to verify if it was an error in the port, or something deeper. Clearly something is broken. An object instead of a string appears to be a port error.

@matthiasblaesing
Copy link
Contributor Author

matthiasblaesing commented Oct 31, 2019

@tracyboehrer you mean this should be the reference?

https://github.com/microsoft/botbuilder-dotnet/blob/master/libraries/Microsoft.Bot.Builder/TranscriptLoggerMiddleware.cs

Yeah the C# implementation looks saner than the (original) java version - the transliteration should be straight forward. If you prefer to keep the two implementation feature/bug equals, I can live with it.

@tracyboehrer tracyboehrer merged commit e5dcc60 into microsoft:master Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants