fluxera / Fluxera.Repository

A generic repository implementation.
MIT License
16 stars 3 forks source link

Add support for EFCore AsNoTracking() Linq extension. #57

Closed mgernand closed 1 year ago

mgernand commented 2 years ago

If we use AsNoTracking as default for EFCore queries we throw some exceptions because the attaching does not work with StronglyTypedIds somehow. Need to look into it.

jeffward01 commented 1 year ago

Hello!

Any thoughts on: EntityFrameworkQueryableExtensions.AsNoTrackingWithIdentityResolution also?

My implementation thought process:

I very much like your libraries and you have a very similar development thought process to my own in your libraries. I have my own 'Repository' implementation which I have been refactoring for a while, recently I added many of the Fluxera libraries to my own projects.

The only reason why I created my own implementation at this point was so that I could have more verbose methods, projections, pagination via Gridify rather than IQueryOptions<>, and a few other differences.


Code Snippets on implementation

My AsNoTracking implementation comes from a PR I made here: EasyRepository.EfCore

The owner of the Repo made had some opinions on how he made projections in his library which were a major limitation for me. So I forked it and have been expanding the feature set.

Below are snippets of how I implemented AsNoTracking, AsTracking and AsNoTrackingWithIdentityResolution in my similar library.

This is taken from the equivalent of your RepositoryBase class:

    /// <inheritdoc />
    async Task<PagedResponse<TProjected>> IAeonicCanFind<TEntity>.FindManyAsync<TProjected>(
        EfTrackingOptions asNoTracking,
        IPaginatedRequest filter,
        Expression<Func<TEntity, TProjected>> projectExpression,
        CancellationToken cancellationToken)
    {
        return await this._innerRepository.FindManyAsync(asNoTracking, filter, projectExpression, cancellationToken);
    }

    /// <inheritdoc />
    async Task<PagedResponse<TEntity>> IAeonicCanFind<TEntity>.FindManyAsync(
        EfTrackingOptions asNoTracking,
        IPaginatedRequest filter,
        Func<IQueryable<TEntity>, IIncludableQueryable<TEntity, object>> includeExpression,
        CancellationToken cancellationToken)
    {
        return await this._innerRepository.FindManyAsync(asNoTracking, filter, includeExpression, cancellationToken);
    }

    /// <inheritdoc />
    async Task<PagedResponse<TEntity>> IAeonicCanFind<TEntity>.FindManyAsync(
        EfTrackingOptions asNoTracking,
        IPaginatedRequest filter,
        CancellationToken cancellationToken)
    {
        return await this._innerRepository.FindManyAsync(asNoTracking, filter, cancellationToken);
    }

    /// <inheritdoc />
    IEnumerable<TProjected> IAeonicCanFind<TEntity>.FindMany<TProjected>(
        EfTrackingOptions asNoTracking,
        Expression<Func<TEntity, TProjected>> projectExpression)
    {
        return this._innerRepository.FindMany(asNoTracking, projectExpression);
    }

    /// <inheritdoc />
    IEnumerable<TProjected> IAeonicCanFind<TEntity>.FindMany<TProjected>(
        EfTrackingOptions asNoTracking,
        Expression<Func<TEntity, bool>> whereExpression,
        Expression<Func<TEntity, TProjected>> projectExpression)
    {
        return this._innerRepository.FindMany(asNoTracking, whereExpression, projectExpression);
    }

I have an enum called EfTrackingOptions that is:

/// <summary>
///     Sets EntityFramework queries to 
/// <see cref="AsNoTracking" />, <see cref="AsNoTrackingWithIdentityResolution"/> or <see cref="WithTracking" />
/// </summary>
public enum EfTrackingOptions
{
    /// <summary>
    ///     Disables EfCore change tracking
    /// <remarks>The fastest option, however I your query returns 4 books and their authors,
    ///  you’ll get 8 instances in total even if 2 books have the same author. </remarks>
    /// </summary>
    AsNoTracking,

