Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AttachAndReloadAssociatedEntity #136

Open
josh-dastmalchi opened this issue Mar 26, 2015 · 2 comments
Open

AttachAndReloadAssociatedEntity #136

josh-dastmalchi opened this issue Mar 26, 2015 · 2 comments

Comments

@josh-dastmalchi
Copy link
Contributor

public object AttachAndReloadAssociatedEntity(object entity)
        {
            var localCopy = FindTrackedEntity(entity);
            if (localCopy != null)
            {
                return localCopy;
            }

            if (_context.Entry(entity).State == EntityState.Detached)
            {
                // TODO look into a possible better way of doing this, I don't particularly like it
                // will add a key-only object to the change tracker. at the moment this is being reloaded,
                // performing a db query which would impact performance
                var entityType = ObjectContext.GetObjectType(entity.GetType());
                var instance = _entityManager.CreateEmptyEntityWithKey(entity);

                _context.Set(entityType).Attach(instance);
                _context.Entry(instance).Reload();

                AttachRequiredNavigationProperties(entity, instance);
                return instance;
            }

            if (GraphDiffConfiguration.ReloadAssociatedEntitiesWhenAttached)
            {
                _context.Entry(entity).Reload();
            }

            return entity;
        }

I think this method has some issues. There are two paths:

Path A: The entity exists in memory in the context

  1. Return it.

Path B: The entity is not in memory in the context

  1. Create an empty object with just the key
  2. Attach that item to the context
  3. Reload the item, so that it is correctly populated with all the database values
  4. Change the navigation properties on it to reflect the navigation properties of the item that was passed to GraphDiff.

In Path B, steps 1/2/3 accomplish the same thing as Path A-1.
Path A-4 performs an extra step that I think is mistaken, at least if proxies are turned on. The previous Reload() call is going to populate all the navigation properties, so there's no need to do more work. Additionally, if Path A is hit we also get a non-hydrated object if proxies are off. I think this is the cause of #117

Additionally, the AttachRequiredNavigationProperties() method, rather than loading the navigation properties from the database, is copying them from the updated object - which, being an AssociatedEntity may not be fully hydrated or may contain changes that we should ignore - which this method won't do.

I believe this method needs to take into account proxies, and can have two paths:

Proxies on - create from id and reload. (Path B 1 to 3)
Proxies off - inject IQueryLoader and perform LoadEntity on each newly-added AssociatedEntity.

If this makes sense, I'll create a pull request for the changes.

@ghost ghost added the potential bug label Jul 22, 2015
@mortb
Copy link

mortb commented Oct 9, 2015

I have a model which reminds something like (I have drawn an example model that has the same relationships as the real model)
simpledb

Where each region has an associated list of countries.
Each country has a boss which is a non null reference to the people table

The class diagram reminds something like:
classdiagram

There is one page in the system for managing countries and their bosses, and another page for managing regions and their connected country.
Now if I post back a region using json. The region already has one country (id = 1). I have just populated the ids of the countries since we're just interested in the connection. Country id = 2 is the added country

{
   Id : 1,
   Name : Europe,
   Countries : [{Id : 1}, {Id :2}]
}

The json is converted to classes using an automapper solution
I update the region graph using the statement below

   context.UpdateGraph(regionEntity, map => 
             map.AssociatedCollection(regionEntity => Country));

For each added country (in this case the country with id=2) the method AttachAndReloadAssociatedEntity will be called.
AttachAndReloadAssociatedEntity will load the object from the database.
Alright, that is nice :)
But inside AttachAndReloadAssociatedEntity it calls the AttachRequiredNavigationProperties which will null out the required Boss property since it reads that from the object that I have added...

When I call Save

   context.SaveChanges();

it will fail as the required Boss field is null. I didn't expect that the AssociatedCollection relationship would alter the connected objects. It took some time to debug.

The exception message is:
The operation failed: The relationship could not be changed because one or more of the foreign-key properties is non-nullable. When a change is made to a relationship, the related foreign-key property is set to a null value. If the foreign-key does not support null values, a new relationship must be defined, the foreign-key property must be assigned another non-null value, or the unrelated object must be deleted.

@JobaDiniz
Copy link

what is the workaround?

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

No branches or pull requests

3 participants