Change default graph behavior of Add/Attach/etc #4424

Closed
rowanmiller opened this Issue Jan 28, 2016 · 35 comments

Comments

Projects
None yet
9 participants
@rowanmiller
Member

rowanmiller commented Jan 28, 2016

In EF6.x the default behavior of these methods was to start tracking every reachable entity in the state being requested for the root entity. This caused a lot of issues because:

  1. Attaching a root would often end in exceptions because there were new entities in the graph (with no PK set) which would attempt to be tracked as unchanged, resulting in duplicate PK values.
  2. There was no easy mechanism to traverse over a graph and chose the state of each entity as you go.
  3. There was no ability to leave parts of the graph un-tracked (i.e. I want to add an order and I don't care about telling you the state of the Customer).

In EF Core we tried to alleviate some of these issues by initially making Add/Attach/etc. just work on a single object, but this caused a lot of confusion since it seems the natural expectation (based on the feedback we saw) is that you could add an Order and the OrderLines would be automatically added too.

We then tried an "Include Dependents" approach, where if you add a parent, we also include the children as added. This works well for some cases... but it gets pretty confusing. This is compounded by the fact that it's not always clear from your model which is the principal/dependent of a relationship.

After discussion, we realized that with some small variations on the rules from EF6.x, we already have APIs in EF Core to enable these scenarios:

There was no easy mechanism to traverse over a graph and chose the state of each entity as you go.

We have the TrackGraph API for exactly this purpose.

There was no ability to leave parts of the graph un-tracked (i.e. I want to add an order and I don't care about telling you the state of the Customer).

This is now supported in EF Core, our stack is capable of handling entities in the graph that it is not tracking. We have the DbContext.Entry(...).State API that allows you to put a single entity into the context without any graph behavior (i.e. no related entities are brought in).

Attaching a root would often end in exceptions because there were new entities in the graph (with no PK set) which would attempt to be tracked as unchanged, resulting in duplicate PK values.

We will adjust the graph behavior of these APIs as follows:

  • Add: Adds every reachable entity that is not already tracked
  • Attach: Attaches every reachable entity, except where a reachable entity has a store generated key and no key value is assigned, these will be marked as added.
  • Update: Same as Attach, but entities are marked as modified
  • Remove: Same as Attach, and then mark the root as deleted. Since cascade delete now happens on SaveChanges, this allows cascade rules to flow to entities later on.

Some important results of this behavior:

  • It works as expected for attached scenarios (i.e. non N-Tier scenarios). I.e. if I create a graph of objects and then call Add, they all get added.
  • For N-Tier scenarios, it leaves you with the ability to cycle over each entity and tell EF the state (which was blocked in EF6.x because you would get duplicate key exceptions). Of course, TrackGraph may also be a better API for these scenarios too.
@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Jan 28, 2016

Member

Nice write up. :-)

Member

ajcvickers commented Jan 28, 2016

Nice write up. :-)

ajcvickers added a commit that referenced this issue Jan 29, 2016

Remove GraphBehavior enum and track all reachable entities
Issue #4424

This makes the behavior for Add/Attach/Update be to track all reachable entities, as in EF6. Single object operations can be done with the EntityEntry API. In the future we made add a context-wide flag to modify this behavior.

Also, make Remove still be single object, but do attachment of reachable entities such that cascade deletes will be processed correctly on SaveChanges.

@ajcvickers ajcvickers added the 2 - Done label Jan 29, 2016

@ajcvickers ajcvickers closed this Jan 29, 2016

@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Feb 1, 2016

:(

Thought I should clarify that ... :) .... sorry you weren't able to work it out. I liked that idea but also know that people are going to have a hard time with changed behavior no matter how many times you tell them "it's not EF6". I'm glad we still have ENTRY to add just the root. It was always a hard problem not being able to tear apart graphs for this purpose so that's still a big win. Agree re great detailed writeup. Thanks. Even Frans said "Tho, reading about the defaults, what they've picked is the best choice for POCOs I think." But he also adds "Root cause is lack of self tracking." (Don't ever bring that back! :) )

:(

Thought I should clarify that ... :) .... sorry you weren't able to work it out. I liked that idea but also know that people are going to have a hard time with changed behavior no matter how many times you tell them "it's not EF6". I'm glad we still have ENTRY to add just the root. It was always a hard problem not being able to tear apart graphs for this purpose so that's still a big win. Agree re great detailed writeup. Thanks. Even Frans said "Tho, reading about the defaults, what they've picked is the best choice for POCOs I think." But he also adds "Root cause is lack of self tracking." (Don't ever bring that back! :) )

@tonysneed

This comment has been minimized.

Show comment
Hide comment
@tonysneed

