Data corruption when fixing up navigation properties

[jods4]

There is a critical, silent data-corruption bug when setting navigation properties in AttachCyclicNavigationProperty.

The issue is how the navigation property that must be fixed in child entity is found, here:
https://github.com/zzzprojects/GraphDiff/blob/master/GraphDiff/GraphDiff.Shared/Internal/Graph/GraphNode.cs#L116-L119

The logic is to look at all navigation properties in child entity, keep only navigations to the correct (Parent) type, and keep one randomly FirstOrDefault.

If you have a complex model with multiple relations between the same entities, this might set the wrong property (it happened to us and took an entire afternoon to debug). Because there is no error, this will then corrupt your data in DB when you save, without anyone noticing -- at first 😏

AttachCyclicNavigationProperty is for example called when updating a many-to-one collection.
So the following example has a good chance of corrupting the data:

class Parent {
  public int Id { get; set; }
  // Assume you map Children to the reverse property Inverse
  public ICollection<Child> Children { get; set; }
}

class Child {
  // Child has multiple navigation properties to Parent
  // Assume they have no reverse-mapping, except Inverse, which is the reverse navigation of Children.

  public int aId { get; set; }
  public Parent A { get; set; }

  public int bId { get; set; }
  public Parent B { get; set; }

  public int inverseId { get; set; }
  public Parent Inverse { get; set; }
}

// Code likely to corrupt data
var child = new Child();
var parent = new Parent { Children = new List { child } };
db.UpdateGraph(parent, map => map.AssociatedCollection(p => p.Children));

// When updating p.Children, GraphDiff will look for a reverse navigation property on Child, 
// whose type is Parent.
// Because there are 3 such navigation properties, it'll pick and set the 1st one, 
// which is likely to not be the right one.

// So it is likely that at this point in code, `parent` was incorrectly set into child.A

I guess the fix is that AttachCyclicNavigationProperty needs to know which navigation property is currently being set in parent to find the correct matching property in child.

BTW is this project still maintained? This bug is really bad so I took the time to open an issue but I wasn't sure if this project was abandonned years ago or if someone picked it up again? Thanks!

[JonathanMagnan]

Hello @jods4 ,

This project is still maintained. We took ownership of this library around a year ago and mostly answer and investigate new issue.

We will look at the one you reported to see if something can be done.

Best Regards,

Jon

---

Performance Libraries
context.BulkInsert(list, options => options.BatchSize = 1000);
Entity Framework Extensions • Entity Framework Classic • Bulk Operations • Dapper Plus

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

[JonathanMagnan]

Hello @jods4 ,

The v3.1.3 has been released

In this version, we added a new navigation property to let you choose which one to use. Otherwise, it takes the first one to keep backward compatibility.

Here is an example:

context.UpdateGraph(entity, map => map.AssociatedCollection(c => c.EntitySimpleLists, x => x.B).AssociatedEntity(c => c.EntitySimpleChild));

Let me know if that fixes the current issue.

Best Regards,

Jon

[jods4]

Hello @JonathanMagnan
I appreciate the quickly published new version, thanks 🙇.

This feels more like a work-around than a proper fix, though.
Yes, it can be used to force GraphDiff to behave properly when it doesn't.
But if you aren't aware there might be an issue, you won't use this second nav. property and your code could corrupt data.

Furthermore, this can be introduced as regression by future, possibly unrelated changes.
For one, there's no guarantee in the enumeration order of navigation properties. If there's more than one, the behavior might change with each EF library update.

Second, you can introduce regressions to unrelated properties, unwillingly.
Consider this model:

class Parent
{
  ICollection<Child> Children;
}

class Child 
{
}

Nothing can go wrong as there is no back-nav on Child.

Now imagine in a v2 you add an unrelated Parent-Child 1:1 relationship

class Parent
{
  ICollection<Child> Children;
  Child DirectChild;
}

class Child 
{
  Parent DirectParent;
}

This addition will trip GraphDiff when updating Children, because the unrelated, newly introduced DirectParent will be (incorrectly) updated.

IMHO this is a really bad bug as it can silently corrupt data in your DB. It shouldn't have a chance to occur and I feel uneasy using GraphDiff knowing this potential issue is lurking in my code. Luckily the conditions where it triggers are slightly unusual.

The proper fix IMHO is to find the reverse-navigation that matches the collection that is currently updated, according to EF mapping. Is that not possible?

I don't think back-compat is a strong concern here, this is obviously the (only) correct behaviour.
If another (unrelated) navigation was updated, then there's no way the result was a correct entity graph. If someone's code breaks because of this, you're making them a favour.

[JonathanMagnan]

Hello @jods4 ,

I agree with you that's more a workaround and the problem is still here indeed if you are not aware of it.

Unfortunately, my developer was not able to find out how to make this perfect fix. So he did his best to find something that could work with the time it was allowed to him.

If you wish, you can play with the source code and propose a pull request about how it should be fixed. We will be happy to review and merge it.

[jods4]

I don't have much time atm, but I'll try to have a look whenever I can.
Doesn't the EF model expose which nav. is the reverse of which?

The main problem I think here is that "the problem is still here indeed if you are not aware of it", it's really hard to be aware of it. Just try to explain in the doc in a caveat when the problem happens. Hard. Even if you understand exactly the circumstances, imagine you are working with a very large and complex model. It's very hard.

I'll let you know if I ever find the time to dig into this and find a good solution.

[JonathanMagnan]

Hello @jods4 ,

Since our last conversation, we haven't heard from you.

We estimated that your request would be too complex to implement and we were not able to make this fix.

Your request is currently moved to our "pool" and we will try to work on it in the future.

We are sorry for this inconvenience.

Best regards,

Jon

[jods4]

Yeah sorry, I'm overwhelmed by work right now.

Since we have put some workarounds in our project after discovering this issue, it isn't as high-priority for me as other stuff I have to do.

I still think it's a critical flaw in GraphDiff and I'd like to investigate other fixes but that'll happen whenever I have some time available (i.e. not soon).
I'll post here if I ever find something.

[JonathanMagnan]

Hello @jods4 ,

Thank you for getting back to us.

Please let us know if you need further assistance.

Best regards,

Jon

[jods4]

@JonathanMagnan
EF internals are so complicated to work with 😭
But I think I found what we need.

So basically what your fix is missing is a way to automatically figure out the inverse property of a navigation property.
The code below returns a PropertyInfo (or null if none exists), given a type and the name of a navigation property.
It's only 6 lines of code but it took me a lot of research to find them.

Keep in mind I'm no EF nor GraphDiff expert, so you should review this carefully when integrating into GraphDiff.

https://gist.github.com/jods4/2f44393ea20764682be31f329f545d84

[jods4]

@JonathanMagnan Any thought?
I believe that gist would help solve this issue without any intervention from end-developer.