TempData property attribute #5872

jbagga · 2017-03-01T02:08:08Z

Work in progress for #5600

Support for Controllers added. Working on adding the filter for Pages (after this is merged)

pranavkm · 2017-03-02T23:21:21Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs


 namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
 {
-    public class SaveTempDataPropertyFilter : ISaveTempDataCallback
+    /// <summary>


Shouldn't need docs

pranavkm · 2017-03-02T23:21:48Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

    {
        public string Prefix { get; set; }

        public object Subject { get; set; }

        public IDictionary<PropertyInfo, object> OriginalValues { get; set; }

+        private TempDataPropertyProvider _propertyProvider = new TempDataPropertyProvider();
+
+        private readonly ITempDataDictionaryFactory _factory;


Readonly fields before ctor, properties go after.

pranavkm · 2017-03-02T23:21:53Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

    {
        public string Prefix { get; set; }

        public object Subject { get; set; }

        public IDictionary<PropertyInfo, object> OriginalValues { get; set; }

+        private TempDataPropertyProvider _propertyProvider = new TempDataPropertyProvider();


This class was just temp code, we can get rid of it.

pranavkm · 2017-03-02T23:26:26Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilterProvider.cs

+{
+    public class SaveTempDataPropertyFilterProvider : IFilterFactory
+    {
+        public bool IsReusable => false;


This should be reusable, right?

It's not, it's stateful

pranavkm · 2017-03-02T23:30:08Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+
+            foreach (var controllerModel in context.Result.Controllers)
+            {
+                var properties = controllerModel.ControllerType.GetProperties();


Shouldn't we be using controllerModel.ControllerProperties?

We should use https://github.com/aspnet/Common/blob/dev/shared/Microsoft.Extensions.PropertyHelper.Sources/PropertyHelper.cs#L166 here

pranavkm · 2017-03-02T23:33:30Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                var properties = controllerModel.ControllerType.GetProperties();
+                foreach (var property in properties)
+                {
+                    if (property.CustomAttributes.Select(


This doesn't look like a boolean condition? Maybe we can use property.CustomAttributes.OfType<TempDataAttribute>().Any()

pranavkm · 2017-03-02T23:33:46Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataPropertyProvider.cs

@@ -18,6 +16,12 @@ public class TempDataPropertyProvider
        private ConcurrentDictionary<Type, IEnumerable<PropertyInfo>> _subjectProperties =
            new ConcurrentDictionary<Type, IEnumerable<PropertyInfo>>();

+        /// <summary>


Don't need docs for internal types

rynowak

This is shaping up.

The big piece of feedback here is that there should be 0 or 1 SaveTempDataPropertyFilter per-controller. The property discovery should be based on PropertyHelper (from common repo). The filter should store the PropertyHelpers and avoid looking up the properties again.

rynowak · 2017-03-06T18:52:42Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

    {
        public string Prefix { get; set; }

        public object Subject { get; set; }

        public IDictionary<PropertyInfo, object> OriginalValues { get; set; }

+        private TempDataPropertyProvider _propertyProvider = new TempDataPropertyProvider();


This class was just temp code, we can get rid of it.

rynowak · 2017-03-06T18:54:17Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilterProvider.cs

+{
+    public class SaveTempDataPropertyFilterProvider : IFilterFactory
+    {
+        public bool IsReusable => false;


It's not, it's stateful

rynowak · 2017-03-06T18:55:36Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+
+            foreach (var controllerModel in context.Result.Controllers)
+            {
+                var properties = controllerModel.ControllerType.GetProperties();


We should use https://github.com/aspnet/Common/blob/dev/shared/Microsoft.Extensions.PropertyHelper.Sources/PropertyHelper.cs#L166 here

rynowak · 2017-03-06T18:56:14Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                foreach (var property in properties)
+                {
+                    if (property.CustomAttributes.Select(
+                        a => a.AttributeType == typeof(TempDataAttribute)).FirstOrDefault())


Use GetCustomAttribute(...) != null

rynowak · 2017-03-06T18:56:56Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                    if (property.CustomAttributes.Select(
+                        a => a.AttributeType == typeof(TempDataAttribute)).FirstOrDefault())
+                    {
+                        controllerModel.Filters.Add(new SaveTempDataPropertyFilterProvider());


Let's not add a filter per-property.

Instead, add one filter to the controller, and make that filter remember all of the property helper instances that have [TempData]

rynowak · 2017-03-06T18:57:14Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataPropertyProvider.cs


-namespace Microsoft.AspNetCore.Mvc.RazorPages
+namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
 {
    public class TempDataPropertyProvider


Let's kill this class with fire. Move it into the filter.

Follow up issue filed here: #5979

jbagga · 2017-03-08T02:13:14Z

Working on cleaning up and removing TempDataPropertyProvider (after adding a filter for Razor Pages)

jbagga · 2017-03-08T18:55:29Z

@pranavkm @rynowak Need to decide how to add the filter for Razor Pages. Please take a look at the rest!

rynowak · 2017-03-09T00:48:31Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

-        public string Prefix { get; set; }
+        private readonly ITempDataDictionaryFactory _factory;
+
+        internal IList<PropertyHelper> PropertyHelpers { get; set; }


properties after constructors

rynowak · 2017-03-09T00:50:07Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+        {
+        }
+
+        private IEnumerable<PropertyInfo> GetSubjectProperties()


This should be done in the application model provider. The point is to avoid doing this every request.

rynowak · 2017-03-09T00:53:14Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+                if (value != null && property.PropertyType.IsAssignableFrom(value.GetType()))
+                {
+                    property.SetValue(Subject, value);
+                }


I don't think we need to check this here.

As far as null, you want to set null if the property can accept a null.

rynowak · 2017-03-09T00:54:12Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                    controllerModel.Filters.Add(provider);
+                }
+            }
+        }


This looks great 👍

rynowak · 2017-03-09T00:55:43Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+
+                if (!(propertyHelper.Property.PropertyType.GetTypeInfo().IsPrimitive || propertyHelper.Property.PropertyType == typeof(string)))
+                {
+                    throw new InvalidOperationException("TempData properties must be declared as primitive types or string only.");


These should be in resources

kichalla · 2017-03-09T08:00:29Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

                    {
                        tempData[Prefix + property.Name] = newValue;
                    }
                }
            }
        }
+
+        public void OnActionExecuting(ActionExecutingContext context)


Question for @rynowak : Should we consider async api on ITempDataDictionary & ITempDataProvider at this point( (yeah there would be breaking changes)? Currently these apis are sync (for obvious reason that TempData access is sync in view of user experience), but if we provide this binding support to properties directly, then we could load the data in an async way. (For example, if SessionStateTempDataProvider is used, it could load a session in async way using ISession's LoadAsync). Also we would attempt to load temp data for all actions on a controller even though some of them might use it, which increases chances of thread blockage.

We wouldn't add an async API to ITempDataDictionary, it would go on the factory/provider. As far as doing this async goes, I'm not fully convinced that we need it. Separate issue maybe?

Sure. Filed here #5941

kichalla · 2017-03-09T08:02:11Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilterProvider.cs

+
+namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
+{
+    public class SaveTempDataPropertyFilterProvider : IFilterFactory


SaveTempDataPropertyFilterProvider => SaveTempDataPropertyFilterFactory

kichalla · 2017-03-09T08:23:07Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilterProvider.cs

+    {
+        internal IList<PropertyHelper> TempDataProperties { get; set; }
+
+        public bool IsReusable => false;


True? as one instance of the built (with property helpers) filter should be used for a controller. Also registration of SaveTempDataPropertyFilter in MvcViewFeaturesMvcCoreBuilderExtensions.cs looks incorrect (its currently singleton and should be transient)

Is reusable applies to SaveTempDataPropertyFilter which is stateful.

Good point about reusable. Discussed with @rynowak about the registration, it should be transient

kichalla · 2017-03-09T08:28:58Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+            _factory = factory;
+        }
+
+        public string Prefix = "TempDataProperty-";


@rynowak Wondering if we need this prefix...a scenario that I am thinking is for keep. If a value is bound to a tempdata property, it's as though its read and would be marked for deletion, but if a user chooses to keep it, then he might want to explicitly say TempData.Keep(nameof(OrderStatusMessage)) (where OrderStatusMessage is a TempData property).

I do not want to block this PR for this, but could you log an issue for this as its related to user experience.

private const string

We have another issue for this already. It's been discussed 😆

kichalla · 2017-03-09T08:37:49Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataPropertyTest.cs

+            // Assert 2
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            var body = await response.Content.ReadAsStringAsync();
+            Assert.Equal(expected, body);


Would be good to verify that the message was deleted from tempdata

kichalla · 2017-03-09T08:38:33Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataPropertyTest.cs

+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            var body = await response.Content.ReadAsStringAsync();
+            Assert.Equal(expected, body);
+        }


Add a test for keep scenario too.

jbagga · 2017-03-10T01:06:09Z

@rynowak @pranavkm @kichalla Updated

kichalla · 2017-03-10T22:15:05Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+            _factory = factory;
+        }
+
+        public string Prefix = "TempDataProperty-";


I do not want to block this PR for this, but could you log an issue for this as its related to user experience.

kichalla · 2017-03-10T22:17:47Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+        {
+            Subject = context.Controller;
+            var tempData = _factory.GetTempData(context.HttpContext);
+            var properties = PropertyHelpers.Select(p => p.Property).ToArray();


Need not do ToArray

Let's avoid the Linq call entirely. Also, this needs to throw if PropertyHelpers is not initialized.

Don't need this at all actually, use the property helper to get/set the value.

kichalla · 2017-03-10T22:18:17Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+                {
+                    property.SetValue(Subject, value);
+                }
+


remove line

kichalla · 2017-03-10T22:19:52Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilterProvider.cs

+
+namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
+{
+    public class SaveTempDataPropertyFilterFactory : IFilterFactory


Change the name of this file too

kichalla · 2017-03-10T22:20:53Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                    Resources.FormatTempDataProperties_PrimitiveTypeOrString(property.Name));
+            }
+
+            else


just return from here itself.

pranavkm · 2017-03-13T18:22:40Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs


        public object Subject { get; set; }

        public IDictionary<PropertyInfo, object> OriginalValues { get; set; }

+        internal IList<PropertyHelper> PropertyHelpers { get; set; }


We don't use internal for any code that's not meant to be test-only.

PropertyHelper is an internal class. I cannot make the IList<PropertyHelper> public because then the accessibility is inconsistent. If I set it to private I cannot set them in the SaveTempDataPropertyFilterFactory. Any suggestions to solve this?

pranavkm · 2017-03-13T18:23:07Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+        {
+            Subject = context.Controller;
+            var tempData = _factory.GetTempData(context.HttpContext);
+            var properties = PropertyHelpers.Select(p => p.Property).ToArray();


Let's avoid the Linq call entirely. Also, this needs to throw if PropertyHelpers is not initialized.

pranavkm · 2017-03-13T18:24:02Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+            var properties = PropertyHelpers.Select(p => p.Property).ToArray();
+            OriginalValues = new Dictionary<PropertyInfo, object>();
+
+            foreach (var property in properties)


for (var i = 0; i < PropertyHelpers.Count; i++) { var property = PropertyHelpers[i].Property; ... }

pranavkm · 2017-03-13T18:25:19Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+
+                if (value != null)
+                {
+                    property.SetValue(Subject, value);


Nope. Have to use the PropertyHelper to call SetValue.

pranavkm · 2017-03-13T18:27:50Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilterProvider.cs

+{
+    public class SaveTempDataPropertyFilterFactory : IFilterFactory
+    {
+        internal IList<PropertyHelper> TempDataProperties { get; set; }


Why internal?

pranavkm · 2017-03-13T18:49:47Z

.../Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataApplicationModelProviderTest.cs

+            Assert.Equal("TempData property Test is not declared as a primitive type or string.", exception.Message);
+        }
+
+


Remove extra line. Also needs tests to verify that it initializes the filter with the right PropertyHelpers

pranavkm · 2017-03-13T18:51:46Z

test/WebSites/BasicWebSite/Controllers/TempDataPropertyController.cs

+        [HttpPost]
+        public IActionResult CreateForView(Person person)
+        {
+            if (ModelState.IsValid)


Not sure you need these. We aren't testing ModelBinding here (are we)?

pranavkm · 2017-03-13T18:52:31Z

test/WebSites/BasicWebSite/Controllers/TempDataPropertyController.cs

+            return $"{Message} for person {person.FullName} with id {person.id}.";
+        }
+
+        public void CreateNoRedirect(Person person)


Just use an OkResult. void returning actions are not very typical.

pranavkm · 2017-03-13T18:52:45Z

test/WebSites/BasicWebSite/Views/TempDataProperty/DetailsView.cshtml

@@ -0,0 +1,2 @@
+@model BasicWebSite.Models.Person
+@ViewData["Message"] for person @Model.FullName with id @Model.id.


Add a new line at the end of the file.

pranavkm · 2017-03-13T18:53:29Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+            _factory = factory;
+        }
+
+        public string Prefix = "TempDataProperty-";


private const string

rynowak · 2017-03-14T19:41:12Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+        {
+            Subject = context.Controller;
+            var tempData = _factory.GetTempData(context.HttpContext);
+            var properties = PropertyHelpers.Select(p => p.Property).ToArray();


Don't need this at all actually, use the property helper to get/set the value.

rynowak · 2017-03-14T19:41:46Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+
+                OriginalValues[property] = value;
+
+                var propertyInfo = property.PropertyType.GetTypeInfo();


This isn't a PropertyInfo, it's the TypeInfo of the property.

rynowak · 2017-03-14T19:42:20Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilterProvider.cs

+using Microsoft.AspNetCore.Mvc.Filters;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Internal;
+using System.Collections.Generic;


rynowak · 2017-03-14T19:42:58Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+    public class TempDataApplicationModelProvider : IApplicationModelProvider
+    {
+        /// <inheritdoc />
+        public int Order { get { return -1000 + 10; } }


Leave a comment here explaining why this value is significant.

This value is the same as other ApplicationModelProviders like AuthorizationApplicationModelProvider and CorsApplicationModelProvider. Is that correct?

rynowak · 2017-03-14T19:44:09Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                    Resources.FormatTempDataProperties_PublicGetterSetter(property.Name));
+            }
+
+            if (!(property.PropertyType.GetTypeInfo().IsPrimitive || property.PropertyType == typeof(string)))


I think this is ok for now. There's been lots of confusion about this in the past.

rynowak · 2017-03-14T19:44:25Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                    Resources.FormatTempDataProperties_PublicGetterSetter(property.Name));
+            }
+
+            if (!(property.PropertyType.GetTypeInfo().IsPrimitive || property.PropertyType == typeof(string)))


@pranavkm - please start an email thread or open an issue about this.

rynowak · 2017-03-14T19:48:12Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Resources.resx

+    <value>TempData property {0} is not declared as a primitive type or string.</value>
+  </data>
+  <data name="TempDataProperties_PublicGetterSetter" xml:space="preserve">
+    <value>TempData property {0} does not have a public getter or setter.</value>


These should be more friendly to read, like:

The {0} property with {1} is invalid. A property using {1} must have a public getter and setter.

Never hardcode the names of 'code things' like TempData in a resource. It's better to make it a parameter and pass in the name.

Make sure that the property name that's reported here includes the full type name as well. We want to make sure that you can find the issue 👍

jbagga · 2017-03-15T01:20:05Z

@rynowak @pranavkm Updated. Can you look at the exception messages? I am not sure how to get the FullName from the PropertyHelper.

pranavkm · 2017-03-15T01:29:06Z

$"{PropertyInfo.DeclaringType.FullName}.{PropertyInfo.Name}". There's also TypeNameHelper which might help pretty printing the type name although I'm not sure we need that here.

jbagga · 2017-03-15T19:08:07Z

@pranavkm Updated

rynowak · 2017-03-15T22:09:56Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                property.GetMethod.IsPublic))
+            {
+                throw new InvalidOperationException(
+                    Resources.FormatTempDataProperties_PublicGetterSetter(property.DeclaringType.FullName, property.Name, "TempDataAttribute"));


nameof(TempDataAttribute)

rynowak

from me, make sure to get signoff from @kichalla and @pranavkm

pranavkm · 2017-03-16T15:24:23Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

    {
-        public string Prefix { get; set; }
+        private readonly ITempDataDictionaryFactory _factory;
+        private const string Prefix = "TempDataProperty-";


Typical field order we use in most places in our code: public const, non-public const, public static readonly, 'private static readonly, readonly`, and finally non-readonly fields.

pranavkm · 2017-03-16T15:30:48Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+
+                var propertyTypeInfo = property.PropertyType.GetTypeInfo();
+
+                if (value != null)


var isReferenceTypeOrNullable = !propertyTypeInfo.IsValueType || Nullable.GetUnderlyingType(ModelType) != null; if (value != null || isReferenceTypeOrNullable) { property.SetValue(...); }

You can remove the generic check once you do this.

pranavkm · 2017-03-16T15:33:36Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+            foreach (var controllerModel in context.Result.Controllers)
+            {
+                SaveTempDataPropertyFilterFactory factory = null;
+                var propertyHelpers = PropertyHelper.GetVisibleProperties(controllerModel.ControllerType.AsType());


Could you make this PropertyHelper.GetVisibleProperties(type: controllerModel.ControllerType.AsType()). Should eliminate accidental use of the object overload

pranavkm · 2017-03-16T15:34:34Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/TempDataApplicationModelProvider.cs

+                var propertyHelpers = PropertyHelper.GetVisibleProperties(controllerModel.ControllerType.AsType());
+                for (var i = 0; i < propertyHelpers.Length; i++)
+                {
+                    if (propertyHelpers[i].Property.GetCustomAttribute<TempDataAttribute>() != null)


var propertyHelper = propertyHelpers[i]; if (propertyHelper.Property.IsDefined(typeof(TempDataAttribute)) ...

pranavkm · 2017-03-16T15:38:11Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/TempDataAttribute.cs

@@ -3,7 +3,7 @@

 using System;

-namespace Microsoft.AspNetCore.Mvc.Rendering
+namespace Microsoft.AspNetCore.Mvc.ViewFeatures
 {
    [AttributeUsage(AttributeTargets.Property, Inherited = false, AllowMultiple = false)]
    public sealed class TempDataAttribute : Attribute


pranavkm · 2017-03-16T15:44:44Z

.../Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataApplicationModelProviderTest.cs

+            Assert.NotNull(saveTempDataPropertyFilterFactory);
+            var tempDataPropertyHelper = Assert.Single(saveTempDataPropertyFilterFactory.TempDataProperties);
+            Assert.Equal("Test2", tempDataPropertyHelper.Name);
+            Assert.NotNull(tempDataPropertyHelper.Property.GetCustomAttribute<TempDataAttribute>());


You could simply assert it's the exact property instead of verifying two pieces of information about it:

var expected = typeof(TestController_OneTempDataProperty).GetProperty(nameof(TestController_OneTempDataProperty.Test2); ... Assert.Same(expected, tempDataPropertyHelper.Property);

pranavkm · 2017-03-16T15:46:11Z

.../Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataApplicationModelProviderTest.cs

+            defaultProvider.OnProvidersExecuting(context);
+
+            // Act & Assert
+            var exception = Assert.Throws<InvalidOperationException>(() =>


var exception = Assert.Throws<InvalidOperationException>(() => provider.OnProvidersExecuting(context));

pranavkm · 2017-03-16T15:47:25Z

.../Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataApplicationModelProviderTest.cs

+            public string Test { get; private set; }
+        }
+
+        public class TestController_NonPrimitiveType : Controller


Could you add a scenario where some of the properties are valid and the others aren't allowed?

Can you include collection of primitives as a negative case? Cookie and session temp data provider allow IList<string> for example, but the property does not.

Also, should we print all possible errors when doing this? @rynowak is there any other equivalent in application model where the error includes all issues?

pranavkm · 2017-03-16T15:48:12Z

.../Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataApplicationModelProviderTest.cs

+            public string Test { get; set; }
+
+            [TempData]
+            public string Test2 { get; set; }


Can you add non-string properties to this one? For instance an int?

pranavkm · 2017-03-16T15:52:20Z

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/TempDataPropertyProviderTest.cs

+            };
+
+            // Act & Assert
+            var exception = Assert.Throws<InvalidOperationException>(() =>


var exception = Assert.Throws<InvalidOperationException>(() => provider.LoadAndTrackChanges(controller, controller.TempData));

jbagga · 2017-03-16T20:48:59Z

@pranavkm Updated

pranavkm · 2017-03-16T21:33:06Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataPropertyFilter.cs

+                var isReferenceTypeOrNullable = !propertyTypeInfo.IsValueType || Nullable.GetUnderlyingType(property.GetType()) != null;
+                if (value != null || isReferenceTypeOrNullable)
+                {
+                    PropertyHelpers[i].SetValue(Subject, value);


property.SetValue

dnfclas added the cla-already-signed label Mar 1, 2017

jbagga requested a review from rynowak March 1, 2017 02:08

jbagga assigned rynowak Mar 1, 2017

jbagga requested a review from pranavkm March 1, 2017 02:08

jbagga assigned pranavkm Mar 1, 2017

jbagga force-pushed the jbagga/TempData5600 branch from ade635c to b8ded1f Compare March 2, 2017 00:48

pranavkm suggested changes Mar 3, 2017

View reviewed changes

rynowak suggested changes Mar 6, 2017

View reviewed changes

jbagga force-pushed the jbagga/TempData5600 branch from b8ded1f to 3d219d7 Compare March 8, 2017 02:11

rynowak suggested changes Mar 9, 2017

View reviewed changes

kichalla suggested changes Mar 9, 2017

View reviewed changes

kichalla mentioned this pull request Mar 10, 2017

Async APIs on TempData related apis (ITempDataDictionary, ITempDataProvider etc.) #5941

Closed

jbagga force-pushed the jbagga/TempData5600 branch from 88c2b68 to d1cfd27 Compare March 10, 2017 01:01

jbagga changed the title ~~[Design] TempData property attribute~~ TempData property attribute Mar 10, 2017

jbagga requested review from kichalla and rynowak March 10, 2017 19:17

kichalla approved these changes Mar 10, 2017

View reviewed changes

pranavkm suggested changes Mar 13, 2017

View reviewed changes

rynowak suggested changes Mar 14, 2017

View reviewed changes

jbagga force-pushed the jbagga/TempData5600 branch from d1cfd27 to 05f91d4 Compare March 15, 2017 01:16

jbagga force-pushed the jbagga/TempData5600 branch from 05f91d4 to e512cf5 Compare March 15, 2017 19:07

rynowak reviewed Mar 15, 2017

View reviewed changes

rynowak approved these changes Mar 15, 2017

View reviewed changes

pranavkm suggested changes Mar 16, 2017

View reviewed changes

jbagga force-pushed the jbagga/TempData5600 branch from e512cf5 to 533ccd6 Compare March 16, 2017 20:47

pranavkm approved these changes Mar 16, 2017

View reviewed changes

jbagga added 11 commits March 16, 2017 15:55

PR feedback addressed

9269c2c

Clean up

fccd4fd

PR feedback addressed

9e47286

Resources.Designer

0566d02

Added more tests

a43929f

Addressing PR feedback

7a37ab8

PR feedback incorporated

c769842

Exception message modified

3c8cd89

Use nameof{TempDataAttribute}

04b3ab3

PR feedback addressed

5a22aa5

Feedback addressed

43844ba

jbagga force-pushed the jbagga/TempData5600 branch from 058899b to 43844ba Compare March 16, 2017 23:04

jbagga merged commit 1197657 into dev Mar 16, 2017

jbagga mentioned this pull request Mar 16, 2017

Add a filter provider for TempDataAttribute to RazorPages #5979

Closed

jbagga deleted the jbagga/TempData5600 branch March 17, 2017 00:37

		Assert.Equal("TempData property Test is not declared as a primitive type or string.", exception.Message);
		}

		@@ -0,0 +1,2 @@
		@model BasicWebSite.Models.Person
		@ViewData["Message"] for person @Model.FullName with id @Model.id.


		OriginalValues[property] = value;

		var propertyInfo = property.PropertyType.GetTypeInfo();


		var propertyTypeInfo = property.PropertyType.GetTypeInfo();

		if (value != null)

TempData property attribute #5872

TempData property attribute #5872

Conversation

jbagga commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Mar 8, 2017

jbagga commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Mar 15, 2017

pranavkm commented Mar 15, 2017

jbagga commented Mar 15, 2017

Choose a reason for hiding this comment

rynowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak Mar 6, 2017 •

edited

Loading