Skip to content

Commit

Permalink
Custom handlers not override built in handlers correctly (#392)
Browse files Browse the repository at this point in the history
* Fixed a bug in configuration (configuration values were case-sensitive).  Added ability to disable default handlers for language client / language server.  Added unit tests demonstrating Configuration.Binder, and Options usages

* fixed an issue where the MedatR handler was being registered in the container for 'built-in' handlers, these were overriding any custom ones that were added later

* added default request handler, and decorator to ensure that if a IRequestContext is given, then that handler is used.

* removed some changes that are no longer needed

* Removed extra dependencies, we can add these at another time

* cleanup

* removed unneeded code

* Added additional assertions
  • Loading branch information
david-driscoll authored Oct 21, 2020
1 parent 8b88655 commit 03d7b65
Show file tree
Hide file tree
Showing 20 changed files with 600 additions and 42 deletions.
5 changes: 4 additions & 1 deletion Directory.Build.targets
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
<PackageReference Update="Microsoft.Extensions.Logging" Version="2.0.0" />
<PackageReference Update="Microsoft.Extensions.Logging.Debug" Version="2.0.0" />
<PackageReference Update="Microsoft.Extensions.Configuration" Version="2.0.0" />
<PackageReference Update="Microsoft.Extensions.Configuration.Binder" Version="2.0.0" />
<PackageReference Update="Microsoft.Extensions.Options" Version="2.0.0" />
<PackageReference Update="Microsoft.Extensions.Options.ConfigurationExtensions" Version="2.0.0" />
<PackageReference Update="Microsoft.Extensions.DependencyInjection" Version="2.0.0" />
<PackageReference Update="Newtonsoft.Json" Version="11.0.2" />
<PackageReference Update="Microsoft.NET.Test.Sdk" Version="16.7.1" />
Expand Down Expand Up @@ -45,4 +48,4 @@
<PackageReference Update="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="3.4.0" />
<PackageReference Update="DryIoc.Internal" Version="4.5.0" />
</ItemGroup>
</Project>
</Project>
4 changes: 3 additions & 1 deletion src/Client/LanguageClientOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public LanguageClientOptions()
{
WithAssemblies(typeof(LanguageClientOptions).Assembly, typeof(LspRequestRouter).Assembly);
}

public ClientCapabilities ClientCapabilities { get; set; } = new ClientCapabilities {
Experimental = new Dictionary<string, JToken>(),
Window = new WindowClientCapabilities(),
Expand Down Expand Up @@ -52,7 +53,8 @@ ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHand
ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHandler(JsonRpcHandlerFactory handlerFunc, JsonRpcHandlerOptions? options) =>
AddHandler(handlerFunc, options);

ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHandler(IJsonRpcHandler handler, JsonRpcHandlerOptions? options) => AddHandler(handler, options);
ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHandler(IJsonRpcHandler handler, JsonRpcHandlerOptions? options) =>
AddHandler(handler, options);

ILanguageClientRegistry IJsonRpcHandlerRegistry<ILanguageClientRegistry>.AddHandler<TTHandler>(JsonRpcHandlerOptions? options) => AddHandler<TTHandler>(options);

Expand Down
5 changes: 3 additions & 2 deletions src/Client/LanguageClientServiceCollectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Linq;
using System.Reflection;
using DryIoc;
using MediatR;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
Expand All @@ -28,7 +29,7 @@ internal static IContainer AddLanguageClientInternals(this IContainer container,
nonPublicServiceTypes: true,
ifAlreadyRegistered: IfAlreadyRegistered.Keep
);
if (!EqualityComparer<OnUnhandledExceptionHandler?>.Default.Equals( options.OnUnhandledException, default))
if (!EqualityComparer<OnUnhandledExceptionHandler?>.Default.Equals(options.OnUnhandledException, default))
{
container.RegisterInstance(options.OnUnhandledException);
}
Expand Down Expand Up @@ -79,7 +80,7 @@ internal static IContainer AddLanguageClientInternals(this IContainer container,

if (providedConfiguration != null)
{
builder.CustomAddConfiguration((providedConfiguration.ImplementationInstance as IConfiguration)!);
builder.CustomAddConfiguration(( providedConfiguration.ImplementationInstance as IConfiguration )!);
}

//var didChangeConfigurationProvider = _.GetRequiredService<DidChangeConfigurationProvider>();
Expand Down
5 changes: 3 additions & 2 deletions src/JsonRpc.Generators/GenerateHandlerMethodsGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,9 @@ MemberDeclarationSyntax MakeAction(TypeSyntax syntax)
yield return MakeAction(CreateAsyncAction(true, requestType));
if (capability != null)
{
yield return MakeAction(CreateAction(requestType, capability));
yield return MakeAction(CreateAsyncAction(requestType, capability));
method = method.WithExpressionBody(
GetNotificationCapabilityHandlerExpression(GetMethodName(handlerInterface), requestType, capability)
);
yield return MakeAction(CreateAction(requestType, capability));
yield return MakeAction(CreateAsyncAction(requestType, capability));
}
Expand Down
60 changes: 45 additions & 15 deletions src/JsonRpc/JsonRpcServerServiceCollectionExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using System;
using System.IO.Pipelines;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using DryIoc;
using MediatR;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -79,23 +81,51 @@ internal static IContainer AddJsonRpcMediatR(this IContainer container)
container.RegisterMany(new[] { typeof(IMediator).GetAssembly() }, Registrator.Interfaces, Reuse.ScopedOrSingleton);
container.RegisterMany<RequestContext>(Reuse.Scoped);
container.RegisterDelegate<ServiceFactory>(context => context.Resolve, Reuse.ScopedOrSingleton);
container.Register(typeof(IRequestHandler<,>), typeof(RequestHandler<,>));
container.Register(typeof(IRequestHandler<,>), typeof(RequestHandlerDecorator<,>), setup: Setup.Decorator);

return container;
}

class RequestHandler<T, R> : IRequestHandler<T, R> where T : IRequest<R>
{
private readonly IRequestContext _requestContext;

public RequestHandler(IRequestContext requestContext)
{
_requestContext = requestContext;
}
public Task<R> Handle(T request, CancellationToken cancellationToken)
{
return ((IRequestHandler<T, R>) _requestContext.Descriptor.Handler).Handle(request, cancellationToken);
}
}

class RequestHandlerDecorator<T, R> : IRequestHandler<T, R> where T : IRequest<R>
{
private readonly IRequestHandler<T, R>? _handler;
private readonly IRequestContext? _requestContext;

public RequestHandlerDecorator(IRequestHandler<T, R>? handler = null, IRequestContext? requestContext = null)
{
_handler = handler;
_requestContext = requestContext;
}
public Task<R> Handle(T request, CancellationToken cancellationToken)
{
if (_requestContext == null)
{
if (_handler == null)
{
throw new NotImplementedException($"No request handler was registered for type {typeof(IRequestHandler<T, R>).FullName}");

return container.With(
rules => rules.WithUnknownServiceResolvers(
request => {
if (request.ServiceType.IsGenericType && typeof(IRequestHandler<,>).IsAssignableFrom(request.ServiceType.GetGenericTypeDefinition()))
{
var context = request.Container.Resolve<IRequestContext?>();
if (context != null)
{
return new RegisteredInstanceFactory(context.Descriptor.Handler);
}
}

return null;
}
)
);

return _handler.Handle(request, cancellationToken);
}

return ((IRequestHandler<T, R>) _requestContext.Descriptor.Handler).Handle(request, cancellationToken);
}
}

internal static IContainer AddJsonRpcServerInternals(this IContainer container, JsonRpcServerOptions options)
Expand Down
176 changes: 176 additions & 0 deletions src/Protocol/ConfigureByConfigurationPathExtension.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
#if false
using System;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Configuration.Binder;
using Microsoft.Extensions.Primitives;

// ReSharper disable once CheckNamespace
namespace Microsoft.Extensions.Configuration
{
public static class ConfigureByConfigurationPathExtension
{
/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection Configure<TOptions>(this IServiceCollection services)
where TOptions : class
{
if (services == null)
{
throw new ArgumentNullException(nameof(services));
}

return Configure<TOptions>(services, null);
}

/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
/// <param name="sectionName">The name of the options instance.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection Configure<TOptions>(this IServiceCollection services, string? sectionName)
where TOptions : class
{
if (services == null)
{
throw new ArgumentNullException(nameof(services));
}

services.AddOptions();
services.AddSingleton<IOptionsChangeTokenSource<TOptions>>(
_ => new ConfigurationChangeTokenSource<TOptions>(
Options.Options.DefaultName,
sectionName == null ? _.GetRequiredService<IConfiguration>() : _.GetRequiredService<IConfiguration>().GetSection(sectionName)
)
);
return services.AddSingleton<IConfigureOptions<TOptions>>(
_ => new NamedConfigureFromConfigurationOptions<TOptions>(
Options.Options.DefaultName,
sectionName == null ? _.GetRequiredService<IConfiguration>() : _.GetRequiredService<IConfiguration>().GetSection(sectionName)
)
);
}

/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
/// <param name="configureBinder">Used to configure the <see cref="BinderOptions"/>.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection Configure<TOptions>(this IServiceCollection services, Action<BinderOptions> configureBinder)
where TOptions : class
{
if (services == null)
{
throw new ArgumentNullException(nameof(services));
}

return Configure<TOptions>(services, Options.Options.DefaultName, configureBinder);
}

/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
/// <param name="sectionName">The name of the options instance.</param>
/// <param name="configureBinder">Used to configure the <see cref="BinderOptions"/>.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection Configure<TOptions>(this IServiceCollection services, string sectionName, Action<BinderOptions> configureBinder)
where TOptions : class
{
if (services == null)
{
throw new ArgumentNullException(nameof(services));
}

services.AddOptions();
services.AddSingleton<IOptionsChangeTokenSource<TOptions>>(_ => new ConfigurationChangeTokenSource<TOptions>(Options.Options.DefaultName, _.GetRequiredService<IConfiguration>().GetSection(sectionName)));
return services.AddSingleton<IConfigureOptions<TOptions>>(_ => new NamedConfigureFromConfigurationOptions<TOptions>(Options.Options.DefaultName, _.GetRequiredService<IConfiguration>().GetSection(sectionName), configureBinder));
}

/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="builder">The <see cref="OptionsBuilder&lt;TOptions&gt;"/> to configure.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static OptionsBuilder<TOptions> Configure<TOptions>(this OptionsBuilder<TOptions> builder)
where TOptions : class
{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}

return Configure(builder, Options.Options.DefaultName);
}

/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="builder">The <see cref="OptionsBuilder&lt;TOptions&gt;"/> to configure.</param>
/// <param name="sectionName">The name of the options instance.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static OptionsBuilder<TOptions> Configure<TOptions>(this OptionsBuilder<TOptions> builder, string sectionName)
where TOptions : class
{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}

Configure<TOptions>(builder.Services, name);
return builder;
}

/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="builder">The <see cref="OptionsBuilder&lt;TOptions&gt;"/> to configure.</param>
/// <param name="configureBinder">Used to configure the <see cref="BinderOptions"/>.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static OptionsBuilder<TOptions> Configure<TOptions>(this OptionsBuilder<TOptions> builder, Action<BinderOptions> configureBinder)
where TOptions : class
{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}

return Configure(builder, Options.Options.DefaultName, configureBinder);
}

/// <summary>
/// Registers a injected configuration service which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="builder">The <see cref="OptionsBuilder&lt;TOptions&gt;"/> to configure.</param>
/// <param name="sectionName">The name of the options instance.</param>
/// <param name="configureBinder">Used to configure the <see cref="BinderOptions"/>.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static OptionsBuilder<TOptions> Configure<TOptions>(this OptionsBuilder<TOptions> builder, string sectionName, Action<BinderOptions> configureBinder)
where TOptions : class
{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}


Configure<TOptions>(builder.Services, name, configureBinder);
return builder;
}
}
}
#endif
11 changes: 11 additions & 0 deletions src/Protocol/LanguageProtocolDelegatingHandlers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,17 @@ public NotificationCapability(Func<TParams, TCapability, Task> handler) :
{
}

