dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.63k stars 3.15k forks source link

AutoInclude() for self-referencing entities. Resolving cycles. #34198

Closed douglasg14b closed 1 week ago

douglasg14b commented 1 month ago

Problem

    public class Company
    {
        public int Id { get; set; }

        [Owned]
        public BillingDetails BillingDetailsInternal { get; set; }

        // May be a different company, or this company
        public Company BillingOwner { get; set; }
        public int BillingOwnerId { get; set; }

        // Transparent to business logic,, data-driven, not driven by checks in every location this is accessed
        public BillingDetails BillingDetails => BillingOwner.BillingDetailsInternal;
    }

I have an entity which can have a BillingOwner which may refer to itself, or to another entity. This navigation must always be included. for the purpose of transparently making this change.

This results in:

Cycle detected while auto-including navigations: 'Company.BillingOwner

However, this appears to be a resolvable or avoidable cycle.

Desired Solution

The ability for auto includes to either:

ajcvickers commented 1 month ago

Removing from the backlog since this isn't properly triaged.

Wasserwecken commented 1 month ago

I made two extension methods to resolve this issue for me. Im not sure there is a better approach, if so, please tell!

// example usage
var query = new YourDbContext().AnyTable.IncludeLevels(2).Where(e => e.Foo == true);

/// <summary>
/// Includes all navigations and sub relations until a certain depth.
/// </summary>
public static IQueryable<TEntity> IncludeLevels<TEntity>(this DbSet<TEntity> dbSet, int level)
    where TEntity : class
{
    if (level > 0)
    {
        IQueryable<TEntity> query = dbSet;
        var navigations = dbSet.EntityType.ListNavigationPaths(level - 1);

        foreach (var path in navigations)
            query = query.Include(path);

        return query;
    }

    return dbSet;
}

/// <summary>
/// Returns a list that contains all navigations and their navigations recursivly.
/// </summary>
/// <param name="entityType">Type to search for navigations</param>
/// <param name="depth">Limits the recursion</param>
/// <param name="prefix">Internal use: Prefix for before the property name</param>
/// <param name="propertyPaths">Internal use: List that is created as result</param>
/// <returns></returns>
public static List<string> ListNavigationPaths(this IEntityType entityType, int depth, string prefix = "", List<string> propertyPaths = null!)
{
    var navigations = entityType
        .GetConcreteDerivedTypesInclusive()
        .SelectMany(e => e.GetNavigations())
        .Distinct();

    propertyPaths ??= [];
    propertyPaths.AddRange(navigations.Select(e => $"{prefix}{e.Name}"));

    if (depth > 0)
        foreach (var nav in navigations)
            ListNavigationPaths(nav.TargetEntityType, depth - 1, $"{prefix}{nav.Name}.", propertyPaths);

    return propertyPaths;
}
roji commented 1 month ago

Cycle detected while auto-including navigations: 'Company.BillingOwner

Given the code above, it seems very reasonable for me that this error is produced; it's simply the logical consequence of how the model is configured. It is certainly possible to imagine knobs or switches that configure arbitrary alternative behavior, but I'd rather have this discussion based on a concrete user request for one of those behaviors, rather than discussing what possible other behaviors are imaginable...

douglasg14b commented 1 month ago

@roji

but I'd rather have this discussion based on a concrete user request for one of those behaviors, rather than discussing what possible other behaviors are imaginable...

I'm not following, can you clarify. Is this not such a request?

In this case, the feature request is the ability to resolve resolvable cycles with auto-included navigation. I listed potential, general options, not esoteric arbitrary ones, that seemed reasonable for both this example, and likely others 🤔

Wasserwecken commented 1 month ago

I partially agree with @roji . You cannot know the depth of your hierarchy unless you look into the database for the specific record. Thus there is no "simple" option to write a SQL statement to let the database handle it. The same goes with cyclic dependencies. There might be one, or not, you only know if you query. Many answers on stack overflow are ending up in common table expressions: https://stackoverflow.com/questions/50627165/selecting-entire-hierarchy-of-rows-from-a-self-referencing-table

But CTS's are not implemented yet i guess? https://github.com/dotnet/efcore/issues/29918 https://github.com/dotnet/efcore/issues/26486

My code is just a proposal to workaround for @douglasg14b problem, which I had kindof too. To upport @douglasg14b proposal: It would be great if we can somhow specify for entites to automatically include all Navigations until a certain depth. And the depth counts from the root IQueryable entity? Such as my Code does. This would enable for easy postprocessing to resolve cycles.

roji commented 1 month ago

I'm not following, can you clarify. Is this not such a request?

Sure. The issue you posted basically highlights a current by-design limitation, and then discusses possible alternatives; that's not a user need, but rather a "this is what we could do" kind of discussion.

What I'm looking for is a concrete user request that provides context and justification for one of these behaviors. For example, I'm struggling to imagine a case where it makes sense to auto-include up to a specific depth (e.g. 2), in a universal manner (since this would be defined in the model). If it really does make sense to always load with a depth of 2, that points to there being a different meaning to the different levels (otherwise why stop at 2?), and that could point to incorrect modeling - the user may want to model the different levels as different entity types (expressing the differences that justify the arbitrary depth) instead of trying to model everything as the same navigation, but with an arbitrary depth limit.

Apart from that, if we're talking about loading the entire graph (and auto-resolving), that could possibly be done via CTEs (WITH), as pointed out by @Wasserwecken, but these aren't yet supported by EF and we'd need to carefully think about how it all fits together.

ajcvickers commented 1 week ago

EF Team Triage: Closing this issue as the requested additional details have not been provided and we have been unable to reproduce it.

BTW this is a canned response and may have info or details that do not directly apply to this particular issue. While we'd like to spend the time to uniquely address every incoming issue, we get a lot traffic on the EF projects and that is not practical. To ensure we maximize the time we have to work on fixing bugs, implementing new features, etc. we use canned responses for common triage decisions.