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

Update error when a table has two foreign keys pointing to the same table #168

Open fdahlberg opened 6 years ago

fdahlberg commented 6 years ago

Hi,

I have the following model:

public class Person
    {
        public Person()
        {
            Id = Guid.NewGuid();
            WorkItems = new List<WorkItem>();
        }

        public Guid Id { get; set; }

        public string Name { get; set; }

        public ICollection<WorkItem> WorkItems { get; set; }
    }
public class WorkItem
    {
        public WorkItem()
        {
            Id = Guid.NewGuid();
        }

        public Guid Id { get; set; }

        public string Description { get; set; }

        public Person Creator { get; set; }

    }

My fluent API looks like this:

protected override void OnModelCreating(DbModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);

            modelBuilder.Entity<Person>()
                .HasMany(p => p.WorkItems)
                .WithRequired()
                .WillCascadeOnDelete();
        }

When I use Graphdiff to update this small graph, I find that the Creator_Id Column in the WorkItem table is constantly set to the same as Person_id column. I can't understand why this is happening. Is this a bug?

My graphdiff code to update the graph looks like this:

        public static void UpdatePerson(Person person)
        {
            using (var context = new GraphDiffTestDbContext())
            {
                context.Database.Log = s => System.Diagnostics.Debug.WriteLine(s);

                context.UpdateGraph(person, map => map
                .OwnedCollection(p => p.WorkItems, with => with.AssociatedEntity(p => p.Creator))
                );

                context.SaveChanges();
            }
        }
JonathanMagnan commented 6 years ago

Hello @fdahlberg ,

Thank you for reporting, I will try to check this issue tomorrow or during next week.

Best Regards,

Jonathan


Help us to support this library: Donate

cryo75 commented 6 years ago

This is not a GraphDiff problem but most probably something wrong with your entity configuration. How does your fluent API look like?

fdahlberg commented 6 years ago

Hi cryo75, The fluent api that I'm using is part of the question.

cryo75 commented 6 years ago

How are you mapping the properties to the columns?

And where is the Person_id column? In Person on in WorkItem?

fdahlberg commented 6 years ago

The Person_id column is created automatically by Entity Framework, because I have a naviagation property on Person (WorkItems). The code above gives me the table structure that I'm after. But it's possible I could achieve this in a different way that would work with GraphDiff.

cryo75 commented 6 years ago

So you have Person_Id and Creator_Id in the workitems table? Am I understanding correctly? And why do you want 2 foreign keys pointing to the same table?

I usually do my own mapping. However, what I can see is this:

context.UpdateGraph(person, map => map .OwnedCollection(p => p.WorkItems, with => with.AssociatedEntity(p => p.Creator)) );

Shouldn't that be:

context.UpdateGraph(person, map => map.OwnedCollection(p => p.WorkItems));

fdahlberg commented 6 years ago

That's correct! In my example (the real situation is much more complicated) I want to say that a person has a number of workitems, but the workitem is also created by a person (differenct from the owner). Therefore, as far as I can understand I need 2 foreign keys pointing to the same table (Person). Would you have solved it differently?

The Graphdiff mapping that you suggest is what I started with. I added the associated entity part as an attempt to solve it, but it didn't change anything.

cryo75 commented 6 years ago

This is EF-related. I guess you will need another Person property (Owner) in your WorkItem because Creator will always be the same as Person.

fdahlberg commented 6 years ago

Tried adding a property on WorkItem called Owner and changing the fluen API to: modelBuilder.Entity() .HasMany(p => p.WorkItems) .WithRequired(w => w.Owner) .WillCascadeOnDelete();

Same result unfortunately.

Not sure it's EF related since it works fine if I add a Person object in the normal EF way. This is the code that works fine, meaning that only Owner_id is populated:

        public static void AddPerson()
        {

            var john = new Person { Name = "John" };

            var workItem1 = new WorkItem { Description = "Things to do" };

            john.WorkItems.Add(workItem1);

            using (var context = new GraphDiffTestDbContext())
            {
                context.Persons.Add(john);

                context.SaveChanges();
            }

        }

