Skip to content

Commit

Permalink
[5.0] Fix places where skip navigations are missed (#22884)
Browse files Browse the repository at this point in the history
Fixes #22882

**Description**

Skip navigations (i.e. many-to-many navigations; new for 5.0) are not being returned as members, navigations, or collections from an an API used to access them. In addition, scrubbing the code found three other issues with the same pattern, plus a few that need more investigation. This PR contains fixes and tests for the four clear issues. I have filed #22883 to follow up on the others.

**Customer Impact**

* Navigations for a major new feature are not being returned from an API that should return them.
* Attaching a graph asynchronously may miss navigations.
* Skip navigations are missed as notification properties.
* Exception will say a property is not found, when actually it is a navigation.

**How found**

Customer reported on RC2 daily build.

**Test coverage**

This PR adds tests for the affected cases, including others found from scrubbing the code. #22883 tracks doing more research in this area.

**Regression?**

No.

**Risk**

Low. The change only affects skip navigations, which are new in 5.0.
  • Loading branch information
ajcvickers committed Oct 6, 2020
1 parent f2f4af7 commit 71063cb
Show file tree
Hide file tree
Showing 9 changed files with 502 additions and 148 deletions.
28 changes: 22 additions & 6 deletions src/EFCore/ChangeTracking/EntityEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,18 @@ public virtual NavigationEntry Navigation([NotNull] string propertyName)
/// navigation properties of this entity.
/// </summary>
public virtual IEnumerable<NavigationEntry> Navigations
=> InternalEntry.EntityType.GetNavigations().Select(
navigation => navigation.IsCollection
? (NavigationEntry)new CollectionEntry(InternalEntry, navigation)
: new ReferenceEntry(InternalEntry, navigation));
{
get
{
var entityType = InternalEntry.EntityType;
return entityType.GetNavigations()
.Concat<INavigationBase>(entityType.GetSkipNavigations())
.Select(
navigation => navigation.IsCollection
? (NavigationEntry)new CollectionEntry(InternalEntry, navigation.Name)
: new ReferenceEntry(InternalEntry, navigation.Name));
}
}

/// <summary>
/// Provides access to change tracking information and operations for a given
Expand Down Expand Up @@ -273,8 +281,16 @@ public virtual CollectionEntry Collection([NotNull] string propertyName)
/// collection navigation properties of this entity.
/// </summary>
public virtual IEnumerable<CollectionEntry> Collections
=> InternalEntry.EntityType.GetNavigations().Where(n => n.IsCollection)
.Select(navigation => new CollectionEntry(InternalEntry, navigation));
{
get
{
var entityType = InternalEntry.EntityType;
return entityType.GetNavigations()
.Concat<INavigationBase>(entityType.GetSkipNavigations())
.Where(navigation => navigation.IsCollection)
.Select(navigation => new CollectionEntry(InternalEntry, navigation.Name));
}
}

/// <summary>
/// <para>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ public virtual async Task TraverseGraphAsync<TState>(
}

var internalEntityEntry = node.GetInfrastructure();
var navigations = internalEntityEntry.EntityType.GetNavigations();
var navigations = internalEntityEntry.EntityType.GetNavigations()
.Concat<INavigationBase>(internalEntityEntry.EntityType.GetSkipNavigations());
var stateManager = internalEntityEntry.StateManager;

foreach (var navigation in navigations)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ public virtual void Unsubscribe(InternalEntityEntry entry)

if (changeTrackingStrategy != ChangeTrackingStrategy.Snapshot)
{
foreach (var navigation in entityType.GetNavigations().Where(n => n.IsCollection))
foreach (var navigation in entityType.GetNavigations()
.Concat<INavigationBase>(entityType.GetSkipNavigations())
.Where(n => n.IsCollection))
{
AsINotifyCollectionChanged(entry, navigation, entityType, changeTrackingStrategy).CollectionChanged
-= entry.HandleINotifyCollectionChanged;
Expand Down
3 changes: 2 additions & 1 deletion src/EFCore/Extensions/EntityTypeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,8 @@ public static IProperty GetProperty([NotNull] this IEntityType entityType, [NotN
var property = entityType.FindProperty(name);
if (property == null)
{
if (entityType.FindNavigation(name) != null)
if (entityType.FindNavigation(name) != null
|| entityType.FindSkipNavigation(name) != null)
{
throw new InvalidOperationException(
CoreStrings.PropertyIsNavigation(
Expand Down
11 changes: 9 additions & 2 deletions src/EFCore/Metadata/Internal/EntityTypeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -419,12 +419,19 @@ public static IEnumerable<IPropertyBase> GetNotificationProperties(
{
yield return navigation;
}

foreach (var navigation in entityType.GetSkipNavigations())
{
yield return navigation;
}
}
else
{
// ReSharper disable once AssignNullToNotNullAttribute
var property = (IPropertyBase)entityType.FindProperty(propertyName)
?? entityType.FindNavigation(propertyName);
var property = entityType.FindProperty(propertyName)
?? entityType.FindNavigation(propertyName)
?? (IPropertyBase)entityType.FindSkipNavigation(propertyName);

if (property != null)
{
yield return property;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel;
using Microsoft.EntityFrameworkCore.TestUtilities;
Expand All @@ -22,10 +23,24 @@ protected override void ExecuteWithStrategyInTransaction(
Action<ManyToManyContext> nestedTestOperation2 = null,
Action<ManyToManyContext> nestedTestOperation3 = null)
{
base.ExecuteWithStrategyInTransaction(testOperation, nestedTestOperation1, nestedTestOperation2, nestedTestOperation3);
base.ExecuteWithStrategyInTransaction(
testOperation, nestedTestOperation1, nestedTestOperation2, nestedTestOperation3);

Fixture.Reseed();
}

protected override async Task ExecuteWithStrategyInTransactionAsync(
Func<ManyToManyContext, Task> testOperation,
Func<ManyToManyContext, Task> nestedTestOperation1 = null,
Func<ManyToManyContext, Task> nestedTestOperation2 = null,
Func<ManyToManyContext, Task> nestedTestOperation3 = null)
{
await base.ExecuteWithStrategyInTransactionAsync(
testOperation, nestedTestOperation1, nestedTestOperation2, nestedTestOperation3);

await Fixture.ReseedAsync();
}

protected override bool SupportsDatabaseDefaults
=> false;

Expand Down
Loading

0 comments on commit 71063cb

Please sign in to comment.