Naming inconsistency between ActionSelectionDecisionTree and ActionSelectorDecisionTreeProvider

[tuespetre]

These two classes (and their pseudo-eponymous interfaces) are inconsistent - they should both either be Selector or Selection.

[tuespetre]

It also seems strange that IActionSelectionDecisionTree resides under Routing while IActionSelectorDecisionTreeProvider, ActionSelectionDecisionTree, and ActionSelectorDecisionTreeProvider all reside under Internal. These classes also do not follow the Default prefix convention used by other sets of types.

[rynowak]

Did you find something interesting here for your own usage? I've got plans to delete all of this code and replace it with something much simpler in 1.0.1 😀

[tuespetre]

@rynowak not particularly; I'm just trying to fully learn and document the 'big picture' going on with this codebase 🏆

Would love to chat sometime, or otherwise forward some questions I have your (team's) way.

[tuespetre]

@rynowak more from this area:

There are only two direct implementations of IActionConstraintFactory in this repo and neither of them actually utilize the IServiceProvider that gets passed in:

• VersioningWebSite.VersionAttribute
• VersioningWebSite.VersionRoute

If an IActionConstraint ever needed to use the service locator pattern, it could do so by accessing AccessConstraintContext.RouteContext.HttpContext.RequestServices instead.

If this interface could be removed, the IActionConstraintMetadata interface could also be removed, as well as the ActionConstraintItem.IsReusable property.

It looks like this all ties back into the default ActionSelector implementation which uses the ActionConstraintCache which uses the IActionConstraintProvider, which in itself seems kind of pointless in light of the above (maybe I'm missing something?) Is this part of the code you are planning to delete? ❔

[javiercn]

@tuespetre The goal of having factories is to enable constraints, filters, etc that require services from DI to have a well known composition root, which in this case is the factory.

If a constraint does not require services, it just implements IActionConstraint, if a constraint requires services from DI, you create a factory and resolve the constraint from there. That way the constraint can just take their dependencies on the constructor and the use of service locator is limited to the factory.

VersioningWebSite is probably just a website for functional tests. It might have gotten out of date or overlook.

[tuespetre]

Thanks @javiercn.

The problem that I see with IActionConstraintFactory (and as you pointed out, IFilterFactory) is that they do little beyond providing additional interfaces and infrastructure to maintain.

That way the constraint can just take their dependencies on the constructor and the use of service locator is limited to the factory.

If someone was really dead-set on keeping service location out of a particular action constraint or filter implementation, they could just have their WhateverConstraintAttribute implement IActionConstraint such that it delegates to the dependency-injectable IActionConstraint. It would be the same approach for filters.

I find the fact that there is no 'true' implementation of IActionConstraintFactory within the codebase to be telling. As for IFilterFactory, there are a handful:

• AutoValidateAntiForgeryTokenAttribute
• CorsAuthorizationFilterFactory
• FormatFilterAttribute
• ResponseCacheAttribute
• SaveTempDataAttribute
• ServiceFilterAttribute
• TypeFilterAttribute
• ValidateAntiForgeryTokenAttribute

These do very little besides resolving an instance of IFilter from services. ResponseCacheAttribute looks to be more involved because it has a little more ceremony to set up its filter. CorsAuthorizationFilterFactory seems to add no real value of any kind, lending itself as evidence that these concepts only make sense for attributes. All of them except for TypeFilterAttribute and ServiceFilterAttribute return true for IsReusable, and those two seem like fluffy, 'magical' affordances that are questionable in themselves considering that an author who felt the need to use one of those could have written a proper IFilterMetadata attribute with little effort.

The IsReusable thing seems to be a bit of a premature optimization, because even if a given action had like, five filters or constraints that needed to resolve a delegate filter or constraint like those listed above, the fact that the filter or constraint is 'reusable' indicates that it is a singleton -- so what is being saved per request to that action, a tiny handful of singleton resolutions?

[rynowak]

It looks like this all ties back into the default ActionSelector implementation which uses the ActionConstraintCache which uses the IActionConstraintProvider, which in itself seems kind of pointless in light of the above (maybe I'm missing something?) Is this part of the code you are planning to delete? ❔

This is all provided for users, not for the framework. No planning to delete any extensibility points, just to reimplement action selection e713017

[tuespetre]

@rynowak ok, I will close this one out then 👍