tonysneed Feb 1, 2016

Contributor

So just to clarify, we'll still be able to set an entity state to Added via TrackGraph and EF will only set the state of that single entity to Added?

Contributor

tonysneed commented Feb 1, 2016

So just to clarify, we'll still be able to set an entity state to Added via TrackGraph and EF will only set the state of that single entity to Added?

@rowanmiller

This comment has been minimized.

Show comment
Hide comment
@rowanmiller

rowanmiller Feb 1, 2016

Member

@tonysneed if you want to set the state of a single entity then context.Entry(myEntity).State = EntityState.Added is probably the easiest. TrackGraph will work... but that is really designed for iterating over a graph of entities and setting the state for each one.

Member

rowanmiller commented Feb 1, 2016

@tonysneed if you want to set the state of a single entity then context.Entry(myEntity).State = EntityState.Added is probably the easiest. TrackGraph will work... but that is really designed for iterating over a graph of entities and setting the state for each one.

@tonysneed

This comment has been minimized.

Show comment
Hide comment
@tonysneed

tonysneed Feb 1, 2016

Contributor

Thanks @rowanmiller, so context.Entry(myEntity).State = EntityState.Added will not set child entity states to Added, as it did in EF6. Just wanted to confirm, thanks!

Contributor

tonysneed commented Feb 1, 2016

Thanks @rowanmiller, so context.Entry(myEntity).State = EntityState.Added will not set child entity states to Added, as it did in EF6. Just wanted to confirm, thanks!

@rowanmiller

This comment has been minimized.

Show comment
Hide comment
@rowanmiller

rowanmiller Feb 1, 2016

Member

@tonysneed correct, EF Core is now ok with having tracked entities reference entities that are not tracked.

Member

rowanmiller commented Feb 1, 2016

@tonysneed correct, EF Core is now ok with having tracked entities reference entities that are not tracked.

@tonysneed

This comment has been minimized.

Show comment
Hide comment
@tonysneed

tonysneed Feb 2, 2016

Contributor

Excellent. In case anyone is interested, Here is a test which shows the difference between EF 6 and EF Core when it comes to setting entity state to Added on a disconnected graph.

[Fact]
public void Set_state_should_mark_single_entity()
{
    // Arrange
    var detail = new OrderDetail();
    var order = new Order
    {
        OrderDetails = new List<OrderDetail> { detail }
    };

    // Act: Set parent state to Added
    _context.Entry(order).State = EntityState.Added;

    // Assert for EF 6: Child state set to Added
    Assert.Equal(EntityState.Added, _context.Entry(detail).State);

    // Assert for EF Core: Child state remains Detached
    Assert.Equal(EntityState.Detached, _context.Entry(detail).State);
}

Here is the full source code for the tests.

Contributor

tonysneed commented Feb 2, 2016

Excellent. In case anyone is interested, Here is a test which shows the difference between EF 6 and EF Core when it comes to setting entity state to Added on a disconnected graph.

[Fact]
public void Set_state_should_mark_single_entity()
{
    // Arrange
    var detail = new OrderDetail();
    var order = new Order
    {
        OrderDetails = new List<OrderDetail> { detail }
    };

    // Act: Set parent state to Added
    _context.Entry(order).State = EntityState.Added;

    // Assert for EF 6: Child state set to Added
    Assert.Equal(EntityState.Added, _context.Entry(detail).State);

    // Assert for EF Core: Child state remains Detached
    Assert.Equal(EntityState.Detached, _context.Entry(detail).State);
}

Here is the full source code for the tests.

@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Feb 2, 2016

yeah I better go do a RC2 branch and update all of my disconnected data/graph tests in my ef7 repo (https://github.com/julielerman/EF7WhoAreYou/tree/master/Familiar%20and%20Enhancements%20Console%20Full%20NET%20After/Tests.RC)

yeah I better go do a RC2 branch and update all of my disconnected data/graph tests in my ef7 repo (https://github.com/julielerman/EF7WhoAreYou/tree/master/Familiar%20and%20Enhancements%20Console%20Full%20NET%20After/Tests.RC)

@rowanmiller

This comment has been minimized.

Show comment
Hide comment
@rowanmiller

rowanmiller Feb 2, 2016

Member

@julielerman and rename your repo to EFCore 😉

BTW our nightly builds are pretty unstable at the moment due to moving over to .NET CLI... so you may want to hold off a couple of weeks.

Member

rowanmiller commented Feb 2, 2016

@julielerman and rename your repo to EFCore 😉

BTW our nightly builds are pretty unstable at the moment due to moving over to .NET CLI... so you may want to hold off a couple of weeks.

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters May 31, 2016

So right now I am adding a Project entity (a new one) that has a list of Folder entities (also new). Each folder points to a FolderType (existing). For it to work, I have to attach all the existing folder types to the context before saving, or it tries to insert new folder types. What is the correct approach when part of the graph (root and some bits) are new, and part references existing objects? I was hoping for one method that would look at the IDs <= 0 to infer whether that object was new or existing..

So right now I am adding a Project entity (a new one) that has a list of Folder entities (also new). Each folder points to a FolderType (existing). For it to work, I have to attach all the existing folder types to the context before saving, or it tries to insert new folder types. What is the correct approach when part of the graph (root and some bits) are new, and part references existing objects? I was hoping for one method that would look at the IDs <= 0 to infer whether that object was new or existing..

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega May 31, 2016

Member

I was hoping for one method that would look at the IDs <= 0 to infer whether that object was new or existing

It doesn't do that automatically because that would only work if you have setup database generated keys, which is only a subset of the scenarios EF supports. If you know you did that in your entities you should be able to use TrackGraph() passing a lambda that will pick the right state for each of the entities based on the value of its key.

Member

divega commented May 31, 2016

I was hoping for one method that would look at the IDs <= 0 to infer whether that object was new or existing

It doesn't do that automatically because that would only work if you have setup database generated keys, which is only a subset of the scenarios EF supports. If you know you did that in your entities you should be able to use TrackGraph() passing a lambda that will pick the right state for each of the entities based on the value of its key.

@aflx

This comment has been minimized.

Show comment
Hide comment
@aflx

aflx Jun 1, 2016

I'm a little confused now. Until today (I upgraded to the latest build), I just used Add and everything worked like a charm ;)

Can someone give an example for the following scenario? How to realize this?

public class A {
  public int Id { get; set;} 
  public virtual B ObjectB { get; set;}
  public virtual C ObjectC { get; set;}
}

public class B {
   public int Id { get; set;} 
}

public class C {
   public int Id { get; set;} 
}

A a = new A();
a.ObjectB = b; //b has been fetched from the database in a using statement => no global context
a.ObjectC = new ObjectC();

using (MyContext context = new MyContext())
{
 //What to use, if I don't want to add every new child (C) manually and there are references to an existing object (B) too
  context.A.Add(a); 
  context.SaveChanges();
}

aflx commented Jun 1, 2016

I'm a little confused now. Until today (I upgraded to the latest build), I just used Add and everything worked like a charm ;)

Can someone give an example for the following scenario? How to realize this?

public class A {
  public int Id { get; set;} 
  public virtual B ObjectB { get; set;}
  public virtual C ObjectC { get; set;}
}

public class B {
   public int Id { get; set;} 
}

public class C {
   public int Id { get; set;} 
}

A a = new A();
a.ObjectB = b; //b has been fetched from the database in a using statement => no global context
a.ObjectC = new ObjectC();

using (MyContext context = new MyContext())
{
 //What to use, if I don't want to add every new child (C) manually and there are references to an existing object (B) too
  context.A.Add(a); 
  context.SaveChanges();
}
@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Jun 1, 2016

I know you asked Diego, but if I may jump in as this is a topic near & dear to my heart...

First of all, the problem would go away if you would just use FKs .

public class A {
  public int Id { get; set;} 
  public virtual B ObjectB { get; set;}
  public int BId {get;set}
  public virtual C ObjectC { get; set;}
  public int CId {get;set}
}

Then you could set the relationships like:

A a = new A();
a.BId=b.Id; <existing object ..this is the one that is causing you trouble :)
a.ObjectC = new ObjectC();

And context.Add(a) would work perfectly! :)

Because it's a new context and unaware of the existing objects, Add (update delete and attach) will affect the entire graph with whatever state you imply with the method.

As of EFCore, you can use the DbContext.Entry(). State method e.g.

context.Entry(a).State=EntityState.Added

to apply that state only to the entity that you specified and it will ignore anything else that is attached. That is new behavior for Entry and gives us a great opportunity to have a consistent pattern. This is good in your example when you know for 100% sure that you only want to affect "a" and that you 100% surely kow that you don't want b & c to be updated. But that is not your scenario, so Entry doesn't 'work either because you want to skip B but you DO want to add C as well.

If your method does not have that confidence then you need to choose a pattern to employ. There is no magic. If you know that in your system a pre-existing key value ("Id" in your case) indicates that an entity should not get dragged into the update, then you can iterate through and check for Id =0 vs Id has a value >0. (or whatever your rule is). The new TrackGraph method gives you a chance to do this.
context.ChangeTracker.TrackGraph(a, a=>[somefunction to perform])

TrackGraph iterates through every item in the graph and performs the specified function on it.

I have an example of using TrackGraph in my RC2 sample on Github. (https://github.com/julielerman/EFCore-RC2-Demo). But I'm not just using a "if id=0, added, if not unchanged" rule. I'm using trackgraph in combination with an old pattern that I wrote about yet again in a recent MSDN Mag article: https://msdn.microsoft.com/magazine/mt694083 (Rowan & I wrote about it in our DbContext book, too and that was back with EF 4.1 :) )

Look at the weathercontroller FixSeasonInReactions method. It uses trackgraph to update state based on a known state flag applied to each of my classes.

Hope this helps and sorry if you wanted to get an answer straight from Diego because that would represent the guidance directly from the team.

julielerman commented Jun 1, 2016

I know you asked Diego, but if I may jump in as this is a topic near & dear to my heart...

First of all, the problem would go away if you would just use FKs .

public class A {
  public int Id { get; set;} 
  public virtual B ObjectB { get; set;}
  public int BId {get;set}
  public virtual C ObjectC { get; set;}
  public int CId {get;set}
}

Then you could set the relationships like:

A a = new A();
a.BId=b.Id; <existing object ..this is the one that is causing you trouble :)
a.ObjectC = new ObjectC();

And context.Add(a) would work perfectly! :)

Because it's a new context and unaware of the existing objects, Add (update delete and attach) will affect the entire graph with whatever state you imply with the method.

As of EFCore, you can use the DbContext.Entry(). State method e.g.

context.Entry(a).State=EntityState.Added

to apply that state only to the entity that you specified and it will ignore anything else that is attached. That is new behavior for Entry and gives us a great opportunity to have a consistent pattern. This is good in your example when you know for 100% sure that you only want to affect "a" and that you 100% surely kow that you don't want b & c to be updated. But that is not your scenario, so Entry doesn't 'work either because you want to skip B but you DO want to add C as well.

If your method does not have that confidence then you need to choose a pattern to employ. There is no magic. If you know that in your system a pre-existing key value ("Id" in your case) indicates that an entity should not get dragged into the update, then you can iterate through and check for Id =0 vs Id has a value >0. (or whatever your rule is). The new TrackGraph method gives you a chance to do this.
context.ChangeTracker.TrackGraph(a, a=>[somefunction to perform])

TrackGraph iterates through every item in the graph and performs the specified function on it.

I have an example of using TrackGraph in my RC2 sample on Github. (https://github.com/julielerman/EFCore-RC2-Demo). But I'm not just using a "if id=0, added, if not unchanged" rule. I'm using trackgraph in combination with an old pattern that I wrote about yet again in a recent MSDN Mag article: https://msdn.microsoft.com/magazine/mt694083 (Rowan & I wrote about it in our DbContext book, too and that was back with EF 4.1 :) )

Look at the weathercontroller FixSeasonInReactions method. It uses trackgraph to update state based on a known state flag applied to each of my classes.

Hope this helps and sorry if you wanted to get an answer straight from Diego because that would represent the guidance directly from the team.

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Jun 1, 2016

Member

@aflx I am having a little trouble understanding everything Julie said, but given the code you posted I think you just need two steps:

  1. Call context.Attach for all objects that already existed in the database -- so for b in your code.
  2. Call context.Add for all new objects - for a and c in your code.

Attach tells EF that the entity already exists in the database, and sets the state of the entity to Unchanged. Add tells EF that the entity is new and should be inserted into the database, and so sets the state to Added.

Since Attach and Add both operate on the graph of reachable objects you may not need to call them for every single object in your graph. For example, in the code you wrote you can Attach b and then Add a, and since c is reachable from a it will also get added.

If you want to automatically decide whether the entity should be Added or Attached based on key value, then you can use TrackGraph as Diego suggested. But this may be overkill.

Member

ajcvickers commented Jun 1, 2016

@aflx I am having a little trouble understanding everything Julie said, but given the code you posted I think you just need two steps:

  1. Call context.Attach for all objects that already existed in the database -- so for b in your code.
  2. Call context.Add for all new objects - for a and c in your code.

Attach tells EF that the entity already exists in the database, and sets the state of the entity to Unchanged. Add tells EF that the entity is new and should be inserted into the database, and so sets the state to Added.

Since Attach and Add both operate on the graph of reachable objects you may not need to call them for every single object in your graph. For example, in the code you wrote you can Attach b and then Add a, and since c is reachable from a it will also get added.

If you want to automatically decide whether the entity should be Added or Attached based on key value, then you can use TrackGraph as Diego suggested. But this may be overkill.

@aflx

This comment has been minimized.

Show comment
Hide comment
@aflx

aflx Jun 1, 2016

@julielerman Thank you! (I removed Diego 10 seconds after publishing the comment...so don't worry ;)

I already tried it with just setting the Id...but no chance...

public int Add<T>(T entity) where T : class
{
    using (MyContext context = new MyContext())
    {
        DbSet<T> dbSet = context.Set<T>();
        dbSet.Add(entity); // Here the object reference gets loaded by Id
        context.SaveChanges(); // And here I get the exception (unique constraint...)
    }

    return (entity as BaseObject).Id;
}

I tried the easy example with TrackGraph (checking for Id), but it seems that I'm doing something wrong, because the lambda function is just called once for the parent (A)

public int Add<T>(T entity) where T : class
{
    using (TraxContext context = new TraxContext())
    {
        DbSet<T> dbSet = context.Set<T>();

        context.ChangeTracker.TrackGraph(entity, e => {
            if ((e.Entry.Entity as BaseObject).Id > 0) e.Entry.State = EntityState.Unchanged;
        });

        dbSet.Add(entity);
        context.SaveChanges();
    }

    return (entity as BaseObject).Id;
}

@ajcvickers Thx. But I don't like to use attach for b. I think, this only makes sense if you have something like a global context. Otherwise you always have to keep in mind storing and attaching existing objects. For a larger solution with complex objects and relationships this will be hard.
Everything was fine with the "old" behavior for Add for me (I liked the magic) :D

And maybe I have a total wrong concept of MVVM in mind.
I have a ViewModel for every View and every ViewModel uses a service which wraps every db-action in using statements... That means, that I don't even a local context for a view. Maybe I should change that...

aflx commented Jun 1, 2016

@julielerman Thank you! (I removed Diego 10 seconds after publishing the comment...so don't worry ;)

I already tried it with just setting the Id...but no chance...

public int Add<T>(T entity) where T : class
{
    using (MyContext context = new MyContext())
    {
        DbSet<T> dbSet = context.Set<T>();
        dbSet.Add(entity); // Here the object reference gets loaded by Id
        context.SaveChanges(); // And here I get the exception (unique constraint...)
    }

    return (entity as BaseObject).Id;
}

I tried the easy example with TrackGraph (checking for Id), but it seems that I'm doing something wrong, because the lambda function is just called once for the parent (A)

public int Add<T>(T entity) where T : class
{
    using (TraxContext context = new TraxContext())
    {
        DbSet<T> dbSet = context.Set<T>();

        context.ChangeTracker.TrackGraph(entity, e => {
            if ((e.Entry.Entity as BaseObject).Id > 0) e.Entry.State = EntityState.Unchanged;
        });

        dbSet.Add(entity);
        context.SaveChanges();
    }

    return (entity as BaseObject).Id;
}

@ajcvickers Thx. But I don't like to use attach for b. I think, this only makes sense if you have something like a global context. Otherwise you always have to keep in mind storing and attaching existing objects. For a larger solution with complex objects and relationships this will be hard.
Everything was fine with the "old" behavior for Add for me (I liked the magic) :D

And maybe I have a total wrong concept of MVVM in mind.
I have a ViewModel for every View and every ViewModel uses a service which wraps every db-action in using statements... That means, that I don't even a local context for a view. Maybe I should change that...

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 1, 2016

There are certainly some interesting changes here! I found another one today... I was adding a new Project object and it has a Client property, that may be an existing Client or a new One. For new ones, we had set the Id to -1. Client and Project have IDENTITY columns. I believe this used to cause it to be Inserted, but omitting the ID. But NOW, it tries to insert the -1! This of cause fails. To further complicate matters, 0 is a valid ID values. So now I need the client to specify a negative ID to mean add, then I need to CHANGE that Id to 0 before saving it!
I think these changes are geared toward making things more explicit and less automagic, and the TrackChanges is a nice place to put your 'business logic' for IDs, but a lot of people will be confused, so some careful documentation will be needed!

There are certainly some interesting changes here! I found another one today... I was adding a new Project object and it has a Client property, that may be an existing Client or a new One. For new ones, we had set the Id to -1. Client and Project have IDENTITY columns. I believe this used to cause it to be Inserted, but omitting the ID. But NOW, it tries to insert the -1! This of cause fails. To further complicate matters, 0 is a valid ID values. So now I need the client to specify a negative ID to mean add, then I need to CHANGE that Id to 0 before saving it!
I think these changes are geared toward making things more explicit and less automagic, and the TrackChanges is a nice place to put your 'business logic' for IDs, but a lot of people will be confused, so some careful documentation will be needed!

@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Jun 1, 2016

oh no! If what I suggested was so convoluted that @ajcvickers didn't grok it, that makes me nervous. Maybe we should chat offline about this Arthur because I want to make sure I'm not inventing anti-patterns.

@aflx Add works the same as it did in EF6 and earlier. I wouldn't combine TrackGraph and dbset.add. Use one of the other. The result of what you are doing is not much different than Arthur's suggestion. You are having the changetracker essentially call Attach on the object that has a value in id. Then you are calling Add on the dbset. The trackgraph is itereating through all of the objects. That's its job. Your code is telling it to ignore A & C. Make it say (this is not real syntax) e.entry.state= if (id>0) Unchanged else Added. Get rid of dbset.add.

oh no! If what I suggested was so convoluted that @ajcvickers didn't grok it, that makes me nervous. Maybe we should chat offline about this Arthur because I want to make sure I'm not inventing anti-patterns.

@aflx Add works the same as it did in EF6 and earlier. I wouldn't combine TrackGraph and dbset.add. Use one of the other. The result of what you are doing is not much different than Arthur's suggestion. You are having the changetracker essentially call Attach on the object that has a value in id. Then you are calling Add on the dbset. The trackgraph is itereating through all of the objects. That's its job. Your code is telling it to ignore A & C. Make it say (this is not real syntax) e.entry.state= if (id>0) Unchanged else Added. Get rid of dbset.add.

@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Jun 1, 2016

@johnkwaters

think these changes are geared toward making things more explicit and less automagic,
Totally agreed! That's how I've been explaining it.

TrackChanges is a nice place to put your 'business logic' for IDs,
also agreed

but a lot of people will be confused, so some careful documentation will be needed!

This is where "this is not EF6" comes in. People are going to see familiar methods and presume they will act the same as they always did.

@johnkwaters

think these changes are geared toward making things more explicit and less automagic,
Totally agreed! That's how I've been explaining it.

TrackChanges is a nice place to put your 'business logic' for IDs,
also agreed

but a lot of people will be confused, so some careful documentation will be needed!

This is where "this is not EF6" comes in. People are going to see familiar methods and presume they will act the same as they always did.

@aflx

This comment has been minimized.

Show comment
Hide comment
@aflx

aflx Jun 1, 2016

@julielerman Ok, with TrackGraph and without Add => works ;) Thank you for this hint.

using (TraxContext context = new TraxContext())
{
    DbSet<T> dbSet = context.Set<T>();

    context.ChangeTracker.TrackGraph(entity, e => {
        if ((e.Entry.Entity as BaseObject).Id > 0)
        {
            e.Entry.State = EntityState.Unchanged;
        }
        else
        {
            e.Entry.State = EntityState.Added;
        }
    });

    context.SaveChanges();
}

aflx commented Jun 1, 2016

@julielerman Ok, with TrackGraph and without Add => works ;) Thank you for this hint.

using (TraxContext context = new TraxContext())
{
    DbSet<T> dbSet = context.Set<T>();

    context.ChangeTracker.TrackGraph(entity, e => {
        if ((e.Entry.Entity as BaseObject).Id > 0)
        {
            e.Entry.State = EntityState.Unchanged;
        }
        else
        {
            e.Entry.State = EntityState.Added;
        }
    });

    context.SaveChanges();
}
@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 1, 2016

Funny, I came up with the same code.

Funny, I came up with the same code.

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Jun 2, 2016

Member

You shouldn't need a base entity class. Try e.Entry.IsKeySet.

Member

divega commented Jun 2, 2016

You shouldn't need a base entity class. Try e.Entry.IsKeySet.

@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Jun 2, 2016

IsKeySet? Oh what a great property!! I need to start iterating through the APIs to see what gems I've missed!! Which reminds me there was a graphnode property who's purpose I have to figure out..

julielerman commented Jun 2, 2016

IsKeySet? Oh what a great property!! I need to start iterating through the APIs to see what gems I've missed!! Which reminds me there was a graphnode property who's purpose I have to figure out..

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 2, 2016

But don't you still need to examine the value of the key? How does IsKeySet help with that?

But don't you still need to examine the value of the key? How does IsKeySet help with that?

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 2, 2016

Wouldnt it be cool if when specifying a key, you could give a lambda specifying what constitutes a 'new' value, for instances SetKey( e => e.Id, () => e.Id <=0 ). Then the fwk could go back to doing the heavy lifting in a generic way... and with some good defaults, you could even not need to do this for traditional patterns (Guid.Empty, Int<0 etc).

johnkwaters commented Jun 2, 2016

Wouldnt it be cool if when specifying a key, you could give a lambda specifying what constitutes a 'new' value, for instances SetKey( e => e.Id, () => e.Id <=0 ). Then the fwk could go back to doing the heavy lifting in a generic way... and with some good defaults, you could even not need to do this for traditional patterns (Guid.Empty, Int<0 etc).

@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Jun 2, 2016

@johnkwaters did you check the code base? It checks to make sure that the key is not at the default value. That's pretty good coverage. And it's virtual so perhaps you can override it ... though I haven't tried it out yet.

public virtual bool IsKeySet => !EntityType.FindPrimaryKey().Properties.Any(p => p.ClrType.IsDefaultValue(this[p]));

julielerman commented Jun 2, 2016

@johnkwaters did you check the code base? It checks to make sure that the key is not at the default value. That's pretty good coverage. And it's virtual so perhaps you can override it ... though I haven't tried it out yet.

public virtual bool IsKeySet => !EntityType.FindPrimaryKey().Properties.Any(p => p.ClrType.IsDefaultValue(this[p]));

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 2, 2016

Awesome. No, I didnt check the codebase... next time I will! Right now I am working on a DB that was ported over from Access to SQL server, and sadly they use 0 as a valid key value for all kinds of tables, so having it be negative is the lambda I would need, or the override.

Awesome. No, I didnt check the codebase... next time I will! Right now I am working on a DB that was ported over from Access to SQL server, and sadly they use 0 as a valid key value for all kinds of tables, so having it be negative is the lambda I would need, or the override.

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 2, 2016

@julielerman @divega @ajcvickers And I must say it is such a treat to be able to discuss and possibly influence the evolution of EF7 as it is crafted! I am using it in several production projects, due to ship in the August time frame (!!), so having this dialog definately makes me feel good about my bet on .net core.

@julielerman @divega @ajcvickers And I must say it is such a treat to be able to discuss and possibly influence the evolution of EF7 as it is crafted! I am using it in several production projects, due to ship in the August time frame (!!), so having this dialog definately makes me feel good about my bet on .net core.

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 2, 2016

image
So I am passing this root object to track changes. It is NOT iterating over any of the referenced objects or any of the child object lists.
The relationships ARE spelled out in the fluent config.
Do I need to use virtual for this to work?

image
So I am passing this root object to track changes. It is NOT iterating over any of the referenced objects or any of the child object lists.
The relationships ARE spelled out in the fluent config.
Do I need to use virtual for this to work?

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Jun 2, 2016

Member

and sadly they use 0 as a valid key value for all kinds of tables

If zero is a valid key you can change the type of the key to be nullable, e.g. int? instead of int. Then IsKeySet will return true for zero and false for null.

So I am passing this root object to track changes. It is NOT iterating over any of the referenced objects or any of the child object lists.
The relationships ARE spelled out in the fluent config.
Do I need to use virtual for this to work?

Not sure what the problem could be. You don't need to make the navigation properties virtual, in fact virtual doesn't make any difference with EF Core. Could you please create a new issue with a self-contained repro of this issue?

Member

divega commented Jun 2, 2016

and sadly they use 0 as a valid key value for all kinds of tables

If zero is a valid key you can change the type of the key to be nullable, e.g. int? instead of int. Then IsKeySet will return true for zero and false for null.

So I am passing this root object to track changes. It is NOT iterating over any of the referenced objects or any of the child object lists.
The relationships ARE spelled out in the fluent config.
Do I need to use virtual for this to work?

Not sure what the problem could be. You don't need to make the navigation properties virtual, in fact virtual doesn't make any difference with EF Core. Could you please create a new issue with a self-contained repro of this issue?

@johnkwaters

This comment has been minimized.

Show comment
Hide comment
@johnkwaters

johnkwaters Jun 2, 2016

But then wouldn't I need a nullable pk in the db?

Sent from my iPhone

On Jun 2, 2016, at 2:23 PM, Diego Vega notifications@github.com wrote:

and sadly they use 0 as a valid key value for all kinds of tables

If zero is a valid key you can change the type of the key to be nullable, e.g. int? instead of int. Then IsKeySet will return true for zero and false for null.

So I am passing this root object to track changes. It is NOT iterating over any of the referenced objects or any of the child object lists.
The relationships ARE spelled out in the fluent config.
Do I need to use virtual for this to work?

Not sure what the problem could be. You don't need to make the navigation properties virtual, in fact virtual doesn't make any difference with EF Core. Could you please create a new issue with a self-contained repro of this issue?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

But then wouldn't I need a nullable pk in the db?

Sent from my iPhone

On Jun 2, 2016, at 2:23 PM, Diego Vega notifications@github.com wrote:

and sadly they use 0 as a valid key value for all kinds of tables

If zero is a valid key you can change the type of the key to be nullable, e.g. int? instead of int. Then IsKeySet will return true for zero and false for null.

So I am passing this root object to track changes. It is NOT iterating over any of the referenced objects or any of the child object lists.
The relationships ARE spelled out in the fluent config.
Do I need to use virtual for this to work?

Not sure what the problem could be. You don't need to make the navigation properties virtual, in fact virtual doesn't make any difference with EF Core. Could you please create a new issue with a self-contained repro of this issue?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Jun 2, 2016

Member

No. The column will still be non-nullable as null is not a valid key value in a persisted object. It only means that the key can have a null value temporarily while it is in memory and before it is added to a DbContext.

Member

divega commented Jun 2, 2016

No. The column will still be non-nullable as null is not a valid key value in a persisted object. It only means that the key can have a null value temporarily while it is in memory and before it is added to a DbContext.

@julielerman

This comment has been minimized.

Show comment
Hide comment
@julielerman

julielerman Jun 21, 2016

@ajcvickers when Attaching a graph (no previously tracked entities) and child does NOT have it's key value filled, it becomes Added. When there is a a value in key, it becomes Unchanged. I did not expect this. Is Attach now reading key values to make state decisions? Did I miss that explanation somewhere?

edit: yeah okay I see that re-reading the original post in this thread....

julielerman commented Jun 21, 2016

@ajcvickers when Attaching a graph (no previously tracked entities) and child does NOT have it's key value filled, it becomes Added. When there is a a value in key, it becomes Unchanged. I did not expect this. Is Attach now reading key values to make state decisions? Did I miss that explanation somewhere?

edit: yeah okay I see that re-reading the original post in this thread....

@sjb-sjb

This comment has been minimized.

Show comment
Hide comment
@sjb-sjb

sjb-sjb Aug 13, 2016

It's great to follow this discussion, especially given how long these challenges have been kicking around.

I believe the TrackGraph pattern described by @julielerman may be superceded by the IsKeySet approach, i.e. simply use store-generated keys and then do one of the following: either
(a) Attach copies of the entities that already exist in the store ("b" in @aflx's example) with their primary keys set, and then Add the (root of the) new entities; or
(b) Refer to existing entities by putting their store-generated keys in the new entities, and then Add the (root of the) new entities.

@julielerman 's comment of June 21, and the description of Attach in @rowanmiller 's original posting, seem to say that in case (a) you could use Attach instead of Add for the new entities. The new entities' store-generated keys will not have been set and therefore the Attach will actually set the state to Added. In other words you can just construct the complete graph, including store-generated Id's in the case where the entity is a copy of an object already in the data store, and just Attach the whole thing. Or you can refer to the existing objects by Id instead of by reference, in which case you can either Add or Attach the graph.

Have I gotten that right? It would be great to have an update to @julielerman 's April MSDN article that takes into account the June discussion above.

sjb-sjb commented Aug 13, 2016

It's great to follow this discussion, especially given how long these challenges have been kicking around.

I believe the TrackGraph pattern described by @julielerman may be superceded by the IsKeySet approach, i.e. simply use store-generated keys and then do one of the following: either
(a) Attach copies of the entities that already exist in the store ("b" in @aflx's example) with their primary keys set, and then Add the (root of the) new entities; or
(b) Refer to existing entities by putting their store-generated keys in the new entities, and then Add the (root of the) new entities.

@julielerman 's comment of June 21, and the description of Attach in @rowanmiller 's original posting, seem to say that in case (a) you could use Attach instead of Add for the new entities. The new entities' store-generated keys will not have been set and therefore the Attach will actually set the state to Added. In other words you can just construct the complete graph, including store-generated Id's in the case where the entity is a copy of an object already in the data store, and just Attach the whole thing. Or you can refer to the existing objects by Id instead of by reference, in which case you can either Add or Attach the graph.

Have I gotten that right? It would be great to have an update to @julielerman 's April MSDN article that takes into account the June discussion above.

@ErikEJ

This comment has been minimized.

Show comment
Hide comment
@sjb-sjb

This comment has been minimized.

Show comment
Hide comment
@sjb-sjb

sjb-sjb Aug 15, 2016

Cool, thanks!

Sent from my iPhone

On Aug 13, 2016, at 2:07 AM, Erik Ejlskov Jensen notifications@github.com wrote:

@sjb-sjb did you see this https://msdn.microsoft.com/magazine/mt767693 ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

sjb-sjb commented Aug 15, 2016

Cool, thanks!

Sent from my iPhone

On Aug 13, 2016, at 2:07 AM, Erik Ejlskov Jensen notifications@github.com wrote:

@sjb-sjb did you see this https://msdn.microsoft.com/magazine/mt767693 ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment