Entity Framework not correctly modifying or deleting child items

Question

I am trying save Firm object that contains addresses and websites. I have developed the ability to add and remove address controls from the UI using reactive forms in Angular 7. While saving the Firm object, it is creating additional entries for addresses and websites and not treating it as existing record.

So if I delete websites and addresess from the UI, I can see that I am passing the correct amount of array elements to the backend api. So I am rest assured that the issue is with Entity Framework.

So what I am trying to achieve is that if the user deletes addresses or websites from the client side, it should update the same when calling the update method in Entity Framework. I am using Entity Framework 6

UI - Where I can add multiple addressess

Here are my model classes

NewFirmViewModel

 public class NewFirmViewModel
    {
        public int FirmId { get; set; }

        public string FirmName { get; set;}

        public Nullable<DateTime> DateFounded { get; set; }

        public ICollection<AddressViewModel> Addresses { get; set; }

        public ICollection<WebsiteViewModel> Websites { get; set; }

        public bool hasIntralinks { get; set; }
    }

AddressViewModel

public class AddressViewModel
{
    public int AddressId { get; set; }
    public string Line1 { get; set; }
    public string Line2 { get; set; }
    public string Line3 { get; set; }
    public string Phone { get; set; }
    public bool IsHeadOffice { get; set; }
    public int FirmId { get; set; }
}

WebsiteViewModel

public class WebsiteViewModel
{
    private int FirmWebsiteId { get; set; }
    private string WebsiteUrl { get; set; }
    public string Username { get; set; }
    public string Password { get; set; }
    public int FirmId { get; set; }
}

Entities

public class FIRM: Entity,IHasAUMs<FIRM_AUM> 
    {
        public FIRM()
        {
            //this.FIRM_PERSON = new HashSet<FIRM_PERSON>();
            this.MANAGERSTRATEGies = new HashSet<MANAGERSTRATEGY>();
            this.FIRM_ACTIVITY = new HashSet<FIRM_ACTIVITY>();
            this.FIRM_AUMs = new HashSet<FIRM_AUM>();
            this.FIRM_REGISTRATION = new HashSet<FIRM_REGISTRATION>();
            //this.ACTIVITies = new HashSet<ACTIVITY>();
            Addresses = new HashSet<ADDRESS>();
            //People = new HashSet<PERSON>();   
           // Websites = new HashSet<FIRM_WEBSITE>();
        }

        //public decimal ID { get; set; }
        //
        //
        //
        //
        public string NAME { get; set; }
        public string SHORT_NAME { get; set; }
        public string ALTERNATE_NAME { get; set; }
        public string WEBSITE { get; set; }
        public string WEBSITE_USERNAME { get; set; }
        public string WEBSITE_PASSWORD { get; set; }
        public bool? INTRALINKS_FIRM { get; set; }        
        public string NOTES_TEXT { get; set; }
        public string NOTES_HTML { get; set; }
        public string HISTORY_TEXT { get; set; }
        public string HISTORY_HTML { get; set; }

        public string HISTORY_SUM_TEXT { get; set; }
        public string HISTORY_SUM_HTML { get; set; }

        public Nullable<decimal> OLD_ORG_REF { get; set; }
        public Nullable<decimal> SOURCE_ID { get; set; }

        [DisplayFormat(DataFormatString = PermalConstants.DateFormat)]
        public Nullable<DateTime> DATE_FOUNDED { get; set; }

        public virtual  ICollection<ADDRESS> Addresses { get; set; }

      //  public ICollection<FIRM_WEBSITE> Websites { get; set; }
        // public ICollection<PERSON> People { get; set; }

        //public SOURCE SOURCE { get; set; }
        // public ICollection<FIRM_PERSON> FIRM_PERSON { get; set; }
        public ICollection<MANAGERSTRATEGY> MANAGERSTRATEGies { get; set; }
        public ICollection<FIRM_ACTIVITY> FIRM_ACTIVITY { get; set; }
        public ICollection<FIRM_REGISTRATION> FIRM_REGISTRATION { get; set; }
        //public ICollection<ACTIVITY> ACTIVITies { get; set; }
        public ICollection<FIRM_WEBSITE> Websites { get; set; }

        public Nullable<int> KEY_CONTACT_ID { get; set; }
        [NotMapped]
        public ICollection<FIRM_AUM> AUMs
        {
            get
            {
                return this.FIRM_AUMs;
            }
        }
        public ICollection<FIRM_AUM> FIRM_AUMs { get; set; }
    }


    ADDRESS

      public class ADDRESS : Entity
    {
        public ADDRESS()
        {
            // DATE_CREATED = DateTime.Now;
        }


        public string LINE1 { get; set; }
        public string LINE2 { get; set; }
        public string LINE3 { get; set; }
        public int CITY_ID { get; set; }
        public string POSTAL_CODE { get; set; }
        public string SWITCHBOARD_INT { get; set; }
        public string NOTES { get; set; }
        public int? OLD_ADDRESS_REF { get; set; }
        public int? SOURCE_ID { get; set; }

        public int FIRM_ID { get; set; }
        [ForeignKey("FIRM_ID")]
        public FIRM FIRM { get; set; }

        [ForeignKey("CITY_ID")]
        public CITY City { get; set; }

        public ICollection<PERSON> People { get; set; }

        // public SOURCE SOURCE { get; set; }

        public bool IS_HEAD_OFFICE { get; set; }
        [NotMapped]
        public string AddressBlurb
        {
            get
            {
                return string.Join(",", new[] { LINE1, LINE2, City != null ? City.NAME : "", City != null && City.Country != null ? City.Country.NAME : "" }.Where(x => !string.IsNullOrEmpty(x)));
            }
        }
    }


    FIRM_WEBSITE

      public class FIRM_WEBSITE : Entity
    {
        public FIRM_WEBSITE()
        {

        }
        private string _WEBSITE_URL;

        public string WEBSITE_URL
        {
            get
            {
                if (string.IsNullOrEmpty(_WEBSITE_URL))
                    return _WEBSITE_URL;
                try
                {

                    var ubuilder = new System.UriBuilder(_WEBSITE_URL ?? "");

                    return ubuilder.Uri.AbsoluteUri;
                }
                catch (UriFormatException ex)
                {
                    return _WEBSITE_URL;
                }

            }
            set { _WEBSITE_URL = value; }
        }

        public string USERNAME { get; set; }
        public string PASSWORD { get; set; }


        public int FIRM_ID { get; set; }
        [ForeignKey("FIRM_ID")]
        public FIRM FIRM { get; set; }
    }

