From 264371f6b8e6b11db5205cb40a5f9c72fb96a4d2 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Wed, 11 Feb 2015 22:54:38 -0800 Subject: [PATCH] * Modify DefaultControllerTypeProvider to look at the object graph to determine if any ancestor has the "Controller" suffix. * Introduce NonControllerAttribute to opt out of Controller detection. Fixes #1274 --- .../DefaultControllerTypeProvider.cs | 35 +++- .../NonControllerAttribute.cs | 16 ++ .../DefaultControllerTypeProviderTest.cs | 155 +++++++++++++++++- .../BasicTests.cs | 31 +++- .../ControllerFromServicesTests.cs | 24 ++- .../Controllers/ApplicationBaseController.cs | 10 ++ .../BasicWebSite/Controllers/Appointments.cs | 20 +++ .../Controllers/SqlDataController.cs | 17 ++ .../ClientUIStubController.cs | 16 ++ .../Inventory.cs | 16 ++ .../ResourcesController.cs | 13 ++ 11 files changed, 344 insertions(+), 9 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/NonControllerAttribute.cs create mode 100644 test/WebSites/BasicWebSite/Controllers/ApplicationBaseController.cs create mode 100644 test/WebSites/BasicWebSite/Controllers/Appointments.cs create mode 100644 test/WebSites/BasicWebSite/Controllers/SqlDataController.cs create mode 100644 test/WebSites/ControllersFromServicesClassLibrary/ClientUIStubController.cs create mode 100644 test/WebSites/ControllersFromServicesClassLibrary/Inventory.cs create mode 100644 test/WebSites/ControllersFromServicesClassLibrary/ResourcesController.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerTypeProvider.cs b/src/Microsoft.AspNet.Mvc.Core/DefaultControllerTypeProvider.cs index 4e4ae48742..212eb6936b 100644 --- a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerTypeProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/DefaultControllerTypeProvider.cs @@ -16,6 +16,7 @@ namespace Microsoft.AspNet.Mvc /// public class DefaultControllerTypeProvider : IControllerTypeProvider { + private const string ControllerTypeName = nameof(Controller); private readonly IAssemblyProvider _assemblyProvider; private readonly ILogger _logger; @@ -76,17 +77,45 @@ protected internal virtual bool IsController([NotNull] TypeInfo typeInfo) { return false; } - if (typeInfo.Name.Equals("Controller", StringComparison.OrdinalIgnoreCase)) + if (typeInfo.Name.Equals(ControllerTypeName, StringComparison.OrdinalIgnoreCase)) { return false; } - if (!typeInfo.Name.EndsWith("Controller", StringComparison.OrdinalIgnoreCase) && - !typeof(Controller).GetTypeInfo().IsAssignableFrom(typeInfo)) + if (!typeInfo.Name.EndsWith(ControllerTypeName, StringComparison.OrdinalIgnoreCase) && + !DerivesFromController(typeInfo)) + { + return false; + } + if (typeInfo.IsDefined(typeof(NonControllerAttribute))) { return false; } return true; } + + private static bool DerivesFromController(TypeInfo typeInfo) + { + // A type is a controller if it derives from a type that is either named "Controller" or has the suffix + // "Controller". We'll optimize the most common case of types deriving from the Mvc Controller type and + // walk up the object graph if that's not the case. + if (typeof(Controller).GetTypeInfo().IsAssignableFrom(typeInfo)) + { + return true; + } + + while (typeInfo != typeof(object).GetTypeInfo()) + { + var baseTypeInfo = typeInfo.BaseType.GetTypeInfo(); + if (baseTypeInfo.Name.EndsWith(ControllerTypeName, StringComparison.Ordinal)) + { + return true; + } + + typeInfo = baseTypeInfo; + } + + return false; + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/NonControllerAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/NonControllerAttribute.cs new file mode 100644 index 0000000000..cd924a7807 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/NonControllerAttribute.cs @@ -0,0 +1,16 @@ +// 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; + +namespace Microsoft.AspNet.Mvc +{ + /// + /// Indicates that the type and any derived types that this attribute is applied to + /// is not considered a controller by the default controller discovery mechanism. + /// + [AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)] + public sealed class NonControllerAttribute : Attribute + { + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/DefaultControllerTypeProviderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/DefaultControllerTypeProviderTest.cs index 04357a1a0a..a462059e4a 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/DefaultControllerTypeProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/DefaultControllerTypeProviderTest.cs @@ -1,7 +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.Linq; +using System; using System.Reflection; using Microsoft.AspNet.Mvc.DefaultControllerTypeProviderControllers; using Xunit; @@ -108,6 +108,20 @@ public void IsController_OpenGenericClass() Assert.False(isController); } + [Fact] + public void IsController_WithoutSuffixOrAncestorWithController() + { + // Arrange + var typeInfo = typeof(NoSuffixPoco).GetTypeInfo(); + var provider = GetControllerTypeProvider(); + + // Act + var isController = provider.IsController(typeInfo); + + // Assert + Assert.False(isController); + } + [Fact] public void IsController_ClosedGenericClass() { @@ -164,6 +178,55 @@ public void IsController_NoControllerSuffix() Assert.True(isController); } + public static TheoryData TypesWithPocoControllerAncestor + { + get + { + return new TheoryData + { + typeof(ChildWithoutSuffix).GetTypeInfo(), + typeof(DescendantLevel1).GetTypeInfo(), + typeof(DescendantLevel2).GetTypeInfo() + }; + } + } + + [Theory] + [InlineData(typeof(ChildWithoutSuffix))] + [InlineData(typeof(DescendantLevel1))] + [InlineData(typeof(DescendantLevel2))] + public void IsController_ReturnsTrue_IfAncestorTypeNameHasControllerSuffix(Type type) + { + // Arrange + var provider = GetControllerTypeProvider(); + + // Act + var isController = provider.IsController(type.GetTypeInfo()); + + // Assert + Assert.True(isController); + } + + [Theory] + [InlineData(typeof(BaseNonControllerController))] + [InlineData(typeof(BaseNonControllerControllerChild))] + [InlineData(typeof(BasePocoNonControllerController))] + [InlineData(typeof(BasePocoNonControllerControllerChild))] + [InlineData(typeof(NonController))] + [InlineData(typeof(NonControllerChild))] + [InlineData(typeof(PersonModel))] + public void IsController_ReturnsFalse_IfTypeOrAncestorHasNonControllerAttribute(Type type) + { + // Arrange + var provider = GetControllerTypeProvider(); + + // Act + var isController = provider.IsController(type.GetTypeInfo()); + + // Assert + Assert.False(isController); + } + private static DefaultControllerTypeProvider GetControllerTypeProvider() { var assemblyProvider = new FixedSetAssemblyProvider(); @@ -211,7 +274,97 @@ public class NoSuffix : Mvc.Controller { } + public class NoSuffixPoco + { + + } + public class PocoController { } + + public class CustomBaseController + { + + } + + public abstract class CustomAbstractBaseController + { + + } + + public class ChildWithoutSuffix : CustomBaseController + { + + } + + public class DescendantLevel1 : CustomBaseController + { + + } + + public class DescendantLevel2 : DescendantLevel1 + { + + } + + public class AbstractChildWithoutSuffix : CustomAbstractBaseController + { + + } + + [NonController] + public class BasePocoNonControllerController + { + + } + + public class BasePocoNonControllerControllerChild : BasePocoNonControllerController + { + + } + + [NonController] + public class BaseNonControllerController : Controller + { + + } + + public class BaseNonControllerControllerChild : BaseNonControllerController + { + + } + + [NonController] + public class PocoNonController + { + + } + + [NonController] + public class NonControllerChild : Controller + { + + } + + [NonController] + public class NonController : Controller + { + + } + + public class DataModelBase + { + + } + + public class EntityDataModel : DataModelBase + { + + } + + public class PersonModel : EntityDataModel + { + + } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs index bd28688ced..907c67cc97 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/BasicTests.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Net; using System.Net.Http; using System.Net.Http.Headers; @@ -18,7 +19,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests { public class BasicTests { - private readonly IServiceProvider _provider = TestHelper.CreateServices("BasicWebSite"); + private readonly IServiceProvider _provider = TestHelper.CreateServices(nameof(BasicWebSite)); private readonly Action _app = new Startup().Configure; // Some tests require comparing the actual response body against an expected response baseline @@ -275,5 +276,33 @@ public async Task ConfigureMvcOptionsAddsOptionsProperly() var responseData = await response.Content.ReadAsStringAsync(); Assert.Equal("This is a basic website.", responseData); } + + [Fact] + public async Task TypesWithoutControllerSuffix_DerivingFromTypesWithControllerSuffix_CanBeAccessed() + { + // Arrange + var server = TestServer.Create(_provider, _app); + var client = new HttpClient(server.CreateHandler(), false); + + // Act + var response = await client.GetStringAsync("http://localhost/appointments"); + + // Assert + Assert.Equal("2 appointments available.", response); + } + + [Fact] + public async Task TypesMarkedAsNonAction_AreInaccessible() + { + // Arrange + var server = TestServer.Create(_provider, _app); + var client = new HttpClient(server.CreateHandler(), false); + + // Act + var response = await client.GetAsync("http://localhost/SqlData/TruncateAllDbRecords"); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ControllerFromServicesTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ControllerFromServicesTests.cs index bf1c47962e..f7e94fa191 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ControllerFromServicesTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ControllerFromServicesTests.cs @@ -49,6 +49,21 @@ public async Task TypesDerivingFromControllerAreRegistered() Assert.Equal(expected, response); } + [Fact] + public async Task TypesDerivingFromControllerPrefixedTypesAreRegistered() + { + // Arrange + var expected = "4"; + var server = TestServer.Create(_provider, _app); + var client = server.CreateClient(); + + // Act + var response = await client.GetStringAsync("http://localhost/inventory/"); + + // Assert + Assert.Equal(expected, response); + } + [Fact] public async Task TypesWithControllerSuffixAreRegistered() { @@ -84,9 +99,10 @@ public async Task TypesWithControllerSuffixAreConventionalRouted() } [Theory] - [InlineData("generic")] - [InlineData("nested")] - [InlineData("not-in-services")] + [InlineData("not-discovered/generic")] + [InlineData("not-discovered/nested")] + [InlineData("not-discovered/not-in-services")] + [InlineData("ClientUIStub/GetClientContent/5")] public async Task AddControllersFromServices_UsesControllerDiscoveryContentions(string action) { // Arrange @@ -94,7 +110,7 @@ public async Task AddControllersFromServices_UsesControllerDiscoveryContentions( var client = server.CreateClient(); // Act - var response = await client.GetAsync("http://localhost/not-discovered/" + action); + var response = await client.GetAsync("http://localhost/" + action); // Assert Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); diff --git a/test/WebSites/BasicWebSite/Controllers/ApplicationBaseController.cs b/test/WebSites/BasicWebSite/Controllers/ApplicationBaseController.cs new file mode 100644 index 0000000000..5eaf0265a9 --- /dev/null +++ b/test/WebSites/BasicWebSite/Controllers/ApplicationBaseController.cs @@ -0,0 +1,10 @@ +// 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. + +namespace BasicWebSite.Controllers +{ + public abstract class ApplicationBaseController + { + + } +} \ No newline at end of file diff --git a/test/WebSites/BasicWebSite/Controllers/Appointments.cs b/test/WebSites/BasicWebSite/Controllers/Appointments.cs new file mode 100644 index 0000000000..d4a2bb835a --- /dev/null +++ b/test/WebSites/BasicWebSite/Controllers/Appointments.cs @@ -0,0 +1,20 @@ +// 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 Microsoft.AspNet.Mvc; + +namespace BasicWebSite.Controllers +{ + [Route("/appointments")] + public class Appointments : ApplicationBaseController + { + [HttpGet("")] + public IActionResult Get() + { + return new ContentResult + { + Content = "2 appointments available." + }; + } + } +} \ No newline at end of file diff --git a/test/WebSites/BasicWebSite/Controllers/SqlDataController.cs b/test/WebSites/BasicWebSite/Controllers/SqlDataController.cs new file mode 100644 index 0000000000..8378caab44 --- /dev/null +++ b/test/WebSites/BasicWebSite/Controllers/SqlDataController.cs @@ -0,0 +1,17 @@ +// 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 Microsoft.AspNet.Mvc; + +namespace BasicWebSite +{ + [NonController] + public class SqlDataController + { + public int TruncateAllDbRecords() + { + // Return no. of tables truncated + return 7; + } + } +} \ No newline at end of file diff --git a/test/WebSites/ControllersFromServicesClassLibrary/ClientUIStubController.cs b/test/WebSites/ControllersFromServicesClassLibrary/ClientUIStubController.cs new file mode 100644 index 0000000000..8e0d8f2a8f --- /dev/null +++ b/test/WebSites/ControllersFromServicesClassLibrary/ClientUIStubController.cs @@ -0,0 +1,16 @@ +// 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 Microsoft.AspNet.Mvc; + +namespace ControllersFromServicesClassLibrary +{ + [NonController] + public class ClientUIStubController + { + public object GetClientContent(int id) + { + return new object(); + } + } +} diff --git a/test/WebSites/ControllersFromServicesClassLibrary/Inventory.cs b/test/WebSites/ControllersFromServicesClassLibrary/Inventory.cs new file mode 100644 index 0000000000..41126acb20 --- /dev/null +++ b/test/WebSites/ControllersFromServicesClassLibrary/Inventory.cs @@ -0,0 +1,16 @@ +// 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 Microsoft.AspNet.Mvc; + +namespace ControllersFromServicesClassLibrary +{ + public class Inventory : ResourcesController + { + [HttpGet] + public int Get() + { + return 4; + } + } +} \ No newline at end of file diff --git a/test/WebSites/ControllersFromServicesClassLibrary/ResourcesController.cs b/test/WebSites/ControllersFromServicesClassLibrary/ResourcesController.cs new file mode 100644 index 0000000000..54a046fa08 --- /dev/null +++ b/test/WebSites/ControllersFromServicesClassLibrary/ResourcesController.cs @@ -0,0 +1,13 @@ +// 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 Microsoft.AspNet.Mvc; + +namespace ControllersFromServicesClassLibrary +{ + [Route("/[controller]")] + public class ResourcesController + { + + } +} \ No newline at end of file