Add support for specifying filters on page models. by pranavkm · Pull Request #6417 · aspnet/Mvc

pranavkm · 2017-06-20T21:27:25Z

No description provided.

pranavkm · 2017-06-20T21:29:10Z

            }

+            // Filters are only allowed to be declared on an explicitly specified model.
+            var filters = actionDescriptor.FilterDescriptors;


Layering concerns?

pranavkm · 2017-06-20T21:31:05Z

+    }
+
+    [AttributeUsage(AttributeTargets.Class)]
+    public class HandlerChangingPageFilterAttribute : Attribute, IPageFilter


Did we intentionally choose not to add a PageFilterAttribute or was that an oversight?

I chose not to add it. We can if you think it's good

pranavkm · 2017-06-20T21:31:18Z

 // 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 Microsoft.AspNetCore.Authorization;


pranavkm · 2017-06-20T21:31:23Z

+            var filters = actionDescriptor.FilterDescriptors;
+            if (modelType != null && modelType != pageType)
+            {
+                var modelAttributes = modelType.GetCustomAttributes(inherit: true);


Do we want PageModel to implement IPageFilter, IAsyncPageFilter and discover those too?

pranavkm · 2017-06-24T01:19:42Z

🆙 📅 Still in design

rynowak · 2017-06-27T01:21:40Z

+        /// Executed for the second pass of <see cref="ApplicationModel"/> building. See <see cref="Order"/>.
        /// </summary>
        /// <param name="context">The <see cref="PageApplicationModelProviderContext"/>.</param>
        void OnProvidersExecuted(PageApplicationModelProviderContext context);


I don't think the changes here really clear anything up.

rynowak · 2017-06-27T01:22:00Z

+        /// </summary>
+        /// <param name="model">The <see cref="PageRouteModel"/>.</param>
+        void Apply(PageRouteModel model);
+    }


I don't love this name, but I don't have a better one in mind.

rynowak · 2017-06-27T01:23:10Z

-        public IList<SelectorModel> Selectors { get; }
+        public IList<PageHandlerModel> Handlers { get; }
+
+        public IList<PagePropertyModel> HandlerProperties { get; }


Call this bound properties (same as controllers)

rynowak · 2017-06-27T01:23:30Z

+
+        public TypeInfo HandlerType { get; }
+
+        public IReadOnlyList<object> HandlerAttributes { get; }


This should probably be HandlerTypeAttributes

rynowak · 2017-06-27T01:24:47Z

+        /// </summary>
+        public string PropertyName { get; set; }
+
+        public bool SupportsGets { get; set; }


This is part of binding info now 👍

rynowak · 2017-06-27T01:25:50Z

+            TypeInfo pageType,
+            TypeInfo modelType,
+            TypeInfo handlerType,
+            IReadOnlyList<object> handlerAttributes)


We should expose the route information here in a read-only format

What does that look like - something that looks like AttributeRouteInfo?

yeah or maybe just the route template as a string. It's mostly for informational purposes.

rynowak · 2017-06-27T01:27:34Z

+            pageModel.Filters.Add(new PageSaveTempDataPropertyFilterFactory());
+
+            // Always require an antiforgery token on post
+            pageModel.Filters.Add(new AutoValidateAntiforgeryTokenAttribute());


These should move to their own providers

rynowak

This looks great! Really on the right track here.

Fixes #6334

pranavkm · 2017-06-28T19:53:41Z

🆙 📅

rynowak · 2017-06-28T19:54:41Z

+        /// Gets the sequence of <see cref="PagePropertyModel"/> instances on <see cref="PageHandlerModel"/>.
        /// </summary>
-        public IList<SelectorModel> Selectors { get; }
+        public IList<PagePropertyModel> HandlerProperties { get; }


This should be BoundProperties like it is on controller model