API controller

  [HttpPut]
    [SkipTokenAuthorization]
    [Route("api/firm/update")]
    public IHttpActionResult Update(NewFirmViewModel model)
    {


          var firmService = GetService<FIRM>();

        if (model == null) return StatusCode(HttpStatusCode.NotFound);

        var firm = firmService.GetWithIncludes(model.FirmId);

        if (firm != null)
        {
            firm.NAME = model.FirmName;
            firm.DATE_FOUNDED = model.DateFounded;
            firm.Addresses = model.Addresses.Select(x => new ADDRESS() {ID = x.AddressId, LINE1 = x.Line1, LINE2 = x.Line2, LINE3 = x.Line3, FIRM_ID = x.FirmId}).ToList();
            firm.Websites = model.Websites.Select(x => new FIRM_WEBSITE() {ID = x.FirmWebsiteId, WEBSITE_URL = x.WebsiteUrl, USERNAME = x.Username, PASSWORD = x.Password, FIRM_ID = x.FirmId}).ToList();


            var addressIds = model.Addresses.Select(x => x.AddressId).ToList();
            var addresses = firm.Addresses.Where(x => addressIds.Contains(x.ID)).ToList(); // All of the addresses we want to associate to this firm.
            // Identify addresses to remove from this firm.
            var addressesToRemove = firm.Addresses.Where(x => !addressIds.Contains(x.ID)).ToList();
            foreach (var address in addressesToRemove)
                firm.Addresses.Remove(address);

            // Identify addresses to associate to this firm.
            var existingAddressIds = firm.Addresses.Select(x => x.ID).ToList();
            var addressesToAdd = addresses.Where(x => !existingAddressIds.Contains(x.ID)).ToList();
            foreach (var address in addressesToAdd)
                firm.Addresses.Add(address);

            firmService.Update(firm);
        }
        else
        {

        }

        return Ok(firm);
}

DbContext

     public class Repo<T> : IRepo<T> where T : Entity, new()
        {
            public readonly Db dbContext;

            private ILogger _logger;
            private IQueryable<T> lastQuery { get; set; }
            private bool? _enablelazyloading;
            private IEntityWatcher<T> _watcherNotification;
            private bool _EnableChangeNotification;
            public string ID { get; set; }
            private string _clientId;

            #region Constructors
            public Repo(IDbContextFactory f)
            {
                if (typeof(T).GetCustomAttribute<SeparateDbContext>() != null)
                    dbContext = f.GetContext<T>();
                else
                    dbContext = f.GetContext();
                _logger = IoC.Resolve<ILogger>();
                try
                {
                    _watcherNotification = IoC.Resolve<IEntityWatcher<T>>();
                }
                catch (Exception ex)
                {
                    _logger.Error("Change Notification failed to resolve in Repo.  The Repo will continue to function without notification.", ex);

                }
            }
            public Repo() : this(new DbContextFactory()) { }
            #endregion

            public bool? EnableLazyLoading
            {
                get { return dbContext.EnableLazyLoading; }
                set { dbContext.EnableLazyLoading = value; }
            }

            public void SetClientId(string clientId)
            {
                var oc = dbContext.Database.Connection as OracleConnection;

                if (oc != null)
                {
                    oc.Open();
                    oc.ClientId = clientId;
                    oc.Close();
                }
            }


            public T Update(T obj)
            {
                _logger.Info("Repo.Update {0}", obj);
                var entity = Get(obj.ID);
                var oldEntity = new T();
                var entry = dbContext.Entry(entity);
                oldEntity.InjectFrom(entry.OriginalValues.ToObject());
                if (dbContext.Entry(obj).State == System.Data.Entity.EntityState.Detached)
                {
                    entry.CurrentValues.SetValues(obj);
                }
                    LogAllModifiedEntities(dbContext);
                dbContext.SaveChanges();
                if (_watcherNotification != null)
                    _watcherNotification.EntityChanged(ChangeNotificationType.Modified, entity, oldEntity);
                return Get(obj.ID);
            }


 public void EntityChanged(ChangeNotificationType changeNotificationType, T newEntity, T oldEntity) {
            if(_entityAuditEnabled) {
                var filter = IoC.Resolve<IEntityWatchFilter<T>>();
                filter.Filter(changeNotificationType, newEntity, oldEntity);
            }
        }
    }

   public bool Filter(ChangeNotificationType changeNotificationType, T newEntity, T oldEntity) {
            try {
                ///only 
                if(_WatchList.Contains(typeof(T).Name) || !_WatchList.Any()) {
                    var newLegacyStratImpl = newEntity as ILegacyStrategy;
                    var oldLegacyStratImpl = oldEntity as ILegacyStrategy;
                    var blankStrategies = IoC.Resolve<ICrudService<LEGACY_STRATEGY>>().Where(x => x.NAME.Trim() == "").Select(x => x.ID).AsEnumerable();
                    if(changeNotificationType == ChangeNotificationType.Added && newLegacyStratImpl != null && newLegacyStratImpl.LEGACY_STRATEGY_ID.HasValue && !blankStrategies.Contains(newLegacyStratImpl.LEGACY_STRATEGY_ID.Value)) {

                        _action.Added(newEntity);
                        return true;
                    } else if(changeNotificationType == ChangeNotificationType.Deleted && newLegacyStratImpl != null) {
                        _action.Deleted(newEntity);
                        return true;
                    } else if(changeNotificationType == ChangeNotificationType.Modified && newLegacyStratImpl != null && oldLegacyStratImpl != null) {
                        ///need to go the extra distance and make sure the legacy strategy was changed and not some other property.
                        var hasChanged = newLegacyStratImpl.LEGACY_STRATEGY_ID != oldLegacyStratImpl.LEGACY_STRATEGY_ID;
                        if(hasChanged) {
                            _action.Modified(newEntity, oldEntity);
                            return true;
                        } else {
                            return false;
                        }
                    }
                }
                return false;///all else fails...
            } catch(Exception ex) {
                _logger.Error(ex);
                return false;
            }
        }

Answer 1

        firm.Addresses = model.Firm.Addresses;
        firm.Websites=  model.Firm.Websites;

This... You are effectively telling this instance of the context to treat the Addresses and Websites provided by your "model" as entities. The context doesn't know about these entities so it treats them no differently than if you had done something like the following:

foreach(var address in model.Firm.Addresses)
{
   firm.Addresses.Add(new Address { AddressId = address.AddressId, City = address.City, /* ... */ });
}

As far as the context is concerned, these objects are "new".

As a general rule, avoid passing entities to the client, and never trust/accept entities back from the client. If the Firm is associating existing addresses, then a list of AddressIDs is more than sufficient for the Firm update model. (Assuming that if the user created or updated an address' content, that would have been saved separately.) If the user can pass a new address with the Firm update, then you need a suitable address view model and detect the new or updated entries.

A simple apparent solution to the above problem is to associate the entities to the context using Attach() but I never recommend this because it trusts that the entity has not been modified in unintended ways. (Plus raises other edge cases that crop up such as where the context may already have an entity associated with that ID)

When updating child references like addresses where we are not updating address content as part of the firm update:

var addressIds = model.Firm.Addresses.Select(x => x.AddressId).ToList();
var addresses = dbContext.Addresses.Where(x => addressIds.Contains(x => x.AddressId)).ToList(); // All of the addresses we want to associate to this firm.

// Identify addresses to remove from this firm.
var addressesToRemove = firm.Addresses.Where(x => !addressIds.Contains(x.AddressId)).ToList();
// Identify addresses to associate to this firm.
var addressesToAdd = addresses
        .Except(firm.Addresses, new LamdaComparer((a1,a2) => a1.AddressId == a2.AddressId));

foreach(var address in addressesToRemove)
    firm.Addresses.Remove(address);

if(addressesToAdd.Any())
    firm.Addresses.AddRange(addressesToAdd);

If you are potentially updating address details, then a bit more work will be needed, but the crux of the matter is that you can't trust the entities that you passed to the client and receive back via the model. View models should be POCO classes, not entities. To avoid issues like this, anything passed back from a view should be validated and the applicable entity(ies) should be loaded from the context handling the request.

The LamdaComparer can be found here.

Edit: If there are issues with implementing the comparer.. Without the LamdaComparer, you can do something like:

// Identify addresses to associate to this firm.
var existingAddressIds = firm.Addresses.Select(x => x.AddressId).ToList();
var addressesToAdd = addresses.Where(x => !existingAddressIds.Contains(x.AddressId)).ToList();

Edit 2: Repository classes are helpful for enabling unit testing. Generic repository classes are evil. If you're not using unit testing, then I would avoid adding the complexity of trying to abstract away the EF functionality into a repository, especially a Generic repository. In your case, to avoid potentially breaking other areas of the code, I'd add a method called SaveChanges to your service which just calls the context's SaveChanges, and then instead of calling your service.Update(entiny) method, call service.SaveChanges().

Trying to abstract away EF's functionality in a repository is very much counter-productive. For instance to try and do the checks for added and removed related entities requires knowledge of the entity in question, which isn't knowledge that a Generic implementation would know. Accept that EF is a core part of your application, no different than the .Net Framework is a core part of your application. This allows you to leverage the full capability of EF without having to code around trying to hide things like sorting expressions, paging, reduce and map operations, etc. or simply not taking advantage of these features because they might "leak" EF-isms.

It's not to say that the Repo/Context Wrapping implementation your project has is bad or wrong, but it is complex and it has led to behaviour that is difficult to explain. From what I can see from the code you've provided is that it's geared towards treating entities as 2 separate roles, the model, and a detached representation of the model. IMO this violates Single Responsibility, an entity should represent the model, and nothing more. A ViewModel or DTO is the transport of relevant information to a view or external consumer, not the entity. Yes, EF provides functionality to detach/reattach, and copy values across between entities, but a key point I'd make against using this with entities that have been repurposed as view models is that view models/DTOs coming back from the client cannot be trusted. An entity exposes far more information than a client action may wish to update, but the entity coming back could contain changes to any of those values if intercepted by a debugger.

Maybe this is something you inherited from another developer, or something you've constructed from examples found out in the wild. Complexity has to serve a very specific purpose to justify it's existence. Unfortunately in most cases it is added in blind faith that it will solve some future problem or simply because it's a challenge. Design patterns were developed as a means to communicate related concepts, but have been taken as a gospel for what all code should look like. Refactoring, refining, and consolidating code is a good thing to reduce bugs, but it's something that should be done after the objective of the code is proven and understood. Otherwise it is premature optimization, and leads to head-scratching problems like this.