Fix to special case Dispose method to be treated as non-action

src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Reflection;
@@ -16,9 +17,9 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
     public class DefaultActionModelBuilder : IActionModelBuilder
     {
         /// <inheritdoc />
-        public IEnumerable<ActionModel> BuildActionModels([NotNull] MethodInfo methodInfo)
+        public IEnumerable<ActionModel> BuildActionModels([NotNull] TypeInfo typeInfo, [NotNull] MethodInfo methodInfo)
         {
-            if (!IsAction(methodInfo))
+            if (!IsAction(typeInfo, methodInfo))
             {
                 return Enumerable.Empty<ActionModel>();
             }
@@ -131,29 +132,70 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
         /// Returns <c>true</c> if the <paramref name="methodInfo"/> is an action. Otherwise <c>false</c>.
         /// </summary>
         /// <param name="methodInfo">The <see cref="MethodInfo"/>.</param>
+        /// <param name="typeInfo">The <see cref="TypeInfo"/>.</param>
         /// <returns><c>true</c> if the <paramref name="methodInfo"/> is an action. Otherwise <c>false</c>.</returns>
         /// <remarks>
         /// Override this method to provide custom logic to determine which methods are considered actions.
         /// </remarks>
-        protected virtual bool IsAction([NotNull] MethodInfo methodInfo)
+        protected virtual bool IsAction([NotNull] TypeInfo typeInfo, [NotNull] MethodInfo methodInfo)
         {
+            // The SpecialName bit is set to flag members that are treated in a special way by some compilers
+            // (such as property accessors and operator overloading methods).
+            if (methodInfo.IsSpecialName)
+            {
+                return false;
+            }
+
+            if (methodInfo.IsDefined(typeof(NonActionAttribute)))
+            {
+                return false;
+            }
+
+            // Overriden methods from Object class, e.g. Equals(Object), GetHashCode(), etc., are not valid.
+            if (methodInfo.GetBaseDefinition().DeclaringType == typeof(object))
+            {
+                return false;
+            }
+
+            // Dispose method implemented from IDisposable is not valid
+            if (IsIDisposableMethod(methodInfo, typeInfo))
+            {
+                return false;
+            }
+
+            if (methodInfo.IsStatic)
+            {
+                return false;
+            }
+
+            if (methodInfo.IsAbstract)
+            {
+                return false;
+            }
+
+            if (methodInfo.IsConstructor)
+            {
+                return false;
+            }
+
+            if (methodInfo.IsGenericMethod)
+            {
+                return false;
+            }
+
             return
-                methodInfo.IsPublic &&
+                methodInfo.IsPublic;
-                !methodInfo.IsStatic &&
-                !methodInfo.IsAbstract &&
-                !methodInfo.IsConstructor &&
-                !methodInfo.IsGenericMethod &&
-
-                // The SpecialName bit is set to flag members that are treated in a special way by some compilers
-                // (such as property accessors and operator overloading methods).
-                !methodInfo.IsSpecialName &&
-                !methodInfo.IsDefined(typeof(NonActionAttribute)) &&
-
-                // Overriden methods from Object class, e.g. Equals(Object), GetHashCode(), etc., are not valid.
-                methodInfo.GetBaseDefinition().DeclaringType != typeof(object);
         }
 
-        /// <summary>
+        private bool IsIDisposableMethod(MethodInfo methodInfo, TypeInfo typeInfo)
+        {
+            return
+                (typeof(IDisposable).GetTypeInfo().IsAssignableFrom(typeInfo) &&
+                 typeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0] == methodInfo);
+        }
+
+
+        /// <suethodandlemary>
         /// Creates an <see cref="ActionModel"/> for the given <see cref="MethodInfo"/>.
         /// </summary>
         /// <param name="methodInfo">The <see cref="MethodInfo"/>.</param>

src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs

@@ -37,7 +37,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
 
             foreach (var methodInfo in typeInfo.AsType().GetMethods())
             {
-                var actionModels = _actionModelBuilder.BuildActionModels(methodInfo);
+                var actionModels = _actionModelBuilder.BuildActionModels(typeInfo, methodInfo);
                 if (actionModels != null)
                 {
                     foreach (var actionModel in actionModels)

src/Microsoft.AspNet.Mvc.Core/ApplicationModels/IActionModelBuilder.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Generic;
 using System.Reflection;
 
@@ -16,11 +17,12 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
         /// <paramref name="methodInfo"/> is not an action method.
         /// </summary>
         /// <param name="methodInfo">The <see cref="MethodInfo"/>.</param>
+        /// <param name="typeInfo">The <see cref="TypeInfo"/>.</param>
         /// <returns>A set of <see cref="ActionModel"/> or null.</returns>
         /// <remarks>
         /// Instances of <see cref="ActionModel"/> returned from this interface should have their
         /// <see cref="ActionModel.Parameters"/> initialized. 
         /// </remarks>
-        IEnumerable<ActionModel> BuildActionModels([NotNull] MethodInfo methodInfo);
+        IEnumerable<ActionModel> BuildActionModels([NotNull] TypeInfo typeInfo, [NotNull] MethodInfo methodInfo);
     }
 }

src/Microsoft.AspNet.Mvc.Core/Controller.cs

@@ -720,7 +720,6 @@ namespace Microsoft.AspNet.Mvc
                                                                 bindingContext.ValidatorProvider);
         }
 
