Entity Framework proper way to replace collection in one to many

Question

Suppose a customer has many phone numbers and a phone number has only one customer.

public class PhoneNumber : IValueObject {
  public string Number {get; set;}
  public string Type {get; set;}
}

public class Customer : IEntity {
   public ICollection<PhoneNumber> phones {get; private set;} //ew at no encapsulated collection support
   public void SetPhones(params PhoneNumber[] phones) {
       this.phones.Clear();
       this.phones.AddRange(phones);
   }
}

If I do an EF mapping like this and run it, every time I set phone numbers it will create new PhoneNumbers but not delete the old ones. There are no other entities referencing phone numbers, I don't even expose it on my dbcontext, is there a way to tell EF that Customer owns PhoneNumbers completely and therefore if phone numbers were removed from the collection they should be deleted?

I realize that there's a dozen ways to hack around this problem, but this isn't a weird edge case, what's the "right" way to handle this.

Solution 1

I had the exact same question :)

This answer on identifying relationships solved my issue.

Note: You have to load the collection (eagerly, explicitly or lazily) so that it can be tracked before setting the new values and calling save. Otherwise you will not be replacing the collection but, just be adding to it.

For example:

var entity = unitOfWork.EntityRepository.GetById(model.Id);
// I have something like this to load collection because
// I don't have the collection's entities exposed to the context
unitOfWork.EntityRepository.LoadCollection(entity, e => e.CollectionProperty);
entity.CollectionProperty = newCollectionValuesList;
unitOfWork.Save();

This will remove the previous collection values from the 'collection table' and only add the newly set values.

Hope that helps.

Solution 2

First (optional):

I recommend you to make

public ICollection<PhoneNumber> phones {get; private set;}

a virtual property, to let the Entity Framework know that it should be lazy loaded (even if you do not have Lazy Load enabled, it is a good practice).

public virtual ICollection<PhoneNumber> phones {get; private set;}

Second:

Add an Inverse Navigation Property on your PhoneNumber class (it will be required in order to achieve the solution I give you below):

public class PhoneNumber : IValueObject {
  public string Number {get; set;}
  public string Type {get; set;}

  public virtual Customer {get; set;}
}

public class Customer : IEntity {
   public ICollection<PhoneNumber> phones {get; private set;} //ew at no encapsulated collection support
   public void SetPhones(params PhoneNumber[] phones) {
       this.phones.Clear();
       this.phones.AddRange(phones);
   }
}

Third (Possible solution for your problem):

Remove the PhoneNumber objects from Context instead of doing so from the Customer:

public ICollection<PhoneNumber> phones {get; private set;} //ew at no encapsulated collection support
   public void SetPhones(params PhoneNumber[] phones) {
       Context.PhoneNumbers.RemoveRange(this.phones);
       this.phones.AddRange(phones);
   }
}

Comments

[Abhishek]

Error during Update-Database. An error occurred while saving entities that do not expose foreign key properties for their relationships. The EntityEntries property will return null because a single entity cannot be identified as the source of the exception. Handling of exceptions while saving can be made easier by exposing foreign key properties in your entity types...A relationship from the 'Customer_phones' AssociationSet is in the 'Deleted' state. Given multiplicity constraints, a corresponding 'Customer_phones_Target' must also in the 'Deleted' state.

[Alexandre Severino]

@GeorgeMauer try commenting the Third step and do only the first two. They should be enough to solve your problem.

[Abhishek]

Removing the third step (even when I keep the PhoneNumber->Customer relationship) causes the issue in the screenshot above. Old phone numbers now just have a null customer, they are not removed.

[Alexandre Severino]

Also, instead of doing my new "third step" you could try adding [Required] right above the public virtual Customer {get; set;} or adding the property public int CustomerId {get; set;} which signs EF that the Customer MUST NOT be null.

[Alexandre Severino]

Please let me know if any of the new ideas on my comments work so I can edit the answer.

[Abhishek]

Have to run out now, will be back to it in a few hours

[Abhishek]

In your update, where does Context come from? Wouldn't that mean that entities have to be connected to the db context? How on earth would you uni test that?

[Alexandre Severino]

Nevermind my Third answer if you need a uni test... have you tried the [Required] and adding the CustomerId property instead?

[Abhishek]

I have but the core issue is that this doesn't tell Entity Framework that PhoneNumber is not used by a different class. So required or not, how would it know that it is ok to delete this?

[Alexandre Severino]

EF knows that your PhoneNumber is not being used anywhere else than in Costumer. Thus, if the Required is set on PhoneNumber's Customer inverse property and this very Customer ceases to exist, PhoneNumber should be automatically cascade deleted.

[Abhishek]

+1 for actually doing things correctly and having repositories flow "out of" a unit of work. I still don't really get the answer though. What would Entity look like here? Are you saying that this trick with phone number ids alone is enough to get it to trigger a delete properly?

[Quinton Smith]

@GeorgeMauer - Entity here would be your Customer and CollectionProperty would be your phones property. In my case I'm loading my collections explicitly so LoadCollection looks like context.Entry(entity).Collection(navigationProperty).Load(); under the hood. If you have an identifying relationship between Customer and PhoneNumber and you have loaded phones for a customer then the code in your screenshot should leave you with only 123-456-7890 in the PhoneNumbers table.

[Kieran Ojakangas]

@QuintonSmith is that "Save" method you have written out which is really eventually just calling "SaveChanges()?"

[Quinton Smith]

@KieranOjakangas you are exactly right. Under the hood it's simply context.SaveChanges();

[Rex]

Thank you very much @QuintonSmith, you saved my day with your comment that child collections need to be loaded to be replaced.