SyncObjectGraph Working as Expected?

Aug 23, 2014 at 4:31 AM
Hi Le et al,

I’m experiencing something a little unexpected. I have been playing around with the InsertOrUpdateGraph method which calls the SyncObjectGraph method. In that method, the line of text:
var items = prop.GetValue(entity, null) as IList<IObjectState>;
always returns null (assigns null to items). I was sure to make my entities inherit from Entity and I also made collection properties IList<IObjectState> (instead of ICollection).

I put together a very simple Console application which gets straight to business - Demo

Is this a known issue and can be replicated (by someone other than me)?

Cheers
Coordinator
Aug 23, 2014 at 6:50 AM
Did this fix the issue?

Aug 23, 2014 at 11:39 PM
Sorry Le, I was a bit ambiguous in my first post. The Demo is replicating the "odd" behaviour. I haven't logged an Issue as I am not sure its a bug yet.

I cannot, for the life of me, see why that would return null where the property in question is an IList<T> (where T is an Entity).

I have been looking into this a bit more and it is a bit of an odd one.

I took a look at the CreateAndUpdateAndDeleteCustomerGraphTest in your tests project and the behaviour was not replicated there. In that case, an array was being assigned to the collection navigation property. So, I changed the code in my demo application from
software.Licences.Add(licence1);
software.Licences.Add(licence2);

to

var licences = new Licence[] { licence1, licence2 };
software.Licences = licences;

and the behaviour went away (i.e. the value for the Licences Navigation Property was not null, as expected).
I also found that if I changed the line in question to the following, it seemed to behave as expected:
var items = prop.GetValue(entity, null) as IEnumerable<IObjectState>;

It's definitely a bit of a puzzling one. Do you think it’s worth formally opening an issue regarding this?
I think it would be good if another community member tried to reproduce the issue.
Coordinator
Sep 12, 2014 at 10:18 PM
hi dave, noticed you have multiple discussion/topics out, if you could be so kind to do a pull request, so that we can review these with better clarity, that would be helpful.
Marked as answer by lelong37 on 9/12/2014 at 3:18 PM
Sep 16, 2014 at 4:16 PM
This is happening to me as well. I am declaring my navigation properties as "virtual" and of type "ICollection<T> where T : Entity" so that entity framework uses HashSet<T> as the ICollection's implementation. This makes it so that the cast for "IList<IObjectState>" returns a null value because ICollection cannot be cast to IList.
However, since the HashSet class also doesn't implement the IList interface, that cast will always return null.

Should this particular line of code also be taking into account the possibility of navigation properties that implement ICollection<T> instead of IList<T>?
Dec 18, 2014 at 6:01 PM
I am noticing this as well. It came up in digging into issues InsertOrUpdateGraph (I made another post about this). Changing it to ICollection<T> does not work but ICollection works as expected. I think the reason this is happening is you can not cast a Collection to a Collection of a base class directly:

http://stackoverflow.com/questions/1817300/convert-listderivedclass-to-listbaseclass
Dec 18, 2014 at 10:27 PM
Hi Guys,
I've submitted a pull request which hopefully addresses this issue.
Cheers
Dec 19, 2014 at 12:33 PM
Dave,

That worked great so thanks! I wonder why prop.GetValue(entity, null) as IEnumerable<IObjectState>; is fine but prop.GetValue(entity, null) as ICollection<IObjectState>; was null even though my entity collections where ICollection.

Also do you know how SyncObjectGraph was intended to function? In the code it is commented as "// Set tracking state for child collections" but unless I am missing something, all it is doing is going through all the properties of the entity passed in and if the value of that property is an entity with an ObjectState of Added, it calls Attach with the main entity and calls SyncObjectState for that main entity. I guess I do not understand that point of doing the same thing over and over each time that condition is met.

In addition to that we are having issues with the InsertOrUpdateGraph in general that I have outlined in another discussion here:

http://genericunitofworkandrepositories.codeplex.com/discussions/575741

It looks like the issues we are having where related to the call(s) to Attach on the main entity prior to the call to Add. It has always been my understanding that Attach was only used if the entity was know to exist in the database.

http://msdn.microsoft.com/en-us/library/system.data.entity.dbset.attach%28v=vs.113%29.aspx
Attach is used to repopulate a context with an entity that is known to already exist in the database
It looks like attach with a state of Added is used for Insert as well so perhaps this has changed or was not really valid information to start with.

Either way for the time being I have had to comment out the call to SyncObjectGraph entirely leaving only the call to Add.

