dataobjects-net icon indicating copy to clipboard operation
dataobjects-net copied to clipboard

EntitySet and ToListAsync extension method runtime error

Open axmty opened this issue 1 year ago • 1 comments

Hi,

In my company we are migrating to DataObjects v7, but we encountered some problems concerning the AsAsync extension method that seemed to disappear, and that we used everywhere (on IQueryable<T> or EntitySet<T> objects). It seems that we can use ToListAsync instead, but we have a problem when using it on EntitySet.

When reading the code of EntitySet, we expected ToListAsync to work since EntitySet<T> implements IQueryable<T>, but there is a runtime error because EntitySet does not implement IAsyncEnumerable. Is there a reason why EntitySet<T> does not implement IAsyncEnumerable<T>?

After reading the implementation of Queryable<T>.GetAsyncEnumerator, I think we could do the same in the EntitySet<T> class (that is using EntitySet.Provider to get the same result):

public async IAsyncEnumerator<T> GetAsyncEnumerator(CancellationToken cancellationToken = default)
{
  var result = await Provider.ExecuteSequenceAsync<T>(expression, cancellationToken).ConfigureAwait(false);
  var asyncSource = result.AsAsyncEnumerable().WithCancellation(cancellationToken).ConfigureAwait(false);
  await foreach (var element in asyncSource) {
    yield return element;
  }
}

But maybe we are missing something that prevents us to do so.

We found a small workaround (myEntitySet.Where(x => 1 == 1).ToListAsync()) for the moment, but I would greatly appreciate some explanation.

Thanks a lot, Alex

axmty avatar Aug 02 '24 08:08 axmty

I have opened a PR here : #395

Let me know if that is ok.

axmty avatar Aug 02 '24 12:08 axmty

Hello @axmty

I appreciate your help, this is really cool that you want to commit to the project.

But there is a catch when working with/changing code of EntitySet<T> - it has internal state, a mixture of items that are in storage, locally added items and locally removed items. It was done so to minimize interactions with database, otherwise, changes of collection would spam database with INSERTs/DELETEs. Local state makes it possible to save changes of collection in batches, not wasting time on interactions with database.

This internal state should be taken into account, because I believe that IAsyncEnumerator and IEnumerator should return the same results for any case. Your implementation of GetAsyncEnumerator doesn't meet this requirement, it will work differently when there are local changes of EntitySet, because it will query database directly, so async method will return part of internal state, without added and with already deleted items. You can take a look at GetEnumerator() implementation and deeper into EntitySetBase.Entities property and how it provides results.

Internal state is even more important when session has no SessionOptions.AutoSaveChanges, because lack of this option makes DO keep and accumulate changes in internal states until manual saving happens.

So, it is not that simple task to make EntitySet<T> truly IAsyncEnumerable<T>, even for me, because it will probably require changes not in only ``EntitySet``` itself, but in its state and internal fetching mechanisms, which requires proper testing. Probably you don't want to contribute that much time and afford :)

I think that better idea, at least for now, is to have set of methods .ToXxxAsync() compatible with EntitySet<T>, similar to old .AsAsync(). I believe this is what you can contribute. What do you think? If you agree we'll discuss details.

alex-kulakov avatar Oct 25 '24 11:10 alex-kulakov

@axmty,

Since you didn't answer but the idea worth implementing, I made my own PR #413 to give ability to use the extensions you mentioned with EntitySets. One caviar is that usage of extensions performs queries to database which means it always returns results which are saved to database so in, for example, Client Profile session mode unsaved changes won't appear in results. This is known behavior of queries in Client Profile sessions.

I'm rejecting your PR.

alex-kulakov avatar Dec 24 '24 11:12 alex-kulakov