From 9e8d4db7d8088ab0642e15c2d6430d530672b117 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Fri, 31 Mar 2017 10:48:47 -0700 Subject: [PATCH] Move TempDataPropertyProvider into filter --- .../MvcRazorPagesMvcCoreBuilderExtensions.cs | 3 - .../Internal/PageActionInvoker.cs | 6 +- .../Internal/PageActionInvokerProvider.cs | 4 - .../Internal/SaveTempDataPropertyFilter.cs | 55 +++++++-- .../Internal/TempDataPropertyProvider.cs | 67 ---------- .../Internal/PageActionInvokerProviderTest.cs | 1 - .../Internal/PageActionInvokerTest.cs | 1 - .../SaveTempDataPropertyFilterTest.cs | 100 ++++++++++----- .../Internal/TempDataPropertyProviderTest.cs | 114 ------------------ 9 files changed, 115 insertions(+), 236 deletions(-) delete mode 100644 src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataPropertyProvider.cs delete mode 100644 test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataPropertyProviderTest.cs diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/MvcRazorPagesMvcCoreBuilderExtensions.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/MvcRazorPagesMvcCoreBuilderExtensions.cs index 495d137bcc..406d44661c 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/MvcRazorPagesMvcCoreBuilderExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/MvcRazorPagesMvcCoreBuilderExtensions.cs @@ -81,9 +81,6 @@ namespace Microsoft.Extensions.DependencyInjection // Action executors services.TryAddSingleton(); services.TryAddSingleton(); - - // Random infrastructure - services.TryAddSingleton(); } } } diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs index fccc695c44..c3b6a75c8e 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs @@ -22,7 +22,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal { private readonly IPageHandlerMethodSelector _selector; private readonly PageContext _pageContext; - private readonly TempDataPropertyProvider _propertyProvider; private Page _page; private object _model; @@ -30,7 +29,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal public PageActionInvoker( IPageHandlerMethodSelector handlerMethodSelector, - TempDataPropertyProvider propertyProvider, DiagnosticSource diagnosticSource, ILogger logger, PageContext pageContext, @@ -45,7 +43,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal valueProviderFactories) { _selector = handlerMethodSelector; - _propertyProvider = propertyProvider; _pageContext = pageContext; CacheEntry = cacheEntry; } @@ -359,11 +356,10 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal } } - var originalValues = _propertyProvider.LoadAndTrackChanges(_page, _pageContext.TempData); if (propertyFilter != null) { - propertyFilter.OriginalValues = originalValues; propertyFilter.Subject = _page; + propertyFilter.ApplyTempDataChanges(_pageContext.HttpContext); } IActionResult result = null; diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvokerProvider.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvokerProvider.cs index 0a2bd0dd32..3848303a88 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvokerProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvokerProvider.cs @@ -39,7 +39,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal private readonly HtmlHelperOptions _htmlHelperOptions; private readonly RazorPagesOptions _razorPagesOptions; private readonly IPageHandlerMethodSelector _selector; - private readonly TempDataPropertyProvider _propertyProvider; private readonly RazorProject _razorProject; private readonly DiagnosticSource _diagnosticSource; private readonly ILogger _logger; @@ -59,7 +58,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal IOptions htmlHelperOptions, IOptions razorPagesOptions, IPageHandlerMethodSelector selector, - TempDataPropertyProvider propertyProvider, RazorProject razorProject, DiagnosticSource diagnosticSource, ILoggerFactory loggerFactory) @@ -77,7 +75,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal _htmlHelperOptions = htmlHelperOptions.Value; _razorPagesOptions = razorPagesOptions.Value; _selector = selector; - _propertyProvider = propertyProvider; _razorProject = razorProject; _diagnosticSource = diagnosticSource; _logger = loggerFactory.CreateLogger(); @@ -159,7 +156,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal return new PageActionInvoker( _selector, - _propertyProvider, _diagnosticSource, _logger, pageContext, diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs index 4079e6f378..62c419e207 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Reflection; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.Extensions.Internal; @@ -26,6 +27,10 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public IDictionary OriginalValues { get; set; } + /// + /// Puts the modified values of into . + /// + /// The to be updated. public void OnTempDataSaving(ITempDataDictionary tempData) { if (Subject != null && OriginalValues != null) @@ -44,38 +49,66 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } } - public void OnActionExecuting(ActionExecutingContext context) + /// + /// Applies values from TempData from to the . + /// + /// The used to find TempData. + public void ApplyTempDataChanges(HttpContext httpContext) { - if (PropertyHelpers == null) + if (Subject == null) { - throw new ArgumentNullException(nameof(PropertyHelpers)); + throw new ArgumentNullException(nameof(Subject)); } + var tempData = _factory.GetTempData(httpContext); + + if (OriginalValues == null) + { + OriginalValues = new Dictionary(); + } + + SetPropertyVaules(tempData, Subject); + } + + /// + public void OnActionExecuting(ActionExecutingContext context) + { Subject = context.Controller; var tempData = _factory.GetTempData(context.HttpContext); OriginalValues = new Dictionary(); + SetPropertyVaules(tempData, Subject); + } + + /// + public void OnActionExecuted(ActionExecutedContext context) + { + } + + private void SetPropertyVaules(ITempDataDictionary tempData, object subject) + { + if (PropertyHelpers == null) + { + return; + } + for (var i = 0; i < PropertyHelpers.Count; i++) { - var property = PropertyHelpers[i].Property; + var property = PropertyHelpers[i]; var value = tempData[Prefix + property.Name]; - OriginalValues[property] = value; + OriginalValues[property.Property] = value; - var propertyTypeInfo = property.PropertyType.GetTypeInfo(); + var propertyTypeInfo = property.Property.PropertyType.GetTypeInfo(); var isReferenceTypeOrNullable = !propertyTypeInfo.IsValueType || Nullable.GetUnderlyingType(property.GetType()) != null; if (value != null || isReferenceTypeOrNullable) { - property.SetValue(Subject, value); + property.SetValue(subject, value); } } } - - public void OnActionExecuted(ActionExecutedContext context) - { - } } } diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataPropertyProvider.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataPropertyProvider.cs deleted file mode 100644 index 4a4297746c..0000000000 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataPropertyProvider.cs +++ /dev/null @@ -1,67 +0,0 @@ -// Copyright (c) .NET Foundation. 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.Concurrent; -using System.Collections.Generic; -using System.Linq; -using System.Reflection; - -namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal -{ - public class TempDataPropertyProvider - { - public static readonly string Prefix = "TempDataProperty-"; - - private ConcurrentDictionary> _subjectProperties = - new ConcurrentDictionary>(); - - /// - /// Loads and tracks any changes to the properties of the . - /// - /// The properties of the subject are loaded and tracked. May be a . - /// The . - /// - public IDictionary LoadAndTrackChanges(object subject, ITempDataDictionary tempData) - { - var properties = GetSubjectProperties(subject); - var result = new Dictionary(); - - foreach (var property in properties) - { - var value = tempData[Prefix + property.Name]; - - result[property] = value; - - // TODO: Clarify what behavior should be for null values here - if (value != null && property.PropertyType.IsAssignableFrom(value.GetType())) - { - property.SetValue(subject, value); - } - } - - return result; - } - - private IEnumerable GetSubjectProperties(object subject) - { - return _subjectProperties.GetOrAdd(subject.GetType(), subjectType => - { - var properties = subjectType.GetRuntimeProperties() - .Where(pi => pi.GetCustomAttribute() != null); - - if (properties.Any(pi => !(pi.SetMethod != null && pi.SetMethod.IsPublic && pi.GetMethod != null && pi.GetMethod.IsPublic))) - { - throw new InvalidOperationException("TempData properties must have a public getter and setter."); - } - - if (properties.Any(pi => !(pi.PropertyType.GetTypeInfo().IsPrimitive || pi.PropertyType == typeof(string)))) - { - throw new InvalidOperationException("TempData properties must be declared as primitive types or string only."); - } - - return properties; - }); - } - } -} \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerProviderTest.cs index 1401f29d19..b4326bb87f 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerProviderTest.cs @@ -820,7 +820,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal new TestOptionsManager(), new TestOptionsManager(razorPagesOptions ?? new RazorPagesOptions()), Mock.Of(), - new TempDataPropertyProvider(), razorProject, new DiagnosticListener("Microsoft.AspNetCore"), NullLoggerFactory.Instance); diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs index 0860e883b4..a86203f431 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs @@ -611,7 +611,6 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal var invoker = new PageActionInvoker( selector, - new TempDataPropertyProvider(), diagnosticSource, logger, pageContext, diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataPropertyFilterTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataPropertyFilterTest.cs index bd4057029b..f810ed48b9 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataPropertyFilterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataPropertyFilterTest.cs @@ -25,24 +25,11 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal ["TempDataProperty-Test"] = "FirstValue" }; - var factory = new Mock(); - factory.Setup(f => f.GetTempData(httpContext)) - .Returns(tempData); - - var filter = new SaveTempDataPropertyFilter(factory.Object); + var filter = CreateSaveTempDataPropertyFilter(httpContext, tempData); var controller = new TestController(); - var controllerType = controller.GetType().GetTypeInfo(); - var propertyHelper1 = new PropertyHelper(controllerType.GetProperty(nameof(TestController.Test))); - var propertyHelper2 = new PropertyHelper(controllerType.GetProperty(nameof(TestController.Test2))); - var propertyHelpers = new List - { - propertyHelper1, - propertyHelper2, - }; - - filter.PropertyHelpers = propertyHelpers; + filter.PropertyHelpers = BuildPropertyHelpers(); var context = new ActionExecutingContext( new ActionContext { @@ -75,23 +62,10 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal ["TempDataProperty-Test"] = "FirstValue" }; - var factory = new Mock(); - factory.Setup(f => f.GetTempData(httpContext)) - .Returns(tempData); - - var filter = new SaveTempDataPropertyFilter(factory.Object); + var filter = CreateSaveTempDataPropertyFilter(httpContext, tempData: tempData); var controller = new TestController(); - var controllerType = controller.GetType().GetTypeInfo(); - var propertyHelper1 = new PropertyHelper(controllerType.GetProperty(nameof(TestController.Test))); - var propertyHelper2 = new PropertyHelper(controllerType.GetProperty(nameof(TestController.Test2))); - var propertyHelpers = new List - { - propertyHelper1, - propertyHelper2, - }; - - filter.PropertyHelpers = propertyHelpers; + filter.PropertyHelpers = BuildPropertyHelpers(); var context = new ActionExecutingContext( new ActionContext @@ -113,6 +87,72 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal Assert.Equal(0, controller.Test2); } + [Fact] + public void ApplyTempDataChanges_SetsPropertyValue() + { + // Arrange + var httpContext = new DefaultHttpContext(); + + var tempData = new TempDataDictionary(httpContext, Mock.Of()) + { + { "TempDataProperty-Test", "Value" } + }; + tempData.Save(); + + var controller = new TestControllerStrings() + { + TempData = tempData, + }; + + var provider = CreateSaveTempDataPropertyFilter(httpContext, tempData: tempData); + provider.Subject = controller; + provider.PropertyHelpers = BuildPropertyHelpers(); + + // Act + provider.ApplyTempDataChanges(httpContext); + + // Assert + Assert.Equal("Value", controller.Test); + Assert.Null(controller.Test2); + } + + private IList BuildPropertyHelpers() + { + var subjectType = typeof(TSubject); + + var properties = subjectType.GetProperties( + BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly); + + var result = new List(); + + foreach (var property in properties) + { + result.Add(new PropertyHelper(property)); + } + + return result; + } + + private SaveTempDataPropertyFilter CreateSaveTempDataPropertyFilter( + HttpContext httpContext, + TempDataDictionary tempData) + { + var factory = new Mock(); + factory.Setup(f => f.GetTempData(httpContext)) + .Returns(tempData); + + return new SaveTempDataPropertyFilter(factory.Object); + } + + public class TestControllerStrings : Controller + { + [TempData] + public string Test { get; set; } + + [TempData] + public string Test2 { get; set; } + } + public class TestController : Controller { [TempData] diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataPropertyProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataPropertyProviderTest.cs deleted file mode 100644 index 65dd5499bd..0000000000 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataPropertyProviderTest.cs +++ /dev/null @@ -1,114 +0,0 @@ -// Copyright (c) .NET Foundation. 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 Microsoft.AspNetCore.Http; -using Moq; -using Xunit; - -namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal -{ - public class TempDataPropertyProviderTest - { - [Fact] - public void LoadAndTrackChanges_SetsPropertyValue() - { - // Arrange - var provider = new TempDataPropertyProvider(); - - var tempData = new TempDataDictionary(new DefaultHttpContext(), new NullTempDataProvider()); - tempData["TempDataProperty-TestString"] = "Value"; - tempData.Save(); - - var controller = new TestController() - { - TempData = tempData, - }; - - // Act - provider.LoadAndTrackChanges(controller, controller.TempData); - - // Assert - Assert.Equal("Value", controller.TestString); - Assert.Null(controller.TestString2); - } - - [Fact] - public void LoadAndTrackChanges_ThrowsInvalidOperationException_PrivateSetter() - { - // Arrange - var provider = new TempDataPropertyProvider(); - - var tempData = new TempDataDictionary(new DefaultHttpContext(), new NullTempDataProvider()); - tempData["TempDataProperty-Test"] = "Value"; - tempData.Save(); - - var controller = new TestController_PrivateSet() - { - TempData = tempData, - }; - - // Act & Assert - var exception = Assert.Throws(() => - provider.LoadAndTrackChanges(controller, controller.TempData)); - - Assert.Equal("TempData properties must have a public getter and setter.", exception.Message); - } - - [Fact] - public void LoadAndTrackChanges_ThrowsInvalidOperationException_NonPrimitiveType() - { - // Arrange - var provider = new TempDataPropertyProvider(); - - var tempData = new TempDataDictionary(new DefaultHttpContext(), new NullTempDataProvider()); - tempData["TempDataProperty-Test"] = new object(); - tempData.Save(); - - var controller = new TestController_NonPrimitiveType() - { - TempData = tempData, - }; - - // Act & Assert - var exception = Assert.Throws(() => - provider.LoadAndTrackChanges(controller, controller.TempData)); - - Assert.Equal("TempData properties must be declared as primitive types or string only.", exception.Message); - } - - public class TestController : Controller - { - [TempData] - public string TestString { get; set; } - - [TempData] - public string TestString2 { get; set; } - } - - public class TestController_PrivateSet : Controller - { - [TempData] - public string Test { get; private set; } - } - - public class TestController_NonPrimitiveType : Controller - { - [TempData] - public object Test { get; set; } - } - - private class NullTempDataProvider : ITempDataProvider - { - public IDictionary LoadTempData(HttpContext context) - { - return null; - } - - public void SaveTempData(HttpContext context, IDictionary values) - { - } - } - } -}