OnActionExecutionAsync not overridable

[steveschmitt]

Problem introduced in PR #191

[biegehydra]

I think BreezeQueryFilterAttribute : Attribute, IAsyncActionFilter can be changed back to BreezeQueryFilterAttribute : ActionFilterAttribute and still work - while keeping the changes made in #191. I didn't realize but ActionFilterAttribute : IAsyncActionFilter and is overridable.

Dotnet Source Viewer

[graphicsxp]

hello, any news on this ? can it be fixed soon ?

[steveschmitt]

Busy on customer projects, but I hope to get to it this week.

[graphicsxp]

ok, let me know here when it's done, we really need this :) thanks a lot !

[steveschmitt]

Fixed now, in Breeze.AspNetCore.NetCore version 7.2.2.

[graphicsxp]

We want to keep using OnActionExecuted but although breeze let us override it, it does not implement it anymore, am i correct ? Is OnActionExecuting not supposed to call OnActionExecuted ?

Could you shine some light here please ?

[steveschmitt]

Looks like another oversight by me and @biegehydra .

In the base ActionFilterAttribute class, OnActionExecutionAsync calls both OnActionExecuting and OnActionExecuted. The Breeze QueryFilter does not call those methods, but it should.

Can you help me determine where & when QueryFilter should call those methods?

[biegehydra]

Yeah @steveschmitt seems we just can't get a break. I reckon OnActionExecuting(executingContext); can be called right at the beginning of OnActionExecutionAsync.

For OnActionExecuted, it's a little more confusing. Something like this after var executedContext = await next(); might work (didn't actually open ide):

OnActionExecuted(new ActionExecutedContext(executedContext, executedContext.Filters, executedContext.Controller) {
            Canceled = executedContext.Canceled,
            Exception = executedContext.Exception,
            ExceptionDispatchInfo = executedContext.ExceptionDispatchInfo,
            Result = executedContext.Result
        });

It may work, but it might supposed be executingContext in the ActionExecutedContext constructor

This is assuming we have to put it in OnActionExecutionAsync and we can't just override OnActionExecuted and OnActionExecuting??

Edit: I don't you're supposed to ever override the synchronous and asynchronous versions in the same filter. Source "When using abstract classes like ActionFilterAttribute, override only the synchronous methods or the asynchronous methods for each filter type"

[biegehydra]

This stackoverflow post seems to confirm my guess. https://stackoverflow.com/a/59062033/15947449

[steveschmitt]

I'm thinking that we would have the OnActionExecuted at the end, after we had run the query, like this:

    override public async Task OnActionExecutionAsync(ActionExecutingContext executingContext, ActionExecutionDelegate next) {
      var cancellationToken = executingContext.HttpContext.RequestAborted;

      OnActionExecuting(executingContext);
      if (!executingContext.ModelState.IsValid) {
        executingContext.Result = new BadRequestObjectResult(executingContext.ModelState);
      }
      if (executingContext.Result != null) {
        return;
      }

      var executedContext = await next();

      // ... apply and execute query ... set executedContext.Result ...

      OnActionExecuted(executedContext);
    }

@graphicsxp Do you think that would work for you?

[steveschmitt]

I'm having second thoughts about using ToListAsync() because of this issue and this SO post. Basically, queries take 10x as long to complete with ToListAsync() whenever there are varbinary(max) or nvarchar(max) columns.

Is it worth paying that penalty for the ability to pass a CancellationToken?

[graphicsxp]

personnaly I don't have the need to pass a cancellation token, so if you rollback to onactionexecuted I'll be happy.

[biegehydra]

I don't know about others but a lot of our usage with Breeze is for client side pagination.

Cancellation tokens are particular useful when paginating a table that has long queries. If a user changes their query or goes to the next page while a previous query is still running, we just cancel that request and start a new one.

This isn't possible without ToListAsync(). My experience before the change was that was it would have to complete all queries leading up to the last one before displaying results, which is unacceptable for us.

What do you think about a BreezeAsyncQueryFilter or something along those lines as a separate filter? Best of both worlds, the class isn't very long

[graphicsxp]

this sounds like a good solution to me, giving the choice between async and non-async filters. especially considering the issues reported about performance issues with async. It seems important to keep a fallback solution.

[steveschmitt]

As of release 7.3.0, there are now separate [BreezeQueryFilter] and [BreezeAsyncQueryFilter] attributes. Use the one that you prefer.