From 0cee00aae1a4064aa2ab2608015dac92d6389c2e Mon Sep 17 00:00:00 2001
From: Kiran Challa <kichalla@microsoft.com>
Date: Wed, 2 Nov 2016 15:54:51 -0700
Subject: [PATCH] [Fixes #5352] When replacing Controller.Dispose with an
 explicit implementation the base Dispose is an action

---
 .../DefaultApplicationModelProvider.cs        | 17 ++++--
 .../DefaultApplicationModelProviderTest.cs    | 52 +++++++++++++++++++
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs
index ccfead7293..23f9876d0a 100644
--- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs
+++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs
@@ -390,7 +390,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             }
 
             // Dispose method implemented from IDisposable is not valid
-            if (IsIDisposableMethod(methodInfo, typeInfo))
+            if (IsIDisposableMethod(methodInfo))
             {
                 return false;
             }
@@ -627,11 +627,20 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             return selectorModel;
         }
 
-        private bool IsIDisposableMethod(MethodInfo methodInfo, TypeInfo typeInfo)
+        private bool IsIDisposableMethod(MethodInfo methodInfo)
         {
+            // Ideally we do not want Dispose method to be exposed as an action. However there are some scenarios where a user
+            // might want to expose a method with name "Dispose" (even though they might not be really disposing resources)
+            // Example: A controller deriving from MVC's Controller type might wish to have a method with name Dispose,
+            // in which case they can use the "new" keyword to hide the base controller's declaration.
+
+            // Find where the method was originally declared
+            var baseMethodInfo = methodInfo.GetBaseDefinition();
+            var declaringTypeInfo = baseMethodInfo.DeclaringType.GetTypeInfo();
+
             return
-                (typeof(IDisposable).GetTypeInfo().IsAssignableFrom(typeInfo) &&
-                 typeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0] == methodInfo);
+                (typeof(IDisposable).GetTypeInfo().IsAssignableFrom(declaringTypeInfo) &&
+                 declaringTypeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0] == baseMethodInfo);
         }
 
         private bool IsSilentRouteAttribute(IRouteTemplateProvider routeTemplateProvider)
diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs
index bfe0be0051..3879cc3c51 100644
--- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs
+++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs
@@ -883,6 +883,41 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             Assert.Contains(selectorModel.AttributeRouteModel.Attribute, action.Attributes);
         }
 
+        [Fact]
+        public void ControllerDispose_ExplicitlyImplemented_IDisposableMethods_AreTreatedAs_NonActions()
+        {
+            // Arrange
+            var builder = new TestApplicationModelProvider();
+            var typeInfo = typeof(DerivedFromControllerAndExplicitIDisposableImplementationController).GetTypeInfo();
+            var context = new ApplicationModelProviderContext(new[] { typeInfo });
+
+            // Act
+            builder.OnProvidersExecuting(context);
+
+            // Assert
+            var model = Assert.Single(context.Result.Controllers);
+            Assert.Empty(model.Actions);
+        }
+
+        [Fact]
+        public void ControllerDispose_MethodsNamedDispose_AreTreatedAsActions()
+        {
+            // Arrange
+            var builder = new TestApplicationModelProvider();
+            var typeInfo = typeof(DerivedFromControllerAndHidesBaseDisposeMethodController).GetTypeInfo();
+            var context = new ApplicationModelProviderContext(new[] { typeInfo });
+
+            // Act
+            builder.OnProvidersExecuting(context);
+
+            // Assert
+            var model = Assert.Single(context.Result.Controllers);
+            var action = Assert.Single(model.Actions);
+
+            // Make sure that the Dispose method is from the derived controller and not the base 'Controller' type
+            Assert.Equal(typeInfo, action.ActionMethod.DeclaringType.GetTypeInfo());
+        }
+
         private IList<AttributeRouteModel> GetAttributeRoutes(IList<SelectorModel> selectors)
         {
             return selectors
@@ -891,6 +926,23 @@ namespace Microsoft.AspNetCore.Mvc.Internal
                 .ToList();
         }
 
+        private class DerivedFromControllerAndExplicitIDisposableImplementationController
+            : Mvc.Controller, IDisposable
+        {
+            void IDisposable.Dispose()
+            {
+                throw new NotImplementedException();
+            }
+        }
+
+        private class DerivedFromControllerAndHidesBaseDisposeMethodController : Mvc.Controller
+        {
+            public new void Dispose()
+            {
+                throw new NotImplementedException();
+            }
+        }
+
         private class BaseClassWithAttributeRoutesController
         {
             [Route("A")]