-        [NonAction]
         public void Dispose()
         {
             Dispose(disposing: true);

src/Microsoft.AspNet.Mvc.WebApiCompatShim/ApiController.cs

@@ -372,7 +372,6 @@ namespace System.Web.Http
             return new HttpStatusCodeResult((int)status);
         }
 
-        [NonAction]
         public void Dispose()
         {
             Dispose(true);

test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs

@@ -23,7 +23,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.NotNull(method);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(DerivedController).GetTypeInfo(), method);
 
             // Assert
             Assert.Equal(expected, isValid);
@@ -38,7 +38,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.NotNull(method);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(BaseController).GetTypeInfo(), method);
 
             // Assert
             Assert.False(isValid);
@@ -55,7 +55,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.NotNull(method);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(DerivedController).GetTypeInfo(), method);
 
             // Assert
             Assert.False(isValid);
@@ -71,7 +71,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.True(method.IsSpecialName);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(OperatorOverloadingController).GetTypeInfo(), method);
 
             // Assert
             Assert.False(isValid);
@@ -86,7 +86,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.NotNull(method);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(DerivedController).GetTypeInfo(), method);
 
             // Assert
             Assert.False(isValid);
@@ -101,7 +101,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.NotNull(method);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(DerivedController).GetTypeInfo(), method);
 
             // Assert
             Assert.False(isValid);
@@ -122,12 +122,144 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.NotNull(method);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(DerivedController).GetTypeInfo(), method);
 
             // Assert
             Assert.False(isValid);
         }
 
+        [Fact]
+        public void IsAction_DerivedControllerIDisposableDisposeMethod()
+        {
+            // Arrange
+            var builder = new AccessibleActionModelBuilder();
+            var typeInfo = typeof(DerivedController).GetTypeInfo();
+            var methodInfo =
+                typeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0];
+            var method = typeInfo.GetMethods().Where(m => (m == methodInfo)).SingleOrDefault();
+            Assert.NotNull(method);
+
+            // Act
+            var isValid = builder.IsAction(typeInfo, method);
+
+            // Assert
+            Assert.False(isValid);
+        }
+
+        [Fact]
+        public void IsAction_DerivedControllerDisposeMethod()
+        {
+            // Arrange
+            var builder = new AccessibleActionModelBuilder();
+            var typeInfo = typeof(DerivedController).GetTypeInfo();
+            var methodInfo =
+                typeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0];
+            var methods = typeInfo.GetMethods().Where(m => m.Name.Equals("Dispose") && m != methodInfo);
+
+            Assert.NotEmpty(methods);
+
+            foreach (var method in methods)
+            {
+                // Act
+                var isValid = builder.IsAction(typeInfo, method);
+
+                // Assert
+                Assert.True(isValid);
+            }
+        }
+
+        [Fact]
+        public void IsAction_OverriddenDisposeMethod()
+        {
+            // Arrange
+            var builder = new AccessibleActionModelBuilder();
+            var typeInfo = typeof(DerivedOverriddenDisposeController).GetTypeInfo();
+            var method = typeInfo.GetDeclaredMethods("Dispose").SingleOrDefault();
+            Assert.NotNull(method);
+
+            // Act
+            var isValid = builder.IsAction(typeInfo, method);
+
+            // Assert
+            Assert.False(isValid);
+        }
+
+        [Fact]
+        public void IsAction_NewDisposeMethod()
+        {
+            // Arrange
+            var builder = new AccessibleActionModelBuilder();
+            var typeInfo = typeof(DerivedNewDisposeController).GetTypeInfo();
+            var method = typeInfo.GetDeclaredMethods("Dispose").SingleOrDefault();
+            Assert.NotNull(method);
+
+            // Act
+            var isValid = builder.IsAction(typeInfo, method);
+
+            // Assert
+            Assert.True(isValid);
+        }
+
+        [Fact]
+        public void IsAction_PocoControllerIDisposableDisposeMethod()
+        {
+            // Arrange
+            var builder = new AccessibleActionModelBuilder();
+            var typeInfo = typeof(IDisposablePocoController).GetTypeInfo();
+            var methodInfo =
+                typeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0];
+            var method = typeInfo.GetMethods().Where(m => (m == methodInfo)).SingleOrDefault();
+            Assert.NotNull(method);
+
+            // Act
+            var isValid = builder.IsAction(typeInfo, method);
+
+            // Assert
+            Assert.False(isValid);
+        }
+
+        [Fact]
+        public void IsAction_PocoControllerDisposeMethod()
+        {
+            // Arrange
+            var builder = new AccessibleActionModelBuilder();
+            var typeInfo = typeof(IDisposablePocoController).GetTypeInfo();
+            var methodInfo =
+                typeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0];
+            var methods = typeInfo.GetMethods().Where(m => m.Name.Equals("Dispose") && m != methodInfo);
+
+            Assert.NotEmpty(methods);
+
+            foreach (var method in methods)
+            {
+                // Act
+                var isValid = builder.IsAction(typeInfo, method);
+
+                // Assert
+                Assert.True(isValid);
+            }
+        }
+
+        [Fact]
+        public void IsAction_SimplePocoControllerDisposeMethod()
+        {
+            // Arrange
+            var builder = new AccessibleActionModelBuilder();
+            var typeInfo = typeof(SimplePocoController).GetTypeInfo();
+            var methods = typeInfo.GetMethods().Where(m => m.Name.Equals("Dispose"));
+
+            Assert.NotEmpty(methods);
+
+            foreach (var method in methods)
+            {
+                // Act
+                var isValid = builder.IsAction(typeInfo, method);
+
+                // Assert
+                Assert.True(isValid);
+            }
+        }
+
         [Theory]
         [InlineData("StaticMethod")]
         [InlineData("ProtectedStaticMethod")]
