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

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

Closed
douglasg14b opened this issue Jul 10, 2024 · 7 comments
Closed

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

douglasg14b opened this issue Jul 10, 2024 · 7 comments
Assignees
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported type-enhancement

Comments

@douglasg14b
Copy link

douglasg14b commented Jul 10, 2024

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:

  • Auto-resolve resolvable cycles
  • Allow a configuration to specify the behavior for a self-referencing cycle
  • Allow a configuration to specify the max depth in the case of cycles for a navigation (ie. 1)
  • Allow "conditional" auto-includes
  • ??? Something else?
@douglasg14b douglasg14b changed the title AutoInclude() for self-referenecing entities. Resolving cycles. AutoInclude() for self-referencing entities. Resolving cycles. Jul 10, 2024
@maumar maumar added this to the Backlog milestone Jul 10, 2024
@ajcvickers
Copy link
Contributor

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

@ajcvickers ajcvickers removed this from the Backlog milestone Jul 11, 2024
@Wasserwecken
Copy link

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
Copy link
Member

roji commented Jul 12, 2024

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
Copy link
Author

douglasg14b commented Jul 15, 2024

@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
Copy link

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?
#29918
#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
Copy link
Member

roji commented Jul 19, 2024

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
Copy link
Contributor

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.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Aug 26, 2024
@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported type-enhancement
Projects
None yet
Development

No branches or pull requests

5 participants