    /// <summary>
    ///     Enables EfCore Change Tracking
    /// <remarks>Slowest option - however entities utilize full change tracking</remarks>
    /// </summary>
    WithTracking,

    /// <summary>
    ///  Disables EfCore change tracking - Will still perform identity resolution
    /// to eliminate duplicate instances of objects.
    /// <remarks><see cref="AsNoTrackingWithIdentityResolution"/> is a little slower than 
    /// the fastest option using <see cref="AsNoTracking"/></remarks> 
    /// </summary>
    AsNoTrackingWithIdentityResolution
}

Then I have an extension class similar to your QueryableExtensions that has the method:

    /// <summary>
    /// Applies change tracking to the <see cref="IQueryable{T}"/>
    /// </summary>
    /// <param name="queryable">The target <see cref="IQueryable{T}"/></param>
    /// <param name="trackingOptions">Tracking options. 
     ///  Default: <see cref="EfTrackingOptions.WithTracking"/></param>
    /// <typeparam name="T"></typeparam>
    /// <returns>Returns the <see cref="IQueryable{T}"/> with configured change tracking
    ///  based on <see cref="EfTrackingOptions"/></returns>
    public static IQueryable<T> Apply<T>(
        this IQueryable<T> queryable,
        EfTrackingOptions? trackingOptions)
        where T : class
    {
        Guard.Against.Null(queryable, nameof(queryable));
        if (trackingOptions == null)
        {
            return queryable.AsTracking();
        }

        return _configureChangeTracking();

        IQueryable<T> _configureChangeTracking()
        {
            return trackingOptions switch
            {
                EfTrackingOptions.WithTracking => queryable.AsTracking(),
                EfTrackingOptions.AsNoTrackingWithIdentityResolution => queryable.AsNoTrackingWithIdentityResolution(),
                _ => queryable.AsNoTracking()
            };
        }   

Then I can do this (this snippet is from the equivalent of LinqRepositoryBase

    /// <inheritdoc />
    protected sealed override IAsyncEnumerable<TProjected> FindManyAsync<TProjected>(
        EfTrackingOptions asNoTracking,
        IAeonicSpecification<TEntity> specification,
        Expression<Func<TEntity, TProjected>> projectExpression,
        CancellationToken cancellationToken = default)
    {
        var queryable = this.Queryable.Apply(asNoTracking)
            .Apply(specification)
            .ToProjection(projectExpression);

        return this.ToListAsync(queryable, cancellationToken);
    }

    /// <inheritdoc />
    protected sealed override IAsyncEnumerable<TEntity> FindManyAsync(
        EfTrackingOptions asNoTracking,
        CancellationToken cancellationToken = default)
    {
        var queryable = this.Queryable.Apply(asNoTracking);

        return this.ToListAsync(queryable, cancellationToken);
    }

    /// <inheritdoc />
    protected sealed override IAsyncEnumerable<TEntity> FindManyAsync(
        EfTrackingOptions asNoTracking,
        Expression<Func<TEntity, bool>> whereExpression,
        CancellationToken cancellationToken = default)
    {
        var queryable = this.Queryable.Apply(asNoTracking)
            .Apply(whereExpression);

        return this.ToListAsync(queryable, cancellationToken);
    }
jeffward01 commented 1 year ago

I can create a PR on Fluxera.Repository - but i'd like to discuss a few points:

IAsyncEnumerable<TEntity> FindManyAsync(
        EfTrackingOptions asNoTracking,
        CancellationToken cancellationToken = default)

Side question:

Does your team have a Discord server or anything? I'd love to become more involved in your codebase and libraries if possible.

If you have a Discord server or account, feel free to add me so that we can chat and collaborate more

Thanks, Jeff

My Dicscord Username: crypto_wizard#0725

jeffward01 commented 1 year ago

If we use AsNoTracking as default for EFCore queries we throw some exceptions because the attaching does not work with StronglyTypedIds somehow. Need to look into it.

Perhaps you can reference a value converter and 'manually' attach it each time?

I'm not sure - im just thinking outloud. I will look into it as well, I don't use StronglyTypedId's yet with my code, but I plan to integrate them asap.

I'm sure you have read this, but maybe the links will be helpful:

Entity Framework Core

When I get around to implementing this, i'll test it and let you know my findings

mgernand commented 1 year ago

Hi!

sorry for my late reply. I've been very busy the last few days.

Any thoughts on: EntityFrameworkQueryableExtensions.AsNoTrackingWithIdentityResolution also?

Yes, absolutely. In fact, I'd like to have a single point of abstraction leakage to allow anything storage specific to be configured.

The only reason why I created my own implementation at this point was so that I could have more verbose methods, projections, pagination via Gridify rather than IQueryOptions<>, and a few other differences.

One big advantage of the IQueryOptions<> is that it is not dependend on storage-specific APIs and can be re-used just like ISpecification. We actually re-use specification classes across different storages using the same domain model. Therefore adding Includes and NoTracking there would break this.

Adding storage-specific APIs lite EfTrackingOptions as parameters to the IRepository contract also breaks compatibility with other storages. So at the moment I think the right place to add a controlled abstraction leak would still be the IQueryOptions.


/// <inheritdoc />
   async Task<PagedResponse<TEntity>> IAeonicCanFind<TEntity>.FindManyAsync(
       EfTrackingOptions asNoTracking,
       IPaginatedRequest filter,
       Func<IQueryable<TEntity>, IIncludableQueryable<TEntity, object>> includeExpression,
       CancellationToken cancellationToken)
   {
       return await this._innerRepository.FindManyAsync(asNoTracking, filter, includeExpression, >cancellationToken);
   }

in this example of yours you pass in a Func<IQueryable<TEntity>, IIncludableQueryable<TEntity, object>> which is essentially what I built today for the IQueryOptions. Just ab bit more hiden. So I am thinking we could extend the IQueryOptions with something like a ILeakQueryableOptions where one has access to the underlying IQueryable and can do anything with it.

Do you want the default to be AsTracking (tracking enabled by default)?

I think it is the default at the moment. But maybe it would be a good idea to make AsNoTracking the default later.

Does your team have a Discord server or anything? I'd love to become more involved in your codebase and libraries if possible.

The team is I at the moment. :-D I don't have a discord server. But I could invite you to a Teams channel if you like.

Cheers, Matt

mgernand commented 1 year ago

If we use AsNoTracking as default for EFCore queries we throw some exceptions because the attaching does not work with StronglyTypedIds somehow. Need to look into it.

Perhaps you can reference a value converter and 'manually' attach it each time?

I'm not sure - im just thinking outloud. I will look into it as well, I don't use StronglyTypedId's yet with my code, but I plan to integrate them asap.

I'm sure you have read this, but maybe the links will be helpful:

Entity Framework Core

  • dotnet/efcore#11597 addresses the problem of database-generated values with converters. It’s been postponed for several releases but hopefully it will be implemented at some point.
  • dotnet/efcore#10784 will provide a mechanism to set a converter for all properties of a given type; it’s in the 6.0 milestone, so unless it’s postponed, it should be in the next major release.

When I get around to implementing this, i'll test it and let you know my findings

There is actually a value converter available for the strongly typed IDs. I really don't know anymore what the problem was with them in combination with AsNoTracking. I think something with the re-attaching of objects when updating. But you can use strongly typed IDs with the ERCore repository right now. The value converter dos it's thing and the IDs are (de)serialized.

Using strongly typed IDs has saved me alot of headaches. I like this pattern a lot.

C# 9 records as strongly-typed ids - Part 5: final bits and conclusion

Yes, I read this article. The implementation of the calue onverter is somewhat based on it. I will have a look at the EF Core isses you linked when I have the time. Maybe I'll remember what the problem was and write a failing unit test for it.

Thanks for your input. 👍

mgernand commented 1 year ago

Another isse is that not every potential new underlying storage will support LINQ/IQueryable. Actually LiteDB does not have a IQueryable implementation. last week I implemented a very simple one myself. It is mostly a warpper around the ILiteCollection. I needed this quickly, but it may use some more eyeballs looking at it.