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

DbSet.Local does not automatically call DetectChanges #14001

Closed
fiseni opened this issue Nov 22, 2018 · 8 comments · Fixed by #15435
Closed

DbSet.Local does not automatically call DetectChanges #14001

fiseni opened this issue Nov 22, 2018 · 8 comments · Fixed by #15435
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

@fiseni
Copy link

fiseni commented Nov 22, 2018

If child is removed from parent's collection, the child's FK is not null when accessing through Set<>.Local property. If we analyze further, we see that CurrentValue is set to null, and I assume it returns the OriginalValue (not null). I'm not sure if this is done by design on EF Core (EF6 has correct behavior). The relationship is nullable.

    public class Order
    {
        public Order()
        {
            this.OrderLines = new HashSet<OrderLine>();
        }
        public int OrderID { get; set; }
        public string OrderDescription { get; set; }
        public virtual ICollection<OrderLine> OrderLines { get; set; }
    }

    public class OrderLine
    {
        public int OrderLineID { get; set; }
        public string OrderLineDescription { get; set; }

        public int? OrderID { get; set; }
        public Order Order { get; set; }
    }

    class Program
    {
        static void Main(string[] args)
        {
            //dotnet ef database update -c AppContext
            //SeedData();
            DoTest();
            Console.ReadLine();
        }
        private static void DoTest()
        {
            var context = new AppContext();
            var order = context.Orders.Include(x => x.OrderLines).FirstOrDefault();

            Console.WriteLine("Before Removal");
            foreach (OrderLine item in context.OrderLines.Local)
            {
                Console.WriteLine($"OrderLine ID: {item.OrderLineID}, OrderID: {item.OrderID}");
            }
            Console.WriteLine();

            order.OrderLines.Remove(order.OrderLines.FirstOrDefault());

            Console.WriteLine("After Removal");
            foreach (OrderLine item in context.OrderLines.Local)
            {
                Console.WriteLine($"OrderLine ID: {item.OrderLineID}, " +
                            $"OrderID: {item.OrderID}");//We're not getting null here, so it's same as the Original value
                Console.WriteLine($"OrderLine ID: {item.OrderLineID}, " +
                            $"Original Value: {context.Entry(item).Property("OrderID").OriginalValue}");
                Console.WriteLine($"OrderLine ID: {item.OrderLineID}, " +
                            $"Current Value: {context.Entry(item).Property("OrderID").CurrentValue}");
                Console.WriteLine();
            }
        }
        private static void SeedData()
        {
            using (var context = new AppContext())
            {
                context.Orders.Add(new Order()
                {
                    OrderDescription = "Order1",
                    OrderLines = new List<OrderLine>()
                    {
                        new OrderLine()
                        {
                            OrderLineDescription = "OrderLine1"
                        },
                        new OrderLine()
                        {
                            OrderLineDescription = "OrderLine2"
                        }
                    }
                });
                context.SaveChanges();
            }
        }
    }
@ajcvickers
Copy link
Member

Note for triage: the difference from EF6 here is that DbSet.Local doesn't call DetectChanges automatically.

@fiseni EF needs DetectChanges to be called to know that the change to the navigation property has been made. For example:

 order.OrderLines.Remove(order.OrderLines.FirstOrDefault());
context.ChangeTracker.DetectChanges();

@ajcvickers ajcvickers changed the title Set<T>.Local returns OriginalValue instead of CurrentValue for child's FK property DbSet.Local does not automatically call DetectChanges Nov 26, 2018
@fiseni
Copy link
Author

fiseni commented Nov 26, 2018

Thank you @ajcvickers .

I might be reasoning wrong, but I can't see the benefit of having these actions decoupled. Now we'll have to call DetectChanges anytime we want to use DbSet.Local. Most of the time, if not always, they'll be paired anyway.

@ajcvickers
Copy link
Member

@fiseni DetectChanges can be expensive, and depending on how Local is being used it may or may not be needed.

@ajcvickers
Copy link
Member

Triage: we will make calling .Local call DetectChanges automatically. (Unless, of course, automatic DetectChanges is switched off.)

@ajcvickers ajcvickers added this to the 3.0.0 milestone Nov 28, 2018
@ajcvickers ajcvickers self-assigned this Nov 28, 2018
@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 Apr 21, 2019
ajcvickers added a commit that referenced this issue Apr 21, 2019
* Fixes #15294 - BoolToStringConverter should not check if falseValue or trueValue is empty
* Fixes #15266 - Small typo in resource file
* Fixes #14037 - XML comments for DbContext.Attach unclear
* Fixes #14001 - DbSet.Local does not automatically call DetectChanges
* Fixes #13210 - WithExtension suggests that 'this' is updated, as well as returning 'this'
ajcvickers added a commit that referenced this issue Apr 21, 2019
* Fixes #15294 - BoolToStringConverter should not check if falseValue or trueValue is empty
* Fixes #15266 - Small typo in resource file
* Fixes #14037 - XML comments for DbContext.Attach unclear
* Fixes #14001 - DbSet.Local does not automatically call DetectChanges
* Fixes #13210 - WithExtension suggests that 'this' is updated, as well as returning 'this'
ajcvickers added a commit that referenced this issue Apr 22, 2019
* Fixes #15294 - BoolToStringConverter should not check if falseValue or trueValue is empty
* Fixes #15266 - Small typo in resource file
* Fixes #14037 - XML comments for DbContext.Attach unclear
* Fixes #14001 - DbSet.Local does not automatically call DetectChanges
* Fixes #13210 - WithExtension suggests that 'this' is updated, as well as returning 'this'
ajcvickers added a commit that referenced this issue Apr 22, 2019
* Fixes #15294 - BoolToStringConverter should not check if falseValue or trueValue is empty
* Fixes #15266 - Small typo in resource file
* Fixes #14037 - XML comments for DbContext.Attach unclear
* Fixes #14001 - DbSet.Local does not automatically call DetectChanges
* Fixes #13210 - WithExtension suggests that 'this' is updated, as well as returning 'this'
ajcvickers added a commit that referenced this issue Apr 22, 2019
* Fixes #15294 - BoolToStringConverter should not check if falseValue or trueValue is empty
* Fixes #15266 - Small typo in resource file
* Fixes #14037 - XML comments for DbContext.Attach unclear
* Fixes #14001 - DbSet.Local does not automatically call DetectChanges
* Fixes #13210 - WithExtension suggests that 'this' is updated, as well as returning 'this'
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview6 Jun 5, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview6, 3.0.0 Nov 11, 2019
@Prinsn
Copy link

Prinsn commented Jul 8, 2020

I might be reasoning wrong, but I can't see the benefit of having these actions decoupled

I believe my 2.2 -> 3.1 code is currently suffering a performance degredation approximating x7500 by this coupling, of 24 seconds to 50 minutes, and it's all been isolated to accessing local context.

@fiseni
Copy link
Author

fiseni commented Jul 8, 2020

That's a huge difference. Did you try disabling DetectChanges to ensure that this coupling is causing the issue?

dbContext.ChangeTracker.AutoDetectChangesEnabled = false

Weird, EF6 always kept DbSet.Local in sync and never experienced such low performances.

@fiseni
Copy link
Author

fiseni commented Jul 8, 2020

DbSet.Local usually is utilized through ToBindingList() and ToObservableCollection(). Since, the changes where not updated automatically, the two-way databinding was not working as expected. That's the reason of raising this issue.

Of course your use case might be totally different. Now when I see the internal implementation, if you're working directly with DbSet.Local and accessing it frequently, that would be a problem.

@Prinsn
Copy link

Prinsn commented Jul 8, 2020 via email

@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.

3 participants