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.

Sign up for GitHub

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

Jump to bottom

[5.0.1] Avoid stack overflow in negative many-to-many cases #23455

Merged
merged 2 commits into from
Nov 25, 2020
Merged

[5.0.1] Avoid stack overflow in negative many-to-many cases #23455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
762ba95
Avoid stack overflow in negative many-to-many cases
AndriySvyryd Nov 24, 2020
ade87da
Add quirk mode for 23377 and 23354
AndriySvyryd Nov 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/EFCore.Relational/Extensions/RelationalEntityTypeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,13 @@ public static string GetDefaultSchema([NotNull] this IEntityType entityType)
}
else
{
var skipNavigationSchema = entityType.GetForeignKeys().SelectMany(fk => fk.GetReferencingSkipNavigations())
.FirstOrDefault(n => !n.IsOnDependent)
?.DeclaringEntityType.GetSchema();
var useOldBehavior = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23377", out var enabled) && enabled;
var skipReferencingTypes = entityType.GetForeignKeys().SelectMany(fk => fk.GetReferencingSkipNavigations())
.Where(n => !n.IsOnDependent && (n.DeclaringEntityType != entityType || useOldBehavior))
.ToList();
var skipNavigationSchema = skipReferencingTypes.FirstOrDefault()?.DeclaringEntityType.GetSchema();
if (skipNavigationSchema != null
&& entityType.GetForeignKeys().SelectMany(fk => fk.GetReferencingSkipNavigations())
.Where(n => !n.IsOnDependent)
&& skipReferencingTypes.Skip(1)
.All(n => n.DeclaringEntityType.GetSchema() == skipNavigationSchema))
{
return skipNavigationSchema;
Expand Down
5 changes: 4 additions & 1 deletion src/EFCore.SqlServer/Metadata/Conventions/SqlServerOnDeleteConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Linq;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Metadata.Builders;
Expand Down Expand Up @@ -58,9 +59,11 @@ protected override DeleteBehavior GetTargetDeleteBehavior(IConventionForeignKey
return deleteBehavior;
}

var useOldBehavior = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23354", out var enabled) && enabled;
if (selfReferencingSkipNavigation
== selfReferencingSkipNavigation.DeclaringEntityType.GetDeclaredSkipNavigations()
.First(s => s == selfReferencingSkipNavigation || s == selfReferencingSkipNavigation.Inverse))
.First(s => s == selfReferencingSkipNavigation || s == selfReferencingSkipNavigation.Inverse)
&& (selfReferencingSkipNavigation != selfReferencingSkipNavigation.Inverse || useOldBehavior))
{
selfReferencingSkipNavigation.Inverse.ForeignKey?.Builder.OnDelete(
GetTargetDeleteBehavior(selfReferencingSkipNavigation.Inverse.ForeignKey));
Expand Down
9 changes: 6 additions & 3 deletions src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,11 +721,14 @@ public virtual void ProcessKeyRemoved(
.SelectMany(t => t.GetDeclaredForeignKeys()).ToList();
foreach (var foreignKey in foreignKeys)
{
if ((!foreignKey.IsUnique
|| foreignKey.DeclaringEntityType.BaseType != null))
if ((foreignKey.IsUnique
&& foreignKey.DeclaringEntityType.BaseType == null)
|| foreignKey.Builder == null)
{
DiscoverProperties(foreignKey.Builder, context);
continue;
}

DiscoverProperties(foreignKey.Builder, context);
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/EFCore/Metadata/Conventions/KeyDiscoveryConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ protected virtual void TryConfigurePrimaryKey([NotNull] IConventionEntityTypeBui
{
var manyToManyForeignKeys = entityType.GetForeignKeys()
.Where(fk => fk.GetReferencingSkipNavigations().Any(n => n.IsCollection)).ToList();
if (manyToManyForeignKeys.Count == 2)
if (manyToManyForeignKeys.Count == 2
&& (!manyToManyForeignKeys.Any(fk => fk.PrincipalEntityType == entityType)
|| (AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23377", out var enabled) && enabled)))
{
keyProperties.AddRange(manyToManyForeignKeys.SelectMany(fk => fk.Properties));
}
Expand Down
29 changes: 29 additions & 0 deletions test/EFCore.Tests/ModelBuilding/ManyToManyTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,35 @@ public virtual void Throws_for_many_to_many_with_only_one_navigation_configured(
.WithMany(d => d.ManyToManyPrincipals)).Message);
}

[ConditionalFact]
public virtual void Throws_for_self_ref_with_same_navigation()
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<SelfRefManyToOne>().Ignore(s => s.SelfRef1);
modelBuilder.Entity<SelfRefManyToOne>().HasMany(t => t.SelfRef2)
.WithMany(t => t.SelfRef2);

Assert.Equal(CoreStrings.EntityRequiresKey("SelfRefManyToOneSelfRefManyToOne (Dictionary<string, object>)"),
Assert.Throws<InvalidOperationException>(() => modelBuilder.FinalizeModel()).Message);
}

[ConditionalFact]
public virtual void Throws_for_self_ref_using_self()
{
var modelBuilder = CreateModelBuilder();

modelBuilder.Entity<SelfRefManyToOne>().Ignore(s => s.Id);
modelBuilder.Entity<SelfRefManyToOne>().HasMany(t => t.Relateds)
.WithMany(t => t.RelatedSelfRefs)
.UsingEntity<SelfRefManyToOne>(
t => t.HasOne(a => a.Related).WithMany(b => b.DirectlyRelatedSelfRefs),
t => t.HasOne(a => a.SelfRef1).WithMany(b => b.SelfRef2));

Assert.Equal(CoreStrings.EntityRequiresKey(nameof(SelfRefManyToOne)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is EntityRequiresKey thrown in these cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the join entity is same as one side of skip navigation entities then we don't configure the PK to be composite key of FK properties in join entity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the normal rule the PK will include the FKs to the both of the types in many-to-many, and since one of them is the same entity type we are configuring then after setting the PK we will change the FK to target all PK properties and if the FK is changed we'll change the PK to include all of the FK properties and so on.

Assert.Throws<InvalidOperationException>(() => modelBuilder.FinalizeModel()).Message);
}

[ConditionalFact]
public virtual void Throws_for_ForeignKeyAttribute_on_navigation()
{
Expand Down
15 changes: 14 additions & 1 deletion test/EFCore.Tests/ModelBuilding/TestModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,22 @@ protected class SelfRef
protected class SelfRefManyToOne
{
public int Id { get; set; }
public int SelfRefId { get; set; }
public SelfRefManyToOne SelfRef1 { get; set; }
public ICollection<SelfRefManyToOne> SelfRef2 { get; set; }
public int SelfRefId { get; set; }

[NotMapped]
public ManyToManyRelated Related { get; set; }

[NotMapped]
public ICollection<ManyToManyRelated> Relateds { get; set; }
}

protected class ManyToManyRelated
{
public int Id { get; set; }
public ICollection<SelfRefManyToOne> DirectlyRelatedSelfRefs { get; set; }
public ICollection<SelfRefManyToOne> RelatedSelfRefs { get; set; }
}

protected class User
Expand Down