zzzprojects / GraphDiff

GraphDiff is a library that allows the automatic update of a detached graph using Entity Framework code first.
https://entityframework-graphdiff.net/overview
MIT License
333 stars 101 forks source link

GraphDiff duplicate returned entities #116

Closed amiliou closed 4 years ago

amiliou commented 9 years ago

Hello, As I've said on stackoverflow, I have a problem adding new entities to my 'parent' entity's list of children using GraphDiff. I'll try to reproduce my situation and show the problem.

I have an entity called FilterTemplate that has a list of FilterConditions. So in reality the class FilterTemplate has a list of FilterConditions and the class FilterCondition has an attribute

public virtual FilterTemplate FilterTemplate { get; set; }

and EntityFramework does the rest for me. The list of FilterConditions can change as the user may add, update or delete conditions but I don't know exactly what and that's why I need GraphDiff.

Classes:

public class FilterTemplate
{
    public FilterTemplate()
    {
        this.FilterConditions = new List<FilterCondition>();
    }
    public int Id { get; set; }
    public string TemplateName { get; set; }
    public virtual ICollection<FilterCondition> FilterConditions { get; set; }
}

public class FilterCondition
{
    public int Id { get; set; }
    public int FilterTemplateId { get; set; }
    public string Name { get; set; }
    public string Value { get; set; }
    public virtual FilterTemplate FilterTemplate { get; set; }
}

For example, I have exactly one filterCondition and the user adds another one, then when I want to update the list I call dbContext.UpdateGraph(filterTemplate, map => map.OwnedCollection(ft => ft.FilterConditions));

This leads me to CollectionGraphNode.Update where variable 'existing' has a list of one item and 'entity' has a list of 2 items with the 2nd having an Id=0. Variable 'dbCollection' gets populated with one filterCondition. When the loop gets to the new filterCondition, AddElement gets called. There as I said before calling "changeTracker.UpdateItem(updateItem, instance);" dbCollection has one item and after it two (existing variable has also a child list of 2). When the code gets to

dbCollection.GetType().GetMethod("Add").Invoke(dbCollection, new[] {updateItem});

the dbCollections has now 3 items with the last two being exactly the same (similarly in variable existing). From that point onwards the list keeps having my addition as duplicate (with the same new Id) and of course causing a lot of trouble.

Commenting out the line

dbCollection.GetType().GetMethod("Add").Invoke(dbCollection, new[] {updateItem});

seems to fix the issue but I don't find it proper to change the code that I honestly don't get completely as drilling down to it will take way too much time. So I was wondering if you are aware that is an issue with the Add logic or if my case's specifics are the cause of the problem. I don't know if more code snippets will help as most of the logic lies with GraphDiff. But if so, let me know. Thanks in advance.

amiliou commented 9 years ago

Just a note, I use GraphDiff to update many entities that have children lists and all of them have the same issue. Although all of them have similar class structure and that's why I am just showing one of my cases here.

ghost commented 9 years ago

Hi,

I just tried to have a look at your problem but failed to reproduce your issue, so I've two more questions. First, did you try to run your code against the most current code on our develop branch and did the error still occur?

If your problem persists with the most current code, how does your setup (classes, navigation properties, mapping etc.) differ from our unit tests for this scenario?

amiliou commented 9 years ago

Hello, I tried with the latest version in the develop branch and it still has the same issue. I am working with .NET 4.5 and the tests have been failing due to some issues. I 'll try to fix that tomorrow and let you know about the unit test. Thanks again.

amiliou commented 9 years ago

Ok tests are running now and test ShouldAddNewItemInOwnedCollection() works as expected, so no duplicates. That probably means that my case causes the issue but I cannot narrowed it down. Class structure is similar but I guess that doesn't say much. I'll keep looking or worse case scenario, I'll put a logical check to avoid duplicates.

ghost commented 9 years ago

Hi,

I wouldn't mind having a closer look if you could upload your stuff as a gist. Please include a simple test that demonstrates your problem, so I can start debugging right away. I'm still not entirely convinced there isn't a bug in GraphDiff causing this..

amiliou commented 9 years ago

I wish I could do that but I don't own the code and I am not allowed to post it. I'd have to reproduce the whole structure with other entities and even then I am not sure that would be OK with my company. If I find what's causing the issue, I'll let you know or else I'd hope that someone else would face a similar problem and be able to help you find it and sort it out. Thanks again.

ghost commented 9 years ago

Okay, good luck. I'll close this issue then (for now).

friedroman commented 9 years ago

Just wanted to say that i encountered exactly the same problem a while ago. Also tried to narrow it down and put together some tests to reproduce but with no success. Ended up forking and doing a dirty fix by replacing reflection based call to cast to ICollection and checking for Contains() before add. It most likely has to do with entities being used. Mine were self-tracking proxies implementing INotifyPropertyChanged and collection were ObservableCollection. We no longer use EntityFramework so i can't really be of much help with reproducing and testing.

ghost commented 9 years ago

Ok, thanks for the info anyway.

amiliou commented 9 years ago

Thanks friedroman, that's what I end up doing, add some logic in AddElement method to avoid duplicates. Just a note for andypelzer, I noticed today that this behavior of duplicates happens in all child entities that have an Id (that is =0 when they are being added) while when updating one entity that has a list of children that have no Id (and thus nothing equal to 0), when there is a new one, it is not being added twice.

sven5 commented 9 years ago

Hi,

I have exactly the same problem. AddElement is adding the element twice. The following line adds the item to the collection at first: _context.Entry(to).CurrentValues.SetValues(from); And then the same item is added once again through this line: dbCollection.GetType().GetMethod("Add").Invoke(dbCollection, new[] {updateItem});