ControllerModel has ControllerProperties (https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/ControllerModel.cs#L96). BoundProperties is on the ControllerAD.

rynowak · 2017-06-28T19:56:10Z

+        /// <summary>
+        /// Gets the page <see cref="TypeInfo"/>.
+        /// </summary>
+        public TypeInfo PageType { get; }


Do we need this here? Isn't this redundant with the AD?

The type isn't super useful on it's own because it's always generated, I'd say just the AD should be here.

PageActionDescriptor doesn't have a type info (just the paths). We need to pass this to the provider in some format, no?

Yeah ok, I guess we do need it. 👍

rynowak · 2017-06-28T19:56:31Z

+            IReadOnlyList<object> attributes)
+        {
+            MethodInfo = handlerMethod ?? throw new ArgumentNullException(nameof(handlerMethod));
+            Attributes = attributes ?? throw new ArgumentNullException(nameof(attributes));


Don't worry, one of these days all our null checks are going to look like this 😈

rynowak · 2017-06-28T19:56:50Z

+            if (other == null)
+            {
+                throw new ArgumentNullException(nameof(other));
+            }


Either you're with us or you're against us.

Well, that's the annoying bit - it only works for assignments, not regular null checks. Super inconsistent. Sort of one of the reasons I wasn't a big fan of it.

Yep. This is the using static of this round of features

rynowak · 2017-06-28T20:06:33Z

+        public string Name { get; set; }
+
+        /// <summary>
+        /// Gets the seqeunce of <see cref="PageParameterModel"/> instances.


rynowak · 2017-06-28T20:11:29Z


            var authorizeFilter = new AuthorizeFilter(policy);
-            options.Conventions.Add(new FolderConvention(folderPath, model => model.Filters.Add(authorizeFilter)));
+            options.ApplicationModelConventions.Add(new FolderApplicationModelConvention(folderPath, model => model.Filters.Add(authorizeFilter)));


Can you log a follow up to deal with the fact that we have a collection of conventions on MvcOptions and RazorPagesOptions. I don't like this but I don't want to block you.

Where would you want them to go? (So I know for the work item)

I don't know the answer yet

rynowak · 2017-06-28T20:13:47Z

+
+            var handlerModel = new PageHandlerModel(
+                method,
+                method.GetCustomAttributes(inherit: false))


We always do inherit: true for these things

rynowak · 2017-06-28T20:18:01Z

+            pageApplicationModel.Filters.Add(new PageSaveTempDataPropertyFilterFactory());
+
+            // Always require an antiforgery token on post
+            pageApplicationModel.Filters.Add(new AutoValidateAntiforgeryTokenAttribute());


This should be separate providers for each of these

rynowak · 2017-06-28T20:18:49Z

+        /// <summary>
+        /// Gets a list of <see cref="IPageRouteModelConvention"/> instances that will be applied to
+        /// the <see cref="PageModel"/> when discovering Razor Pages.
+        /// </summary>


I think both of these need to go into MvcOptions. Let's log an issue for this

rynowak

This is beautiful 👍 💯 🍺 🐶 😻

pranavkm requested a review from rynowak June 20, 2017 21:27

dnfclas added the cla-already-signed label Jun 20, 2017

pranavkm force-pushed the prkrishn/page-filters branch from 80fdecd to 260e60c Compare June 20, 2017 21:28

pranavkm commented Jun 20, 2017

View reviewed changes

pranavkm force-pushed the prkrishn/page-filters branch from 260e60c to bf63fe5 Compare June 24, 2017 01:16

rynowak reviewed Jun 27, 2017

View reviewed changes

rynowak mentioned this pull request Jun 27, 2017

Rationalize what happens when you have filter attributes on a page model or handler method #6334

Closed

pranavkm force-pushed the prkrishn/page-filters branch from bf63fe5 to edb1250 Compare June 28, 2017 19:49

pranavkm changed the title ~~[Design] Add support for specifying filters on page models.~~ Add support for specifying filters on page models. Jun 28, 2017

Add support for specifying filters on page models.

fa18eba

Fixes #6334

pranavkm force-pushed the prkrishn/page-filters branch from edb1250 to fa18eba Compare June 28, 2017 19:52

rynowak reviewed Jun 28, 2017

View reviewed changes

Refactor duplicate code

0879b35

rynowak reviewed Jun 28, 2017

View reviewed changes

rynowak approved these changes Jun 28, 2017

View reviewed changes

This was referenced Jun 28, 2017

Revisit convention types hanging off of MvcOptions and RazorPageOptions #6461

Closed

Move conventions defined in RazorPagesOptions to MvcOptions #6462

Closed

pranavkm closed this Jun 28, 2017

pranavkm deleted the prkrishn/page-filters branch June 28, 2017 22:14


		public TypeInfo HandlerType { get; }

		public IReadOnlyList<object> HandlerAttributes { get; }

Uh oh!

Conversation

pranavkm commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Jun 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!