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

InMemory: Cascade Delete Support #3924

Closed
hkoestin opened this issue Nov 30, 2015 · 11 comments
Closed

InMemory: Cascade Delete Support #3924

hkoestin opened this issue Nov 30, 2015 · 11 comments
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported

Comments

@hkoestin
Copy link

Dear all

During my tests with EF RC1, I found, that the EF.InMemory is lacking a quite important feature (IMHO). Cascading deletes are not supported by EF.InMemory!

Having the following context and classes, you can reproduce the issue quite easily:

    public class DatabaseContext : DbContext
    {
        public DatabaseContext(DbContextOptions options)
            : base(options)
        {
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            var factory = Database.GetService<ILoggerFactory>();
            factory.AddDebug(LogLevel.Verbose);

            modelBuilder.Entity<Apple>().HasKey(x => x.Identity);
            modelBuilder.Entity<Apple>().Property(x => x.Identity).ValueGeneratedNever();
            modelBuilder.Entity<Apple>().Property(x => x.Version).IsConcurrencyToken();

            modelBuilder.Entity<AggregateIndexes<Apple>>().HasKey(x => new {x.AggregateIdentity, x.AggregateVersion});

            modelBuilder.Entity(typeof (MapIndex)).HasKey("AggregateIdentity", "AggregateVersion");
            modelBuilder.Entity(typeof (MapIndex)).Property<Guid>("AggregateIdentity").ValueGeneratedNever();
            modelBuilder.Entity(typeof (MapIndex)).Property<Guid>("AggregateVersion").ValueGeneratedNever();

            modelBuilder.Entity(typeof (MapIndex))
                .HasOne(typeof (AggregateIndexes<Apple>))
                .WithOne()
                .HasForeignKey(typeof (MapIndex), "AggregateIdentity", "AggregateVersion")
                .IsRequired()
                .OnDelete(DeleteBehavior.Cascade);

            modelBuilder.Entity<ComplexMapIndex>().HasKey(x => x.Key);
            modelBuilder.Entity<ComplexMapIndex>().Property(x => x.Key).ValueGeneratedNever();

            modelBuilder.Entity(typeof (ComplexMapIndex)).HasKey("Key");
            modelBuilder.Entity(typeof (ComplexMapIndex)).Property<Guid>("Key").ValueGeneratedNever();

            modelBuilder.Entity(typeof (ComplexMapIndex))
                .HasOne(typeof (AggregateIndexes<Apple>))
                .WithMany()
                .HasForeignKey("AggregateIdentity", "AggregateVersion")
                .IsRequired()
                .OnDelete(DeleteBehavior.Cascade);
        }
    }

    public abstract class Aggregate
    {
        public Guid Identity { get; set; }
        public Guid Version { get; set; }
    }

    public class Apple : Aggregate
    {
        public string Color { get; set; }        
    }

    public class AggregateIndexes<T> where T: Aggregate
    {
        public Guid AggregateIdentity { get; set; }
        public Guid AggregateVersion { get; set; } 
    }

    public class MapIndex
    {
        public Guid AggregateIdentity { get; set; }
        public Guid AggregateVersion { get; set; }

        public string IndexValue { get; set; }
    }

    public class ComplexMapIndex
    {
        public Guid Key { get; set; }

        public Guid AggregateIdentity { get; set; }
        public Guid AggregateVersion { get; set; }

        public string IndexValue { get; set; }
    }

With the following lines of code, you can see the results:

        private static void Main(string[] args)
        {
            var builder = new DbContextOptionsBuilder();
//            builder.UseInMemoryDatabase();
            builder.UseSqlCe(@"Data Source=AggregatesIndexingWithDeleteCascade.sdf");

            var appleIdentity = Guid.NewGuid();

            // fill the context with some data
            using (var context = new DatabaseContext(builder.Options))
            {
                context.Database.EnsureCreated();

                var apple = new Apple
                {
                    Identity = appleIdentity,
                    Version = Guid.NewGuid(),
                    Color = "Red"
                };

                var indexes = new AggregateIndexes<Apple>()
                {
                    AggregateIdentity = apple.Identity,
                    AggregateVersion = apple.Version
                };


                context.Set<Aggregate>().Add(apple);
                context.Set<AggregateIndexes<Apple>>().Add(indexes);

                var mapIndex = new MapIndex
                {
                    AggregateIdentity = apple.Identity,
                    AggregateVersion = apple.Version,
                    IndexValue = "index-value-for-the-red-apple"
                };
                context.Set<MapIndex>().Add(mapIndex);

                for (int i = 0; i < 5; i++)
                {
                    var index = new ComplexMapIndex()
                    {
                        Key = Guid.NewGuid(),
                        AggregateIdentity = apple.Identity,
                        AggregateVersion = apple.Version,
                        IndexValue = "complex-index-value-for-the-red-apple"
                    };
                    context.Set<ComplexMapIndex>().Add(index);
                }

                context.SaveChanges();
            }

            // now we delete the AggregateIndexes<Apple> again,
            // the cascade should remove all entries now (map-index, complex-map-index)
            using (var context = new DatabaseContext(builder.Options))
            {
                var appleIndexes = context.Set<AggregateIndexes<Apple>>().Single(x => x.AggregateIdentity == appleIdentity);
                context.Remove(appleIndexes);
                context.SaveChanges();
            }

            // all entries should now be gone, as they are deleted by the cascade
            using (var context = new DatabaseContext(builder.Options))
            {
                var appleIndexes = 
                    context.Set<AggregateIndexes<Apple>>().SingleOrDefault(x => x.AggregateIdentity == appleIdentity);
                Console.WriteLine("AggregateIndexes<Apple> gone: " + (appleIndexes == null));

                var mapIndex = context.Set<MapIndex>().SingleOrDefault(x => x.AggregateIdentity == appleIdentity);
                Console.WriteLine("MapIndex gone: " + (mapIndex == null));

                var complexMapIndex = context.Set<ComplexMapIndex>().Where(x => x.AggregateIdentity == appleIdentity);
                Console.WriteLine("ComplexMapIndex gone: " + (!complexMapIndex.Any()));
            }

            Console.WriteLine("Finished ... Press any key to continue");
            Console.ReadLine();
        }

