IHandlersManager and Handler linking improvments (#334)

david-driscoll · web-flow · commit 2311cbb7cc1d · 2020-08-25T20:47:59.000-04:00
* Added extension method to get all the unique handlers contained in the IHandlersManager

* renamed link parameter names to better reflect their meaning.  Also added exceptions to help users diagnose issues

* Added unit tests

* Added dap tests
diff --git a/src/Client/LanguageClientOptions.cs b/src/Client/LanguageClientOptions.cs
@@ -64,8 +64,8 @@ ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHand
         ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHandler(string method, Type type, JsonRpcHandlerOptions options) =>
             AddHandler(method, type, options);
 
-        ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHandlerLink(string sourceMethod, string destinationMethod) =>
-            AddHandlerLink(sourceMethod, destinationMethod);
+        ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHandlerLink(string fromMethod, string toMethod) =>
+            AddHandlerLink(fromMethod, toMethod);
 
         ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.OnJsonRequest(string method, Func<JToken, Task<JToken>> handler, JsonRpcHandlerOptions options) =>
             OnJsonRequest(method, handler, options);
diff --git a/src/Dap.Client/DebugAdapterClientOptions.cs b/src/Dap.Client/DebugAdapterClientOptions.cs
@@ -54,8 +54,8 @@ IDebugAdapterClientRegistry IJsonRpcHandlerRegistry<IDebugAdapterClientRegistry>
         IDebugAdapterClientRegistry IJsonRpcHandlerRegistry<IDebugAdapterClientRegistry>.AddHandler(string method, Type type, JsonRpcHandlerOptions options) =>
             AddHandler(method, type, options);
 
-        IDebugAdapterClientRegistry IJsonRpcHandlerRegistry<IDebugAdapterClientRegistry>.AddHandlerLink(string sourceMethod, string destinationMethod) =>
-            AddHandlerLink(sourceMethod, destinationMethod);
+        IDebugAdapterClientRegistry IJsonRpcHandlerRegistry<IDebugAdapterClientRegistry>.AddHandlerLink(string fromMethod, string toMethod) =>
+            AddHandlerLink(fromMethod, toMethod);
 
         IDebugAdapterClientRegistry IJsonRpcHandlerRegistry<IDebugAdapterClientRegistry>.OnJsonRequest(
             string method, Func<JToken, Task<JToken>> handler, JsonRpcHandlerOptions options
diff --git a/src/Dap.Server/DebugAdapterServerOptions.cs b/src/Dap.Server/DebugAdapterServerOptions.cs
@@ -42,8 +42,8 @@ IDebugAdapterServerRegistry IJsonRpcHandlerRegistry<IDebugAdapterServerRegistry>
         IDebugAdapterServerRegistry IJsonRpcHandlerRegistry<IDebugAdapterServerRegistry>.AddHandler(string method, Type type, JsonRpcHandlerOptions options) =>
             AddHandler(method, type, options);
 
-        IDebugAdapterServerRegistry IJsonRpcHandlerRegistry<IDebugAdapterServerRegistry>.AddHandlerLink(string sourceMethod, string destinationMethod) =>
-            AddHandlerLink(sourceMethod, destinationMethod);
+        IDebugAdapterServerRegistry IJsonRpcHandlerRegistry<IDebugAdapterServerRegistry>.AddHandlerLink(string fromMethod, string toMethod) =>
+            AddHandlerLink(fromMethod, toMethod);
 
         IDebugAdapterServerRegistry IJsonRpcHandlerRegistry<IDebugAdapterServerRegistry>.OnJsonRequest(
             string method, Func<JToken, Task<JToken>> handler, JsonRpcHandlerOptions options
diff --git a/src/Dap.Shared/DebugAdapterHandlerCollection.cs b/src/Dap.Shared/DebugAdapterHandlerCollection.cs
@@ -43,12 +43,26 @@ public IDisposable Add(string method, Type handlerType, JsonRpcHandlerOptions op
             method, ActivatorUtilities.CreateInstance(_serviceProvider, handlerType) as IJsonRpcHandler, options
         );
 
-        IDisposable IHandlersManager.AddLink(string sourceMethod, string destinationMethod)
+        IDisposable IHandlersManager.AddLink(string fromMethod, string toMethod)
         {
-            var source = _descriptors.First(z => z.Method == sourceMethod);
+            var source = _descriptors.FirstOrDefault(z => z.Method == fromMethod);
+            if (source == null)
+            {
+                if (_descriptors.Any(z => z.Method == toMethod))
+                {
+                    throw new ArgumentException(
+                        $"Could not find descriptor for '{fromMethod}', but I did find one for '{toMethod}'.  Did you mean to link '{toMethod}' to '{fromMethod}' instead?", fromMethod
+                    );
+                }
+
+                throw new ArgumentException(
+                    $"Could not find descriptor for '{fromMethod}', has it been registered yet?  Descriptors must be registered before links can be created!", nameof(fromMethod)
+                );
+            }
+
             HandlerDescriptor descriptor = null;
             descriptor = GetDescriptor(
-                destinationMethod,
+                toMethod,
                 source.HandlerType,
                 source.Handler,
                 source.RequestProcessType.HasValue ? new JsonRpcHandlerOptions { RequestProcessType = source.RequestProcessType.Value } : null,
diff --git a/src/JsonRpc/CompositeHandlersManager.cs b/src/JsonRpc/CompositeHandlersManager.cs
@@ -55,9 +55,9 @@ public IDisposable Add(string method, Type handlerType, JsonRpcHandlerOptions op
             return result;
         }
 
-        public IDisposable AddLink(string sourceMethod, string destinationMethod)
+        public IDisposable AddLink(string fromMethod, string toMethod)
         {
-            var result = _parent.AddLink(sourceMethod,destinationMethod);
+            var result = _parent.AddLink(fromMethod,toMethod);
             _compositeDisposable.Add(result);
             return result;
         }
diff --git a/src/JsonRpc/HandlerCollection.cs b/src/JsonRpc/HandlerCollection.cs
@@ -91,10 +91,24 @@ public IDisposable Add(string method, Type handlerType, JsonRpcHandlerOptions op
             method, ActivatorUtilities.CreateInstance(_serviceProvider, handlerType) as IJsonRpcHandler, options
         );
 
-        public IDisposable AddLink(string sourceMethod, string destinationMethod)
+        public IDisposable AddLink(string fromMethod, string toMethod)
         {
-            var source = _descriptors.FirstOrDefault(z => z.Method == sourceMethod);
-            var descriptor = new LinkedHandler(destinationMethod, source, () => _descriptors.RemoveAll(z => z.Method == destinationMethod));
+            var source = _descriptors.FirstOrDefault(z => z.Method == fromMethod);
+            if (source == null)
+            {
+                if (_descriptors.Any(z => z.Method == toMethod))
+                {
+                    throw new ArgumentException(
+                        $"Could not find descriptor for '{fromMethod}', but I did find one for '{toMethod}'.  Did you mean to link '{toMethod}' to '{fromMethod}' instead?", fromMethod
+                    );
+                }
+
+                throw new ArgumentException(
+                    $"Could not find descriptor for '{fromMethod}', has it been registered yet?  Descriptors must be registered before links can be created!", nameof(fromMethod)
+                );
+            }
+
+            var descriptor = new LinkedHandler(toMethod, source, () => _descriptors.RemoveAll(z => z.Method == toMethod));
             ImmutableInterlocked.InterlockedExchange(ref _descriptors, _descriptors.Add(descriptor));
             return descriptor;
         }
diff --git a/src/JsonRpc/HandlersManagerExtensions.cs b/src/JsonRpc/HandlersManagerExtensions.cs
@@ -0,0 +1,18 @@
+﻿using System.Collections.Generic;
+using System.Linq;
+
+namespace OmniSharp.Extensions.JsonRpc
+{
+    public static class HandlersManagerExtensions
+    {
+        /// <summary>
+        /// Gets all the unique handlers currently registered with the manager
+        /// </summary>
+        /// <param name="handlersManager"></param>
+        /// <returns></returns>
+        public static IEnumerable<IJsonRpcHandler> GetHandlers(this IHandlersManager handlersManager)
+        {
+            return handlersManager.Descriptors.Select(z => z.Handler).Distinct();
+        }
+    }
+}
diff --git a/src/JsonRpc/IHandlersManager.cs b/src/JsonRpc/IHandlersManager.cs
@@ -11,7 +11,7 @@ public interface IHandlersManager
         IDisposable Add(string method, JsonRpcHandlerFactory factory, JsonRpcHandlerOptions options);
         IDisposable Add(Type handlerType, JsonRpcHandlerOptions options);
         IDisposable Add(string method, Type handlerType, JsonRpcHandlerOptions options);
-        IDisposable AddLink(string sourceMethod, string destinationMethod);
+        IDisposable AddLink(string fromMethod, string toMethod);
         IEnumerable<IHandlerDescriptor> Descriptors { get; }
     }
 }
diff --git a/src/JsonRpc/IJsonRpcHandlerRegistry.cs b/src/JsonRpc/IJsonRpcHandlerRegistry.cs
@@ -25,7 +25,7 @@ public interface IJsonRpcHandlerRegistry<out T> : IJsonRpcHandlerRegistry where
         T AddHandler<TTHandler>(string method, JsonRpcHandlerOptions options = null) where TTHandler : IJsonRpcHandler;
         T AddHandler(Type type, JsonRpcHandlerOptions options = null);
         T AddHandler(string method, Type type, JsonRpcHandlerOptions options = null);
-        T AddHandlerLink(string sourceMethod, string destinationMethod);
+        T AddHandlerLink(string fromMethod, string toMethod);
 
         T OnJsonRequest(string method, Func<JToken, Task<JToken>> handler, JsonRpcHandlerOptions options = null);
         T OnJsonRequest(string method, Func<JToken, CancellationToken, Task<JToken>> handler, JsonRpcHandlerOptions options = null);
diff --git a/src/JsonRpc/InterimJsonRpcServerRegistry.cs b/src/JsonRpc/InterimJsonRpcServerRegistry.cs
@@ -58,9 +58,9 @@ public sealed override T AddHandler(string method, Type type, JsonRpcHandlerOpti
             return (T) (object) this;
         }
 
-        public sealed override T AddHandlerLink(string sourceMethod, string destinationMethod)
+        public sealed override T AddHandlerLink(string fromMethod, string toMethod)
         {
-            _handlersManager.AddLink(sourceMethod, destinationMethod);
+            _handlersManager.AddLink(fromMethod, toMethod);
             return (T) (object) this;
         }
     }
diff --git a/src/JsonRpc/JsonRpcCommonMethodsBase.cs b/src/JsonRpc/JsonRpcCommonMethodsBase.cs
@@ -112,7 +112,7 @@ public T OnNotification(string method, Func<CancellationToken, Task> handler, Js
         public abstract T AddHandler<THandler>(string method, JsonRpcHandlerOptions options = null) where THandler : IJsonRpcHandler;
         public abstract T AddHandler(Type type, JsonRpcHandlerOptions options = null);
         public abstract T AddHandler(string method, Type type, JsonRpcHandlerOptions options = null);
-        public abstract T AddHandlerLink(string sourceMethod, string destinationMethod);
+        public abstract T AddHandlerLink(string fromMethod, string toMethod);
 
         #endregion
     }
diff --git a/src/JsonRpc/JsonRpcOptionsRegistryBase.cs b/src/JsonRpc/JsonRpcOptionsRegistryBase.cs
@@ -69,9 +69,9 @@ public sealed override T AddHandler(string method, Type type, JsonRpcHandlerOpti
             return (T) (object) this;
         }
 
-        public sealed override T AddHandlerLink(string sourceMethod, string destinationMethod)
+        public sealed override T AddHandlerLink(string fromMethod, string toMethod)
         {
-            Handlers.Add(JsonRpcHandlerDescription.Link(sourceMethod, destinationMethod));
+            Handlers.Add(JsonRpcHandlerDescription.Link(fromMethod, toMethod));
             return (T) (object) this;
         }
 
diff --git a/src/JsonRpc/JsonRpcServerOptionsBase.cs b/src/JsonRpc/JsonRpcServerOptionsBase.cs
@@ -129,9 +129,9 @@ public T WithMaximumRequestTimeout(TimeSpan maximumRequestTimeout)
             return (T) (object) this;
         }
 
-        public T WithLink(string source, string destination)
+        public T WithLink(string fromMethod, string toMethod)
         {
-            Handlers.Add(JsonRpcHandlerDescription.Link(source, destination));
+            Handlers.Add(JsonRpcHandlerDescription.Link(fromMethod, toMethod));
             return (T) (object) this;
         }
     }
diff --git a/src/Server/LanguageServerOptions.cs b/src/Server/LanguageServerOptions.cs
@@ -41,8 +41,8 @@ ILanguageServerRegistry IJsonRpcHandlerRegistry<ILanguageServerRegistry>.AddHand
         ILanguageServerRegistry IJsonRpcHandlerRegistry<ILanguageServerRegistry>.AddHandler(string method, Type type, JsonRpcHandlerOptions options) =>
             AddHandler(method, type, options);
 
-        ILanguageServerRegistry IJsonRpcHandlerRegistry<ILanguageServerRegistry>.AddHandlerLink(string sourceMethod, string destinationMethod) =>
-            AddHandlerLink(sourceMethod, destinationMethod);
+        ILanguageServerRegistry IJsonRpcHandlerRegistry<ILanguageServerRegistry>.AddHandlerLink(string fromMethod, string toMethod) =>
+            AddHandlerLink(fromMethod, toMethod);
 
         ILanguageServerRegistry IJsonRpcHandlerRegistry<ILanguageServerRegistry>.OnJsonRequest(string method, Func<JToken, Task<JToken>> handler, JsonRpcHandlerOptions options) =>
             OnJsonRequest(method, handler, options);
diff --git a/src/Shared/SharedHandlerCollection.cs b/src/Shared/SharedHandlerCollection.cs
@@ -60,12 +60,26 @@ IDisposable IHandlersManager.Add(Type handlerType, JsonRpcHandlerOptions options
         IDisposable IHandlersManager.Add(string method, Type handlerType, JsonRpcHandlerOptions options) =>
             Add(method, ActivatorUtilities.CreateInstance(_serviceProvider, handlerType) as IJsonRpcHandler, options);
 
-        IDisposable IHandlersManager.AddLink(string sourceMethod, string destinationMethod)
+        IDisposable IHandlersManager.AddLink(string fromMethod, string toMethod)
         {
-            var source = _descriptors.First(z => z.Method == sourceMethod);
+            var source = _descriptors.FirstOrDefault(z => z.Method == fromMethod);
+            if (source == null)
+            {
+                if (_descriptors.Any(z => z.Method == toMethod))
+                {
+                    throw new ArgumentException(
+                        $"Could not find descriptor for '{fromMethod}', but I did find one for '{toMethod}'.  Did you mean to link '{toMethod}' to '{fromMethod}' instead?", fromMethod
+                    );
+                }
+
+                throw new ArgumentException(
+                    $"Could not find descriptor for '{fromMethod}', has it been registered yet?  Descriptors must be registered before links can be created!", nameof(fromMethod)
+                );
+            }
+
             LspHandlerDescriptor descriptor = null;
             descriptor = GetDescriptor(
-                destinationMethod,
+                toMethod,
                 source.HandlerType,
                 source.Handler,
                 source.RequestProcessType.HasValue ? new JsonRpcHandlerOptions { RequestProcessType = source.RequestProcessType.Value } : null,
diff --git a/test/Dap.Tests/Integration/HandlersManagerIntegrationTests.cs b/test/Dap.Tests/Integration/HandlersManagerIntegrationTests.cs
@@ -0,0 +1,54 @@
+﻿using System;
+using System.Threading.Tasks;
+using FluentAssertions;
+using Microsoft.Extensions.DependencyInjection;
+using NSubstitute;
+using OmniSharp.Extensions.DebugAdapter.Protocol.Requests;
+using OmniSharp.Extensions.DebugAdapter.Testing;
+using OmniSharp.Extensions.JsonRpc;
+using OmniSharp.Extensions.JsonRpc.Testing;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Dap.Tests.Integration
+{
+    public class HandlersManagerIntegrationTests : DebugAdapterProtocolTestBase
+    {
+        public HandlersManagerIntegrationTests(ITestOutputHelper testOutputHelper) : base(new JsonRpcTestOptions().ConfigureForXUnit(testOutputHelper))
+        {
+        }
+
+        [Fact]
+        public async Task Should_Return_Default_Handlers()
+        {
+            var (client, server) = await Initialize(options => {}, options => {});
+
+            var handlersManager = server.GetRequiredService<IHandlersManager>();
+            handlersManager.Descriptors.Should().HaveCount(2);
+            handlersManager.GetHandlers().Should().HaveCount(2);
+        }
+
+        [Fact]
+        public async Task Link_Should_Fail_If_No_Handler_Is_Defined()
+        {
+            var (client, server) = await Initialize(options => {}, options => {});
+
+            var handlersManager = server.GetRequiredService<IHandlersManager>();
+
+            Action a  = () => handlersManager.AddLink(RequestNames.Completions, "my/completions");
+            a.Should().Throw<ArgumentException>().Which.Message.Should().Contain("Descriptors must be registered before links can be created");
+        }
+
+        [Fact]
+        public async Task Link_Should_Fail_If_Link_Is_On_The_Wrong_Side()
+        {
+            var (client, server) = await Initialize(options => {}, options => {});
+
+            var handlersManager = server.GetRequiredService<IHandlersManager>();
+            handlersManager.Add(Substitute.For(new Type[] { typeof(ICompletionsHandler) }, Array.Empty<object>()) as IJsonRpcHandler, new JsonRpcHandlerOptions());
+
+            Action a  = () => handlersManager.AddLink("my/completions", RequestNames.Completions);
+            a.Should().Throw<ArgumentException>().Which.Message.Should().Contain($"Did you mean to link '{RequestNames.Completions}' to 'my/completions' instead");
+        }
+    }
+}
diff --git a/test/JsonRpc.Tests/TestLanguageServerRegistry.cs b/test/JsonRpc.Tests/TestLanguageServerRegistry.cs
@@ -28,7 +28,7 @@ public override IJsonRpcServerRegistry AddHandler(string method, IJsonRpcHandler
         public override IJsonRpcServerRegistry AddHandler(Type type, JsonRpcHandlerOptions options = null) => throw new NotImplementedException();
 
         public override IJsonRpcServerRegistry AddHandler(string method, Type type, JsonRpcHandlerOptions options = null) => throw new NotImplementedException();
-        public override IJsonRpcServerRegistry AddHandlerLink(string sourceMethod, string destinationMethod) => throw new NotImplementedException();
+        public override IJsonRpcServerRegistry AddHandlerLink(string fromMethod, string toMethod) => throw new NotImplementedException();
 
         public override IJsonRpcServerRegistry AddHandler(string method, JsonRpcHandlerFactory handlerFunc, JsonRpcHandlerOptions options = null)
         {
diff --git a/test/Lsp.Tests/Integration/HandlersManagerIntegrationTests.cs b/test/Lsp.Tests/Integration/HandlersManagerIntegrationTests.cs
@@ -0,0 +1,66 @@
+﻿using System;
+using System.Threading.Tasks;
+using FluentAssertions;
+using Microsoft.Extensions.DependencyInjection;
+using NSubstitute;
+using OmniSharp.Extensions.JsonRpc;
+using OmniSharp.Extensions.JsonRpc.Testing;
+using OmniSharp.Extensions.LanguageProtocol.Testing;
+using OmniSharp.Extensions.LanguageServer.Protocol;
+using OmniSharp.Extensions.LanguageServer.Protocol.Document;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Lsp.Tests.Integration
+{
+    public class HandlersManagerIntegrationTests : LanguageProtocolTestBase
+    {
+        public HandlersManagerIntegrationTests(ITestOutputHelper testOutputHelper) : base(new JsonRpcTestOptions().ConfigureForXUnit(testOutputHelper))
+        {
+        }
+
+        [Fact]
+        public async Task Should_Return_Default_Handlers()
+        {
+            var (client, server) = await Initialize(options => {}, options => {});
+
+            var handlersManager = server.GetRequiredService<IHandlersManager>();
+            handlersManager.Descriptors.Should().HaveCount(8);
+            handlersManager.GetHandlers().Should().HaveCount(5);
+        }
+
+        [Fact]
+        public async Task Should_Return_Additional_Handlers()
+        {
+            var (client, server) = await Initialize(options => {}, options => {});
+
+            server.Register(o => o.AddHandler(Substitute.For(new Type[] { typeof (ICompletionHandler), typeof(ICompletionResolveHandler) }, Array.Empty<object>()) as IJsonRpcHandler));
+            var handlersManager = server.GetRequiredService<IHandlersManager>();
+            handlersManager.Descriptors.Should().HaveCount(10);
+            handlersManager.GetHandlers().Should().HaveCount(6);
+        }
+
+        [Fact]
+        public async Task Link_Should_Fail_If_No_Handler_Is_Defined()
+        {
+            var (client, server) = await Initialize(options => {}, options => {});
+
+            var handlersManager = server.GetRequiredService<IHandlersManager>();
+
+            Action a  = () => handlersManager.AddLink(TextDocumentNames.Completion, "my/completion");
+            a.Should().Throw<ArgumentException>().Which.Message.Should().Contain("Descriptors must be registered before links can be created");
+        }
+
+        [Fact]
+        public async Task Link_Should_Fail_If_Link_Is_On_The_Wrong_Side()
+        {
+            var (client, server) = await Initialize(options => {}, options => {});
+
+            server.Register(o => o.AddHandler(Substitute.For(new Type[] { typeof (ICompletionHandler), typeof(ICompletionResolveHandler) }, Array.Empty<object>()) as IJsonRpcHandler));
+            var handlersManager = server.GetRequiredService<IHandlersManager>();
+
+            Action a  = () => handlersManager.AddLink("my/completion", TextDocumentNames.Completion);
+            a.Should().Throw<ArgumentException>().Which.Message.Should().Contain($"Did you mean to link '{TextDocumentNames.Completion}' to 'my/completion' instead");
+        }
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -55,9 +55,9 @@ public IDisposable Add(string method, Type handlerType, JsonRpcHandlerOptions op`
`55`	`55`	`return result;`
`56`	`56`	`}`
`57`	`57`
`58`		`- public IDisposable AddLink(string sourceMethod, string destinationMethod)`
	`58`	`+ public IDisposable AddLink(string fromMethod, string toMethod)`
`59`	`59`	`{`
`60`		`- var result = _parent.AddLink(sourceMethod,destinationMethod);`
	`60`	`+ var result = _parent.AddLink(fromMethod,toMethod);`
`61`	`61`	`_compositeDisposable.Add(result);`
`62`	`62`	`return result;`
`63`	`63`	`}`
Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ public interface IHandlersManager`
`11`	`11`	`IDisposable Add(string method, JsonRpcHandlerFactory factory, JsonRpcHandlerOptions options);`
`12`	`12`	`IDisposable Add(Type handlerType, JsonRpcHandlerOptions options);`
`13`	`13`	`IDisposable Add(string method, Type handlerType, JsonRpcHandlerOptions options);`
`14`		`- IDisposable AddLink(string sourceMethod, string destinationMethod);`
	`14`	`+ IDisposable AddLink(string fromMethod, string toMethod);`
`15`	`15`	`IEnumerable<IHandlerDescriptor> Descriptors { get; }`
`16`	`16`	`}`
`17`	`17`	`}`
Original file line number	Diff line number	Diff line change
`@@ -58,9 +58,9 @@ public sealed override T AddHandler(string method, Type type, JsonRpcHandlerOpti`
`58`	`58`	`return (T) (object) this;`
`59`	`59`	`}`
`60`	`60`
`61`		`- public sealed override T AddHandlerLink(string sourceMethod, string destinationMethod)`
	`61`	`+ public sealed override T AddHandlerLink(string fromMethod, string toMethod)`
`62`	`62`	`{`
`63`		`- _handlersManager.AddLink(sourceMethod, destinationMethod);`
	`63`	`+ _handlersManager.AddLink(fromMethod, toMethod);`
`64`	`64`	`return (T) (object) this;`
`65`	`65`	`}`
`66`	`66`	`}`