Fix method discovery when there are duplicate names (#591)

Author: JamesNK

src/Grpc.AspNetCore.Server/Model/Internal/BinderServiceModelProvider.cs

@@ -37,7 +37,10 @@ public void OnServiceMethodDiscovery(ServiceMethodProviderContext<TService> cont
             // Invoke BindService(ServiceBinderBase, BaseType)
             if (bindMethodInfo != null)
             {
-                var binder = new ProviderServiceBinder<TService>(context);
+                // The second parameter is always the service base type
+                var serviceParameter = bindMethodInfo.GetParameters()[1];
+
+                var binder = new ProviderServiceBinder<TService>(context, serviceParameter.ParameterType);
 
                 try
                 {

src/Grpc.AspNetCore.Server/Model/Internal/ProviderServiceBinder.cs

@@ -18,46 +18,62 @@
 
 using System;
 using System.Collections.Generic;
+using System.Reflection;
 using Grpc.Core;
 
 namespace Grpc.AspNetCore.Server.Model.Internal
 {
     internal class ProviderServiceBinder<TService> : ServiceBinderBase where TService : class
     {
         private readonly ServiceMethodProviderContext<TService> _context;
+        private readonly Type _declaringType;
 
-        internal ProviderServiceBinder(ServiceMethodProviderContext<TService> context)
+        internal ProviderServiceBinder(ServiceMethodProviderContext<TService> context, Type declaringType)
         {
             _context = context;
+            _declaringType = declaringType;
         }
 
         public override void AddMethod<TRequest, TResponse>(Method<TRequest, TResponse> method, ClientStreamingServerMethod<TRequest, TResponse> handler)
         {
-            var (invoker, metadata) = CreateModelCore<ClientStreamingServerMethod<TService, TRequest, TResponse>>(method.Name);
+            var (invoker, metadata) = CreateModelCore<ClientStreamingServerMethod<TService, TRequest, TResponse>>(
+                method.Name,
+                new[] { typeof(IAsyncStreamReader<TRequest>), typeof(ServerCallContext) });
+
             _context.AddClientStreamingMethod<TRequest, TResponse>(method, metadata, invoker);
         }
 
         public override void AddMethod<TRequest, TResponse>(Method<TRequest, TResponse> method, DuplexStreamingServerMethod<TRequest, TResponse> handler)
         {
-            var (invoker, metadata) = CreateModelCore<DuplexStreamingServerMethod<TService, TRequest, TResponse>>(method.Name);
+            var (invoker, metadata) = CreateModelCore<DuplexStreamingServerMethod<TService, TRequest, TResponse>>(
+                method.Name,
+                new[] { typeof(IAsyncStreamReader<TRequest>), typeof(IServerStreamWriter<TResponse>), typeof(ServerCallContext) });
+
             _context.AddDuplexStreamingMethod<TRequest, TResponse>(method, metadata, invoker);
         }
 
         public override void AddMethod<TRequest, TResponse>(Method<TRequest, TResponse> method, ServerStreamingServerMethod<TRequest, TResponse> handler)
         {
-            var (invoker, metadata) = CreateModelCore<ServerStreamingServerMethod<TService, TRequest, TResponse>>(method.Name);
+            var (invoker, metadata) = CreateModelCore<ServerStreamingServerMethod<TService, TRequest, TResponse>>(
+                method.Name,
+                new[] { typeof(TRequest), typeof(IServerStreamWriter<TResponse>), typeof(ServerCallContext) });
+
             _context.AddServerStreamingMethod<TRequest, TResponse>(method, metadata, invoker);
         }
 
         public override void AddMethod<TRequest, TResponse>(Method<TRequest, TResponse> method, UnaryServerMethod<TRequest, TResponse> handler)
         {
-            var (invoker, metadata) = CreateModelCore<UnaryServerMethod<TService, TRequest, TResponse>>(method.Name);
+            var (invoker, metadata) = CreateModelCore<UnaryServerMethod<TService, TRequest, TResponse>>(
+                method.Name,
+                new[] { typeof(TRequest), typeof(ServerCallContext) });
+
             _context.AddUnaryMethod<TRequest, TResponse>(method, metadata, invoker);
         }
 
-        private (TDelegate invoker, List<object> metadata) CreateModelCore<TDelegate>(string methodName) where TDelegate : Delegate
+        private (TDelegate invoker, List<object> metadata) CreateModelCore<TDelegate>(string methodName, Type[] methodParameters) where TDelegate : Delegate
         {
-            var handlerMethod = typeof(TService).GetMethod(methodName);
+            var handlerMethod = GetMethod(methodName, methodParameters);
+
             if (handlerMethod == null)
             {
                 throw new InvalidOperationException($"Could not find '{methodName}' on {typeof(TService)}.");
@@ -73,5 +89,40 @@ public override void AddMethod<TRequest, TResponse>(Method<TRequest, TResponse>
 
             return (invoker, metadata);
         }
+
+        private MethodInfo? GetMethod(string methodName, Type[] methodParameters)
+        {
+            Type? currentType = typeof(TService);
+            while (currentType != null)
+            {
+                var matchingMethod = currentType.GetMethod(
+                    methodName,
+                    BindingFlags.Public | BindingFlags.Instance,
+                    binder: null,
+                    types: methodParameters,
+                    modifiers: null);
+
+                if (matchingMethod == null)
+                {
+                    return null;
+                }
+
+                // Validate that the method overrides the virtual method on the base service type.
+                // If there is a method with the same name it will hide the base method. Ignore it,
+                // and continue searching on the base type.
+                if (matchingMethod.IsVirtual)
+                {
+                    var baseDefinitionMethod = matchingMethod.GetBaseDefinition();
+                    if (baseDefinitionMethod != null && baseDefinitionMethod.DeclaringType == _declaringType)
+                    {
+                        return matchingMethod;
+                    }
+                }
+
+                currentType = currentType.BaseType;
+            }
+
+            return null;
+        }
     }
 }

test/Grpc.AspNetCore.Server.Tests/CallHandlerTests.cs

@@ -25,6 +25,7 @@
 using Grpc.AspNetCore.Server.Internal;
 using Grpc.AspNetCore.Server.Internal.CallHandlers;
 using Grpc.AspNetCore.Server.Tests.Infrastructure;
+using Grpc.AspNetCore.Server.Tests.TestObjects;
 using Grpc.Core;
 using Grpc.Tests.Shared;
 using Microsoft.AspNetCore.Http;
@@ -50,7 +51,7 @@ public class CallHandlerTests
         public async Task MinRequestBodyDataRateFeature_MethodType_HasRequestBodyDataRate(MethodType methodType, bool hasRequestBodyDataRate)
         {
             // Arrange
-            var httpContext = CreateContext();
+            var httpContext = HttpContextHelpers.CreateContext();
             var call = CreateHandler(methodType);
 
             // Act
@@ -67,7 +68,7 @@ public async Task MinRequestBodyDataRateFeature_MethodType_HasRequestBodyDataRat
         public async Task MaxRequestBodySizeFeature_MethodType_HasMaxRequestBodySize(MethodType methodType, bool hasMaxRequestBodySize)
         {
             // Arrange
-            var httpContext = CreateContext();
+            var httpContext = HttpContextHelpers.CreateContext();
             var call = CreateHandler(methodType);
 
             // Act
@@ -84,7 +85,7 @@ public async Task MaxRequestBodySizeFeature_FeatureIsReadOnly_FailureLogged()
             var testSink = new TestSink();
             var testLoggerFactory = new TestLoggerFactory(testSink, true);
 
-            var httpContext = CreateContext(isMaxRequestBodySizeFeatureReadOnly: true);
+            var httpContext = HttpContextHelpers.CreateContext(isMaxRequestBodySizeFeatureReadOnly: true);
             var call = CreateHandler(MethodType.ClientStreaming, testLoggerFactory);
 
             // Act
@@ -102,7 +103,7 @@ public async Task ContentTypeValidation_InvalidContentType_FailureLogged()
             var testSink = new TestSink();
             var testLoggerFactory = new TestLoggerFactory(testSink, true);
 
-            var httpContext = CreateContext(contentType: "text/plain");
+            var httpContext = HttpContextHelpers.CreateContext(contentType: "text/plain");
             var call = CreateHandler(MethodType.ClientStreaming, testLoggerFactory);
 
             // Act
@@ -121,7 +122,7 @@ public async Task SetResponseTrailers_FeatureMissing_ThrowError()
             var testSink = new TestSink();
             var testLoggerFactory = new TestLoggerFactory(testSink, true);
 
-            var httpContext = CreateContext(skipTrailerFeatureSet: true);
+            var httpContext = HttpContextHelpers.CreateContext(skipTrailerFeatureSet: true);
             var call = CreateHandler(MethodType.ClientStreaming, testLoggerFactory);
 
             // Act
@@ -138,7 +139,7 @@ public async Task ProtocolValidation_InvalidProtocol_FailureLogged()
             var testSink = new TestSink();
             var testLoggerFactory = new TestLoggerFactory(testSink, true);
 
-            var httpContext = CreateContext(protocol: "HTTP/1.1");
+            var httpContext = HttpContextHelpers.CreateContext(protocol: "HTTP/1.1");
             var call = CreateHandler(MethodType.ClientStreaming, testLoggerFactory);
 
             // Act
@@ -157,7 +158,7 @@ public async Task ProtocolValidation_IISHttp2Protocol_Success()
             var testSink = new TestSink();
             var testLoggerFactory = new TestLoggerFactory(testSink, true);
 
-            var httpContext = CreateContext(protocol: GrpcProtocolConstants.Http20Protocol);
+            var httpContext = HttpContextHelpers.CreateContext(protocol: GrpcProtocolConstants.Http20Protocol);
             var call = CreateHandler(MethodType.ClientStreaming, testLoggerFactory);
 
             // Act
@@ -210,125 +211,6 @@ private static ServerCallHandlerBase<TestService, TestMessage, TestMessage> Crea
                     throw new ArgumentException();
             }
         }
-
-        private static HttpContext CreateContext(
-            bool isMaxRequestBodySizeFeatureReadOnly = false,
-            bool skipTrailerFeatureSet = false,
-            string? protocol = null,
-            string? contentType = null)
-        {
-            var httpContext = new DefaultHttpContext();
-            var responseFeature = new TestHttpResponseFeature();
-            var responseBodyFeature = new TestHttpResponseBodyFeature(httpContext.Features.Get<IHttpResponseBodyFeature>(), responseFeature);
-
-            httpContext.Request.Protocol = protocol ?? GrpcProtocolConstants.Http2Protocol;
-            httpContext.Request.ContentType = contentType ?? GrpcProtocolConstants.GrpcContentType;
-            httpContext.Features.Set<IHttpMinRequestBodyDataRateFeature>(new TestMinRequestBodyDataRateFeature());
-            httpContext.Features.Set<IHttpMaxRequestBodySizeFeature>(new TestMaxRequestBodySizeFeature(isMaxRequestBodySizeFeatureReadOnly, 100));
-            httpContext.Features.Set<IHttpResponseFeature>(responseFeature);
-            httpContext.Features.Set<IHttpResponseBodyFeature>(responseBodyFeature);
-            if (!skipTrailerFeatureSet)
-            {
-                httpContext.Features.Set<IHttpResponseTrailersFeature>(new TestHttpResponseTrailersFeature());
-            }
-
-            return httpContext;
-        }
-    }
-
-    public class TestService { }
-
-    public class TestMessage { }
-
-    public class TestHttpResponseBodyFeature : IHttpResponseBodyFeature
-    {
-        private readonly IHttpResponseBodyFeature _innerResponseBodyFeature;
-        private readonly TestHttpResponseFeature _responseFeature;
-
-        public Stream Stream => _innerResponseBodyFeature.Stream;
-        public PipeWriter Writer => _innerResponseBodyFeature.Writer;
-
-        public TestHttpResponseBodyFeature(IHttpResponseBodyFeature innerResponseBodyFeature, TestHttpResponseFeature responseFeature)
-        {
-            _innerResponseBodyFeature = innerResponseBodyFeature ?? throw new ArgumentNullException(nameof(innerResponseBodyFeature));
-            _responseFeature = responseFeature ?? throw new ArgumentNullException(nameof(responseFeature));
-        }
-
-        public Task CompleteAsync()
-        {
-            return _innerResponseBodyFeature.CompleteAsync();
-        }
-
-        public void DisableBuffering()
-        {
-            _innerResponseBodyFeature.DisableBuffering();
-        }
-
-        public Task SendFileAsync(string path, long offset, long? count, CancellationToken cancellationToken = default)
-        {
-            return _innerResponseBodyFeature.SendFileAsync(path, offset, count, cancellationToken);
-        }
-
-        public Task StartAsync(CancellationToken cancellationToken = default)
-        {
-            _responseFeature.HasStarted = true;
-            return _innerResponseBodyFeature.StartAsync(cancellationToken);
-        }
-    }
-
-    public class TestHttpResponseFeature : IHttpResponseFeature
-    {
-        public Stream Body { get; set; }
-        public bool HasStarted { get; internal set; }
-        public IHeaderDictionary Headers { get; set; }
-        public string? ReasonPhrase { get; set; }
-        public int StatusCode { get; set; }
-
-        public TestHttpResponseFeature()
-        {
-            StatusCode = 200;
-            Headers = new HeaderDictionary();
-            Body = Stream.Null;
-        }
-
-        public void OnCompleted(Func<object, Task> callback, object state)
-        {
-        }
-
-        public void OnStarting(Func<object, Task> callback, object state)
-        {
-            HasStarted = true;
-        }
-    }
-
-    public class TestMinRequestBodyDataRateFeature : IHttpMinRequestBodyDataRateFeature
-    {
-        public MinDataRate MinDataRate { get; set; } = new MinDataRate(1, TimeSpan.FromSeconds(5));
-    }
-
-    public class TestMaxRequestBodySizeFeature : IHttpMaxRequestBodySizeFeature
-    {
-        public TestMaxRequestBodySizeFeature(bool isReadOnly, long? maxRequestBodySize)
-        {
-            IsReadOnly = isReadOnly;
-            MaxRequestBodySize = maxRequestBodySize;
-        }
-
-        public bool IsReadOnly { get; }
-        public long? MaxRequestBodySize { get; set; }
-    }
-
-    internal class TestGrpcServiceActivator<TGrpcService> : IGrpcServiceActivator<TGrpcService> where TGrpcService : class, new()
-    {
-        public GrpcActivatorHandle<TGrpcService> Create(IServiceProvider serviceProvider)
-        {
-            return new GrpcActivatorHandle<TGrpcService>(new TGrpcService(), false, null);
-        }
-
-        public ValueTask ReleaseAsync(GrpcActivatorHandle<TGrpcService> service)
-        {
-            return default;
-        }
     }
 
     public class TestServiceProvider : IServiceProvider
@@ -340,4 +222,8 @@ public object GetService(Type serviceType)
             throw new NotImplementedException();
         }
     }
+
+    public class TestService { }
+
+    public class TestMessage { }
 }