public NotificationCapability(Action<TParams, TCapability, CancellationToken> handler) :
this(
Guid.Empty, (request, capability, ct) => {
handler(request, capability, ct);
return Task.CompletedTask;
}
)
{
}


public NotificationCapability(Func<TParams, TCapability, CancellationToken, Task> handler) :
this(Guid.Empty, handler)
{
Expand Down
5 changes: 5 additions & 0 deletions src/Protocol/Protocol.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Configuration" />
<!--
TODO: Add these in a future version
<PackageReference Include="Microsoft.Extensions.Configuration.Binder" />
<PackageReference Include="Microsoft.Extensions.Options.ConfigurationExtensions" />
-->
</ItemGroup>

<ItemGroup>
Expand Down
5 changes: 3 additions & 2 deletions src/Protocol/Workspace/IDidChangeConfigurationHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,16 @@ namespace OmniSharp.Extensions.LanguageServer.Protocol.Workspace
[GenerateHandlerMethods]
[GenerateRequestMethods(typeof(IWorkspaceLanguageClient), typeof(ILanguageClient))]
public interface IDidChangeConfigurationHandler : IJsonRpcNotificationHandler<DidChangeConfigurationParams>,
IRegistration<object>, ICapability<DidChangeConfigurationCapability>
IRegistration<object>, // TODO: Remove this in the future
ICapability<DidChangeConfigurationCapability>
{
}

public abstract class DidChangeConfigurationHandler : IDidChangeConfigurationHandler
{
public object GetRegistrationOptions() => new object();
public abstract Task<Unit> Handle(DidChangeConfigurationParams request, CancellationToken cancellationToken);
public virtual void SetCapability(DidChangeConfigurationCapability capability) => Capability = capability;
protected DidChangeConfigurationCapability Capability { get; private set; } = null!;
public object GetRegistrationOptions() => new object();
}
}
3 changes: 2 additions & 1 deletion src/Protocol/Workspace/IDidChangeWorkspaceFoldersHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ namespace OmniSharp.Extensions.LanguageServer.Protocol.Workspace
[Method(WorkspaceNames.DidChangeWorkspaceFolders, Direction.ClientToServer)]
[GenerateHandlerMethods]
[GenerateRequestMethods(typeof(IWorkspaceLanguageClient), typeof(ILanguageClient))]
public interface IDidChangeWorkspaceFoldersHandler : IJsonRpcNotificationHandler<DidChangeWorkspaceFoldersParams>, IRegistration<object>
public interface IDidChangeWorkspaceFoldersHandler : IJsonRpcNotificationHandler<DidChangeWorkspaceFoldersParams>,
IRegistration<object> // TODO: Remove this in the future
{
}

Expand Down
Loading

0 comments on commit 03d7b65

Please sign in to comment.