csharp-language-server-protocol icon indicating copy to clipboard operation
csharp-language-server-protocol copied to clipboard

MediatR v9.0.0 includes a breaking change.

Open chrissimon-au opened this issue 3 years ago • 1 comments

This is probably an issue that is not going to affect many people, but I believe it is a legit dependency issue that will have to be resolved at some point:

Directory.Build.targets includes MediatR at version 8.1.0.

When adding as a dependency via dotnet or nuget, (e.g. dotnet add package OmniSharp.Extensions.LanguageServer), which is what I'd guess most folk are doing, it follows the Nuget Versioning rules and pulls in the lowest version >= the specified version - which resolves to 8.1.0 and works fine.

When adding via paket add OmniSharp.Extensions.LanguageServer it follows the paket rules which identifies the latest matching version which is 9.0.0 and unfortunately this version has a breaking change. The issue is only detectable at runtime due to the use of reflection I believe. The problem is an attempt to invoke a method that no longer exists on the IMediator interface, due to being moved to a separate interface that IMediator implements.

Compare:

v8.1.0: https://github.com/jbogard/MediatR/blob/v8.1.0/src/MediatR/IMediator.cs

vs

v9.0.0: https://github.com/jbogard/MediatR/blob/v9.0.0/src/MediatR/IMediator.cs

The exception that's thrown is:

OmniSharp.Extensions.JsonRpc.DefaultRequestInvoker: Failed to handle request initialize 1 -
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.MissingMethodException: Method not found: 'System.Threading.Tasks.Task`1<System.__Canon> MediatR.IMediator.Send(MediatR.IRequest`1<System.__Canon>, System.Threading.CancellationToken)'.
   --- End of inner exception stack trace ---
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at OmniSharp.Extensions.JsonRpc.RequestRouterBase`1.HandleRequest(IMediator mediator, IHandlerDescriptor descriptor, Object params, CancellationToken token)
   at OmniSharp.Extensions.JsonRpc.RequestRouterBase`1.<RouteRequest>g__InnerRoute|7_0(IServiceScopeFactory serviceScopeFactory, Request request, TDescriptor descriptor, Object params, CancellationToken token, ILogger logger)
   at OmniSharp.Extensions.JsonRpc.RequestRouterBase`1.RouteRequest(IRequestDescriptor`1 descriptors, Request request, CancellationToken token)
   at OmniSharp.Extensions.JsonRpc.DefaultRequestInvoker.<>c__DisplayClass10_0.<<RouteRequest>b__5>d.MoveNext()

So it seems that the issue is somewhere around https://github.com/OmniSharp/csharp-language-server-protocol/blob/005c87f1c41da3f1b2b35f32c98e3d3132df99a3/src/JsonRpc/RequestRouterBase.cs#L204

It seems that while making one of the SendRequest methods on the RequestRouterBase generic is working, the invocation isn't, because the invocation is attempting to invoke a method on IMediator interface itself, and not resolving to the method which is now on ISender as of MediatR 9.0.0. Not sure exactly why - perhaps if the method was invoked directly, rather than via reflection, the compiler would have worked it all out correctly?

Proposed solutions:

1. Limit the version range

By updating Directory.Build.targets to include MediatR with a floating version of 8.*. It should ensure both nuget and paket resolve the dependency as 8.1.0. (When using floating versions, nuget resolves the highest matching version). This is a more technically correct dependency version spec because it clearly identifies the valid version range. [8.1.0,9.0.0) is an alternative way of expressing the same thing I think.

2. Update to MediatR 9.0.0 properly

I'd hope that updating Directory.Build.targets to include MediatR with a floating version of 9.* should very quickly flush out any issues through the test suite, which could be resolved by perhaps using Mediatr.ISender in the SendRequest methods (and anywhere else).

Workaround for Paket users

Adding:

nuget MediatR 8.1.0

to paket.dependencies resolves the issue as a workaround for paket users.

Summary

I'm happy to contribute a PR to resolve this upstream and avoid using the workaround if you have a preference for either of the above proposals, or any other preferred approach?

chrissimon-au avatar Oct 31 '21 04:10 chrissimon-au

Have setup two PRs, one for each option. Bumping the version to [9.0.0,10.0.0) didn't seem to require any code changes to get all tests to pass locally. It seems that the issue was just attempting to use MediatR 9.0.0 (as restored by paket) using the OmniSharp.Extensions.JsonRpc package built against MediatR 8.1.0, which makes sense now that I think about it!

chrissimon-au avatar Oct 31 '21 10:10 chrissimon-au

Mediatr was updated to 9.0.0 in https://github.com/OmniSharp/csharp-language-server-protocol/pull/1084

JoeRobich avatar Oct 26 '23 21:10 JoeRobich