Problem with InsertGraph Method

Mar 3, 2014 at 12:25 AM
Edited Mar 3, 2014 at 6:41 AM
Hello Le,

There seems to be a problem with your InsertGraph method which calls the DbSet's Add() method.

When you call the DbSet's Add() method, it will set the internal EntityState of all entities in the graph to Added. So far so good.
_dbSet.Add(entity); // Sets the internal EntityState to "Added" but leaves the inherited ObjectState to "Unchanged" since you have no sync method for a graph.
The issue comes when you call your UOW's SaveChanges() method. Your SyncObjectsStatePreCommit() method takes the inherited ObjectState property (which would be undefined at this point if you did not explicitly set it) and sets the internal EntityState back to Unchanged from intended Added state:
// State property is "Added" at this point.
dbEntityEntry.State = StateHelper.ConvertState(((IObjectState)dbEntityEntry.Entity).ObjectState);
// State property is set back to "Unchanged" as this point.
This causes no changes to take place when you call the UOW's SaveChanges() method.

Of course you can explicitly set the Entity.State to Added when creating each and every entity but that defeats the purpose of the DbSet's .Add() method IMO.

The goal would be to traverse the entity graph using reflection and set the ObjectState property for each entity to "Added". I'm working on the solution and will post once I'm confident with it.

I'd like your thoughts on this.
Mar 3, 2014 at 8:51 AM
Edited Mar 3, 2014 at 10:06 AM
Ok, so I was thinking about the situation a little more and here are my thoughts and explanation on this. Entity Framework 6.x Code First with POCO classes out of the box can do this with no errors or issues:
MyContext context = new MyContext();

Customer newCustomer = new Customer();
Order newOrder = new Order();

newCustomer.Orders.Add(newOrder);

context.Customers.Add(newCustomer);
context.SaveChanges();
The Add method here will automatically set the internal EntityState for each of these entities to "Added" so a SaveChanges() does an Insert of these entities.

If we try to do this using the generic repository/uow as such:
MyContext context = new MyContext();
UnitOfWork uow = new UnitOfWork(context);

Customer newCustomer = new Customer();
Order newOrder = new Order();

newCustomer.Orders.Add(newOrder);

uow.Repository<Customer>().InsertGraph(Customer);
uow.SaveChanges();
We will get no inserts into the database. The reason for this is quite simple as I explained in my first post. The InsertGraph() method of the generic repository simply calls the DbSet's Add() method for the specific type which again, sets the internal EntityState to "Added". So far so good. The problem is the inherited custom Entity.ObjectState property used to set state remains "Unchanged". (The states become out of sync).

Now when you go to call the uow.SaveChanges(); method, the SyncObjectsStatePreCommit() method is invoked which sets the internal EntityState to that of the inherited ObjectState (in which case is Unchanged). Essentially setting the "Added" entities back to "Unchanged" so no changes are committed to the database.

One could argue that this is by design because you have exposed a custom entity state property so you must use it each and every time. But looking at it from a feature viewpoint, this becomes something you can't do where the EF can right out of the box and this to me requires a solution.

Here are the solutions I've come up with. Some more elegant than others.

1) Always set ObjectState to "Added" for all entities before calling the Repository.InsertGraph(); method. Not an ideal solution IMO:
Customer newCustomer = new Customer { ObjectState = Added };
Order newOrder = new Order { ObjectState = Added };
newCustomer.Orders(newOrder);
uow.Repository<Customer>().InsertGraph(newCustomer);
2) Add a "Create()" method to the Repository or Service classes which creates the new entity and assign the ObjectState to Added and NEVER instantiate a new entity outside of your repository or service layers. Adding a Create() method to the Generic Repository:
// Allows you to create a new "Added" entity using Repository<Entity>().Create();
public virtual TEntity Create()
{
      object o = _dbSet.Create();
      ((IObjectState)o).ObjectState = ObjectState.Added;
      return (TEntity)o;
}
3) Traverse the object graph and set all ObjectState properties to "Added". Not a clean solution, uses reflection and requires an additional flag to avoid recursive cycles. A possible solution here. This must be further debugged/improved. This method would go in the Repository class:
// Modified InsertGraph() method to traverse the object graph and set each ObjectState to "Added"
public virtual void InsertGraph(TEntity entity)
{
    MarkGraphAdded(entity);
    _dbSet.Add(entity);
}

// Recursive method to traverse an object graph and set each Entity type's ObjectState to "Added"
internal void MarkGraphAdded(object entity)
{
    ((IObjectState)entity).Visited = true;
    ((IObjectState)entity).ObjectState = ObjectState.Added;
    foreach (System.Reflection.PropertyInfo pi in entity.GetType().GetProperties())
    {
        if (pi.PropertyType.IsGenericType && pi.PropertyType.GetGenericTypeDefinition() == typeof(ICollection<>))
        {
            Type collectionType = pi.PropertyType.GetGenericArguments()[0];
            if (collectionType.BaseType == typeof(Entity))
            {
                object collection = pi.GetValue(entity, null);
                int collectionCount = (int)collection.GetType().GetProperty("Count").GetValue(collection, null);
                for (int i = 0; i < collectionCount; i++)
                {
                    object ent = collection.GetType().GetMethod("get_Item").Invoke(collection, new object[] { i });
                    if (ent != null && !((IObjectState)ent).Visited)
                    {
                        MarkGraphAdded(ent);
                    }
                }
            }
        }
        else if (pi.PropertyType.BaseType == typeof(Entity))
        {
            object ent = pi.GetValue(entity, null);
            if (ent != null && !((IObjectState)ent).Visited)
                MarkGraphAdded(ent);
        }
    }
}
4) ?
Coordinator
Mar 3, 2014 at 3:09 PM
NarbehM,

Yes a requirement of the Framework is that you must set the IObjectState for all objects in the entity graph. We are working with integrating with Trackable Entities to get around this however due to the are release cycles and bandwidth this is not complete. You are more than welcome to get this integration working if you absolutely not wanting to set the IObjectState of the objects in a graph.

https://genericunitofworkandrepositories.codeplex.com/SourceControl/latest#concepts/Trackable Entities/
https://trackable.codeplex.com/SourceControl/network/forks/lelong37/UnitofWorkAndGenericRepositories

You can also contact Tony Sneed from the Trackable Entities Team.
Marked as answer by lelong37 on 3/31/2014 at 10:33 PM
Mar 3, 2014 at 6:18 PM
Edited Mar 3, 2014 at 6:34 PM
Lelong,

Thanks for your reply and work on this great project. I think what I pointed out goes beyond the lack support for Trackable Entities, let me explain. First of all, I assume the reason you implemented an explicit user defined state is to support n-Tier environments with a lack of a context correct? EF out of the box tracks entities as long as they are connected to a context as you may already know.

I think the decision for Trackable vs Non-Trackable is a big enough decision to warrant two separate distinct frameworks. The current implementation you have doesn't give the developer a clear enough understanding of the approach you took for user trackability. I think the framework should either completely handle state or not handle it at all. Your current implementation of the framework is ambiguous when it comes to state, Let me give you an example of what I mean:
MyContext context = new MyContext();
UnitOfWork uow = new UnitOfWork(context);

Customer newCustomer = new Customer();

// This will successfully insert a record even though we have not explicitly set state
uow.Repository<Customer>().Insert(newCustomer);

// This will not insert the record because we have not explicitly set state
uow.Repository<Customer>().InsertGraph(newCustomer);
Both of those insert methods should be consistent but they are not. This is because you are implicitly setting the state in the Insert() method but not in the InsertGraph() method:
        public virtual void Insert(TEntity entity)
        {
            ((IObjectState) entity).ObjectState = ObjectState.Added;
            _dbSet.Attach(entity);
            _context.SyncObjectState(entity);
        }

        public virtual void InsertGraph(TEntity entity)
        {
            _dbSet.Add(entity);
        }
Furthermore, both Insert and InsertGraph do essentially the same thing here. Going from the MSDN documentation, DbSet.Attach() attaches entitie/graphs as "Unchanged" and DbSet.Add() attaches entitie/graphs as "Added". So Your Insert() method is basically doing what the InsertGraph method does only with implicit state setting. One can also use the Insert() method for graphs further causing ambiguity.

I would think about changing your approach here. I would completely remove the implicit state setting from the Repository and let the user handle all state. At least the user will know they HAVE to set ObjectState explicitly each and every time. You could also implement a method to set the ObjectState to "Added" for a graph (look at my implementation above). Or offer two solutions, one for n-Tier projects (with self tracking) and one that lets EF handle state out of the box, removing the framework dependency on the POCO classes. Another option is to let the user have the choice to use trackable vs non-trackable in one solution.

You could also leave the approach almost as is and offer an option to let the user decide whether to explicitly set state or let EF handle it for them. I did this by simply adding a "Default" enum to the ObjectState enum and skipping the state syncing operations of the SaveChange() method:
    public enum ObjectState
    {
        Default,
        Unchanged,
        Added,
        Modified,
        Deleted
    }

        public void SyncObjectState(object entity)
        {
            ObjectState objectState = ((IObjectState)entity).ObjectState;
            if (objectState != ObjectState.Default)
                Entry(entity).State = StateHelper.ConvertState(objectState);
        }

        private void SyncObjectsStatePreCommit()
        {
            foreach (var dbEntityEntry in ChangeTracker.Entries())
            {
                SyncObjectState(dbEntityEntry.Entity);
            }
        }

        public void SyncObjectsStatePostCommit()
        {
            foreach (var dbEntityEntry in ChangeTracker.Entries())
            {
                IObjectState entity = (IObjectState)dbEntityEntry.Entity;
                if (entity.ObjectState != ObjectState.Default)
                    entity.ObjectState = StateHelper.ConvertState(dbEntityEntry.State);
            }
        }
This sort of gives you best of both worlds where by default, state is handled by EF unless you set ObjectState to something other than "Default". This also solves the problem of Insert() and InsertGraph() not consistently setting ObjectState as long as ObjectState is set to Default.
Coordinator
Mar 3, 2014 at 6:33 PM
Edited Mar 3, 2014 at 6:37 PM
Thanks for the positive feedback.

First and foremost the framework nor I has ever stated that it will handle state (IObjectState), it is a requirement when using the framework that you explicitly set IObjectState whether you are passing in a graph or not. Now, if one reviews the Insert, Update, Delete implementations from Repostiory.cs and derives, that the states are being set and that there is not a need to do so, and only when passing in graphs when they should, I can't help that (welcome to the wonderful world of open source). This was put in place as as convenience, however I'll officially reinforce it is a requirement to always set the IObjectState for all entities/objects that are being passed into the Framework (graph or not). I could be at fault here for not making this clear, if this is the case my apologies. This is because of the different ORM providers (e.g. nHibernate, etc.) that are in plans to be brought online with the framework.

This pattern was leveraged from Julie Lerman in handling states with Repositories and Unit of Work (while connected and disconnected from the DbContext).

You are also correct, in terms of what DbSet.Add and DbSet.Attach each do, we could have used DbSet.Attach for the InserGraph vs. versa Insert and effectively achieved the same end results, because in the end the IObjectState is the ultimate state that will get set with EF from the Framework. Meaning it doesn't matter if we use DbSet.Add/Attach, the IObjectState will be the final state that will be synced with EF's DbEntry State.
Mar 3, 2014 at 8:13 PM
Thanks for the explanation. I think it makes sense and I understand that you're setting the state for convenience purposes. I can tell you that as an experienced developer first getting started with the framework, this bit of detail caused confusion until I tracked down the cause. For consistency,, both insert methods should set state equally.

I understand why you chose to leave it out for the InsertGraph() method, The solution to set state in a graph requires reflection, increases footprint and complexity.

When you say you'll enforce it, do you mean in documentation or implementation? You could easily default each IObjectState entity to a new "NetSet" enum and throw an exception in the SyncObjectsStatePreCommit() method for an entity that state has not been set. This will ensure that all entities have a valid state before the commit.

Thanks again.
Coordinator
Mar 3, 2014 at 8:32 PM
Edited Mar 3, 2014 at 8:33 PM
Thanks Narbeh, you've made some very valid points, I think your absolutely right, I'll go ahead an plan (add to the backlog) to enforce setting the IObjectState through implementation and documentation, again thanks.
Mar 4, 2014 at 12:06 AM
Thanks for listening to my suggestions! I'm glad to be able to contribute to making this framework even better in future releases. Keep up the good work.
Sep 16, 2014 at 2:46 PM
Edited Sep 16, 2014 at 2:47 PM
Why the function to Insert uses Attach and not Add?
public virtual void Insert(TEntity entity)
{
            entity.ObjectState = ObjectState.Added;
            _dbSet.Attach(entity); // I think it should be _dbSet.Add(entity)
            _context.SyncObjectState(entity);
}