duplicate values inserted when saving the EF Core context

Question

I have a small application that manages soils of a region(zone). A Zone has Soils it can associate to it.

When I create the Zone I can select the soils ans then save, but once created, when I just open it in modification and hit "Save", without anything else, it throws me:

SqlException: Violation of PRIMARY KEY constraint 'PK_SoilZone'. Cannot insert duplicate key in object 'dbo.SoilZone'. The duplicate key value is (1, 1).

The view is like this

Business Class:

public class Zone
{
    public string Name { get; set; }
    
    public int CountryId { get; set; }
    public Country Country { get; set; }
    
    #region navigation props
    public ICollection<Soil> Soils { get; set; } = new List<Soil>();        
    public List<SoilZone> SoilZones { get; set; } = new List<SoilZone>();
    #endregion
}

DTO object (ViewModel):

public class ZoneDTO
{
    public string Name { get; set; }
    
    public int CountryId { get; set; }
    public string CountryName { get; set; }

    public int[] AvailableSoilIds { get; set; } = new int[] { };
    public int[] SoilIds { get; set; } = new int[] { };
    public string[] SoilNames { get; set; } = new string[] { };
}

AutoMapping:

CreateMap<Zone, ZoneDTO>()
    .ForMember(p => p.CountryName, o => o.MapFrom(p => p.Country.Name))
    .ForMember(p => p.SoilIds, o => o.MapFrom(p => p.Soils.Select(s => s.Id).ToArray()))
    .ForMember(p => p.SoilNames, o => o.MapFrom(p => p.Soils.Select(s => s.Name).ToArray()))
    .ReverseMap();

View:

@model MyApp.Web.DTOs.ZoneDTO

<form asp-action="Edit">
    <div>
        <label asp-for="Name"></label>
        <input asp-for="Name" />
    </div>

    <div>
        <label asp-for="CountryId" class="control-label"></label>
        <select asp-for="CountryId" class="form-control" asp-items="ViewBag.CountryId"></select>

        <label asp-for="SoilIds"></label>               
        <select asp-for="AvailableSoilIds" asp-items="ViewBag.AvailableSoils" multiple="multiple"></select>
        <a href="#" id="addSoil">Add</a>

        <select asp-for="SoilIds" asp-items="ViewBag.Soils" multiple="multiple"></select>
        <a href="#" id="removeSoil">Remove</a>
    </div>

    <input type="hidden" asp-for="Id" />
    <div>
        <input type="submit" value="Save" />
        <a asp-action="Index" >Cancel</a>
    </div>
</form>

Controller's Edit:

[HttpPost]
[ValidateAntiForgeryToken]
public async Task<IActionResult> Edit(int id, ZoneDTO zoneDto)
{
    if (ModelState.IsValid)
    {
        try
        {
            var zone = _mapper.Map<Zone>(zoneDto);
            
            //  from here I am not sure if it's OK <<<<<<<<<<<<
            zone.Soils.Clear();
            foreach (var soilId in zoneDto.SoilIds)
            {
                var soil = _context.Soils.Where(s => s.Id == soilId).FirstOrDefault();
                zone.Soils.Add(soil);
            }
            zone.Country = _context.Country.Find(zoneDto.CountryId);

            _context.Update(zone);
            await _context.SaveChangesAsync();
            // >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
        }
        catch (DbUpdateConcurrencyException)
        {
            if (!ZoneExists(zoneDto.Id)) {
                return NotFound();
            }
            else {
                throw;
            }
        }
        return RedirectToAction(nameof(Index));
    }
    var dto = _mapper.Map<ZoneDTO>(zoneDto);
    ViewData["CountryId"] = new SelectList(_context.Country, "Id", "Code", zoneDto.CountryId);
    ViewData["AvailableSoils"] = new MultiSelectList(_context.Soils, "Id", "Name", dto.SoilIds);
    return View(dto);
}

and the EF configuration:

public override void Configure(EntityTypeBuilder<Zone> builder)
{
    base.Configure(builder);
    builder.HasIndex(p => p.Nom);
    builder
        .HasOne(p => p.Country)
        .WithMany(p => p.Zones)
        .HasForeignKey(p => p.CountryId)
        .IsRequired();

    builder
        .HasMany(p => p.Soils)
        .WithMany(p => p.Zones)
        .UsingEntity<SoilZone>(
            p => p
                .HasOne(p => p.Soil)
                .WithMany(p => p.SoilZones)
                .HasForeignKey(p => p.SoilId),
            p => p
                .HasOne(p => p.Zone)
                .WithMany(p => p.SoilZones)
                .HasForeignKey(p => p.ZoneId),
            p =>
            {
                p.HasKey(k => new { k.ZoneId, k.SoilId });
            });
}

