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.

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

Refreshing Db Context single entity with ReloadAsync after using raw SQL to delete data results with strange behavior #15645

Closed
veizx opened this issue May 7, 2019 · 2 comments · Fixed by #16455
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@veizx
Copy link

veizx commented May 7, 2019

I'm facing a strange behavior when I try to refresh single entity form Db Context using .ReloadAsync for tracked entity.
Simply after deleting the single entity using raw SQL and forcing EF to reload changes form DB is not enough to delete the entity from the cache. I have read couple of issues that may be related to this one but I want to have an explanation for this kind of behavior because it is really strange.

Explanation of the problem

I have self-referenced domain model named Menu which looks like:

public class Menu
   {
       public int Id { get; set; }

       [Required]
       [MaxLength(DataValidation.StrLength)]
       public string Name { get; set; }

       public int? ParentId { get; set; }

       [ForeignKey("ParentId")]
       public virtual Menu Parent { get; set; }

       public virtual List<Menu> Items { get; set; }
       .
       .
       .
   }

Steps to reproduce

  1. Lets say I have prepared data (8 menu items) that will give this menu (parent-children) tree structure:
//menu items structure
RootMenuItem 1                   ----    ChildItem_1_1
                                 ----    ChildItem_To_Delete
                                 ----    ChildItem_1_2       ----    ChildItem_1_2_1
                                                    |
                                                    ----     ----    ChildItem_1_2_2
RootMenuItem 2      ----    ChildItem_2_1
  1. RootMenuItem 1 :

    • has no parent (ParentId = NULL),
    • has 3 children (ChildItem_1_1, ChildItem_To_Delete, ChildItem_1_2) where one of them (ChildItem_1_2) also has 2 children items(ChildItem_1_2_1, ChildItem_1_2_2)
  2. RootMenuItem 2 :

    • has no parent (ParentId = NULL),
    • has 1 child (ChildItem_2_1)

Because it is self-referenced table, the functionality for menu deletion (recursively) is done with stored procedure on database level because of performance.

Here I will provide the simpler example just to point the strange behavior.

Lets say I want to delete the ChildItem_To_Delete item that belongs to RootMenuItem 1 and has no children.

Deletion of Menu entity and strange behavior occurs

Here I will provide a piece of code with enough comments that I believe will explain the behavior and expectations.

async Task DeleteMenuEntity(int menuItemId)
{
   var databaseEntity = await _applicationDb.Menus.FirstOrDefaultAsync(x => x.Id == menuItemId);
   if(databaseEntity == null)
	throw new Exception("Entity Not Found!");

   var parentId = databaseEntity.ParentId;

   await _applicationDb.Database.ExecuteSqlCommandAsync($"Delete from Menus where Id = {menuItemId}");

   //Deleted entity is not retrieved with this query, so it seems fine!
   var allEntitiesAfterAction = await _applicationDb.Menus.ToListAsync();
   if(allEntitiesAfterAction.Any(x => x.Id == menuItemId))
	throw new Exception("Entity should have been deleted!");

   var trackedEntity = _applicationDb.ChangeTracker.Entries<Menu>().FirstOrDefault(x => x.Entity.Id == menuItemId);
   if(trackedEntity == null)
	throw new Exception("This entity was not tracked by db context!");

   //Forcing entity reload! To force DbContex to delete it from its cache I believe.
   await trackedEntity.ReloadAsync();

    //I CHECKED AND THE ENTITY IS DELETED FROM DATABASE.
   //Attempt to load deleted entity. 
   //This query returns null object as expected!
   databaseEntity = await _applicationDb.Menus.FirstOrDefaultAsync(x => x.Id == menuItemId);
   if (databaseEntity != null)
	throw new Exception("Entity should have been deleted!");

   var menuItems = await _applicationDb.Menus.ToListAsync();
   if(menuItems.Any(x => x.Id == menuItemId))
	throw new Exception("Entity with Id: "+ menuItemId + " should have been deleted!");

   //HERE IT IS: When I use the query with Include for navigational property 
   var menuItemsTree = await _applicationDb.Menus.Include(x => x.Items).ToAsyncEnumerable().ToList();

   //THE ENTITY SEEMED TO BE DELETED FROM DATABASE. BUT IT HAS BEEN FOUND IN 
   //NAVIGATIONAL PROPERTY LOAD. HOW ???
   //Somehow after executed query above, deleted entity is retrieved from somewhere (probably 
   //DBContext cache).
   var deletedEntityExistence = menuItemsTree.First(x => x.Id == parentId)
   			.Items.FirstOrDefault(x => x.Id == menuItemId);
   
   //Somehow deleted entity is found with the last query above, but not with the first query.
   if (deletedEntityExistence != null)
	throw new Exception("Strange behaviour!!!");
}