@@ -142,7 +274,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             Assert.NotNull(method);
 
             // Act
-            var isValid = builder.IsAction(method);
+            var isValid = builder.IsAction(typeof(DerivedController).GetTypeInfo(), method);
 
             // Assert
             Assert.False(isValid);
@@ -157,7 +289,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(ConventionallyRoutedController.Edit);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -177,7 +309,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(ConventionallyRoutedController.Update);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -199,7 +331,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(ConventionallyRoutedController.Delete);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -222,7 +354,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(ConventionallyRoutedController.Details);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -242,7 +374,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(ConventionallyRoutedController.List);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -263,7 +395,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(NoRouteAttributeOnControllerController.Edit);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -289,7 +421,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(NoRouteAttributeOnControllerController.Update);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -314,7 +446,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(NoRouteAttributeOnControllerController.List);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -339,7 +471,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(NoRouteAttributeOnControllerController.Index);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             Assert.Equal(2, actions.Count());
@@ -372,7 +504,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var actionName = nameof(NoRouteAttributeOnControllerController.Remove);
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod(actionName));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod(actionName));
 
             // Assert
             var action = Assert.Single(actions);
@@ -397,7 +529,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var typeInfo = controller.GetTypeInfo();
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod("Delete"));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod("Delete"));
 
             // Assert
             var action = Assert.Single(actions);
@@ -422,7 +554,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             var typeInfo = controller.GetTypeInfo();
 
             // Act
-            var actions = builder.BuildActionModels(typeInfo.GetMethod("Index"));
+            var actions = builder.BuildActionModels(typeInfo, typeInfo.GetMethod("Index"));
 
             // Assert
             Assert.Equal(2, actions.Count());
@@ -446,9 +578,9 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
 
         private class AccessibleActionModelBuilder : DefaultActionModelBuilder
         {
-            public new bool IsAction([NotNull]MethodInfo methodInfo)
+            public new bool IsAction([NotNull] TypeInfo typeInfo, [NotNull]MethodInfo methodInfo)
             {
-                return base.IsAction(methodInfo);
+                return base.IsAction(typeInfo, methodInfo);
             }
         }
 
@@ -508,6 +640,80 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels
             private static void PrivateStaticMethod()
             {
             }
+
+            public string Dispose(string s)
+            {
+                return s;
+            }
+
+            public new void Dispose()
+            {
+            }
+        }
+
+        private class IDisposablePocoController : IDisposable
+        {
+            public string Index()
+            {
+                return "Hello world";
+            }
+
+            public void Dispose()
+            {
+                Dispose(disposing: true);
+                GC.SuppressFinalize(this);
+            }
+
+            protected virtual void Dispose(bool disposing)
+            {
+            }
+            public string Dispose(string s)
+            {
+                return s;
+            }
+        }
+
+        private class BaseClass : IDisposable
+        {
+            public virtual void Dispose()
+            {
+                Dispose(disposing: true);
+                GC.SuppressFinalize(this);
+            }
+            protected virtual void Dispose(bool disposing)
+            {
+            }
+        }
+        private class DerivedOverriddenDisposeController : BaseClass
+        {
+            public override void Dispose()
+            {
+                base.Dispose();
+            }
+        }
+
+        private class DerivedNewDisposeController : BaseClass
+        {
+            public new void Dispose()
+            {
+                base.Dispose();
+            }
+        }
+
+        private class SimplePocoController
+        {
+            public string Index()
+            {
+                return "Hello world";
+            }
+
+            public void Dispose()
+            {
+            }
+
+            public void Dispose(string s)
+            {
+            }
         }
 
         private class OperatorOverloadingController : Mvc.Controller

test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs

@@ -28,7 +28,9 @@ namespace Microsoft.AspNet.Mvc.Test
             {
                 return typeof(Controller).GetTypeInfo()
                     .DeclaredMethods
-                    .Where(method => method.IsPublic && !method.IsSpecialName)
+                    .Where(method => method.IsPublic && 
+                    !method.IsSpecialName && 
+                    !method.Name.Equals("Dispose", StringComparison.OrdinalIgnoreCase))
                     .Select(method => new[] { method });
             }
         }