Introduce a base type for executing auth and resource filters. #5575
Conversation
|
GitHub is suppressing the diff for the file changes because it's too large. 😞 Should I send an offline review for this? |
| ControllerContext controllerContext, | ||
| IFilterMetadata[] filters, | ||
| ObjectMethodExecutor objectMethodExecutor) | ||
| : base(diagnosticSource, logger, controllerContext, filters, controllerContext?.ValueProviderFactories) |
There was a problem hiding this comment.
when can ControllerContext be null?
| { | ||
| await Next(ref next, ref scope, ref state, ref isCompleted); | ||
| } | ||
| await InvokeFilterPipelineAsync(); |
There was a problem hiding this comment.
Ideally everything in this method should be in the base class. Nothing here is specific to controllers.
There was a problem hiding this comment.
It is. We kick off the invocation from the subtype, and get called back at the end of resource filter invocation.
There was a problem hiding this comment.
We kick off the invocation from the subtype
Whatever the opposite of that statement is, that's what I am suggesting,
| finally | ||
| // The invoker is implemented using a 'Taskerator' or perhaps an 'Asyncerator' (both terms are correct | ||
| // and in common usage). This method is the main 'driver' loop and will call into the `Next` method | ||
| // (`await`ing the result) until a terminal state is reached. |
There was a problem hiding this comment.
IMA let you finish but there are some of the best comments of all time.
| @@ -1455,7 +1010,6 @@ private static void Rethrow(ResultExecutedContext context) | |||
| private enum Scope | |||
| { | |||
| Invoker, | |||
There was a problem hiding this comment.
Can the state here ever be Invoker?
There was a problem hiding this comment.
I could call this Outer. But it's basically the beginning of the "inner" loop.
There was a problem hiding this comment.
So then the scope here is Resource since we're inside resource filters
There was a problem hiding this comment.
Sure. I'll rename it.
| FilterAsync = filterAsync; | ||
| } | ||
|
|
||
| public int Index { get; } |
There was a problem hiding this comment.
Drop the index from this, it's never used
pranavkm
force-pushed
the
prkrishn/ResourceInvoker
branch
from
e0d0c8b
to
a145a11
Compare
No description provided.