Output for SQL CE Provider (and all other relational providers)

AggregateIndexes<Apple> gone: True
MapIndex gone: True
ComplexMapIndex gone: True
Finished ... Press any key to continue

Output for EF.InMemory

AggregateIndexes<Apple> gone: True
MapIndex gone: False
ComplexMapIndex gone: False
Finished ... Press any key to continue

The results for the actual providers is correct (and as expected). The output for EF.InMemory however isn't - or at least to me, it is surprising.

In your documentation I found the following:

Note
This cascading behavior is only applied to entities that are being tracked by the context. A corresponding cascade behavior should be setup in the database to ensure data that is not being tracked by the context has the same action applied. If you use EF to create the database, this cascade behavior will be setup for you.

Whereas I totally get this for a real context, which would need to evaluate these FK relations in-memory and therefore fetch the entries from the store, I actually cannot understand, why EF.InMemory can't do this for me. It already has the whole graph in-memory and would only need to evaluate the FK relations, which are already in place.

Actually, the EF.Core infrastructure almost does all the things, and handles the delete in the graph correctly. It propagates the DELETE calls along the graph-relations.

Unfortunately, EF.InMemory does not really support this, as shown in these tests.

Why can't EF.InMemory handle this? It would be quite easy to implement I guess.

And why can it delete entries, if they are in the current context? At that point, it already CAN evaluate the FK relations and the cascading deletes. So it could also do for all entries in the store.
For the in-memory case, all entries are always in memory (and thus close to the context). It would be easy to fetch them, in case of a delete call to a cascading FK relation.

Any input to this topic?

Thanks a lot,
Harald

@rowanmiller rowanmiller changed the title DeleteBehavior.Cascade - InMemory implementation InMemory: Cascade Delete Support Dec 8, 2015
@rowanmiller rowanmiller added type-enhancement help wanted This issue involves technologies where we are not experts. Expert help would be appreciated. labels Dec 8, 2015
@rowanmiller rowanmiller added this to the Backlog milestone Dec 8, 2015
@zpbappi
Copy link

zpbappi commented Aug 25, 2016

FYI, I am grabbing this one.

@zpbappi
Copy link

zpbappi commented Sep 1, 2016

Here is an update of the issue:

Why cascade delete isn't working for InMemory database?
When we delete an entry, the related entities are not loaded in StateManager (or, change tracking system), unless we specifically include them using Include(...). So, they are not marked for deletion and stays unaffected. If you include the entries that you think should be deleted or updated when you delete a parent entry, it should work fine. For example, the following test should pass, even for InMemory database:

