From d73281680f1eeba38479d31f56fa43ee5a6065e1 Mon Sep 17 00:00:00 2001 From: maliming <6908465+maliming@users.noreply.github.com> Date: Sat, 16 May 2020 19:33:09 +0800 Subject: [PATCH 1/2] Razor page's exception filter ignores void and task return types. Resolve #3969 --- .../Abp/AspNetCore/Mvc/ActionResultHelper.cs | 11 +++- .../AbpExceptionPageFilter.cs | 18 +++--- .../AspNetCore/Mvc/Uow/AbpUowPageFilter.cs | 6 +- .../ExceptionTestPage.cshtml.cs | 28 ++++++-- .../ExceptionTestPage_Tests.cs | 64 ++++++++++++++++--- .../Mvc/Features/FeatureTestPage.cshtml.cs | 6 +- .../Mvc/Uow/UnitOfWorkTestPage.cshtml.cs | 8 +-- 7 files changed, 104 insertions(+), 37 deletions(-) diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ActionResultHelper.cs b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ActionResultHelper.cs index 577c8d0786..f2d7195e08 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ActionResultHelper.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ActionResultHelper.cs @@ -8,7 +8,7 @@ namespace Volo.Abp.AspNetCore.Mvc { public static class ActionResultHelper { - public static List ObjectResultTypes { get; } + public static List ObjectResultTypes { get; } static ActionResultHelper() { @@ -20,10 +20,15 @@ namespace Volo.Abp.AspNetCore.Mvc }; } - public static bool IsObjectResult(Type returnType) + public static bool IsObjectResult(Type returnType, params Type[] excludeTypes) { returnType = AsyncHelper.UnwrapTask(returnType); + if (!excludeTypes.IsNullOrEmpty() && excludeTypes.Any(t => t.IsAssignableFrom(returnType))) + { + return false; + } + if (!typeof(IActionResult).IsAssignableFrom(returnType)) { return true; @@ -32,4 +37,4 @@ namespace Volo.Abp.AspNetCore.Mvc return ObjectResultTypes.Any(t => t.IsAssignableFrom(returnType)); } } -} \ No newline at end of file +} diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/AbpExceptionPageFilter.cs b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/AbpExceptionPageFilter.cs index fb0a7c5a1f..9e449f0474 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/AbpExceptionPageFilter.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/AbpExceptionPageFilter.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; @@ -25,7 +25,7 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling public AbpExceptionPageFilter( IExceptionToErrorInfoConverter errorInfoConverter, - IHttpExceptionStatusCodeFinder statusCodeFinder, + IHttpExceptionStatusCodeFinder statusCodeFinder, IJsonSerializer jsonSerializer) { _errorInfoConverter = errorInfoConverter; @@ -34,13 +34,12 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling Logger = NullLogger.Instance; } - public Task OnPageHandlerSelectionAsync(PageHandlerSelectedContext context) { return Task.CompletedTask; } - + public async Task OnPageHandlerExecutionAsync(PageHandlerExecutingContext context, PageHandlerExecutionDelegate next) { if (context.HandlerMethod == null || !ShouldHandleException(context)) @@ -54,20 +53,20 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling { return;; } - + await HandleAndWrapException(pageHandlerExecutedContext); } - + protected virtual bool ShouldHandleException(PageHandlerExecutingContext context) { //TODO: Create DontWrap attribute to control wrapping..? if (context.ActionDescriptor.IsPageAction() && - ActionResultHelper.IsObjectResult(context.HandlerMethod.MethodInfo.ReturnType)) + ActionResultHelper.IsObjectResult(context.HandlerMethod.MethodInfo.ReturnType, typeof(void))) { return true; } - + if (context.HttpContext.Request.CanAccept(MimeTypes.Application.Json)) { return true; @@ -107,6 +106,5 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling context.Exception = null; //Handled! } - } -} \ No newline at end of file +} diff --git a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Uow/AbpUowPageFilter.cs b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Uow/AbpUowPageFilter.cs index 993a12b0e7..bcef10ecd4 100644 --- a/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Uow/AbpUowPageFilter.cs +++ b/framework/src/Volo.Abp.AspNetCore.Mvc/Volo/Abp/AspNetCore/Mvc/Uow/AbpUowPageFilter.cs @@ -38,7 +38,7 @@ namespace Volo.Abp.AspNetCore.Mvc.Uow context.HttpContext.Items["_AbpActionInfo"] = new AbpActionInfoInHttpContext { - IsObjectResult = ActionResultHelper.IsObjectResult(context.HandlerMethod.MethodInfo.ReturnType) + IsObjectResult = ActionResultHelper.IsObjectResult(context.HandlerMethod.MethodInfo.ReturnType, typeof(void)) }; if (unitOfWorkAttr?.IsDisabled == true) @@ -71,7 +71,7 @@ namespace Volo.Abp.AspNetCore.Mvc.Uow } } } - + private AbpUnitOfWorkOptions CreateOptions(PageHandlerExecutingContext context, UnitOfWorkAttribute unitOfWorkAttribute) { var options = new AbpUnitOfWorkOptions(); @@ -102,4 +102,4 @@ namespace Volo.Abp.AspNetCore.Mvc.Uow return result.Exception == null || result.ExceptionHandled; } } -} \ No newline at end of file +} diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs index 717d31effe..410f9d124d 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs @@ -1,18 +1,36 @@ -using Microsoft.AspNetCore.Mvc; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc; using Volo.Abp.AspNetCore.Mvc.UI.RazorPages; namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling { public class ExceptionTestPage : AbpPageModel { - public void OnGetUserFriendlyException1() + public void OnGetUserFriendlyException_void() { throw new UserFriendlyException("This is a sample exception!"); } - - public IActionResult OnGetUserFriendlyException2() + + public Task OnGetUserFriendlyException_Task() + { + throw new UserFriendlyException("This is a sample exception!"); + } + + + public IActionResult OnGetUserFriendlyException_ActionResult() { throw new UserFriendlyException("This is a sample exception!"); } + + public JsonResult OnGetUserFriendlyException_JsonResult() + { + throw new UserFriendlyException("This is a sample exception!"); + } + + public Task OnGetUserFriendlyException_Task_JsonResult() + { + throw new UserFriendlyException("This is a sample exception!"); + } + } -} \ No newline at end of file +} diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage_Tests.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage_Tests.cs index 7013c0c676..d7c609aa59 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage_Tests.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage_Tests.cs @@ -1,4 +1,4 @@ -using System.Net; +using System.Net; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; @@ -24,15 +24,33 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling } [Fact] - public async Task Should_Return_RemoteServiceErrorResponse_For_UserFriendlyException_For_Void_Return_Value() + public async Task Should_Not_Handle_Exceptions_For_Void_Return_Values() { - var result = await GetResponseAsObjectAsync("/ExceptionHandling/ExceptionTestPage?handler=UserFriendlyException1", HttpStatusCode.Forbidden); - result.Error.ShouldNotBeNull(); - result.Error.Message.ShouldBe("This is a sample exception!"); + await Assert.ThrowsAsync( + async () => await GetResponseAsStringAsync( + "/ExceptionHandling/ExceptionTestPage?handler=UserFriendlyException_Void" + ) + ); #pragma warning disable 4014 _fakeExceptionSubscriber - .Received() + .DidNotReceive() + .HandleAsync(Arg.Any()); +#pragma warning restore 4014 + } + + [Fact] + public async Task Should_Not_Handle_Exceptions_For_Task_Return_Values() + { + await Assert.ThrowsAsync( + async () => await GetResponseAsStringAsync( + "/ExceptionHandling/ExceptionTestPage?handler=UserFriendlyException_Task" + ) + ); + +#pragma warning disable 4014 + _fakeExceptionSubscriber + .DidNotReceive() .HandleAsync(Arg.Any()); #pragma warning restore 4014 } @@ -41,8 +59,8 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling public async Task Should_Not_Handle_Exceptions_For_ActionResult_Return_Values() { await Assert.ThrowsAsync( - async () => await GetResponseAsObjectAsync( - "/ExceptionHandling/ExceptionTestPage?handler=UserFriendlyException2" + async () => await GetResponseAsStringAsync( + "/ExceptionHandling/ExceptionTestPage?handler=UserFriendlyException_ActionResult" ) ); @@ -50,7 +68,35 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling _fakeExceptionSubscriber .DidNotReceive() .HandleAsync(Arg.Any()); +#pragma warning restore 4014 + } + + [Fact] + public async Task Should_Return_RemoteServiceErrorResponse_For_UserFriendlyException_For_Object_Return_Value() + { + var result = await GetResponseAsObjectAsync("/ExceptionHandling/ExceptionTestPage?handler=UserFriendlyException_JsonResult", HttpStatusCode.Forbidden); + result.Error.ShouldNotBeNull(); + result.Error.Message.ShouldBe("This is a sample exception!"); + +#pragma warning disable 4014 + _fakeExceptionSubscriber + .Received() + .HandleAsync(Arg.Any()); +#pragma warning restore 4014 + } + + [Fact] + public async Task Should_Return_RemoteServiceErrorResponse_For_UserFriendlyException_For_Task_Object_Return_Value() + { + var result = await GetResponseAsObjectAsync("/ExceptionHandling/ExceptionTestPage?handler=UserFriendlyException_Task_JsonResult", HttpStatusCode.Forbidden); + result.Error.ShouldNotBeNull(); + result.Error.Message.ShouldBe("This is a sample exception!"); + +#pragma warning disable 4014 + _fakeExceptionSubscriber + .Received() + .HandleAsync(Arg.Any()); #pragma warning restore 4014 } } -} \ No newline at end of file +} diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Features/FeatureTestPage.cshtml.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Features/FeatureTestPage.cshtml.cs index 6a051a29ee..d9f600d9ed 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Features/FeatureTestPage.cshtml.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Features/FeatureTestPage.cshtml.cs @@ -14,9 +14,9 @@ namespace Volo.Abp.AspNetCore.Mvc.Features } [RequiresFeature("NotAllowedFeature")] - public void OnGetNotAllowedFeature() + public ObjectResult OnGetNotAllowedFeature() { - + return new ObjectResult(42); } public ObjectResult OnGetNoFeature() @@ -24,4 +24,4 @@ namespace Volo.Abp.AspNetCore.Mvc.Features return new ObjectResult(42); } } -} \ No newline at end of file +} diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestPage.cshtml.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestPage.cshtml.cs index ce54e8d34f..1428f2c88c 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestPage.cshtml.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/Uow/UnitOfWorkTestPage.cshtml.cs @@ -32,23 +32,23 @@ namespace Volo.Abp.AspNetCore.Mvc.Uow } [UnitOfWork(isTransactional: true)] - public void OnGetHandledException() + public ObjectResult OnGetHandledException() { CurrentUnitOfWork.ShouldNotBeNull(); CurrentUnitOfWork.Options.IsTransactional.ShouldBeTrue(); throw new UserFriendlyException("This is a sample exception!"); } - + public ObjectResult OnGetExceptionOnComplete() { CurrentUnitOfWork.ShouldNotBeNull(); CurrentUnitOfWork.Options.IsTransactional.ShouldBeFalse(); _testUnitOfWorkConfig.ThrowExceptionOnComplete = true; - + //Prevent rendering of pages. return new ObjectResult(""); } } -} \ No newline at end of file +} From b72ba88752f31e3ebafd7f31b36ce45f3747257f Mon Sep 17 00:00:00 2001 From: maliming <6908465+maliming@users.noreply.github.com> Date: Sat, 16 May 2020 19:34:51 +0800 Subject: [PATCH 2/2] Update ExceptionTestPage.cshtml.cs --- .../AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs index 410f9d124d..dd4edebec1 100644 --- a/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs +++ b/framework/test/Volo.Abp.AspNetCore.Mvc.Tests/Volo/Abp/AspNetCore/Mvc/ExceptionHandling/ExceptionTestPage.cshtml.cs @@ -16,7 +16,6 @@ namespace Volo.Abp.AspNetCore.Mvc.ExceptionHandling throw new UserFriendlyException("This is a sample exception!"); } - public IActionResult OnGetUserFriendlyException_ActionResult() { throw new UserFriendlyException("This is a sample exception!");