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 <ccastro@microsoft.com>

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

libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Observers/CycleDetectionObserver.cs

@@ -19,8 +19,18 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Observers
     /// </remarks>
     internal class CycleDetectionObserver : IJsonLoadObserver
     {
+        private readonly bool allowCycle;
         private readonly Dictionary<int, object> cache = new Dictionary<int, object>();
-        
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="CycleDetectionObserver"/> class.
+        /// </summary>
+        /// <param name="allowCycle">If allowCycle is set to false, throw an exception when detecting cycle.</param>
+        public CycleDetectionObserver(bool allowCycle = true)
+        {
+            this.allowCycle = allowCycle;
+        }
+
         /// <summary>
         /// Gets or sets the current pass of the algorithm.
         /// </summary>
@@ -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.

libraries/Microsoft.Bot.Builder.Dialogs.Declarative/Resources/ResourceExplorer.cs

@@ -29,6 +29,8 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources
         private readonly ConcurrentDictionary<string, ICustomDeserializer> kindDeserializers = new ConcurrentDictionary<string, ICustomDeserializer>();
         private readonly ConcurrentDictionary<string, Type> kindToType = new ConcurrentDictionary<string, Type>();
         private readonly ConcurrentDictionary<Type, List<string>> typeToKinds = new ConcurrentDictionary<Type, List<string>>();
+        private readonly ResourceExplorerOptions options;
+
         private List<ResourceProvider> resourceProviders = new List<ResourceProvider>();
         private List<IComponentDeclarativeTypes> declarativeTypes;
         private CancellationTokenSource cancelReloadToken = new CancellationTokenSource();
@@ -42,6 +44,7 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources
         /// Initializes a new instance of the <see cref="ResourceExplorer"/> class.
         /// </summary>
         public ResourceExplorer()
+            : this(new ResourceExplorerOptions())
         {
         }
 
@@ -50,7 +53,7 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources
         /// </summary>
         /// <param name="providers">The list of resource providers to initialize the current instance.</param>
         public ResourceExplorer(IEnumerable<ResourceProvider> providers)
-            : this(providers, null)
+            : this(new ResourceExplorerOptions() { Providers = providers })
         {
         }
 
@@ -60,9 +63,27 @@ namespace Microsoft.Bot.Builder.Dialogs.Declarative.Resources
         /// <param name="providers">The list of resource providers to initialize the current instance.</param>
         /// <param name="declarativeTypes">A list of declarative types to use. Falls back to <see cref="ComponentRegistration.Components" /> if set to null.</param>
         public ResourceExplorer(IEnumerable<ResourceProvider> providers, IEnumerable<IComponentDeclarativeTypes> declarativeTypes)
+            : this(new ResourceExplorerOptions() { Providers = providers, DeclarativeTypes = declarativeTypes })
         {
-            this.resourceProviders = providers.ToList();
+        }
-            this.declarativeTypes = declarativeTypes?.ToList();
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ResourceExplorer"/> class.
+        /// </summary>
+        /// <param name="options">Configuration optiosn for <see cref="ResourceExplorer"/>.</param>
+        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();
+            }
         }
 
         /// <summary>
@@ -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))

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
+{
+    /// <summary>
+    /// Configuration options for <see cref="ResourceExplorer"/>.
+    /// </summary>
+    public class ResourceExplorerOptions
+    {
+        /// <summary>
+        /// Gets or sets a value indicating whether whether cycles are allowed in resources managed by the <see cref="ResourceExplorer"/>.
+        /// </summary>
+        /// <value>
+        /// Whether cycles are allowed in resources managed by the <see cref="ResourceExplorer"/>.
+        /// </value>
+        public bool AllowCycles { get; set; } = true;
+
+        /// <summary>
+        /// Gets or sets the list of resource providers to initialize the current the <see cref="ResourceExplorer"/>.
+        /// </summary>
+        /// <value>
+        /// The list of resource providers to initialize the current the <see cref="ResourceExplorer"/>.
+        /// </value>
+        public IEnumerable<ResourceProvider> Providers { get; set; }
+
+        /// <summary>
+        /// Gets or sets the list of declarative types to use. Falls back to <see cref="ComponentRegistration.Components" /> if set to null.
+        /// </summary>
+        /// <value>
+        /// The list of declarative types to use. Falls back to <see cref="ComponentRegistration.Components" /> if set to null.
+        /// </value>
+        public IEnumerable<IComponentDeclarativeTypes> DeclarativeTypes { get; set; }
+    }   
+}

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<ResourceExplorerFixture>
     {
         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<InvalidOperationException>(() => 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<IQnAMakerClient>(new MockQnAMakerClient());
 
             return new TestFlow(adapter, async (turnContext, cancellationToken) =>
@@ -444,5 +454,12 @@ namespace Microsoft.Bot.Builder.Dialogs.Loader.Tests
             var dialog = _resourceExplorer.LoadType<Dialog>(resourceName);
             return GetTestFlow(dialog, adapter);
         }
+
+        private TestFlow BuildNoCycleTestFlow(string resourceName, string testName, bool sendTrace = false)
+        {
+            var adapter = InitializeAdapter(testName, sendTrace);
+            var dialog = _noCycleResourceExplorer.LoadType<Dialog>(resourceName);
+            return GetTestFlow(dialog, adapter, false);
+        }
     }
 }

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();
         }
     }
 }