Skip to content
This repository has been archived by the owner. It is now read-only.

Filter overrides #474

Closed
NTaylorMullen opened this issue May 10, 2014 · 11 comments

Comments

@NTaylorMullen
Copy link
Member

commented May 10, 2014

In MVC 5 and Web API 2 we can override filters by type, MVC 6 will need to add same or similar functionality.

@JaneZhouQ JaneZhouQ added this to the Post Alpha milestone May 13, 2014

@yishaigalatzer yishaigalatzer removed this from the Post Alpha milestone May 23, 2014

@danroth27 danroth27 added this to the Backlog milestone Oct 16, 2014

@filipw

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2015

what is the status of this?

@yishaigalatzer

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2015

Hey @filipw this is currently not planned for RTM (you can see from the backlog milestone).

You can achieve this currently by using an ActionDescriptorProvider that will remove the filter from the action descriptor and introspect properties on the ActionDescriptor (or attributes on the methodinfo). It's a pretty big hammer, but it can definitely work and it's not too complicated to write.

@filipw

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2015

thanks. just to make sure I understood you correctly.
so you are suggesting something like this (not sure if this would even work, typing in github not VS 😄 )

Let's say I create a marker filter that allows me to override any defined filter type:

public class OverrideFilter : ActionFilterAttribute
{
    public Type Type { get; set; }
}

Then I create a custom IActionDescriptorProvider that does something like this in the Invoke?

    public void Invoke(ActionDescriptorProviderContext context, Action callNext)
    {
        var descriptorsWithFiltersToOverride = context.Results.Where(x => x.FilterDescriptors.Any(f => f.Filter is OverrideFilter));
        if (descriptorsWithFiltersToOverride.Any())
        {
            foreach (var actionDescriptor in descriptorsWithFiltersToOverride)
            {
                var toRemove = actionDescriptor.FilterDescriptors.FirstOrDefault(f => f.Filter is OverrideFilter);
                actionDescriptor.FilterDescriptors.RemoveAll(x => x.Filter.GetType() == ((OverrideFilter)toRemove.Filter).Type || x.Filter is OverrideFilter);
            }
        }

        context.Results.AddRange(GetDescriptors());
        callNext();
    }

so look for descriptors which have my marker filter at any level, and use its Type property to remove the matching filter from the pipeline?

ps. would be even easier if ControllerActionDescriptorProvider had virtual members

@yishaigalatzer

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2015

The code looks reasonable but I have a few issues with it. First some code comments

Code feedback

  1. You don't need to remove the override filter when you are done, you can just leave it there. Its harmless
  2. You need to inspect for factory filters (namely TypeFilter or ServiceFilter) if you care.
  3. The loop can go over context.Results.Where() directly (so no var and if)
  4. You need an inner loop for all instances of overrides.

The limitations of this approach

  1. This will remove defined filters, but removing by type is hard (or not 100% guaranteed) for factory filters for example.
  2. Remove by Type was something I never liked, it is not specific enough and quite limiting. By the way MVC 6 doesn't take the approach of Web API where only one type of filter per action will run.
  3. Filters can be created by FilterProvider (though none are today), these don't show up on the list of filters on the action descriptor.

Alternative

I looked at the MVC code again, and I think there is another approach for this. You can write a FilterProvider. FilterProviders run at execution time, and can remove the filters materialized from metadata, so it will work for factory filters or filters coming from the provider. Since the provider is a nested model, you can easily interact with other providers if someone else adds them (just like ActionDescriptorProvider).

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/Filters/FilterProviderContext.cs
https://github.com/aspnet/Mvc/blob/12c2759cec61674eb00f60a66a4df73b1af0e309/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs

yield return describe.Transient<INestedProvider<FilterProviderContext>, DefaultFilterProvider>();

@filipw

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2015

Thank you - this is very helpful. I looked at the filter provider and indeed it looks rather straight forward to override or reshuffle the necessary filters from there.

I am only concerned about the unnecessary work happening over and over on every request; would you advise against caching the result of the filtering?
It seems caching this would be a reasonable thing to do, especially as, intuitively, whatever filter pipeline is relevant on request number 1 should be relevant forever. Unless the provider model was intended to also support scenarios such as injecting extra filters whenever needed, for example if it's Thursday, use this extra filter?

I didn't realize that multiple filters of same type could run per action, I'll keep that in mind.

@yishaigalatzer

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2015

I thought about it, and i wouldn't worry about running it again and again without any form of measurements proving its an issue. My wild guess here is that you wouldn't see it show up in your profile

@filipw

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

thanks - it really helped. I have published a blog post summarizing this discussion

@yishaigalatzer

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

Awesome blog: One comment though, don't derive from the base type. It might do more than you bargained for. If you do, do not call base.

It's better to just go with the interface it is super simple. And you get to explain the next() call 😊

-----Original Message-----
From: "Filip W" notifications@github.com
Sent: ‎2/‎12/‎2015 3:07 AM
To: "aspnet/Mvc" Mvc@noreply.github.com
Cc: "Yishai Galatzer" yishai_galatzer@yahoo.com
Subject: Re: [Mvc] Filter overrides (#474)

thanks - it really helped. I have published a blog post summarizing this discussion

Reply to this email directly or view it on GitHub.

@filipw

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2015

I'm not sure what is the use case for IFilterContainer (doesn't seem to be used anywhere at the moment), but if I don't call the base, the reconciliation between IFilterContainer as created by the filter factory and the filter metadata will never happen - and if there ever is an IFilterContainer in the list, it simply didn't seem right; that was really the only reason.

@yishaigalatzer

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2015

Probably because you didn't call next

-----Original Message-----
From: "Filip W" notifications@github.com
Sent: ‎2/‎13/‎2015 4:44 AM
To: "aspnet/Mvc" Mvc@noreply.github.com
Cc: "Yishai Galatzer" yishai_galatzer@yahoo.com
Subject: Re: [Mvc] Filter overrides (#474)

I'm not sure what is the use case for IFilterContainer (doesn't seem to be used anywhere at the moment), but if I don't call the base, the reconciliation between IFilterContainer as created by the filter factory and the filter metadata will never happen - and if there ever is an IFilterContainer in the list, it simply didn't seem right; that was really the only reason.

Reply to this email directly or view it on GitHub.

@Eilon

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

Closing because we have no plans to support this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.