From 81be624cddc7071a8b2a7409b2ed774812008257 Mon Sep 17 00:00:00 2001 From: "Hongyang Du (hond)" Date: Thu, 24 Sep 2020 09:17:41 +0800 Subject: [PATCH] Make cycle detection throw an exception instead of returning null. (#4606) * init * fix ci * throw a more specific exception type * fix error * fix * Resource explorer: bundle 'allow cycles' into ResourceExplorerOptions to prevent overload explosion due to binary compat requirements Co-authored-by: carlosscastro --- .../Converters/DialogExpressionConverter.cs | 4 ++ .../Observers/CycleDetectionObserver.cs | 17 ++++++++- .../Resources/ResourceExplorer.cs | 33 ++++++++++++++-- .../Resources/ResourceExplorerOptions.cs | 38 +++++++++++++++++++ .../JsonLoadTests.cs | 21 +++++++++- .../ResourceExplorerFixture.cs | 7 ++++ 6 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorerOptions.cs diff --git a/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Memory/Expressions/Converters/DialogExpressionConverter.cs b/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Memory/Expressions/Converters/DialogExpressionConverter.cs index cf2737bf5..c4ae2eba7 100644 --- a/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Memory/Expressions/Converters/DialogExpressionConverter.cs +++ b/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Memory/Expressions/Converters/DialogExpressionConverter.cs @@ -71,6 +71,10 @@ namespace Microsoft.Bot.Builder.Dialogs.Adaptive.Converters result = new DialogExpression((Dialog)converter.ReadJson(jsonTextReader, objectType, existingValue, serializer)); } } + catch (InvalidOperationException) + { + throw; + } #pragma warning disable CA1031 // Do not catch general exception types (return an empty if an exception happens). catch (Exception) #pragma warning restore CA1031 // Do not catch general exception types diff --git a/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Observers/CycleDetectionObserver.cs b/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Observers/CycleDetectionObserver.cs index a85e0dfa0..67007efd5 100644 --- a/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Observers/CycleDetectionObserver.cs +++ b/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Observers/CycleDetectionObserver.cs @@ -19,8 +19,18 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Observers /// internal class CycleDetectionObserver : IJsonLoadObserver { + private readonly bool allowCycle; private readonly Dictionary cache = new Dictionary(); - + + /// + /// Initializes a new instance of the class. + /// + /// If allowCycle is set to false, throw an exception when detecting cycle. + public CycleDetectionObserver(bool allowCycle = true) + { + this.allowCycle = allowCycle; + } + /// /// Gets or sets the current pass of the algorithm. /// @@ -49,6 +59,11 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Observers // If the same source range appears twice in the stack, we have a cycle. var isCycle = context.CallStack.Count(s => s.Equals(range)) > 1; + if (isCycle && !allowCycle) + { + throw new InvalidOperationException($"Cycle detected for range: {range}"); + } + if (isCycle && CycleDetectionPass == CycleDetectionPasses.PassOne) { // If we found a cycle, stop iterating and set the value to null in pass 1. diff --git a/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorer.cs b/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorer.cs index b08e51a0f..423c2a593 100644 --- a/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorer.cs +++ b/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorer.cs @@ -29,6 +29,8 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources private readonly ConcurrentDictionary kindDeserializers = new ConcurrentDictionary(); private readonly ConcurrentDictionary kindToType = new ConcurrentDictionary(); private readonly ConcurrentDictionary> typeToKinds = new ConcurrentDictionary>(); + private readonly ResourceExplorerOptions options; + private List resourceProviders = new List(); private List declarativeTypes; private CancellationTokenSource cancelReloadToken = new CancellationTokenSource(); @@ -42,6 +44,7 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources /// Initializes a new instance of the class. /// public ResourceExplorer() + : this(new ResourceExplorerOptions()) { } @@ -50,7 +53,7 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources /// /// The list of resource providers to initialize the current instance. public ResourceExplorer(IEnumerable providers) - : this(providers, null) + : this(new ResourceExplorerOptions() { Providers = providers }) { } @@ -60,9 +63,27 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources /// The list of resource providers to initialize the current instance. /// A list of declarative types to use. Falls back to if set to null. public ResourceExplorer(IEnumerable providers, IEnumerable declarativeTypes) + : this(new ResourceExplorerOptions() { Providers = providers, DeclarativeTypes = declarativeTypes }) { - this.resourceProviders = providers.ToList(); - this.declarativeTypes = declarativeTypes?.ToList(); + } + + /// + /// Initializes a new instance of the class. + /// + /// Configuration optiosn for . + public ResourceExplorer(ResourceExplorerOptions options) + { + this.options = options ?? throw new ArgumentNullException(nameof(options)); + + if (options.Providers != null) + { + this.resourceProviders = options.Providers.ToList(); + } + + if (options.DeclarativeTypes != null) + { + this.declarativeTypes = options.DeclarativeTypes.ToList(); + } } /// @@ -188,6 +209,10 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources return result; } } + catch (InvalidOperationException) + { + throw; + } catch (Exception err) { if (err.InnerException is SyntaxErrorException) @@ -589,7 +614,7 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources } // Create a cycle detection observer - var cycleDetector = new CycleDetectionObserver(); + var cycleDetector = new CycleDetectionObserver(options.AllowCycles); // Register our cycle detector on the converters that support observer registration foreach (var observableConverter in converters.Where(c => c is IObservableJsonConverter)) diff --git a/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorerOptions.cs b/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorerOptions.cs new file mode 100644 index 000000000..a29eee84b --- /dev/null +++ b/libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorerOptions.cs @@ -0,0 +1,38 @@ +// Licensed under the MIT License. +// Copyright (c) Microsoft Corporation. All rights reserved. + +using System; +using System.Collections.Generic; + +namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources +{ + /// + /// Configuration options for . + /// + public class ResourceExplorerOptions + { + /// + /// Gets or sets a value indicating whether whether cycles are allowed in resources managed by the . + /// + /// + /// Whether cycles are allowed in resources managed by the . + /// + public bool AllowCycles { get; set; } = true; + + /// + /// Gets or sets the list of resource providers to initialize the current the . + /// + /// + /// The list of resource providers to initialize the current the . + /// + public IEnumerable Providers { get; set; } + + /// + /// Gets or sets the list of declarative types to use. Falls back to if set to null. + /// + /// + /// The list of declarative types to use. Falls back to if set to null. + /// + public IEnumerable DeclarativeTypes { get; set; } + } +} diff --git a/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/JsonLoadTests.cs b/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/JsonLoadTests.cs index 48a233325..68284f1f4 100644 --- a/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/JsonLoadTests.cs +++ b/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/JsonLoadTests.cs @@ -1,6 +1,7 @@ // Licensed under the MIT License. // Copyright (c) Microsoft Corporation. All rights reserved. +using System; using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.Bot.Builder.Adapters; @@ -16,10 +17,12 @@ namespace Microsoft.Bot.Builder.Dialogs.Loader.Tests public class JsonLoadTests : IClassFixture { private static ResourceExplorer _resourceExplorer; + private static ResourceExplorer _noCycleResourceExplorer; public JsonLoadTests(ResourceExplorerFixture resourceExplorerFixture) { _resourceExplorer = resourceExplorerFixture.ResourceExplorer; + _noCycleResourceExplorer = resourceExplorerFixture.NoCycleResourceExplorer; } [Fact] @@ -46,6 +49,12 @@ namespace Microsoft.Bot.Builder.Dialogs.Loader.Tests .StartTestAsync(); } + [Fact] + public void JsonDialogLoad_CycleDetectionWithNoCycleMode() + { + Assert.Throws(() => BuildNoCycleTestFlow(@"Root.dialog", nameof(JsonDialogLoad_CycleDetectionWithNoCycleMode))); + } + [Fact] public async Task JsonDialogLoad_Actions() { @@ -425,11 +434,12 @@ namespace Microsoft.Bot.Builder.Dialogs.Loader.Tests return adapter; } - private TestFlow GetTestFlow(Dialog dialog, TestAdapter adapter) + private TestFlow GetTestFlow(Dialog dialog, TestAdapter adapter, bool allowCycle = true) { var dm = new DialogManager(dialog) - .UseResourceExplorer(_resourceExplorer) + .UseResourceExplorer(allowCycle ? _resourceExplorer : _noCycleResourceExplorer) .UseLanguageGeneration(); + dm.InitialTurnState.Add(new MockQnAMakerClient()); return new TestFlow(adapter, async (turnContext, cancellationToken) => @@ -444,5 +454,12 @@ namespace Microsoft.Bot.Builder.Dialogs.Loader.Tests var dialog = _resourceExplorer.LoadType(resourceName); return GetTestFlow(dialog, adapter); } + + private TestFlow BuildNoCycleTestFlow(string resourceName, string testName, bool sendTrace = false) + { + var adapter = InitializeAdapter(testName, sendTrace); + var dialog = _noCycleResourceExplorer.LoadType(resourceName); + return GetTestFlow(dialog, adapter, false); + } } } diff --git a/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/ResourceExplorerFixture.cs b/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/ResourceExplorerFixture.cs index c5c3f6dff..89965a62a 100644 --- a/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/ResourceExplorerFixture.cs +++ b/tests/Microsoft.Bot.Builder.Dialogs.Declarative.Tests/ResourceExplorerFixture.cs @@ -12,11 +12,14 @@ namespace Microsoft.Bot.Builder.Dialogs.Loader.Tests public ResourceExplorerFixture() { ResourceExplorer = new ResourceExplorer(); + NoCycleResourceExplorer = new ResourceExplorer(new ResourceExplorerOptions { AllowCycles = false }); Initialize(); } public ResourceExplorer ResourceExplorer { get; set; } + public ResourceExplorer NoCycleResourceExplorer { get; set; } + public ResourceExplorerFixture Initialize() { var projPath = Path.GetFullPath(Path.Combine(Environment.CurrentDirectory, PathUtils.NormalizePath($@"..\..\..\..\..\tests\Microsoft.Bot.Builder.TestBot.Json\Microsoft.Bot.Builder.TestBot.Json.csproj"))); @@ -24,12 +27,16 @@ namespace Microsoft.Bot.Builder.Dialogs.Loader.Tests ResourceExplorer = new ResourceExplorer() .LoadProject(projPath, monitorChanges: false); + NoCycleResourceExplorer = new ResourceExplorer(options: new ResourceExplorerOptions() { AllowCycles = false }) + .LoadProject(projPath, monitorChanges: false); + return this; } public void Dispose() { ResourceExplorer.Dispose(); + NoCycleResourceExplorer.Dispose(); } } }