Efcore: Stackoverflow exception for circular model configuration for eager load (AutoInclude)

[vflame]

If two entity navigations with references to each other are both configured with AutoInclude() for eager loading, a stackoverflow exception is thrown. Related feature: https://github.com/dotnet/efcore/issues/21540

Occurs regardless of whether or not the query is tracked via AsNoTracking().

Steps to reproduce

Configurations:
```c#
public class CurrencyBagConfig : IEntityTypeConfiguration
{
public void Configure(EntityTypeBuilder builder)
{
builder.Property("Id").ValueGeneratedOnAdd();
builder.HasKey("Id");

        builder.HasMany(p => p.Currencies).WithOne("_currencyBag");
        builder.Navigation(p => p.Currencies).AutoInclude();


    }
}


public class CurrencyConfig : IEntityTypeConfiguration<Currency>
{
    public void Configure(EntityTypeBuilder<Currency> builder)
    {
        builder.Property<string>("Id").ValueGeneratedOnAdd();
        builder.HasKey("Id");

        builder.HasOne<CurrencyBag>("_currencyBag"); //optional
        builder.Navigation("_currencyBag").AutoInclude();

    }
}

Models:

```c#
 public class CurrencyBag
    {
        public IEnumerable<Currency> Currencies { get; set; }
    }



    public class Currency
    {
        public decimal Amount { get; set; }
        public decimal Code { get; set; }

        private CurrencyBag _currencyBag;

    }

Test harness:
c# var ctx = host.Services.CreateScope().ServiceProvider.GetRequiredService<ApplicationDbContext>(); ctx.Database.EnsureDeleted(); ctx.Database.EnsureCreated(); //ctx.Database.Migrate(); var currencyBag = ctx.Set<CurrencyBag>().ToList(); //stackoverflow thrown

Can provide reproduction if needed.

Further technical details

EF Core version: RC1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .net 5 RC1
Operating system: WIN10
IDE: VS 2019 Preview 3

[smitpatel]

@ajcvickers - RC2 candidate.

[smitpatel]

@AndriySvyryd - Can our Graph implementation figure out cycles in graph?

[AndriySvyryd]

@smitpatel Yes

[ajcvickers]

Agreed; please prepare a fix for RC2 and I'll send it for approval.

[AndriySvyryd]

@smitpatel But we don't need to detect cycles, we just need to avoid auto-including the same navigation more than once for a given query.

[smitpatel]

@AndriySvyryd - Is that correct behavior though? It could easily cause same thing when people try to load n levels of navigations.

[AndriySvyryd]

n would be under the user's control

[smitpatel]

How?
Suppose A1 loads B1, B1 loads C1 and C1 loads A2 then we have to follow the chain "forever".
Another example, Employee has manager and auto including manager. Each level will bring in new employee which need more levels of includes.

If we only include 1 level then it causes inconsistency that bringing in more query roots won't apply auto includes.

[AndriySvyryd]

@smitpatel What I am saying is that we only autoinclude each navigation once, so for the first case we won't autoinclude A2.B and for the second we only include one manager.

We'll log a warning that an autoincluded navigation wasn't included in a particular query to avoid unconstrained recursion and it should be included manually if needed.

[smitpatel]

Decision for 5.0:
We will add validation at query level.
If the navigations involved are involved in same relationship then we will make it work.
If fixup is going to load both side then we don't include the inverse navigation.
If inverse navigation bringing more records (loading collection) then we will traverse 1 level more to include dependents.

For any other cycles we will throw error. Users can call IgnoreAutoInclude and specify manual includes upto the levels they want.

[smitpatel]

@dotnet/efteam - What should be the exception message here?

[vflame]

Given the model and configuration below, the following exception is being thrown.
System.InvalidOperationException: 'System.InvalidOperationException: 'Cycle detected while auto-including navigations: 'Parent.SelectedChild', 'Child._parent'. To fix this issue, either don't configure at least one navigation in the cycle as auto included inOnModelCreatingor call 'IgnoreAutoInclude' method on the query.' ?

Shouldn't the inverse navigation be excluded instead of an exception thrown assuming Parent == Parent.SelectedChild._parent?

```c#
public class Parent
{
public Child SelectedChild { get; set; }

    public string SelectedChildId { get; private set; }

    public void SetSelectedChild(Child child)
    {            
        SelectedChild = child;
    }
}



public class Child
{
    public string Id { get; private set; }
    public decimal Amount { get; set; }
    public decimal Code { get; set; }

    private Parent _parent;

    private Child() { }

    public Child(Parent parent)
    {
        _parent = parent;
    }
}

```c#
public class ParentConfig : IEntityTypeConfiguration<Parent>
    {
        public void Configure(EntityTypeBuilder<Parent> builder)
        {
            builder.Property<string>("Id").ValueGeneratedOnAdd();
            builder.HasKey("Id");

            builder.HasOne<Child>(p => p.SelectedChild).WithMany().HasForeignKey(p => p.SelectedChildId);

            builder.HasMany<Child>().WithOne("_parent").HasForeignKey(p => p.Id);

            builder.Navigation(p => p.SelectedChild).AutoInclude();
        }
    }


    public class ChildConfig : IEntityTypeConfiguration<Child>
    {
        public void Configure(EntityTypeBuilder<Child> builder)
        {
            builder.Navigation("_parent").AutoInclude();

        }
    }

[smitpatel]

  builder.HasOne<Child>(p => p.SelectedChild).WithMany().HasForeignKey(p => p.SelectedChildId);

            builder.HasMany<Child>().WithOne("_parent").HasForeignKey(p => p.Id);

You are configured them as separate relationships. They are not inverse navigation for each other. We exclude inverse navigations from throwing error.