After deletion of ChildItem_To_Delete there are 7 menu items in the database left but when I use the last query with .Include, returned RootMenuItem 1 still has 3 children including the deleted item.
I know that after using raw SQL, DB Context may be outdated and we need to reload all entities that we want to make Db Context aware for changes, but why some queries are returning the expected result and some are not (as .Include)?
After this example I think even .ReloadAsync of tracked entry is not working as expected.
Please correct me If I'm wrong.

Further technical details

EF Core version: 2.1.2
Database Provider: Microsoft.EntityFrameworkCore.SqlServer (V2.1.2)
Operating system: Windows 10 x64

@ajcvickers
Copy link
Contributor

Note for triage--repro below. The issue here is that when the refreshed entity is detached we do not fix-up navigation properties to it. I think this was originally by-design, but we have since revised fix-up behaviors in this area and I think it makes sense to fixup the references from tracked entities so that they no longer refer to the detached entities. This is technically a breaking change.

public class Menu
{
    [DatabaseGenerated(DatabaseGeneratedOption.None)]
    public int Id { get; set; }

    [Required]
    [MaxLength(64)]
    public string Name { get; set; }

    public int? ParentId { get; set; }

    [ForeignKey("ParentId")]
    public virtual Menu Parent { get; set; }

    public virtual List<Menu> Items { get; set; }
 }

public class BloggingContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder
            .UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=Test;ConnectRetryCount=0");
    }

    public DbSet<Menu> Menus { get; set; }
}

public class Program
{
    public static async Task Main()
    {
        using (var context = new BloggingContext())
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();

            context.AddRange(
                new Menu
                {
                    Id = 1,
                    Name = "RootMenuItem 1",
                    Items = new List<Menu>
                    {
                        new Menu
                        {
                            Id = 2,
                            Name = "ChildItem_To_Delete"
                        }
                    }
                });

            context.SaveChanges();
        }

        using (var context = new BloggingContext())
        {
            var menuItems = await context.Menus.Include(x => x.Items).ToAsyncEnumerable().ToList();
            var toDelete = menuItems.Single(e => e.Id == 2);

            await context.Database.ExecuteSqlCommandAsync($"Delete from Menus where Id = 2");

            await context.Entry(toDelete).ReloadAsync();

            var deletedEntityExistence = menuItems.First(x => x.Id == 1)
                .Items.FirstOrDefault(x => x.Id == 2);

            if (deletedEntityExistence != null)
                throw new Exception("Strange behaviour!!!");
        }
    }
}

@ajcvickers ajcvickers added this to the 3.0.0 milestone May 10, 2019
@ajcvickers ajcvickers self-assigned this May 10, 2019
@ajcvickers
Copy link
Contributor

Note: also check this scenario for owned entities.

@ajcvickers ajcvickers modified the milestones: 3.0.0, Backlog Jun 28, 2019
@ajcvickers ajcvickers modified the milestones: Backlog, 3.0.0 Jul 3, 2019
ajcvickers added a commit that referenced this issue Jul 4, 2019
Fixes #15645
Fixes #13110 (was already working in 3.0, but added a test)
Fixes #15500 (was already working in 3.0, but added a test)

See also #16454
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 4, 2019
ajcvickers added a commit that referenced this issue Jul 5, 2019
Fixes #15645
Fixes #13110 (was already working in 3.0, but added a test)
Fixes #15500 (was already working in 3.0, but added a test)

See also #16454
ajcvickers added a commit that referenced this issue Jul 5, 2019
Fixes #15645
Fixes #13110 (was already working in 3.0, but added a test)
Fixes #15500 (was already working in 3.0, but added a test)

See also #16454
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview8 Jul 29, 2019
ajcvickers added a commit that referenced this issue Oct 22, 2019
…t dramatically change detach behavior

Fixes #17784

In 3.0 we made a [simple change](7e65b82#diff-7767de8ee880eb46efbfc9d2153dd33a) for #15645 that started clearing navigation properties when they referenced detached entities. In retrospect this was the wrong thing to do as people are relying on this to be able to detach all entities from the context.

The new fix restores the pre-3.0 detach behavior, and sends the reload of a deleted entity through a simulated to delete instead.
ajcvickers added a commit that referenced this issue Oct 22, 2019
…t dramatically change detach behavior

Fixes #17784

In 3.0 we made a [simple change](7e65b82#diff-7767de8ee880eb46efbfc9d2153dd33a) for #15645 that started clearing navigation properties when they referenced detached entities. In retrospect this was the wrong thing to do as people are relying on this to be able to detach all entities from the context.

The new fix restores the pre-3.0 detach behavior, and sends the reload of a deleted entity through a simulated to delete instead.
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview8, 3.0.0 Nov 11, 2019
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants