From 1bae3bcaa221c8b4c2d235efc8b19b82f95a2eda Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 11 Jun 2014 15:07:59 -0700 Subject: [PATCH] Issue #592 - ActionResult is not being executed by ResultFilterAttribute The issue here is actually different than described in the bug. ResultFilter should only short circuit when .Cancel is set to true. This is consistent with legacy MVC. Added tests for all of this stuff. There's already good test coverage for the invoker, what was missing was coverage for the attributes and for the methods on Controller. ExceptionFilterAttribute and AuthorizationFilterAttribute don't have short circuiting logic inside of them, so they are already covered by tests for the invoker. --- .../Filters/ResultFilterAttribute.cs | 2 +- .../ControllerTests.cs | 24 +- .../Filters/ActionFilterAttributeTests.cs | 256 ++++++++++++++++++ .../Filters/ResultFilterAttributeTest.cs | 37 +++ .../Microsoft.AspNet.Mvc.Core.Test.kproj | 2 + 5 files changed, 319 insertions(+), 2 deletions(-) create mode 100644 test/Microsoft.AspNet.Mvc.Core.Test/Filters/ActionFilterAttributeTests.cs create mode 100644 test/Microsoft.AspNet.Mvc.Core.Test/Filters/ResultFilterAttributeTest.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/ResultFilterAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/ResultFilterAttribute.cs index f88aed0c4d..7ba4d6b73b 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/ResultFilterAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/ResultFilterAttribute.cs @@ -24,7 +24,7 @@ namespace Microsoft.AspNet.Mvc [NotNull] ResultExecutionDelegate next) { OnResultExecuting(context); - if (context.Result == null) + if (!context.Cancel) { OnResultExecuted(await next()); } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs index 2fbc6b6ceb..1c8fbc8b71 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerTests.cs @@ -3,14 +3,18 @@ using System.Collections.Generic; using System.Linq; +using System.Threading.Tasks; using System.Reflection; using System.Text; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Routing; using Microsoft.AspNet.Testing; +#if NET45 +using Moq; +#endif using Xunit; -namespace Microsoft.AspNet.Mvc.Core +namespace Microsoft.AspNet.Mvc.Test { public class ControllerTests { @@ -426,5 +430,23 @@ namespace Microsoft.AspNet.Mvc.Core }; } } + + // These tests share code with the ActionFilterAttribute tests because the various filter + // implementations need to behave the same way. +#if NET45 + [Fact] + public async Task Controller_ActionFilter_SettingResult_ShortCircuits() + { + await ActionFilterAttributeTests.ActionFilter_SettingResult_ShortCircuits( + new Mock()); + } + + [Fact] + public async Task Controller_ActionFilter_Calls_OnActionExecuted() + { + await ActionFilterAttributeTests.ActionFilter_Calls_OnActionExecuted( + new Mock()); + } +#endif } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ActionFilterAttributeTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ActionFilterAttributeTests.cs new file mode 100644 index 0000000000..cbf434b64a --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ActionFilterAttributeTests.cs @@ -0,0 +1,256 @@ +// 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. + +#if NET45 +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.AspNet.Http; +using Microsoft.AspNet.Routing; +using Moq; +using Xunit; + +namespace Microsoft.AspNet.Mvc.Test +{ + public class ActionFilterAttributeTests + { + [Fact] + public async Task ActionFilterAttribute_ActionFilter_SettingResult_ShortCircuits() + { + await ActionFilter_SettingResult_ShortCircuits(new Mock()); + } + + [Fact] + public async Task ActionAttributeFilter_ActionFilter_Calls_OnActionExecuted() + { + await ActionFilter_Calls_OnActionExecuted(new Mock()); + } + + // This is used as a 'common' test method for ActionFilterAttribute and Controller + public static async Task ActionFilter_Calls_OnActionExecuted(Mock mock) + { + // Arrange + mock.As() + .Setup(f => f.OnActionExecutionAsync( + It.IsAny(), + It.IsAny())) + .CallBase(); + + mock.As() + .Setup(f => f.OnActionExecuting(It.IsAny())) + .Verifiable(); + + mock.As() + .Setup(f => f.OnActionExecuted(It.IsAny())) + .Verifiable(); + + var context = CreateActionExecutingContext(mock.As().Object); + var next = new ActionExecutionDelegate(() => Task.FromResult(CreateActionExecutedContext(context))); + + // Act + await mock.As().Object.OnActionExecutionAsync(context, next); + + // Assert + Assert.Null(context.Result); + + mock.As() + .Verify(f => f.OnActionExecuting(It.IsAny()), Times.Once()); + + mock.As() + .Verify(f => f.OnActionExecuted(It.IsAny()), Times.Once()); + } + + // This is used as a 'common' test method for ActionFilterAttribute and Controller + public static async Task ActionFilter_SettingResult_ShortCircuits(Mock mock) + { + // Arrange + mock.As() + .Setup(f => f.OnActionExecutionAsync( + It.IsAny(), + It.IsAny())) + .CallBase(); + + mock.As() + .Setup(f => f.OnActionExecuting(It.IsAny())) + .Callback(c => c.Result = new NoOpResult()); + + mock.As() + .Setup(f => f.OnActionExecuted(It.IsAny())) + .Verifiable(); + + var context = CreateActionExecutingContext(mock.As().Object); + var next = new ActionExecutionDelegate(() => { throw null; }); // This won't run + + // Act + await mock.As().Object.OnActionExecutionAsync(context, next); + + // Assert + Assert.IsType(context.Result); + + mock.As() + .Verify(f => f.OnActionExecuted(It.IsAny()), Times.Never()); + } + + [Fact] + public async Task ActionAttributeFilter_ResultFilter_Calls_OnResultExecuted() + { + await ResultFilter_Calls_OnResultExecuted(new Mock()); + } + + [Fact] + public async Task ActionFilterAttribute_ResultFilter_SettingResult_DoesNotShortCircuit() + { + await ResultFilter_SettingResult_DoesNotShortCircuit(new Mock()); + } + + [Fact] + public async Task ActionFilterAttribute_ResultFilter_SettingCancel_ShortCircuits() + { + await ResultFilter_SettingCancel_ShortCircuits(new Mock()); + } + + // This is used as a 'common' test method for ActionFilterAttribute and ResultFilterAttribute + public static async Task ResultFilter_Calls_OnResultExecuted(Mock mock) + { + // Arrange + mock.As() + .Setup(f => f.OnResultExecutionAsync( + It.IsAny(), + It.IsAny())) + .CallBase(); + + mock.As() + .Setup(f => f.OnResultExecuting(It.IsAny())) + .Verifiable(); + + mock.As() + .Setup(f => f.OnResultExecuted(It.IsAny())) + .Verifiable(); + + var context = CreateResultExecutingContext(mock.As().Object); + var next = new ResultExecutionDelegate(() => Task.FromResult(CreateResultExecutedContext(context))); + + // Act + await mock.As().Object.OnResultExecutionAsync(context, next); + + // Assert + Assert.False(context.Cancel); + + mock.As() + .Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); + + mock.As() + .Verify(f => f.OnResultExecuted(It.IsAny()), Times.Once()); + } + + // This is used as a 'common' test method for ActionFilterAttribute and ResultFilterAttribute + public static async Task ResultFilter_SettingResult_DoesNotShortCircuit(Mock mock) + { + // Arrange + mock.As() + .Setup(f => f.OnResultExecutionAsync( + It.IsAny(), + It.IsAny())) + .CallBase(); + + mock.As() + .Setup(f => f.OnResultExecuting(It.IsAny())) + .Callback(c => c.Result = new NoOpResult()); + + mock.As() + .Setup(f => f.OnResultExecuted(It.IsAny())) + .Verifiable(); + + var context = CreateResultExecutingContext(mock.As().Object); + var next = new ResultExecutionDelegate(() => Task.FromResult(CreateResultExecutedContext(context))); + + // Act + await mock.As().Object.OnResultExecutionAsync(context, next); + + // Assert + Assert.False(context.Cancel); + + mock.As() + .Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); + + mock.As() + .Verify(f => f.OnResultExecuted(It.IsAny()), Times.Once()); + } + + // This is used as a 'common' test method for ActionFilterAttribute and ResultFilterAttribute + public static async Task ResultFilter_SettingCancel_ShortCircuits(Mock mock) + { + // Arrange + mock.As() + .Setup(f => f.OnResultExecutionAsync( + It.IsAny(), + It.IsAny())) + .CallBase(); + + mock.As() + .Setup(f => f.OnResultExecuting(It.IsAny())) + .Callback(c => c.Cancel = true); + + mock.As() + .Setup(f => f.OnResultExecuted(It.IsAny())) + .Verifiable(); + + var context = CreateResultExecutingContext(mock.As().Object); + var next = new ResultExecutionDelegate(() => { throw null; }); // This won't run + + // Act + await mock.As().Object.OnResultExecutionAsync(context, next); + + // Assert + Assert.True(context.Cancel); + + mock.As() + .Verify(f => f.OnResultExecuting(It.IsAny()), Times.Once()); + + mock.As() + .Verify(f => f.OnResultExecuted(It.IsAny()), Times.Never()); + } + + private static ActionExecutingContext CreateActionExecutingContext(IFilter filter) + { + return new ActionExecutingContext( + CreateActionContext(), + new IFilter[] { filter, }, + new Dictionary()); + } + + private static ActionExecutedContext CreateActionExecutedContext(ActionExecutingContext context) + { + return new ActionExecutedContext(context, context.Filters) + { + Result = context.Result, + }; + } + + private static ResultExecutingContext CreateResultExecutingContext(IFilter filter) + { + return new ResultExecutingContext( + CreateActionContext(), + new IFilter[] { filter, }, + new NoOpResult()); + } + + private static ResultExecutedContext CreateResultExecutedContext(ResultExecutingContext context) + { + return new ResultExecutedContext(context, context.Filters, context.Result); + } + + private static ActionContext CreateActionContext() + { + return new ActionContext(Mock.Of(), new RouteData(), new ActionDescriptor()); + } + + private class NoOpResult : IActionResult + { + public Task ExecuteResultAsync(ActionContext context) + { + return Task.FromResult(null); + } + } + } +} +#endif \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ResultFilterAttributeTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ResultFilterAttributeTest.cs new file mode 100644 index 0000000000..7581379c5c --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Filters/ResultFilterAttributeTest.cs @@ -0,0 +1,37 @@ +// 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. + +#if NET45 +using System.Threading.Tasks; +using Moq; +using Xunit; + +namespace Microsoft.AspNet.Mvc.Test +{ + // These tests share code with the ActionFilterAttribute tests because the IAsyncResultFilter + // implementations need to behave the same way. + public class ResultFilterAttributeTest + { + [Fact] + public async Task ResultFilterAttribute_ResultFilter_Calls_OnResultExecuted() + { + await ActionFilterAttributeTests.ResultFilter_Calls_OnResultExecuted( + new Mock()); + } + + [Fact] + public async Task ResultFilterAttribute_ResultFilter_SettingResult_DoesNotShortCircuit() + { + await ActionFilterAttributeTests.ResultFilter_SettingResult_DoesNotShortCircuit( + new Mock()); + } + + [Fact] + public async Task ResultFilterAttribute_ResultFilter_SettingCancel_ShortCircuits() + { + await ActionFilterAttributeTests.ResultFilter_SettingCancel_ShortCircuits( + new Mock()); + } + } +} +#endif \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj b/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj index b6ab6319fb..3f18a38dcd 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Microsoft.AspNet.Mvc.Core.Test.kproj @@ -34,6 +34,7 @@ + @@ -47,6 +48,7 @@ +