From bb3e89b85f4cb4a58580d7a491a83afc74218b88 Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Thu, 18 Feb 2021 02:10:52 +0800 Subject: [PATCH 1/7] Remove existing controllers if expose service self --- .../Mvc/Conventions/AbpServiceConvention.cs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs index 4d1490ec40..f2367e08ae 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs @@ -73,7 +73,7 @@ namespace Volo.Abp.AspNetCore.Mvc.Conventions protected virtual void RemoveDuplicateControllers(ApplicationModel application) { - var derivedControllerModels = new List(); + var controllerModelsToRemove = new List(); foreach (var controllerModel in application.Controllers) { @@ -87,6 +87,18 @@ namespace Volo.Abp.AspNetCore.Mvc.Conventions continue; } + var exposeServicesAttr = ReflectionHelper.GetSingleAttributeOrDefault(controllerModel.ControllerType); + if (exposeServicesAttr.IncludeSelf) + { + var existingControllerModels = application.Controllers + .Where(cm => exposeServicesAttr.ServiceTypes.Contains(cm.ControllerType)) + .ToArray(); + + controllerModelsToRemove.AddRange(existingControllerModels); + Logger.LogInformation($"Removing the controller{(existingControllerModels.Length > 1 ? "s" : "")} {exposeServicesAttr.ServiceTypes.Select(c => c.AssemblyQualifiedName).JoinAsString(", ")} from the application model since {(existingControllerModels.Length > 1 ? "they are" : "it is")} replaced by the controller: {controllerModel.ControllerType.AssemblyQualifiedName}"); + continue; + } + var baseControllerTypes = controllerModel.ControllerType .GetBaseClasses(typeof(Controller), includeObject: false) .Where(t => !t.IsAbstract) @@ -94,12 +106,12 @@ namespace Volo.Abp.AspNetCore.Mvc.Conventions if (baseControllerTypes.Length > 0) { - derivedControllerModels.Add(controllerModel); + controllerModelsToRemove.Add(controllerModel); Logger.LogInformation($"Removing the controller {controllerModel.ControllerType.AssemblyQualifiedName} from the application model since it replaces the controller(s): {baseControllerTypes.Select(c => c.AssemblyQualifiedName).JoinAsString(", ")}"); } } - application.Controllers.RemoveAll(derivedControllerModels); + application.Controllers.RemoveAll(controllerModelsToRemove); } protected virtual void ConfigureRemoteService(ControllerModel controller, [CanBeNull] ConventionalControllerSetting configuration) From c5049939467511e294f00d625619aea866acaa69 Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Thu, 18 Feb 2021 02:16:42 +0800 Subject: [PATCH 2/7] Only remove controller if has base controller model Some third-party base controllers are non-abstract. --- .../Mvc/Conventions/AbpServiceConvention.cs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs index f2367e08ae..383545dc91 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs @@ -104,11 +104,22 @@ namespace Volo.Abp.AspNetCore.Mvc.Conventions .Where(t => !t.IsAbstract) .ToArray(); - if (baseControllerTypes.Length > 0) + if (baseControllerTypes.Length == 0) { - controllerModelsToRemove.Add(controllerModel); - Logger.LogInformation($"Removing the controller {controllerModel.ControllerType.AssemblyQualifiedName} from the application model since it replaces the controller(s): {baseControllerTypes.Select(c => c.AssemblyQualifiedName).JoinAsString(", ")}"); + continue; + } + + var baseControllerModels = application.Controllers + .Where(cm => baseControllerTypes.Contains(cm.ControllerType)) + .ToArray(); + + if (baseControllerModels.Length == 0) + { + continue; } + + controllerModelsToRemove.Add(controllerModel); + Logger.LogInformation($"Removing the controller {controllerModel.ControllerType.AssemblyQualifiedName} from the application model since it replaces the controller(s): {baseControllerTypes.Select(c => c.AssemblyQualifiedName).JoinAsString(", ")}"); } application.Controllers.RemoveAll(controllerModelsToRemove); From 67a05b999b29e5b034aca788ab3b42f04fc8f40f Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Thu, 18 Feb 2021 02:35:55 +0800 Subject: [PATCH 3/7] Fix typo of AccountController to MyAccountController --- .../en/Customizing-Application-Modules-Overriding-Services.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/en/Customizing-Application-Modules-Overriding-Services.md b/docs/en/Customizing-Application-Modules-Overriding-Services.md index ab6f29816b..892dcc35c8 100644 --- a/docs/en/Customizing-Application-Modules-Overriding-Services.md +++ b/docs/en/Customizing-Application-Modules-Overriding-Services.md @@ -196,12 +196,12 @@ This example replaces the `AccountController` (An API Controller defined in the **`[ExposeServices(typeof(AccountController))]` is essential** here since it registers this controller for the `AccountController` in the dependency injection system. `[Dependency(ReplaceServices = true)]` is also recommended to clear the old registration (even the ASP.NET Core DI system selects the last registered one). -In addition, The `AccountController` will be removed from [`ApplicationModel`](https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.applicationmodels.applicationmodel.controllers) because it defines `ExposeServicesAttribute`. If you don't want to remove it, you can configure `AbpAspNetCoreMvcOptions`: +In addition, the `MyAccountController` will be removed from [`ApplicationModel`](https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.applicationmodels.applicationmodel.controllers) because it defines `ExposeServicesAttribute`. If you don't want to remove it, you can configure `AbpAspNetCoreMvcOptions`: ```csharp Configure(options => { - options.IgnoredControllersOnModelExclusion.AddIfNotContains(typeof(AccountController)); + options.IgnoredControllersOnModelExclusion.AddIfNotContains(typeof(MyAccountController)); }); ``` From 08d7abdd33ea25439b8ee2029ced91395b7a2830 Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Thu, 18 Feb 2021 02:39:16 +0800 Subject: [PATCH 4/7] Mention the behaviour for extending a controller --- .../Customizing-Application-Modules-Overriding-Services.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/en/Customizing-Application-Modules-Overriding-Services.md b/docs/en/Customizing-Application-Modules-Overriding-Services.md index 892dcc35c8..081b142ad7 100644 --- a/docs/en/Customizing-Application-Modules-Overriding-Services.md +++ b/docs/en/Customizing-Application-Modules-Overriding-Services.md @@ -196,7 +196,11 @@ This example replaces the `AccountController` (An API Controller defined in the **`[ExposeServices(typeof(AccountController))]` is essential** here since it registers this controller for the `AccountController` in the dependency injection system. `[Dependency(ReplaceServices = true)]` is also recommended to clear the old registration (even the ASP.NET Core DI system selects the last registered one). -In addition, the `MyAccountController` will be removed from [`ApplicationModel`](https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.applicationmodels.applicationmodel.controllers) because it defines `ExposeServicesAttribute`. If you don't want to remove it, you can configure `AbpAspNetCoreMvcOptions`: +In addition, the `MyAccountController` will be removed from [`ApplicationModel`](https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.applicationmodels.applicationmodel.controllers) because it defines `ExposeServicesAttribute`. + +If `IncludeSelf = true` is specified, i.e. `[ExposeServices(typeof(AccountController), IncludeSelf = true)]`, then `AccountController` will be removed instead. This is useful for **extending** a controller. + +If you don't want to remove either controller, you can configure `AbpAspNetCoreMvcOptions`: ```csharp Configure(options => From ebe92d3bb3f30e6cff614c540bda3a4b2fc65392 Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Thu, 18 Feb 2021 22:09:48 +0800 Subject: [PATCH 5/7] Add tests for AbpServiceConvention --- .../Conventions/AbpServiceConvention_Tests.cs | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs new file mode 100644 index 0000000000..f16e8a1df9 --- /dev/null +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs @@ -0,0 +1,91 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ApplicationModels; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Shouldly; +using System; +using System.Reflection; +using Volo.Abp.DependencyInjection; +using Xunit; + +namespace Volo.Abp.AspNetCore.Mvc.Conventions +{ + public class AbpServiceConvention_Tests : AspNetCoreMvcTestBase + { + private readonly IConventionalRouteBuilder _conventionalRouteBuilder; + private readonly IOptions _options; + + public AbpServiceConvention_Tests() + { + _conventionalRouteBuilder = GetRequiredService(); + _options = GetRequiredService>(); + } + + [Fact] + public void Should_Not_Remove_Derived_Controller_If_Not_Expose_Service() + { + // Arrange + var applicationModel = new ApplicationModel(); + var baseControllerModel = new ControllerModel(typeof(BaseController).GetTypeInfo(), Array.Empty()) + { + Application = applicationModel + }; + applicationModel.Controllers.Add(baseControllerModel); + + var derivedControllerModel = new ControllerModel(typeof(DerivedController).GetTypeInfo(), Array.Empty()) + { + Application = applicationModel + }; + applicationModel.Controllers.Add(derivedControllerModel); + + var abpServiceConvention = new AbpServiceConvention(_options, _conventionalRouteBuilder); + + // Act + abpServiceConvention.Apply(applicationModel); + + // Assert + applicationModel.Controllers.ShouldContain(baseControllerModel); + applicationModel.Controllers.ShouldContain(derivedControllerModel); + } + + [Fact] + public void Should_Remove_Derived_Controller_If_Expose_Service() + { + // Arrange + var applicationModel = new ApplicationModel(); + var baseControllerModel = new ControllerModel(typeof(BaseController).GetTypeInfo(), Array.Empty()) + { + Application = applicationModel + }; + applicationModel.Controllers.Add(baseControllerModel); + + var derivedControllerModel = new ControllerModel(typeof(ExposeServiceDerivedController).GetTypeInfo(), Array.Empty()) + { + Application = applicationModel + }; + applicationModel.Controllers.Add(derivedControllerModel); + + var abpServiceConvention = new AbpServiceConvention(_options, _conventionalRouteBuilder); + + // Act + abpServiceConvention.Apply(applicationModel); + + // Assert + applicationModel.Controllers.ShouldContain(baseControllerModel); + applicationModel.Controllers.ShouldNotContain(derivedControllerModel); + } + } + + public class BaseController : Controller + { + } + + public class DerivedController : BaseController + { + } + + [ExposeServices(typeof(BaseController))] + public class ExposeServiceDerivedController : BaseController + { + } +} From ebacbbd2ea2745ef5e6d967f802f0cf9e69bd930 Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Thu, 18 Feb 2021 22:13:07 +0800 Subject: [PATCH 6/7] Rename variable to exposedControllerModels --- .../Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs index 383545dc91..996e495588 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention.cs @@ -90,12 +90,12 @@ namespace Volo.Abp.AspNetCore.Mvc.Conventions var exposeServicesAttr = ReflectionHelper.GetSingleAttributeOrDefault(controllerModel.ControllerType); if (exposeServicesAttr.IncludeSelf) { - var existingControllerModels = application.Controllers + var exposedControllerModels = application.Controllers .Where(cm => exposeServicesAttr.ServiceTypes.Contains(cm.ControllerType)) .ToArray(); - controllerModelsToRemove.AddRange(existingControllerModels); - Logger.LogInformation($"Removing the controller{(existingControllerModels.Length > 1 ? "s" : "")} {exposeServicesAttr.ServiceTypes.Select(c => c.AssemblyQualifiedName).JoinAsString(", ")} from the application model since {(existingControllerModels.Length > 1 ? "they are" : "it is")} replaced by the controller: {controllerModel.ControllerType.AssemblyQualifiedName}"); + controllerModelsToRemove.AddRange(exposedControllerModels); + Logger.LogInformation($"Removing the controller{(exposedControllerModels.Length > 1 ? "s" : "")} {exposeServicesAttr.ServiceTypes.Select(c => c.AssemblyQualifiedName).JoinAsString(", ")} from the application model since {(exposedControllerModels.Length > 1 ? "they are" : "it is")} replaced by the controller: {controllerModel.ControllerType.AssemblyQualifiedName}"); continue; } From 1325367e18ccd004cd9a6baad335fac02ed94307 Mon Sep 17 00:00:00 2001 From: Aaron Chong Date: Thu, 18 Feb 2021 22:16:38 +0800 Subject: [PATCH 7/7] Add tests for AbpServiceConvention new behaviour --- .../Conventions/AbpServiceConvention_Tests.cs | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs index f16e8a1df9..ae10a00654 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Conventions/AbpServiceConvention_Tests.cs @@ -48,6 +48,53 @@ namespace Volo.Abp.AspNetCore.Mvc.Conventions applicationModel.Controllers.ShouldContain(derivedControllerModel); } + [Fact] + public void Should_Remove_Exposed_Controller_If_Expose_Self() + { + // Arrange + var applicationModel = new ApplicationModel(); + var baseControllerModel = new ControllerModel(typeof(BaseController).GetTypeInfo(), Array.Empty()) + { + Application = applicationModel + }; + applicationModel.Controllers.Add(baseControllerModel); + + var derivedControllerModel = new ControllerModel(typeof(ExposeServiceIncludeSelfDerivedController).GetTypeInfo(), Array.Empty()) + { + Application = applicationModel + }; + applicationModel.Controllers.Add(derivedControllerModel); + + var abpServiceConvention = new AbpServiceConvention(_options, _conventionalRouteBuilder); + + // Act + abpServiceConvention.Apply(applicationModel); + + // Assert + applicationModel.Controllers.ShouldNotContain(baseControllerModel); + applicationModel.Controllers.ShouldContain(derivedControllerModel); + } + + [Fact] + public void Should_Not_Remove_Derived_Controller_If_No_Base_Controller_Model() + { + // Arrange + var applicationModel = new ApplicationModel(); + var derivedControllerModel = new ControllerModel(typeof(ExposeServiceDerivedController).GetTypeInfo(), Array.Empty()) + { + Application = applicationModel + }; + applicationModel.Controllers.Add(derivedControllerModel); + + var abpServiceConvention = new AbpServiceConvention(_options, _conventionalRouteBuilder); + + // Act + abpServiceConvention.Apply(applicationModel); + + // Assert + applicationModel.Controllers.ShouldContain(derivedControllerModel); + } + [Fact] public void Should_Remove_Derived_Controller_If_Expose_Service() { @@ -88,4 +135,9 @@ namespace Volo.Abp.AspNetCore.Mvc.Conventions public class ExposeServiceDerivedController : BaseController { } + + [ExposeServices(typeof(BaseController), IncludeSelf = true)] + public class ExposeServiceIncludeSelfDerivedController : BaseController + { + } }