Add support for specifying filters on page models. #6417

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

No description provided.

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

src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageLoader.cs

@@ -59,14 +69,45 @@ public CompiledPageActionDescriptor Load(PageActionDescriptor actionDescriptor)
                handlerMethods = CreateHandlerMethods(pageType);
            }

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


Layering concerns?

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

test/WebSites/RazorPagesWebSite/ModelWithPageFilter.cs

+    }
+
+    [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

test/WebSites/RazorPagesWebSite/HelloWorldWithPageModelHandler.cs

@@ -1,6 +1,7 @@
 // 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

src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageLoader.cs

+            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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/IPageApplicationModelProvider.cs

        /// </summary>
        /// <param name="context">The <see cref="PageApplicationModelProviderContext"/>.</param>
        void OnProvidersExecuting(PageApplicationModelProviderContext context);

        /// <summary>
-        /// Executed for the second pass of building <see cref="PageApplicationModel"/> instances. See <see cref="Order"/>.
+        /// 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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/IPageRouteModelConvention.cs

+        /// </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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageApplicationModel.cs

-        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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageApplicationModel.cs

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


This should probably be HandlerTypeAttributes

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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PagePropertyModel.cs

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


This is part of binding info now 👍

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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageApplicationModel.cs

+            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

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/DefaultPageApplicationModelProvider.cs

+            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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageApplicationModel.cs

        /// </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

...Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageApplicationModelProviderContext.cs

+        /// <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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageHandlerModel.cs

+            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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageHandlerModel.cs

+            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

src/Microsoft.AspNetCore.Mvc.RazorPages/ApplicationModels/PageHandlerModel.cs

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


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

src/Microsoft.AspNetCore.Mvc.RazorPages/DependencyInjection/RazorPagesOptionsExtensions.cs

@@ -157,7 +157,7 @@ public static RazorPagesOptions AuthorizeFolder(this RazorPagesOptions options,
            }

            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

src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageApplicationModelProvider.cs

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


We always do inherit: true for these things

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

src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageFilterApplicationModelProvider.cs

+            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

src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptions.cs

+        /// <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; }

Add support for specifying filters on page models. #6417

Add support for specifying filters on page models. #6417

Conversation

pranavkm commented Jun 20, 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

pranavkm commented Jun 24, 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

pranavkm commented Jun 28, 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

rynowak left a comment

Choose a reason for hiding this comment