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

Query: Disallow tracking queries for owned entites without their owners #16997

Merged
merged 1 commit into from
Aug 7, 2019
Merged

Query: Disallow tracking queries for owned entites without their owners #16997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 27 additions & 1 deletion src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ private static readonly MethodInfo _startTrackingMethodInfo

private readonly IEntityMaterializerSource _entityMaterializerSource;
private readonly bool _trackQueryResults;
private readonly ISet<IEntityType> _visitedEntityTypes = new HashSet<IEntityType>();
private int _currentEntityIndex;

public EntityMaterializerInjectingExpressionVisitor(
Expand All @@ -188,7 +189,30 @@ public EntityMaterializerInjectingExpressionVisitor(
_trackQueryResults = trackQueryResults;
}

public Expression Inject(Expression expression) => Visit(expression);
public Expression Inject(Expression expression)
{
var result = Visit(expression);
if (_trackQueryResults)
{
bool containsOwner(IEntityType owner)
=> owner != null && (_visitedEntityTypes.Contains(owner) || containsOwner(owner.BaseType));

foreach (var entityType in _visitedEntityTypes)
{
if ((entityType.HasDefiningNavigation()
&& !containsOwner(entityType.DefiningEntityType))
Copy link
Member

Choose a reason for hiding this comment

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

The defining entity isn't always the owner. We shouldn't prevent weak entities to be tracked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it been a while we all had discussion about weak vs owned.
Filed #16999 to discuss when you are back at work.

Copy link

@salaros salaros Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This PR breaks completely scenarios when an entity inherits an owned entity from its base class:

[Owned]
public class OwnedEntity
{
	public string Prop { get; set; }
}

public class A 
{
	public string Foo { get; set; }
}

public class B : A
{
	public string Bar { get; set; }
}

// ...

builder
	.OwnsOne(a => a.AuditLog); //<-- works only for base type

// ...

await dbContext.Set<B>().SingleOrDefaultAsync(e => e.Bar == bar); //<-- throws InvalidOperationException

|| (entityType.FindOwnership() is IForeignKey ownership
&& !containsOwner(ownership.PrincipalEntityType)))
{
throw new InvalidOperationException("A tracking query projects owned entity without corresponding owner in result. " +
"Owned entities cannot be tracked without their owner. " +
"Either include the owner entity in the result or make query non-tracking using AsNoTracking().");
}
}
}

return result;
}

protected override Expression VisitExtension(Expression extensionExpression)
=> extensionExpression is EntityShaperExpression entityShaperExpression
Expand Down Expand Up @@ -379,6 +403,8 @@ var discriminatorValue

if (_trackQueryResults)
{
_visitedEntityTypes.Add(entityType);

expressions.Add(
Expression.Assign(
entryVariable, Expression.Call(
Expand Down
5 changes: 5 additions & 0 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,5 +180,10 @@ public override void Member_pushdown_chain_3_levels_deep()
public override void Member_pushdown_with_collection_navigation_in_the_middle()
{
}

// Invalid test due to projecting owned entity without owner in tracking query
public override void Entries_for_detached_entities_are_removed()
{
}
}
}
20 changes: 19 additions & 1 deletion test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,23 @@ public virtual void Query_loads_reference_nav_automatically_in_projection()
}
}

[ConditionalFact]
public virtual void Throw_for_owned_entities_without_owner_in_tracking_query()
{
using (var context = CreateContext())
{
var query = context.Set<OwnedPerson>().Select(e => e.PersonAddress);

var result = query.AsNoTracking().ToList();
Assert.Equal(4, result.Count);
Assert.Equal(0, context.ChangeTracker.Entries().Count());

Assert.Throws<InvalidOperationException>(() => query.AsTracking().ToList());

Assert.Equal(0, context.ChangeTracker.Entries().Count());
}
}

protected virtual DbContext CreateContext() => Fixture.CreateContext();

public abstract class OwnedQueryFixtureBase : SharedStoreFixtureBase<PoolableDbContext>
Expand Down Expand Up @@ -687,7 +704,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
b.HasData(
new Barton
{
Id = 1, Simple = "Simple"
Id = 1,
Simple = "Simple"
});

});
Expand Down