Thanks
Dec 19, 2014 at 9:46 PM
Hi jlindbo,

As I understand it (and LeLong can correct me on this if I am wrong), SyncObjectGraph is employing a technique which Julie Lerman calls painting the state. The code in that SO answer is her way of doing it (there's more than 1 way to skin a cat).

Painting the state addresses the problem of disconnected entities. EF works beautifully and is simple if it runs in the 1 process the whole time. But in most applications, DTOs (domain objects) are going out of process when serialized to the client and deserialized when the come back (e.g. by the model-binders in MVC). So, we need to maintain the state and note how that state changes. We need to "replay changes" at the server.

Apologies if I'm explaining something you already know. But I believe SyncObjectGraph is part of that process.

Cheers
Coordinator
Dec 20, 2014 at 12:09 AM
Edited Dec 20, 2014 at 12:14 AM
daverogers wrote:
Hi jlindbo,

As I understand it (and LeLong can correct me on this if I am wrong), SyncObjectGraph is employing a technique which Julie Lerman calls painting the state. The code in that SO answer is her way of doing it (there's more than 1 way to skin a cat).

Painting the state addresses the problem of disconnected entities. EF works beautifully and is simple if it runs in the 1 process the whole time. But in most applications, DTOs (domain objects) are going out of process when serialized to the client and deserialized when the come back (e.g. by the model-binders in MVC). So, we need to maintain the state and note how that state changes. We need to "replay changes" at the server.

Apologies if I'm explaining something you already know. But I believe SyncObjectGraph is part of that process.

Cheers
@daverogers is 100% correct, one of these use cases of the framework is to provide a way to mange states while disconnected from the DataContext e.g. handing object graphs states before/after serialization/deserialization, between DataContexts (bounded context) - between/across different repositories & unit of work(s), etc. To @daverogers point the pattern's creator is Julie Lerman (who's expertise is in EF, Domain Driven Design, Data Access, EF MVP, etc.). Yes "SyncObjectGraph" is a key component as well as other components e.g. Repository.Pattern.Ef6.StateHelper.cs in making this happen. I wouldn't recommend simply commenting out SyncObjectGraph, Unit/Integration test will fail (e.g. Northwind.Test.IntegrationTests.CustomRepositoryTests.cs, which illustrates what, why and how this use case can be leveraged in detail). If one doesn't understand the value in this, this would be a good starting point to start to do so. This pattern will take some time and most importantly experience with EF (or any other ORM e.g. nHibernate) for full realization, especially experience and pain points when working with EF in disconnected scenarios e.g. distributed computing, messaging architectures (with or without service bus), Windows Workflow (long running services), (@daverogers point) dealing with DTO's or JavaScript objects from the client - not having to query the database and rehydrating these objects every time an CRUD (updates) occur, using the Bounded Context Pattern from DDD in practice, etc.

Hindsight, seems like there is an issue with casting of collections inside the SyncObjectGraph which we haven't experienced since we are using Entity Framework Power Tools to generate and baseline our entities. EF PowerTools from the EF Team which uses ICollection for navigation properties that are collections, again, hence why we were not seeing any issues with it. @daverogers, has submitted a pull request and we are currently reviewing this with him. Assuming this is the fix, we will deploy a minor release with his fix shortly.
Dec 20, 2014 at 5:37 AM
Ok, I understand the intent and how this is suppose to work but I have the following issues:

1) Unless I am misreading / understanding something, the current code is not updating statuses for the entities in the graph. This section of code here
                if (trackableRef != null && trackableRef.ObjectState == ObjectState.Added)
                {
                    _dbSet.Attach((TEntity)entity);
                    _context.SyncObjectState((IObjectState)entity);
                }
Is evaluated for every property and if that property is an entity with a state of added, Attaches the main entity and calls SyncObjectState with it. Like I said before I do not get how this is accomplishing anything near the behavior you guys have outlined. Perhaps I am missing something obvious here but I believe you would want to be sending the property value into Attach and SyncObjectState. It is also worth nothing that A) The section of code above is never true for any of the test cases in Sample\Northwind.Test\IntegrationTests\CustomerRepositoryTests.cs and as such no attach is executed. Also worth noting all tests pass fine with the call to SyncObjectGraph commented out.

2) As referenced in the other discussion (I apologize for cluttering things up, in the beginning I thought these where separate issues), calling Attach with a number of Entities with a state of Added and Guid.Empty() for primary keys produces an error indicated that duplicate primary keys where found.

A similar result could be produced with your entities with something like the following (I had to add the Employee reference so that the attach code would actually be called and you would see the Key error I am talking about for OrderDetails.
                IRepositoryAsync<Order> orderRepository = new Repository<Order>(context, unitOfWork);
                var orderTest = new Order()
                {
                    CustomerID = "LLE39",
                    Employee = new Employee()
                    {
                        EmployeeID = 10,
                        FirstName = "Test",
                        ObjectState = ObjectState.Added
                    },
                    EmployeeID = 10,
                    OrderDate = DateTime.Now,
                    ObjectState = ObjectState.Added,
                    OrderDetails = new List<OrderDetail>()
                    {
                        new OrderDetail()
                        {
                            ProductID = 1,
                            Quantity = 5,
                            ObjectState = ObjectState.Added
                        },
                        new OrderDetail()
                        {
                            ProductID = 1,
                            Quantity = 5,
                            ObjectState = ObjectState.Added
                        }
                    }
                };
3) Even putting the above aside, one of the issues (beyond the collection problem already identified and corrected) with the current SyncObjectGraph code can be seen by simply adding Shippers to the Orders being added in CreateAndUpdateAndDeleteCustomerGraphTest
                var customerForInsertGraphTest = new Customer
                {
                    CustomerID = "LLE38",
                    CompanyName = "CBRE",
                    ContactName = "Long Le",
                    ContactTitle = "App/Dev Architect",
                    Address = "11111 Sky Ranch",
                    City = "Dallas",
                    PostalCode = "75042",
                    Country = "USA",
                    Phone = "(222) 222-2222",
                    Fax = "(333) 333-3333",
                    ObjectState = ObjectState.Added,
                    Orders = new[]
                    {
                        new Order()
                        {
                            EmployeeID = 1,
                            OrderDate = DateTime.Now,
                            Shipper = new Shipper
                            {
                                CompanyName = "Test",
                                ObjectState = ObjectState.Added
                            },
                            ObjectState = ObjectState.Added,
                        }, 
                        new Order()
                        {
                            EmployeeID = 1,
                            OrderDate = DateTime.Now,
                            Shipper = new Shipper
                            {
                                CompanyName = "Test",
                                ObjectState = ObjectState.Added
                            },
                            ObjectState = ObjectState.Added
                        }, 
                    }
                };
Running the test with that graph will produce a System.InvalidCastException: Unable to cast object of type 'Northwind.Entities.Models.Order' to type 'Northwind.Entities.Models.Customer'. Stepping through the test reveals the call to SynchObjectGraph is passed an Order via the 1-M foreach but the Attach and synch is trying to work with the original DBSet of Customer.

I truly think something is off here. If you would like I could do a paid codementor with you to go through the above if you feel I am just missing something.

Again thanks for all your hard work on this great framework, it is very appreciated.
Coordinator
Dec 20, 2014 at 10:05 AM
Thanks for the test case, will review, if there's an issue, let's fix it. Will keep everyone updated.
Dec 20, 2014 at 4:49 PM
Something like this would be closer I think
        private void SyncObjectGraph(object entity)
        {
            var dbContext = _context as DbContext;
            if (dbContext != null)
            {
                var dbSet = dbContext.Set(entity.GetType());
                dbSet.Attach(entity);
                _context.SyncObjectState((IObjectState)entity);
            }

            // Set tracking state for child collections
            foreach (var prop in entity.GetType().GetProperties())
            {
                // Apply changes to 1-1 and M-1 properties
                var trackableRef = prop.GetValue(entity, null) as IObjectState;
                if (trackableRef != null && trackableRef.ObjectState == ObjectState.Added)
                {
                    SyncObjectGraph(trackableRef);
                }

                // Apply changes to 1-M properties
                var items = prop.GetValue(entity, null) as IList<IObjectState>;
                if (items == null) continue;

                foreach (var item in items.Where(item => item.ObjectState == ObjectState.Added))
                {
                    SyncObjectGraph(item);
                }                    
            }
        }
This would treat 1-1 and M-1 properties in a similar fashion to how the 1-M where being treated. After looking at the existing code, simply changing the attach to the property value versus the main entity would not really be correct because the children of the 1-1 or M-1 would not be synched where the children of the 1-M would (While the 1-M itself would not be attached or synched).

Unfortunately this code suffers from the same issue with duplicate primary keys when attach is called on the main entity. I do not really understand enough about the differences in Attach and Add to know why this is happening or if the code above even really addresses the case correctly.
Coordinator
Dec 21, 2014 at 8:12 AM
Edited Dec 21, 2014 at 8:12 AM
  1. Your test case is failing by design:
                IRepositoryAsync<Order> orderRepository = new Repository<Order>(context, unitOfWork);
                var orderTest = new Order()
                {
                    CustomerID = "LLE39",
                    Employee = new Employee()
                    {
                        EmployeeID = 10,
                        FirstName = "Test",
                        ObjectState = ObjectState.Added
                    },
                    EmployeeID = 10,
                    OrderDate = DateTime.Now,
                    ObjectState = ObjectState.Added,
                    OrderDetails = new List<OrderDetail>()
                    {
                        new OrderDetail()
                        {
                            ProductID = 1,
                            Quantity = 5,
                            ObjectState = ObjectState.Added
                        },
                        new OrderDetail()
                        {
                            ProductID = 1,
                            Quantity = 5,
                            ObjectState = ObjectState.Added
                        }
                    }
                };
OrderDetails has a composite key (OrderId & ProductId) and you have two OrderDetails with the same exact ID's which is why you are receiving this error. I actually when ahead and added your test case to our integration test, you can clone and see this in action yourself https://genericunitofworkandrepositories.codeplex.com/SourceControl/latest#main/Sample/Northwind.Test/IntegrationTests/OrderRepositoryTest.cs.
  1. As for the SyncObjectState we've done some minor refactoring to for: performance, true deep recursion of the graph to all levels (https://genericunitofworkandrepositories.codeplex.com/SourceControl/latest#main/Source/Repository.Pattern.Ef6/UnitOfWork.cs).
Please test this and let me know if your still having issues.
Dec 21, 2014 at 10:28 AM
I'm staring at my code and cannot see that. But no matter. I've cloned latest and run the tests and its all looking good. That's the main thing!
Dec 21, 2014 at 12:13 PM
Edited Dec 21, 2014 at 12:14 PM
Thanks so much guys, I have updated and all of the issues I was having are corrected. I apologize for the bad test, I was trying to come up with something quick using the sample entities that would reflect what I was seeing. Obviously I was not paying close enough attention and called it good when I ran into the same error.

The only question I had on the updated SyncObjectGraph is related to this code:
                // Apply changes to 1-1 and M-1 properties
                var trackableRef = prop.GetValue(entity, null) as IObjectState;
                if (trackableRef != null)
                {
                    if(trackableRef.ObjectState == ObjectState.Added)
                        _context.SyncObjectState((IObjectState) entity);

                    SyncObjectGraph(prop.GetValue(entity, null));
                }
The portion where it syncs the state of entity would have already occurred above. Changing it to the property value would end up being a no-op as well since that would be handled by the recursive call to SyncObjectGraph. I think that call to SyncObjectState can just be removed. All tests run fine with it removed in both my project and Sample.

Again thanks for all the hard work.
Coordinator
Dec 22, 2014 at 9:11 AM
thanks jlindbo.

v3.3.5 has been published with @daverogers pull request and jlindbo's finding, both which ultimately led to some big updates to InsertUpdateGraph. All who cloned the nightly build to resolve issues around InsertUpdateGrah before this moment, please pull down the official v3.3.5 release or simply copy and paste (content or file) into Repository.cs into your project. There was a very important fix in the official release which wasn't in the source code repository up until now.
public virtual void InsertOrUpdateGraph(TEntity entity)
{
    SyncObjectGraph(entity);
    _entitesChecked = null;
    _dbSet.Attach(entity);
}
Long story short, last night's commits did not have _entitesChecked = null, which resets the the tracking of which entities have been processed when InsetOrUpdateGraph calls SyncObjectGraph. This is needed and very important, without this, this collection can get fairly huge before the Repository is disposed depending on how many times it's been called. Even InsertOrUpdateGraph is only called once for the lifecycle of your Repository's instance, you don't want this hanging around especially if it's not needed.
Coordinator
Dec 22, 2014 at 9:37 AM
Edited Dec 22, 2014 at 9:38 AM
jlindbo wrote:
Thanks so much guys, I have updated and all of the issues I was having are corrected. I apologize for the bad test, I was trying to come up with something quick using the sample entities that would reflect what I was seeing. Obviously I was not paying close enough attention and called it good when I ran into the same error.

The only question I had on the updated SyncObjectGraph is related to this code:
                // Apply changes to 1-1 and M-1 properties
                var trackableRef = prop.GetValue(entity, null) as IObjectState;
                if (trackableRef != null)
                {
                    if(trackableRef.ObjectState == ObjectState.Added)
                        _context.SyncObjectState((IObjectState) entity);

                    SyncObjectGraph(prop.GetValue(entity, null));
                }
The portion where it syncs the state of entity would have already occurred above. Changing it to the property value would end up being a no-op as well since that would be handled by the recursive call to SyncObjectGraph. I think that call to SyncObjectState can just be removed. All tests run fine with it removed in both my project and Sample.

Again thanks for all the hard work.
Question on this, the code above this is syncing the the actual root entity, the code you have here is now iterating through all of its properties now to sync them as well, am missing something here from your call out?
Coordinator
Dec 22, 2014 at 9:54 AM
Edited Dec 22, 2014 at 9:55 AM
daverogers wrote:
I'm staring at my code and cannot see that. But no matter. I've cloned latest and run the tests and its all looking good. That's the main thing!
I've commented this out in detail in case you wanted clarity, sounds like you content without it, but anyhow here it is (also committed as well).

https://genericunitofworkandrepositories.codeplex.com/SourceControl/latest#main/Source/Repository.Pattern.Ef6/Repository.cs
Dec 22, 2014 at 12:09 PM
lelong37 wrote:
Question on this, the code above this is syncing the the actual root entity, the code you have here is now iterating through all of its properties now to sync them as well, am missing something here from your call out?
What I am saying is this section of code:
                    // discovered entity with ObjectState.Added, sync this with provider e.g. EF
                    if(trackableRef.ObjectState == ObjectState.Added)
                        _context.SyncObjectState((IObjectState) entity);
inside the property foreach is not needed. The entity itself was already synced prior to entering the property loop. The property value will be synced in the recursive call to SyncObjectGraph. Unless this code is handling the case where the entity was not in a state of added but one of its properties was and the entity object state needs to be synced as well (Which now that I think about it could very well be the intent)
Coordinator
Dec 22, 2014 at 7:01 PM
Edited Dec 22, 2014 at 7:55 PM
jlindbo, think you are on the right path to understanding what the intent is, here is a print out of what SyncObjectState is scanning with the object graph that was from your test case.


Check entity: Order
Check property: OrderID
Check property: CustomerID
Check property: EmployeeID
Check property: OrderDate
Check property: RequiredDate
Check property: ShippedDate
Check property: ShipVia
Check property: Freight
Check property: ShipName
Check property: ShipAddress
Check property: ShipCity
Check property: ShipRegion
Check property: ShipPostalCode
Check property: ShipCountry
Check property: Customer
Check property: Employee
Check entity: Employee
Check property: EmployeeID
Check property: LastName
Check property: FirstName
Check property: Title
Check property: TitleOfCourtesy
Check property: BirthDate
Check property: HireDate
Check property: Address
Check property: City
Check property: Region
Check property: PostalCode
Check property: Country
Check property: HomePhone
Check property: Extension
Check property: Photo
Check property: Notes
Check property: ReportsTo
Check property: PhotoPath
Check property: Employees1
Check property: Employee1
Check property: Orders
Check entity: Order
Check property: Territories
Check property: ObjectState
Check property: OrderDetails
Check entity: OrderDetail
Check property: OrderID
Check property: ProductID
Check property: UnitPrice
Check property: Quantity
Check property: Discount
Check property: Order
Check entity: Order
Check property: Product
Check property: ObjectState
Check entity: OrderDetail
Check property: OrderID
Check property: ProductID
Check property: UnitPrice
Check property: Quantity
Check property: Discount
Check property: Order
Check entity: Order
Check property: Product
Check property: ObjectState
Check property: Shipper
Check property: ObjectState
Dec 28, 2014 at 3:45 PM
Not sure if this should go in a separate discussion or not but we are experiencing some performance issues with the updated SyncObjectGraph code. We are calling InsertOrUpdateGraph on some entities with 5000+ new child records. I know this is a large number but calls to straight Add with this where < 5 seconds where calls with the new Sync code can be over 3 minutes. Analysis shows all of the time is spent when setting state is SyncObjectState: Entry(entity).State.

This echos others experience with EF:

http://stackoverflow.com/questions/5917478/what-causes-attach-to-be-slow-in-ef4

Since we are setting state on all of our entities manually is AutoDetectChangesEnabled required? Disabling it in the Context constructor addresses the performance issue and initial tests indicate data operations are being performed as expected.

Thanks
Jon