Answer 1

When dealing with references you should eager load your child collection then deterministically modify it based on the changed relationships.

So for instance from your example:

//  from here I am not sure if it's OK <<<<<<<<<<<<
zone.Soils.Clear(); 
foreach (var soilId in zoneDto.SoilIds)
{
    var soil = _context.Soils.Where(s => s.Id == soilId).FirstOrDefault();
    zone.Soils.Add(soil);
}

What it looks like you are trying to do is clear any existing soil associations and re-associate them.

I strongly advise not doing things like this:

var zone = _mapper.Map<Zone>(zoneDto);
// ...
_context.Update(zone);

The problem with this approach is that you are trusting the data in the zoneDto to create a Zone and will use that data to overwrite your data record. DTOs should only include enough data to identify a record and only include the data that can be modified by the operation. In your case that might be pretty much everything in Zone, but in other cases if you have other FK relationships etc. that clients cannot change as part of this operation, you don't want to expose them in the DTO. (yet to compose an object to Update the database, the Mapper call would need them) It's also inefficient from a DB operation perspective. When leveraging change tracking and SaveChanges, EF will compose update statements for just values that are confirmed to have been changed. Using Update or setting the entity state to Modified results in an update statement that updates all columns, whether they changed or not.

Instead, as Guru points out, you should fetch the object from the DbContext to be updated, using only the ID from the DTO. However, if you expect 1 record, use Single rather than First. Methods like First should only be used if you expect multiple rows, and should always include an OrderBy* clause to make the selection predictable.

Since we will be associating Soils, We want to eager load those as well:

var zoneDB = _context.Zones
    .Include(z => z.Soils)
    .Single(z => z.Id == zoneDto.Id);

To update the soils, determine what soils need to be added and removed. For soils that need to be added we can fetch those all in one go.

var existingSoilIds = zoneDB.Soils.Select(x => x.Id).ToList();
var soilIdsToRemove = existingSoilIds.Except(zoneDto.SoilIds).ToList();
var soilIdsToAdd = zoneDto.SoilIds.Except(existingSoilIds).ToList();

foreach(var soilId in soilIdsToRemove)
    zoneDb.Soils.Remove(zoneDb.Soils.Single(x => x.Id == soilId));

var soilsToAdd = _context.Soils.Where(x => soilIdsToAdd.Contains(x.Id)).ToList();
foreach(var soil in soilsToAdd)
    zoneDb.Sois.Add(soil);

_context.SaveChanges();

This will determine which soils are added and removed. For added soils we can fetch these from the DbContext in one hit. Removed IDs we just find in our eager loaded collection and remove them. Change tracking will take care of the inserts and deletes.

This assumes Zone.Soils is declared as ICollection<Soil>. If it is declared as List<Soil> you can use AddRange / RemoveRange, though virtual ICollection<Soil> is generally the recommended declaration to ensure change tracking and lazy loading are supported.

Answer 2

Finally, instead of taking the object from DTO, I take the object from dbContext and update it manually from the DTO, like this:

var zoneDB = _context.Zones.Include(z => z.Soils).Where(z => z.Id == zoneDto.Id).First();

zoneDB.Soils.Clear();
foreach (var soilId in zoneDto.SoilIds)
{
    var soil = _context.Sols.Find(soilId);
    if (sol != null)
        zoneDB.Soils.Add(sol);
}
zoneDB.Name = zoneDto.Name;

_context.Update(zoneDB);
await _context.SaveChangesAsync();

maybe it not so pretty, but it works....

Answer 3

Try _context.Soils.Include(s => s.SoilZones).Where(s => s.Id == soilId) inside your foreach.

Also you don't need foreach here, try something like this:

var zone = _mapper.Map<Zone>(zoneDto);
zone.Soils = _context.Soils
    .Include(s => s.SoilZones)
    .Where(s => zoneDto.SoilIds.Contains(s.Id))
    .ToList();
zone.Country = _context.Country.Find(zoneDto.CountryId);
_context.Update(zone);