At first I thought the reason could by the ObservableCollection - but I changed back to HashSet and the issue still persists. Regards Sven

ghost commented 9 years ago

Hi Sven,

are you able to reproduce this in a small unit test that you can share? I'd be willing to debug / fix this, but couldn't reproduce it yet..

sven5 commented 9 years ago

Hi Andy,

sorry I wasn't able to reproduce this in a small unit test. In the unit test the problem disappeared. I will try to find a working test case.

Regards Sven

sven5 commented 9 years ago

Hi Andy,

I nearly have spent the whole day to find the problem and I've been successul. Let me try to explain the issue. If I set the foreign key Id of the parent entity in the child entity then the item will be contained twice in the resulting collection. If I omit setting the key then everything is fine. But I still cannot reproduce this problem in a tear-down unit test.

paradox123 commented 9 years ago

Hi Andy, I encountered the very same error as described by shaper, is there any chance to get this fixed, workaround(ed) or something similar? Regards, Daniel

ghost commented 9 years ago

Hi Daniel,

sorry, I don't have much free time at the moment. Are you able to reproduce the problem in a (small) unit test? If I have a unit test reproducing the problem, I'll try to make time to give it a shot over the weekend or so..

ghost commented 9 years ago

I run into the same issue.

How to reproduce:

  1. Add ParentId property to OneToManyOwnedModel
  2. Set modelBuilder.Entity().HasMany(p => p.OneToManyOwned).WithRequired(p => p.OneParent).HasForeignKey(p => p.ParentId)
  3. Create parent entity with the empty list of child entities. Add it to context with context.UpdateGraph(node1, map => map.OwnedCollection(p => p.OneToManyOwned)); and SaveChanges();
  4. Within the same context create node1 copy with new child entity, where ParentId = node1.Id
  5. Update graph and save changes

Result: OneToManyOwned contains duplicate

rwdalpe commented 9 years ago

I have also run into this issue and can summarize it as so:

EF has some curious behavior. If all of the following are true:

Then as soon as the new object is added to the context or receives an update to its parent reference, either via the Add() method or SetValues(), then the parent reference will automatically have the new object added to its collection.

That's problematic here, because we later call the collection's Add method explicitly. If you're dealing with a Set of some kind, no big deal, but if you're using Lists then you will experience double adds.

I will post a pull request for unit tests in just a moment.

sanj0988 commented 8 years ago

Hi @andypelzer

I am trying the same scenario and have run into the same problem.

Could you please help me with the latest status of this fix ? Is this bug resolved now in the latest build or it's still an open bug ?

Thanks

rwdalpe commented 8 years ago

Based on the github commit log to the master branch this hasn't been resolved there. If it's resolved in develop, I'm not sure.

I don't have the exact code available (and in fact it might not be something you want), but we worked around this issue using a heuristic. I believe this all took place in https://github.com/refactorthis/GraphDiff/blob/8d2675e761923863d57e79166ba6a06bac246786/GraphDiff/GraphDiff/Internal/Graph/CollectionGraphNode.cs#L56

What we did was use reflection to get the current count of items in the collection we intended to add to before we did any child item updating. Then after the child item updating we get the current count of items in the collection again and see if it differs from the original. If it's greater, then we assume that we "automatically, via the bug" added the item to the collection and we don't invoke Add on line 78. Otherwise, we invoke Add as usual.

Obviously this is just a heuristic and not particularly ideal. If updating the child element does some sort of valid addition to the parent collection, you'll get false results from this heuristic. But because this bug appears to be behavior baked into EF at some level that is brought out by GraphDiff, I'm not sure if there's much to do besides some sort of heuristic checking or to dive into EF and figure out what exactly makes this happen.

cryo75 commented 8 years ago

I have the same problem.

My current (and crude) solution is to do the following before calling UpdateGraph:

if (entity.Children != null) { entity.Children .ToList().ForEach(x => { x.ParentId = x.Id == 0 ? 0 : entity.Id; x.ChildId = x.Child.Id; }); }

Is there a better and more reliable way instead of the above?

Adriien-M commented 8 years ago

I found a simple way to resolve this issue. Indeed, I modified the method AddElementof the file CollectionGraphNode.cs to check if the collection contains already the new element :

var contains = (bool) dbCollection.GetType().GetMethod("Contains").Invoke(dbCollection, new[] { updateItem }); if (!contains) dbCollection.GetType().GetMethod("Add").Invoke(dbCollection, new[] { updateItem });

keir-nellyer commented 6 years ago

This is still an issue with version 3.0.0, would be awesome if this was fixed! :)

salmanbabri commented 5 years ago

I have the same problem.

My current (and crude) solution is to do the following before calling UpdateGraph:

if (entity.Children != null) { entity.Children .ToList().ForEach(x => { x.ParentId = x.Id == 0 ? 0 : entity.Id; x.ChildId = x.Child.Id; }); }

Is there a better and more reliable way instead of the above?

This actually works. However it isn't feasible to write such dirty code in production. :-(

JonathanMagnan commented 4 years ago

Hello all,

The v3.1.2 has been released which fixes the duplicate adding entities issue.

Since it has been a long time, we will directly close the issue but if there is anything, just let us know and we will re-open it.

Best Regards,

Jon


Performance Libraries context.BulkInsert(list, options => options.BatchSize = 1000); Entity Framework ExtensionsEntity Framework ClassicBulk OperationsDapper Plus

Runtime Evaluation Eval.Execute("x + y", new {x = 1, y = 2}); // return 3 C# Eval FunctionSQL Eval Function