1

I had this create method:

[HttpPost]
[Route("")]
/// <summary>
/// Create a team
/// </summary>
/// <param name="model">The team model</param>
/// <returns>The modified team model</returns>
public async Task<IHttpActionResult> Create(TeamBindingViewModel model)
{

    // If our model is invalid, return the errors
    if (!ModelState.IsValid)
        return BadRequest(ModelState);

    // Get all our colours
    var colours = await this.colourService.GetAllAsync();

    // Create our new model
    var team = new Team()
    {
        Name = model.Name,
        Sport = model.Sport
    };

    // For each colour, Add to our team
    team.Colours = colours.Where(m => model.Colours.Any(c => c.Id == m.Id)).ToList();

    // Create our team
    this.service.Create(team);

    // Save our changes
    await this.unitOfWork.SaveChangesAsync();

    // Assign our Id to our model
    model.Id = team.Id;

    // Return Ok
    return Ok(model);
}

As you can see, when a Team is created, I need to add the Colours to the lookup table. To do this, I get the Colours from the database and then filter them by the colours that were passed as part of the model.

That tells Entity Framework that these Colours are not new entities, so it just creates a reference in the lookup table rather than creating new Colours.

Now I want to do the same for the update method.

I tried this:

[HttpPut]
[Route("")]
/// <summary>
/// Update a team
/// </summary>
/// <param name="model">The team model</param>
/// <returns>The modified team model</returns>
public async Task<IHttpActionResult> Update(TeamBindingViewModel model)
{

    // If our model is invalid, return the errors
    if (!ModelState.IsValid)
        return BadRequest(ModelState);

    // Get our current team
    var team = await this.service.GetAsync(model.Id, "Colours");

    // Get all our colours
    var colours = await this.colourService.GetAllAsync();

    // Make changes to our team
    team.Name = model.Name;
    team.Sport = model.Sport;

    // For each colour in our team colours but not in our model colours, remove
    foreach (var colour in team.Colours)
        if (!model.Colours.Any(c => c.Id == colour.Id))
            team.Colours.Remove(colour);

    // For each colour that has to be added, add to our team colours
    if (model.Colours != null)
        foreach (var colour in model.Colours)
            if (!team.Colours.Any(c => c.Id == colour.Id))
                team.Colours.Add(colours.Where(m => m.Id == colour.Id).SingleOrDefault());

    // Update the team
    this.service.Update(team);

    // Save our changes
    await this.unitOfWork.SaveChangesAsync();

    // Return Ok
    return Ok(model);
}

But it didn't work. I got an error stating:

Collection was modified; enumeration operation may not execute.

I know it is talking about the Colours but I have no idea how to get around it.

Perhaps someone has had a similar problem and managed to fix it?

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
r3plica
  • 13,017
  • 23
  • 128
  • 290

3 Answers3

1

The problem here is that you are changing the collection you're iterating on.

If you want to remove (or add) items from a collection, you have to do it in 2 steps :

  1. Select the objects you want to remove
  2. Remove the object

In the case your collection is a list, you can use the List.RemoveAll method

Irwene
  • 2,807
  • 23
  • 48
1

You cannot iterate through collection and change it at the same time. If you really want to do this, you have to store all the values to some place that does not get modified. Easiest way is to use ToArray() or ToList() on collection before iteration through.

...
// For each colour in our team colours but not in our model colours, remove
foreach (var colour in team.Colours.ToList()) // <<== note change here
    if (!model.Colours.Any(c => c.Id == colour.Id))
        team.Colours.Remove(colour);

// For each colour that has to be added, add to our team colours
if (model.Colours != null)
    foreach (var colour in model.Colours.ToList()) // <<== note change here
        if (!team.Colours.Any(c => c.Id == colour.Id))
            team.Colours.Add(colours.Where(m => m.Id == colour.Id).SingleOrDefault());
...
Kaspars Ozols
  • 6,967
  • 1
  • 20
  • 33
-1

Ok, so after the 2 comments on here and doing a bit of searching I came up with this:

// Loop through our colours and remove the ones that are not in our new colour list
for (var i = 0; i < team.Colours.Count; i++)
    if (!model.Colours.Any(c => c.Id == team.Colours[i].Id))
        team.Colours.RemoveAt(i);

which works perfectly.

r3plica
  • 13,017
  • 23
  • 128
  • 290
  • This code is wrong and will skip some items in the collection if any element will be removed. For example: you have `var arr = ["a", "b", "c"]` and in first iteration (`i = 0`) you remove element at position 0 (element `"a"`). After this all the array elements will move one position up and array will be `["b", "c"]`. So, in the next iteration (`i=1`) you will check element at position 1 which will be `"c"` not `"b"`. This is wrong. To fix that, you have to move from bottom to the top. – Kaspars Ozols Apr 21 '15 at 15:50
  • I can see what you are saying, but I have tested this. I selected the first three items then saved it to the database. Then I edited the record and deselected the first item and selected another item and it worked fine. So I then edited it again and deselected everything and selected a new set of colour and it still worked. – r3plica Apr 21 '15 at 16:00
  • Try to remove two colors one next to another in array (lets say first and second) at the same time and you will see what I mean. Second color will be skipped and won't be removed. Or - try following 1) add all colors and update; 2) remove all colors and update; You will see that every 2nd color will not be removed – Kaspars Ozols Apr 21 '15 at 16:04
  • Ok, you are right. Can you post a code example of how to fix this so I can mark yours as the answer please? – r3plica Apr 21 '15 at 16:59
  • I already did. http://stackoverflow.com/a/29775353/3070052 – Kaspars Ozols Apr 21 '15 at 21:42
  • so you did :) I have marked your post as the answer – r3plica Apr 22 '15 at 08:49