-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
Add a SetLoaded method with a lambda callback that allows the state of entities in the collection to be set. #27436
Comments
Pinned to v6.0.1 due to dotnet/efcore#27436
This might be caused by the fix to #26779 Please try disabling the fix by calling this on startup: AppContext.SetSwitch("Microsoft.EntityFrameworkCore.Issue26779", true); |
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? |
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 #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
and
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 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 |
Thanks @ajcvickers for explaining why this happens. I would definitely welcome such a 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
If needed, we can pass the Update: my references to "relationship" above should be navigations in EF Core terminology. |
|
Can you elaborate on this? Can you not filter the tracking query in the same way as the no-tracking query?
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 In JSON:API, updating to-many relationships is described here, specifically the next section:
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 I took a stab at implementing 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.
|
@ajcvickers Any thoughts? |
Pinned to v6.0.1 due to dotnet/efcore#27436
@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 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? |
I spent a few hours today understanding your code and debugging the test that failed and I think this modification to 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 json-api-dotnet/JsonApiDotNetCore#1176. This unblocks us from upgrading to EF 7. |
Pinned to v6.0.1 due to dotnet/efcore#27436
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
The text was updated successfully, but these errors were encountered: