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

DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions #3918

Merged
merged 4 commits into from
May 13, 2020

Conversation

carlosscastro
Copy link
Member

@carlosscastro carlosscastro commented May 13, 2020

Fixes #3913

The Add() method of DialogSet has a mechanism for supporting multiple dialogs with the same id, which is adding a unique suffix for internal book keeping.

There is a bug however, if the same exact dialog is added multiple times because it is referenced by multiple dialogs. In this scenario, this is not a name collision, we are just adding the same dialog. But still we are assigning a different Id and treating it as different which brings downstream incorrect behaviors.

The fix here is to more precisely detect whether this is a name collision, by checking whether the dialog that is being added, was already added. If that is the case, we don't assign a new id and just leave things as they are.

Note that this is not a declarative only bug or even an adaptive only bug. But in adaptive code, without declarative, something like the below would also fail without this fix:

var d1 = new AdaptiveDialog();
var d2 = new AdaptiveDialog();

Triggers = new List<OnCondition>()
{
    new OnIntent() 
    {
    	Intent = "i1",
      Actions = new Dialog[] {d1};
    },
    new OnIntent() 
    {
    	Intent = "i2",
      Actions = new Dialog[] {d2};
    },
    new OnIntent() 
    {
    	Intent = "i3",
      // eventually this will re-add d1 to the dialog set, and alter the id to 'd12'
      // Then when i1 triggers, it will look for d1 and not find it in the dialog set
      Actions = new Dialog[] {d1}; 
    }
}

In addition to the repro test, added another cancellation test to verify other scenraios where instances look the same but are expected to behave differently.

Did a quick performance analysis over 500 medium sized json files (500 lines) and saw no measurable difference in performance. The clone is as efficient as the previous deserialization so its a no change in net time spent loading resources.

@carlosscastro carlosscastro requested a review from tomlm May 13, 2020 18:10
@carlosscastro carlosscastro marked this pull request as ready for review May 13, 2020 19:21
@carlosscastro carlosscastro changed the title DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions [Do not merge, verifying] DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions May 13, 2020
@carlosscastro carlosscastro changed the title [Do not merge, verifying] DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions [DO NOT MERGE, verifying] DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions May 13, 2020
@carlosscastro carlosscastro changed the title [DO NOT MERGE, verifying] DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions May 13, 2020
Copy link
Contributor

@tomlm tomlm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@carlosscastro carlosscastro changed the title DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions [DO NOT MERGE, VERIFYING] DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions May 13, 2020
… non-loops, don't clone on loops - just stitch things together.
@carlosscastro carlosscastro changed the title [DO NOT MERGE, VERIFYING] DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions DialogSet.Add: Unique dialog ids for name collisions but not for reference collisions May 13, 2020
@tomlm
Copy link
Contributor

tomlm commented May 13, 2020

I approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants