Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot avoid using synchronous calls in OData with entity framework #1194

Open
Shiney opened this issue Mar 15, 2024 · 6 comments
Open

Cannot avoid using synchronous calls in OData with entity framework #1194

Shiney opened this issue Mar 15, 2024 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@Shiney
Copy link

Shiney commented Mar 15, 2024

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.

@xuzhg
Copy link
Member

xuzhg commented Mar 18, 2024

@ElizabethOkerio Is it related to the previous IAsyncEnumerable fix?

@ElizabethOkerio
Copy link
Contributor

ElizabethOkerio commented Mar 18, 2024

@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

@julealgon
Copy link
Contributor

@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"?

@ElizabethOkerio
Copy link
Contributor

@julealgon you are right.

@Shiney
Copy link
Author

Shiney commented Mar 19, 2024

It does looks like there is a way to do this attached to this item on the WebAPI repository OData/WebApi#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.

@zolotovvv
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants