AspNetCoreOData icon indicating copy to clipboard operation
AspNetCoreOData copied to clipboard

Cannot avoid using synchronous calls in OData with entity framework

Open Shiney opened this issue 11 months ago • 6 comments

Assemblies affected Microsoft.AspNetCore.OData 8.2.5 (but probably all of them

Describe the bug When using an IQueryable generated via entity framework the default behaviour is to have synchronous calls to the database rather than using it as an IAsyncEnumerable to avoid this, there is a workaround for this (described below) but I think the default behaviour should be to do this Asynchronously. Also if you add $count=true then the counting will always happen synchronously and it's not even obvious what sort of workaround there would be.

Reproduction steps Checkout the repo https://github.com/Shiney/AsyncEfCoreOdata/blob/main/AsyncEfCoreOdata/Controllers/Controller.cs here. Then run it, which should go to .../Odata/Customers/$filter=Id%20eq%201%20or%20Id%20eq%203&$count=true

This will then throw an error because I've configured non async DbDataReaders to not work.

The crucial code looks like this


    /// <summary>
    /// Throw an error if the reader is not async
    /// </summary>
    private class ReaderInterceptor : DbCommandInterceptor
    {
      public override DbDataReader ReaderExecuted(DbCommand command, CommandExecutedEventData eventData, DbDataReader result)
      {
        throw new InvalidOperationException("Expect all calls to read to be async");
      }
    }

    [EnableQuery]
    public async Task<ActionResult> Get()
    {

      var options = new DbContextOptionsBuilder<MyDbContext>()
        .AddInterceptors(new ReaderInterceptor())
        .UseSqlServer(
          "Server=(localdb)\\mssqllocaldb;TrustServerCertificate=True;Trusted_Connection=True;MultipleActiveResultSets=true;pooling=true;Command Timeout=300;");
      await Task.Delay(100);
      // do not dispose so that lives for the whole request
      // this is a bad practice, but it is done here to demonstrate the issue
      var context = new MyDbContext(options.Options);

      // Use a Values statement to return customers
      return Ok(context.Database.SqlQueryRaw<Customer>(
        "Select 1 as Id, 'Customer 1' as Name union all Select 2 as Id, 'Customer 2' as Name union all Select 3 as Id, 'Customer 3' as Name")); 
    }

  }
  public class Customer
  {
    public int Id { get; set; }
    public string Name { get; set; }
  }

Partial workaround

If it wasn't for the $count=true I am able to partially work around this by using a custom attribute (see here for working example https://github.com/Shiney/AsyncEfCoreOdata/blob/partial-workaround/AsyncEfCoreOdata/Controllers/Controller.cs)

 public class CustomEnableQueryAttribute : EnableQueryAttribute
  {
    public override void OnActionExecuted(ActionExecutedContext actionExecutedContext)
    {
      base.OnActionExecuted(actionExecutedContext);
      if (actionExecutedContext.Result is ObjectResult { DeclaredType: null } objectResult)
      {
        if (objectResult.Value is IAsyncEnumerable<Customer>)
        {
          objectResult.DeclaredType = typeof(IAsyncEnumerable<Customer>);
        }
      }
    }
  }

Because this means we hit the condition to use the IAsyncEnumerable in ODataResourceSetSerializer.WriteObjectInlineAsync

However I couldn't work out a similar workaround for the count property.

Maybe the check in ODataResourceSetSerializer.WriteObjectInlineAsync should check if writeContex.Type is assignable to an IAsyncEnumerable instead of just checking the type is equal to it.

Shiney avatar Mar 15 '24 09:03 Shiney

@ElizabethOkerio Is it related to the previous IAsyncEnumerable fix?

xuzhg avatar Mar 18 '24 04:03 xuzhg

@Shiney The ODataFeature class includes a property called TotalCountFunc, which accepts a Func<long> delegate. This delegate allows you to provide a custom implementation for retrieving the total count asynchronously.

public Func<long> TotalCountFunc { get; set; }

Then you can set this from the controller like this:

Request.ODataFeature().TotalCountFunc = //customfunc

ElizabethOkerio avatar Mar 18 '24 13:03 ElizabethOkerio

@ElizabethOkerio would it make sense to also provide a version of that property with a Func<Task<long>> signature, to allow for consumers to use an async method for the count as well?

This delegate allows you to provide a custom implementation for retrieving the total count asynchronously.

I feel there is a misunderstanding here on what is meant by "asynchronously". Without exposing that with a Task, it is not possible to use an async call to do it. Did you mean "separately" instead of "asynchronously"?

julealgon avatar Mar 18 '24 14:03 julealgon

@julealgon you are right.

ElizabethOkerio avatar Mar 18 '24 14:03 ElizabethOkerio

It does looks like there is a way to do this attached to this item on the WebAPI repository https://github.com/OData/WebApi/issues/2325 it seems a lot more involved thought than my workaround to make it all think the system think it's an async enumerable. It's also quite scary as it would be quite easy to implement something like that and implement some sort of subtle bug where you counted the data after you'd done the skip/top rather than befor.

Shiney avatar Mar 19 '24 07:03 Shiney

Also if you add $count=true then the counting will always happen synchronously and it's not even obvious what sort of workaround there would be.

Similar behavior appears if the PageSize is specified for the EnableQuery attribute.

zolotovvv avatar Mar 30 '24 14:03 zolotovvv