When I run the following graphdiff it doesn't work:

        public static void UpdatePerson()
        {

            var jenny = new Person { Name = "Jenny" };

            var workItem1 = new WorkItem { Description = "Stuff to do" };

            jenny.WorkItems.Add(workItem1);

            using (var context = new GraphDiffTestDbContext())
            {
                context.Database.Log = s => System.Diagnostics.Debug.WriteLine(s);

                context.UpdateGraph(jenny, map => map
                .OwnedCollection(p => p.WorkItems)
                );

                context.SaveChanges();
            }
        }

After having run these two methods the database looks like this:

2017-11-27_11h16_59

So what I can't understand is why Creator_id is populated when I use graphdiff.

cryo75 commented 6 years ago

Can you set up a sample project, zip it and attach it here so I can have a look at it.

fdahlberg commented 6 years ago

This is the project I'm using to test this issue:

GraphDiffTest.zip

fdahlberg commented 6 years ago

I know that it works fine when you're not using GraphDiff. My problem is when you use GraphDiff...

cryo75 commented 6 years ago

I noticed. I'm getting the same Id's when using GraphDiff. Trying to see what the problem is.

fdahlberg commented 6 years ago

Excellent! Then we're on the same page!

JonathanMagnan commented 6 years ago

Hello @fdahlberg ,

Sorry for the delay, we forget to add this issue to our task list.

My developer tried your scenario, one problem he noted is that you call base.OnModelCreating(modelBuilder); before configuring your model. If you switch the call to put it at the end, the code works fine.

protected override void OnModelCreating(DbModelBuilder modelBuilder)
{
    modelBuilder.Entity<Person>()
        .HasMany(p => p.WorkItems)
        .WithRequired()
        .WillCascadeOnDelete();

    base.OnModelCreating(modelBuilder);
}

Let me know if that once when moving base.OnModelCreating at the end of the method.

Best Regards,

Jonathan


Help us to support this library: Donate

JonathanMagnan commented 6 years ago

Hello @fdahlberg ,

I will close this issue since it seems to be solved.

Feel free to reopen it if there is something else.

Best Regards,

Jonathan


Help us to support this library: Donate

fdahlberg commented 6 years ago

Hi Jonathan,

I tried the suggested solution but couldn't see any difference. Acccording to the documentation for OnModelCreating the base implementation does nothing. So how could it matter whether I call it first or last or not at all in the method?

Kind Regards Fred

JonathanMagnan commented 6 years ago

Hello @fdahlberg ,

I don't understand either, I just know that if we put this line before it doesn't work, but when we put this line after, it works on our side.

Could you at least try it?

Best Regards,

Jonathan


Help us to support this library: Donate

fdahlberg commented 6 years ago

Hi Jonathan,

I have tried it. It didn't work for me. I've found a workaround (changed my db structure) so it's not terribly important for me anymore. But would be nice if you could fix it. I find it hard to believe that moving that line (base.OnModelCreating ) could make any difference though. Would be very interesting to understand how, if that's actaully the case.

Fred

JonathanMagnan commented 6 years ago

We will investigate again, don't worry ;)

I'm also curious why it started to work on our side magically!

Best Regards,

Jonathan


Help us to support this library: Donate

hprutsman commented 5 years ago

Hi @JonathanMagnan, was this ever looked at again? I have come across this same problem and am wondering if there will be a fix coming?

JonathanMagnan commented 5 years ago

Hello @hprutsman ,

To be honest, yes it will be looked again but not in a very short term.

We are currently trying to close all issues related to EF Effort than we might move to this one.

One problem with this request if I remember well is how FK are handled. They are a huge flaw in the logic with this scenario that requires some heavy work on our side. The logic simply doesn't work.

We will need soon to implement GraphDiff for Entity Framework Classic, so it will allow us to look deeper on how we can better do this library but meanwhile, there is nothing planned to fix this.

Best Regards,

Jonathan

Bykiev commented 5 years ago

@JonathanMagnan, hi!

Thank you for this awesome library. I faced with the same problem in EF6, any news when it'll be fixed?

Bykiev commented 2 years ago

@JonathanMagnan, is any progress on this?