Why is this unused line breaking Entity Framework?-CodePudding

I am updating some existing code of a former colleague and have a strange issue where an unused line is causing an error with Entity Framework. If I comment out the code tagged with //This Line!, everything works.

foreach (Place item in ListOfPlaces)
{
    //This line!
    List<Place> PlacesList = context.Places.Where(x => x.PlaceNumberID == item.PlaceNumberID).ToList();

    long PlaceId = context.Places
                          .Where(x => x.PlaceNumberID == item.PlaceNumberID)
                          .Select(x => x.PlaceId)
                          .FirstOrDefault();

    if (PlaceId != 0)
    {
        item.ID = PlaceId;
        context.Places.Attach(item);
        context.Entry(item).State = System.Data.Entity.EntityState.Modified;
    }
}

If I include that line, I get the error shown here on the Attach(item) line:

Attaching an entity of type 'Place' failed because another entity of the same type already has the same primary key value. This can happen when using the 'Attach' method or setting the state of an entity to 'Unchanged' or 'Modified' if any entities in the graph have conflicting key values. This may be because some entities are new and have not yet received database-generated key values. In this case use the 'Add' method or the 'Added' entity state to track the graph and then set the state of non-new entities to 'Unchanged' or 'Modified' as appropriate.

I know how to fix this from a code point of view (remove the line!), but I can't work out why its breaking the application if somebody could kindly explain please.

CodePudding user response：

I can't work out why its breaking the application

Looks to me like the line causes the download of some Place with ID N - see the ToList on the end? It will trigger the query to run and download data. EF creates objects from every row it receives because that's the default behavior(it can be disabled with eg AsNoTracking)

Later you try to create another object with the same primary key value and attach it to the context, but the context already knows about some object with ID 123 (for example) because the first line caused it to have been downloaded/tracked so you get an error when you try and associate another - if EF allowed both into its tracking memory it wouldn't know which one was the true authority of record that should be saved back to the db

Your interim query doesn't cause the problem, I believe, because it doesn't trigger the download of an entire entity, seeing as it just retrieves an ID

If you're trying to implement insert-if-not-exists style behavior, you should attempt to download an entity with ID x using some XOrDefault or Find, and if it results in null/default then create and add a new entity (you don't need to attach). In essence, ditch the first line, just do the ID check and if the returned ID is default, do a context.Places.Add(new Place{...}).

If you're looking for upsert, it's probably easiest to download the whole entity and then inspect if it was default or not; if it is, then make a new one otherwise edit the downloaded one.

If you're trying for "update without download" then skip the querying part entirely and attach an entity you declare as modified.

If you're after some hybrid upsets without download, I think you'll struggle, because you have to at least quiz the db as to whether it knows of an entity before you decide what to do.. or you run a raw MERGE

CodePudding user response：

That entire loop makes no sense. You repeat the same twice. And as soon as you select one of the items, EF marks it as a tracked. And you can't update using another item, before the first one will be untracked or you can use the tracked item.

Try this code

foreach (Place item in ListOfPlaces)
{
 var placesList = context.Places.Where(x => x.PlaceNumberID == item.PlaceNumberID).ToList();

 if(placesList!=null && placesList.Count ==1)  
{
  
 var existedPlace = placesList.FirstOrDefault();
    context.Entry(existedPlace).CurrentValues.SetValues(item);
} 
// and maybe this
else context.Places.Add(item)

}
context.SaveChanges();

UPDATE

Thanks to @CaiusJard for a hint, in this case it is more efficient to use SingleOrDefault instead of ToList

.....
var existedPlace = context.Places.Where(x => x.PlaceNumberID == item.PlaceNumberID).SingleOrDefault();

 if(existedPlace!=null)  
{
    context.Entry(existedPlace).CurrentValues.SetValues(item);
} 
.....