[Fact]
void test_written_in_notepad()
{
  List<int> childIds;
  using(var context = CreateInMemoryContext())
  {
    var parent = context.Parents.Include(p => p.Children).FirstOrDefault(p => p.Id == 1);
    childIds = p.Children.Select(c => c.Id).ToList();
    context.Remove(parent);
    context.SaveChanges();
  }

  using(var context = CreateInMemoryContext())
  {
    Assert.Empty(context.Children.Where(c => childIds.Contains(c.Id));
  }
}

Then why is it working for relational databases?
I don't know if it was intentional or not, but not-handling-of-dependent-entries behavior works pretty well for relational databases. There are three possible outcomes when you delete a parent entry in a relational database.

  1. all the children get deleted
  2. all the reference id of the children get updated to NULL
  3. deletion of the parent is restricted as long as it has children

It turns out, related database are designed to handle that without any tweaking from the EF. All EF has to do is generate the proper SQL (on delete cascade or whatever is appropriate) when creating the tables.

What needs to be done for InMemory database to support cascade deletion?
Since we do not get any magical support from the database here, the easiest possible thing we can do is to load the whole object graph in memory when a query is executed, regardless of Include(...) uses. That way, we can always traverse the object graph and delete and/or update accordingly. However, in this case, users will experience different behavior from other relational databases. So, the next option is to load the dependent entries on demand, make sure that they are tracked by the StateManager and update the entries accordingly. on demands refers to the when all the foreign keys of an entry are evaluated to check for dependent entries for cascade deletion.

What did I do?
I have created a service with a default implementation of Do Nothing and an InMemory implementation of loading the dependent entries (if they are not loaded) and updating the EntityState properly. It appears to be demonstrating expected behavior for cascade deletion.

What's next?
While the tests in the VS solution related to cascade deletion feature are passing, some test's are inconclusive (i.e. failing). Tests that asserts behavior like You cannot add a one-to-one or one-to-many relation entry without specifying the reference field value. Here is one such failing test:

[ConditionalFact]
public virtual void Optional_One_to_one_relationships_are_one_to_one()
{
    using (var context = CreateContext())
    {
        var root = context.Roots.Single(IsTheRoot);

        root.OptionalSingle = new OptionalSingle1();

        Assert.Throws<DbUpdateException>(() => context.SaveChanges());
    }
}

To support this behavior, it is apparent that implementing cascade delete is not going to be enough. We also need to check Added and Modified entries for relational consistency when saving. I understand that it does not fall under the title of this issue. But, without this implementation, I cannot really make the tests pass- unless I overwrite them to be empty, as they are in the dev branch.

So, my question is, should I submit a PR for cascade delete feature only under this issue and implement relational consistency checking in another PR? Or, would you like to change the title/description of the issue so that I can submit a single PR and make all the tests passing?

@ajcvickers
Copy link
Member

@zpbappi The support for cascade delete should be implemented purely in the in-memory database. It should not change the StateManager in any way. It should not change which entities are being loaded. This means that the in-memory database needs to understand the FK constraints that have been created and what their cascade behavior is defined as. It should then follow these constraints, independently of the state manager, when an entity is deleted and make appropriate changes or throw appropriate exceptions based on the constraint and cascade behavior. You would effectively be implementing some part of the FK support that a relational database has.

@Mardoxx
Copy link
Contributor

Mardoxx commented May 17, 2017

This feature would be very welcome. Any updates?

@ajcvickers
Copy link
Member

This issue is in the Backlog milestone. This means that it is not going to happen for the 2.0 release. We will re-assess the backlog following the 2.0 release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

@Mardoxx
Copy link
Contributor

Mardoxx commented May 17, 2017

Brilliant, thank you :)

@PascalHonegger
Copy link

Any updates? I would really appreciate this feature to ensure my test in memory database behaves (more) like my production database.

@divega divega added good first issue This issue should be relatively straightforward to fix. help wanted This issue involves technologies where we are not experts. Expert help would be appreciated. and removed help wanted This issue involves technologies where we are not experts. Expert help would be appreciated. good first issue This issue should be relatively straightforward to fix. labels May 31, 2019
@ajcvickers ajcvickers added good first issue This issue should be relatively straightforward to fix. and removed help wanted This issue involves technologies where we are not experts. Expert help would be appreciated. labels Aug 5, 2019
@ajcvickers ajcvickers removed the good first issue This issue should be relatively straightforward to fix. label Aug 30, 2019
@AndriySvyryd
Copy link
Member

This depends on #2166

Liu233w added a commit to Liu233w/acm-statistics that referenced this issue Apr 14, 2020
有些测试没有通过,因为InMemoryDB目前还不支持外键相关的Cascade删除(见dotnet/efcore#3924
Liu233w added a commit to Liu233w/acm-statistics that referenced this issue Apr 15, 2020
有些测试没有通过,因为InMemoryDB目前还不支持外键相关的Cascade删除(见dotnet/efcore#3924
Liu233w added a commit to Liu233w/acm-statistics that referenced this issue Apr 15, 2020
有些测试没有通过,因为InMemoryDB目前还不支持外键相关的Cascade删除(见dotnet/efcore#3924
mergify bot pushed a commit to Liu233w/acm-statistics that referenced this issue Apr 15, 2020
有些测试没有通过,因为InMemoryDB目前还不支持外键相关的Cascade删除(见dotnet/efcore#3924
ajcvickers added a commit that referenced this issue Nov 26, 2020
Fixes #11552

Failures were due to:
* Lack of cascade update/delete support--tracked by #3924.
* Lack of uniqueness constraint checking--tracked by #2166.
@komdil
Copy link

komdil commented Jul 5, 2022

Any updates?

@ajcvickers
Copy link
Member

@komdil This issue is in the Backlog milestone. This means that it is not planned for the next release (EF Core 7.0). We will re-assess the backlog following the this release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources. Make sure to vote (👍) for this issue if it is important to you.

@ajcvickers ajcvickers added propose-close closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. and removed type-enhancement propose-close area-in-memory labels Oct 20, 2022
@ajcvickers
Copy link
Member

We recommend against using the in-memory provider for testing--see Testing EF Core Applications. While we have no plans to remove the in-memory provider, we will not be adding any new features to this provider because we believe valuable development time is better spent in other areas. When feasible, we plan to still fix regressions in existing behavior.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2022
@ajcvickers ajcvickers removed this from the Backlog milestone Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported
Projects
None yet
Development

No branches or pull requests

10 participants