From 45985056523d589cb8786020a4bc1ca8c3845d8d Mon Sep 17 00:00:00 2001 From: kanchanm Date: Thu, 30 Oct 2014 22:23:51 -0700 Subject: [PATCH] Fix to special case Dispose method to be treated as non-action --- .../DefaultActionModelBuilder.cs | 76 ++++-- .../DefaultControllerModelBuilder.cs | 2 +- .../ApplicationModels/IActionModelBuilder.cs | 4 +- src/Microsoft.AspNet.Mvc.Core/Controller.cs | 1 - .../ApiController.cs | 1 - .../DefaultActionModelBuilderTest.cs | 250 ++++++++++++++++-- .../ControllerTests.cs | 4 +- 7 files changed, 294 insertions(+), 44 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs index 2ce7d90a18..d647cfb450 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs +++ b/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 { /// - public IEnumerable BuildActionModels([NotNull] MethodInfo methodInfo) + public IEnumerable BuildActionModels([NotNull] TypeInfo typeInfo, [NotNull] MethodInfo methodInfo) { - if (!IsAction(methodInfo)) + if (!IsAction(typeInfo, methodInfo)) { return Enumerable.Empty(); } @@ -131,29 +132,70 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels /// Returns true if the is an action. Otherwise false. /// /// The . + /// The . /// true if the is an action. Otherwise false. /// /// Override this method to provide custom logic to determine which methods are considered actions. /// - 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.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); + methodInfo.IsPublic; } - /// + private bool IsIDisposableMethod(MethodInfo methodInfo, TypeInfo typeInfo) + { + return + (typeof(IDisposable).GetTypeInfo().IsAssignableFrom(typeInfo) && + typeInfo.GetRuntimeInterfaceMap(typeof(IDisposable)).TargetMethods[0] == methodInfo); + } + + + /// /// Creates an for the given . /// /// The . diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs index 464c05bc87..d7a4344347 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs +++ b/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) diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/IActionModelBuilder.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/IActionModelBuilder.cs index ad17c6fafc..b6585b5439 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/IActionModelBuilder.cs +++ b/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 /// is not an action method. /// /// The . + /// The . /// A set of or null. /// /// Instances of returned from this interface should have their /// initialized. /// - IEnumerable BuildActionModels([NotNull] MethodInfo methodInfo); + IEnumerable BuildActionModels([NotNull] TypeInfo typeInfo, [NotNull] MethodInfo methodInfo); } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Controller.cs b/src/Microsoft.AspNet.Mvc.Core/Controller.cs index b0e74b6fd4..6c3bb03aef 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Controller.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Controller.cs @@ -720,7 +720,6 @@ namespace Microsoft.AspNet.Mvc bindingContext.ValidatorProvider); } - [NonAction] public void Dispose() { Dispose(disposing: true); diff --git a/src/Microsoft.AspNet.Mvc.WebApiCompatShim/ApiController.cs b/src/Microsoft.AspNet.Mvc.WebApiCompatShim/ApiController.cs index e6298979ce..15daa18ec0 100644 --- a/src/Microsoft.AspNet.Mvc.WebApiCompatShim/ApiController.cs +++ b/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); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs index ef963044b6..16facdff4a 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/DefaultActionModelBuilderTest.cs +++ b/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 diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs index 3631a53391..470800e313 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs +++ b/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 }); } }