Open bart-degreed opened 2 years ago
This might be caused by the fix to https://github.com/dotnet/efcore/issues/26779
Please try disabling the fix by calling this on startup:
AppContext.SetSwitch("Microsoft.EntityFrameworkCore.Issue26779", true);
@ajcvickers
Thanks, I can confirm that adding the AppContext switch makes the problem go away.
Do you consider using this switch as a permanent solution or do you intend to fix this breaking change in the next patch release?
Do you consider using this switch as a permanent solution or do you intend to fix this breaking change in the next patch release?
We need to investigate further to determine whether this can be fixed in a patch release
@bart-degreed Can you point me to where the model is configured? I would expect the relationship here to be required, but it looks like it is optional.
I have created a simplified repro, see below. In reality, our code is much more dynamic. Commenting out the AppContext.SetSwitch
line makes both tests fail on EF Core 6.0.2.
#nullable enable
using System.Diagnostics;
using FluentAssertions;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.Extensions.Logging;
// Bug workaround for https://github.com/dotnet/efcore/issues/27436
AppContext.SetSwitch("Microsoft.EntityFrameworkCore.Issue26779", true);
await CanRemoveSelfFromCyclicManyToManyRelationship();
await CanRemoveFromManyToManyRelationshipWithExtraRemovalsFromResourceDefinition();
async Task CanRemoveSelfFromCyclicManyToManyRelationship()
{
// Arrange
WorkItem otherWorkItem = new();
WorkItem existingWorkItem = new()
{
RelatedFrom = new List<WorkItem>
{
otherWorkItem
}
};
await using (var dbContext = new AppDbContext())
{
await dbContext.Database.EnsureDeletedAsync();
await dbContext.Database.EnsureCreatedAsync();
dbContext.WorkItems.Add(existingWorkItem);
await dbContext.SaveChangesAsync();
existingWorkItem.RelatedFrom.Add(existingWorkItem);
await dbContext.SaveChangesAsync();
}
int relatedFromIdToRemove = existingWorkItem.Id;
// Act
await using (var dbContext = new AppDbContext())
{
// Fetch minimal required data (no tracking)
WorkItem workItemToUpdate = await dbContext.WorkItems
.Include(workItem => workItem.RelatedFrom)
.Where(workItem => workItem.Id == existingWorkItem.Id)
.Select(workItem => new WorkItem
{
Id = workItem.Id,
RelatedFrom = workItem.RelatedFrom
.Where(relatedFromWorkItem => relatedFromWorkItem.Id == relatedFromIdToRemove)
.Select(relatedFromWorkItem =>
new WorkItem
{
Id = relatedFromWorkItem.Id
}).ToList()
}).FirstAsync();
// Ensure retrieved data is tracked
dbContext.GetTrackedOrAttach(workItemToUpdate);
workItemToUpdate.RelatedFrom = workItemToUpdate.RelatedFrom.Select(workItem => dbContext.GetTrackedOrAttach(workItem)).ToList();
dbContext.Entry(workItemToUpdate).Collection(workItem => workItem.RelatedFrom).IsLoaded = true;
// Apply the change
workItemToUpdate.RelatedFrom = workItemToUpdate.RelatedFrom.Where(workItem => workItem.Id != relatedFromIdToRemove).ToList();
await dbContext.SaveChangesAsync();
}
// Assert
await using (var dbContext = new AppDbContext())
{
WorkItem workItemInDatabase = await dbContext.WorkItems
.Include(workItem => workItem.RelatedFrom)
.Include(workItem => workItem.RelatedTo)
.FirstAsync(workItem => workItem.Id == existingWorkItem.Id);
workItemInDatabase.RelatedFrom.Should().HaveCount(1);
workItemInDatabase.RelatedFrom[0].Id.Should().Be(otherWorkItem.Id);
workItemInDatabase.RelatedTo.Should().BeEmpty();
}
}
async Task CanRemoveFromManyToManyRelationshipWithExtraRemovalsFromResourceDefinition()
{
// Arrange
WorkItem existingWorkItem = new()
{
Tags = new List<Tag>
{
new(),
new(),
new()
}
};
await using (var dbContext = new AppDbContext())
{
await dbContext.Database.EnsureDeletedAsync();
await dbContext.Database.EnsureCreatedAsync();
dbContext.WorkItems.Add(existingWorkItem);
await dbContext.SaveChangesAsync();
}
int tagIdToRemove = existingWorkItem.Tags[1].Id;
int extraTagIdToRemove = existingWorkItem.Tags[2].Id;
// Act
await using (var dbContext = new AppDbContext())
{
// Fetch minimal required data (no tracking)
WorkItem workItemToUpdate = await dbContext.WorkItems
.Include(workItem => workItem.Tags)
.Where(workItem => workItem.Id == existingWorkItem.Id)
.Select(workItem => new WorkItem
{
Id = workItem.Id,
Tags = workItem.Tags
.Where(tag => tag.Id == tagIdToRemove)
.Select(tag => new Tag
{
Id = tag.Id
})
.ToList()
})
.FirstAsync();
// Ensure retrieved data is tracked, add extra tag to-be-removed.
dbContext.GetTrackedOrAttach(workItemToUpdate);
workItemToUpdate.Tags.Add(new Tag { Id = extraTagIdToRemove });
workItemToUpdate.Tags = workItemToUpdate.Tags.Select(tag => dbContext.GetTrackedOrAttach(tag)).ToList();
dbContext.Entry(workItemToUpdate).Collection(workItem => workItem.Tags).IsLoaded = true;
// Apply the change
int[] tagIdsToRemove = { tagIdToRemove, extraTagIdToRemove };
workItemToUpdate.Tags = workItemToUpdate.Tags.Where(tag => !tagIdsToRemove.Contains(tag.Id)).ToList();
await dbContext.SaveChangesAsync();
}
// Assert
await using (var dbContext = new AppDbContext())
{
WorkItem workItemInDatabase = await dbContext.WorkItems
.Include(workItem => workItem.Tags)
.FirstAsync(workItem => workItem.Id == existingWorkItem.Id);
workItemInDatabase.Tags.Should().HaveCount(1);
workItemInDatabase.Tags.Single().Id.Should().Be(existingWorkItem.Tags.ElementAt(0).Id);
List<Tag> tagsInDatabase = await dbContext.Tags.ToListAsync();
tagsInDatabase.Should().HaveCount(3);
}
}
public abstract class Entity
{
public int Id { get; set; }
}
public sealed class WorkItem : Entity
{
public IList<Tag> Tags { get; set; } = new List<Tag>();
public IList<WorkItem> RelatedFrom { get; set; } = new List<WorkItem>();
public IList<WorkItem> RelatedTo { get; set; } = new List<WorkItem>();
}
public sealed class Tag : Entity
{
public IList<WorkItem> WorkItems { get; set; } = new List<WorkItem>();
}
public sealed class WorkItemToWorkItem
{
public WorkItem FromItem { get; set; } = null!;
public WorkItem ToItem { get; set; } = null!;
}
public sealed class AppDbContext : DbContext
{
public DbSet<WorkItem> WorkItems => Set<WorkItem>();
public DbSet<Tag> Tags => Set<Tag>();
protected override void OnConfiguring(DbContextOptionsBuilder builder)
{
builder.UseSqlite("Data Source=sample.db;Pooling=False");
builder.LogTo(message => Debug.WriteLine(message), LogLevel.Information);
}
protected override void OnModelCreating(ModelBuilder builder)
{
builder.Entity<WorkItem>()
.HasMany(workItem => workItem.RelatedFrom)
.WithMany(workItem => workItem.RelatedTo)
.UsingEntity<WorkItemToWorkItem>(right => right
.HasOne(joinEntity => joinEntity.FromItem)
.WithMany(),
left => left
.HasOne(joinEntity => joinEntity.ToItem)
.WithMany());
}
}
public static class DbContextExtensions
{
/// <summary>
/// If not already tracked, attaches the specified entity to the change tracker in <see cref="EntityState.Unchanged" /> state.
/// </summary>
public static TEntity GetTrackedOrAttach<TEntity>(this DbContext dbContext, TEntity entity)
where TEntity : Entity
{
var trackedEntity = (TEntity?)dbContext.GetTrackedIdentifiable(entity);
if (trackedEntity == null)
{
dbContext.Entry(entity).State = EntityState.Unchanged;
trackedEntity = entity;
}
return trackedEntity;
}
/// <summary>
/// Searches the change tracker for an entity that matches the type and ID of <paramref name="entity" />.
/// </summary>
private static object? GetTrackedIdentifiable<TEntity>(this DbContext dbContext, TEntity entity)
where TEntity : Entity
{
Type entityClrType = entity.GetType();
EntityEntry? entityEntry = dbContext.ChangeTracker.Entries().FirstOrDefault(entry => IsEntity<TEntity>(entry, entityClrType, entity.Id));
return entityEntry?.Entity;
}
private static bool IsEntity<TEntity>(EntityEntry entry, Type entityClrType, int id)
where TEntity : Entity
{
return entry.Entity.GetType() == entityClrType && ((TEntity)entry.Entity).Id == id;
}
}
Hope this helps to narrow it down.
@bart-degreed The issue here is that the state of join entity remains Added
after the code to attach it and paint appropriate state. For example:
WorkItem {Id: 1} Unchanged
Id: 1 PK
RelatedFrom: []
RelatedTo: [{Id: 1}]
Tags: []
WorkItemToWorkItem {FromItemId: 1, ToItemId: 1} Added
FromItemId: 1 PK FK
ToItemId: 1 PK FK
FromItem: <null>
ToItem: <null>
and
Tag {Id: 2} Unchanged
Id: 2 PK
WorkItems: [{Id: 1}]
Tag {Id: 3} Modified
Id: 3 PK
WorkItems: [{Id: 1}]
TagWorkItem (Dictionary<string, object>) {TagsId: 2, WorkItemsId: 1} Unchanged
TagsId: 2 PK FK
WorkItemsId: 1 PK FK
TagWorkItem (Dictionary<string, object>) {TagsId: 3, WorkItemsId: 1} Added
TagsId: 3 PK FK
WorkItemsId: 1 PK FK
WorkItem {Id: 1} Unchanged
Id: 1 PK
RelatedFrom: []
RelatedTo: []
Tags: []
This tells EF that the join entity does not exist in the database, and hence trying to delete it would be an error. However, before the bug fixed in #26779, EF was attempting to delete the entity anyway, which in your case worked since even though the state is Added, the entity did actually exist.
The fix is to make sure that the join entity is in the correct state when painting the state of attached disconnected entities. (Assuming, of course, that you actually need to attach disconnected entities rather than using a tracking query.) For example, after setting IsLoaded
for the collection:
dbContext.Entry(workItemToUpdate).Collection(workItem => workItem.RelatedFrom).IsLoaded = true;
// Set the state of the join entity
dbContext.Entry(dbContext.Set<WorkItemToWorkItem>().Find(workItemToUpdate.Id, relatedFromIdToRemove)!)
.State = EntityState.Unchanged;
// Apply the change
workItemToUpdate.RelatedFrom = workItemToUpdate.RelatedFrom.Where(workItem => workItem.Id != relatedFromIdToRemove).ToList();
and
dbContext.Entry(workItemToUpdate).Collection(workItem => workItem.Tags).IsLoaded = true;
// Apply the change
int[] tagIdsToRemove = { tagIdToRemove, extraTagIdToRemove };
// Set the state of join entities...
foreach (var tagId in tagIdsToRemove)
{
dbContext.Entry(dbContext.Set<Dictionary<string, object>>("TagWorkItem").Find(tagId, workItemToUpdate.Id)!)
.State = EntityState.Unchanged;
}
workItemToUpdate.Tags = workItemToUpdate.Tags.Where(tag => !tagIdsToRemove.Contains(tag.Id)).ToList();
Note for triage: we should consider making it easier to change the state of join entities such that they are Unchanged, possibly with a new SetLoaded
method that would both mark the collection as loaded and mark all contained entities as Unchanged.
Thanks @ajcvickers for explaining why this happens. I would definitely welcome such a SetLoaded
method.
The reason we're not using a tracking query is for performance. Assume a to-many relationship is connected to 50.000 entities and we want to remove two from the set. Using a tracked query requires first fetching the existing 50.000 entities, which we avoid this way. Furthermore, by only fetching IDs instead of full records we reduce network bandwidth and database pressure.
In the meantime, can you help to do this in a generic way, that works for both implicit and explicit join tables? It would be great if you can provide the implementation for MarkManyToManyJoinEntityAsTracked
below, taking everything into account that matters to get this right.
public abstract class Entity
{
public int Id { get; set; }
}
void MarkManyToManyJoinEntityAsTracked(DbContext dbContext, Entity leftValue,
string relationshipName, int rightId)
{
dbContext.Model. // TODO: ???
}
// usage from CanRemoveFromManyToManyRelationshipWithExtraRemovalsFromResourceDefinition
MarkManyToManyJoinEntityAsTracked(dbContext, workItemToUpdate, relatedFromIdToRemove);
// usage from CanRemoveSelfFromCyclicManyToManyRelationship
foreach (var tagId in tagIdsToRemove)
{
MarkManyToManyJoinEntityAsTracked(dbContext, workItemToUpdate, tagId);
}
If needed, we can pass the System.Type
of left/right entities as well. Or we can pass a placeholder instance where only the Id
property is populated for the right-side relationship value for Tag
and WorkItem
(this also enables access to their CLR type).
Update: my references to "relationship" above should be navigations in EF Core terminology.
MarkManyToManyJoinEntityAsTracked
should probably accept string navigationPropertyName
as well.
@bart-degreed
Using a tracked query requires first fetching the existing 50.000 entities, which we avoid this way.
Can you elaborate on this? Can you not filter the tracking query in the same way as the no-tracking query?
taking everything into account that matters to get this right.
I don't know if this is possible. It would require knowing which entities really are new, and which really are existing, which is scenario-specific.
Let me provide some background information to explain our use case. The JsonApiDotNetCore project I'm working on implements the JSON:API specification, which is a REST protocol for reading and writing resources (entities) and the relationships (navigations) between them. Conceptually it is quite similar to GraphQL, which you may be more familiar with.
In practice, an API developer defines a DbContext with entities and relationships, then adds a NuGet reference to JsonApiDotNetCore. JsonApiDotNetCore then mediates between the JSON:API protocol and the EF Core library, eliminating the need for boilerplate code. From a JsonApiDotNetCore perspective, the actual EF Core entities and relationships are unknown at compile-time, so it builds System.Expression
trees that it feeds to EF Core.
In JSON:API, updating to-many relationships is described here, specifically the next section:
In the following example, comments with IDs of 12 and 13 are removed from the list of comments for the article with ID 1:
DELETE /articles/1/relationships/comments HTTP/1.1 Content-Type: application/vnd.api+json Accept: application/vnd.api+json
{ "data": [ { "type": "comments", "id": "12" }, { "type": "comments", "id": "13" } ] }
For this example, a naive implementation would be:
int requestedArticleId = 1; // from URL int[] commentIdsToRemove = { 12, 13 }; // from request body
Article articleToUpdate = await dbContext.Articles .Include(article => article.Comments) .Where(article => article.Id == requestedArticleId) .FirstAsync();
Comment[] commentsToRemove = articleToUpdate.Comments .Where(comment => commentIdsToRemove.Contains(comment.Id)) .ToArray();
foreach (Comment commentToRemove in commentsToRemove) { articleToUpdate.Comments.Remove(commentToRemove); }
await dbContext.SaveChangesAsync();
This works, but the biggest downside is this fetches all (50.000) existing related comments, which is unneeded to perform this operation. Furthermore, it fetches all article and comment columns, which is unneeded too. To make this efficient, we project into placeholder entity instances (which only have their ID property populated), then ensure they are in the change tracker, then mark the relationship as loaded. This always worked fine but broke in v6.0.2 without using the Issue26779 switch (for reasons I understand now; the breaking change makes sense).
Due to the dynamic nature of JsonApiDotNetCore (where the API-specific EF Core model is unknown at compile-time), we cannot directly access properties and methods on the project-specific DbContext instance. So instead, we need to inspect the `DbContext.Model` to find these join entities and mark them as Unchanged. The good thing is this does not involve deep objects graphs. We'll always only be fetching the left-side ID (article) and the right-side IDs (comments). And we have code in place that verifies that all affected entities do exist in the database upfront (my examples are intentionally kept simple).
I took a stab at implementing `MarkManyToManyJoinEntityAsTracked`, see below.
```c#
public static void MarkManyToManyJoinEntityAsTracked<TEntity>(this DbContext dbContext, TEntity leftValue,
string navigationPropertyName, int rightId)
where TEntity : Entity
{
IEntityType leftEntityType = dbContext.Model.FindEntityType(leftValue.GetType())!;
ISkipNavigation skipNavigation = leftEntityType.FindSkipNavigation(navigationPropertyName)!;
IEntityType joinEntityType = skipNavigation.JoinEntityType;
IForeignKey foreignKey = skipNavigation.ForeignKey;
string joinEntityLeftPropertyName = foreignKey.Properties[0].Name; // "ToItemId" or "WorkItemsId"
IForeignKey otherKey = joinEntityType.GetForeignKeys().Single(key => key != foreignKey);
string joinEntityRightPropertyName = otherKey.Properties[0].Name; // "FromItemId" or "TagsId"
EntityEntry[] entries = dbContext.ChangeTracker.Entries().ToArray();
foreach (EntityEntry entry in entries)
{
if (entry.Metadata.Equals(joinEntityType))
{
if (joinEntityType.IsPropertyBag)
{
var map = (IDictionary<string, object>)entry.Entity;
bool matchesLeftKey = map.ContainsKey(joinEntityLeftPropertyName) &&
map[joinEntityLeftPropertyName].Equals(leftValue.Id);
bool matchesRightKey = map.ContainsKey(joinEntityRightPropertyName) &&
map[joinEntityRightPropertyName].Equals(rightId);
if (matchesLeftKey && matchesRightKey)
{
entry.State = EntityState.Unchanged;
}
}
else
{
int? leftIdValue = (int?)entry.CurrentValues[joinEntityLeftPropertyName];
int? rightIdValue = (int?)entry.CurrentValues[joinEntityRightPropertyName];
if (leftIdValue == leftValue.Id && rightIdValue == rightId)
{
entry.State = EntityState.Unchanged;
}
}
}
}
}
This works for the provided examples, but it feels brittle because there are various assumptions of which I don't know if they always hold, depending on how many-to-many relationships are mapped in EF Core.
(IDictionary<string, object>)
when joinEntityType.IsPropertyBag
is true
?@ajcvickers Any thoughts?
@ajcvickers I took the effort to explain our case, would appreciate any response from you or other team members.
@bart-degreed It's on my list to look out, but it's going to take considerable time to investigate and answer all your questions, and it hasn't bubbled up to the top of the list yet.
Fair enough. Thanks for the response.
@ajcvickers I tried to use EF Core v7.0.0-preview.5.22302.2 today and noticed our tests failing, despite the Microsoft.EntityFrameworkCore.Issue26779
switch turned on. On inspection, I found that the switch has been removed in https://github.com/dotnet/efcore/commit/c4b80ff6a9382b9b51002e6a30cff63d8fbe3738#diff-0fe81540ab04cc04237cb889ab550feb2b6885a61e0b8e86fbffebcdbad92780.
With no alternative solution being available and the switch removed, this effectively means we cannot update to EF Core 7.
This is a dealbreaker for our project.
If there's not going to be a solution in the 7.x timeline, can you at least restore the switch?
@bart-degreed
I spent a few hours today understanding your code and debugging the test that failed and I think this modification to MarkRelationshipAsLoaded
should work for you. The code finds the entry for each join entity instance of the relationship you want to mark as loaded and sets that join entity's state to Unchanged
.
private void MarkRelationshipAsLoaded(TResource leftResource, RelationshipAttribute relationship)
{
EntityEntry<TResource> leftEntry = _dbContext.Entry(leftResource);
CollectionEntry rightCollectionEntry = leftEntry.Collection(relationship.Property.Name);
rightCollectionEntry.IsLoaded = true;
if (rightCollectionEntry.Metadata is ISkipNavigation skipNavigation)
{
// Assuming non-composite keys
var pkValue = leftEntry.Property(skipNavigation.ForeignKey.PrincipalKey.Properties[0].Name).CurrentValue;
var fkName = skipNavigation.ForeignKey.Properties[0].Name;
foreach (var joinEntry in _dbContext.ChangeTracker.Entries()
.Where(e => e.Metadata == skipNavigation.JoinEntityType).ToList())
{
if (Equals(pkValue, joinEntry.Property(fkName).CurrentValue))
{
joinEntry.State = EntityState.Unchanged;
}
}
}
}
@ajcvickers Thank you so much for diving in and providing a solution! Based on your answer, I was able to extend it for composite keys in https://github.com/json-api-dotnet/JsonApiDotNetCore/pull/1176. This unblocks us from upgrading to EF 7.
File a bug
After updating EF Core from v6.0.1 to v6.0.2, two of our tests start failing. This looks like a regression in the change tracker, which no longer produces SQL for removing an element from a many-to-many relationship.
The failing tests are:
To understand what's going on: Both tests set up the database, then create an API request to remove an element from an existing many-to-many relationship. Afterwards the database contents is inspected to verify the relationship has been updated.
The logic in the API (system under test) is defined at https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/19f655704c91fa89f5b7561e1dd66586d9b521ee/src/JsonApiDotNetCore/Services/JsonApiResourceService.cs#L460, which delegates to https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/19f655704c91fa89f5b7561e1dd66586d9b521ee/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs#L450. At a global level, the following happens: The resource service builds an EF Core query that retrieves the parent entity, along with the included subset of related resources to remove. The produced SQL for that is unchanged between EF Core versions:
Then the resource repository ensures all returned entities are tracked, marks the to-many relationship as loaded, then removes the requested related resources from the parent entity. In v6.0.1 the change tracker would "see" the removed related entities and produce SQL for that, but in v6.0.2 that no longer happens.
Result from
_dbContext.ChangeTracker.DebugView.LongView
, after calling_dbContext.ChangeTracker.DetectChanges();
just beforeSaveChangesAsync
in v6.0.1:In v6.0.2, this returns:
Include provider and version information
EF Core version: v6.0.2 Database provider: PostgreSQL (tried various 6.0.x versions, no differences observed) Target framework: .NET 6 Operating system: Windows 10 x64 IDE: Visual Studio Enterprise 2022 v17.0.5