C# ASP.Net MVC Updating Multiple rows in database is creating extra rows

Question

I have in my database one to two rows that can and will be updated at a given time based on the Id. This is a List<> of data that I'm able to send to the View and submit it to the [HttpPost] method in the controller just fine but when I call the update method I've created, It finds the corresponding rows and goes thru the foreach loop supposedly updating them but really just adds the updated data as two new rows to the table. I cant seem to figure out what Im doing wrong.

Table before update

+------+--------+-------------+------------+-------------+
|  ID  | ItemID | PriceTypeID | ListTypeID | DirectPrice |
+------+--------+-------------+------------+-------------+
| 1021 |  62238 |           2 |          1 |        0.00 |
| 1022 |  62238 |           3 |          1 |        0.00 |
+------+--------+-------------+------------+-------------+

Update Function

public static void UpdateItem(List<PricingOptionDTO> pricingOptionDTO)      
using (DBContext context = new DBContext()) {

 foreach (var item in pricingOptionDTO)
 {
     Entities.PricingOption pricingOptionTb = context.PricingOptions.Where(a => a.ID.Equals(item.ID)).FirstOrDefault();

        if (pricingOptionTb != null)
           {                                                
            pricingOptionTb .DirectPrice = item.DirectPrice;                
            pricingOptionTb .Increment = item.BidIncrement;
            pricingOptionTb .WarningCap = item.BidWarningCap;
           }

         context.PricingOptions.Add(pricingOptionTb);
         context.SaveChanges(); 
   }
       
      context.SaveChanges(); 
}

Table after update

+------+--------+-------------+------------+-------------+
|  ID  | ItemID | PriceTypeID | ListTypeID | DirectPrice |
+------+--------+-------------+------------+-------------+
| 1021 |  62238 |           2 |          1 |        0.00 |
| 1022 |  62238 |           3 |          1 |        0.00 |
| 1023 |  62238 |           2 |          1 |        1.00 |
| 1024 |  62238 |           3 |          1 |        2.00 |
+------+--------+-------------+------------+-------------+

i thought entity framework has an AddOrUpdate method for this case... why dont you use it? — Bobbey
– Bobbey, Commented Mar 16, 2021 at 21:13

Steve Py · Accepted Answer · 2021-03-16 23:21:00Z

2

Something is missing from your example:

context.PricingOptions.Add(PricingOption);

What is "PricingOption" here?

The most obvious looking thing missing from the example that might explain the behaviour you are seeing is the need for an else condition:

 Entities.PricingOption pricingOptionTb = context.PricingOptions.Where(a => a.ID.Equals(item.ID)).SingleOrDefault();

if (pricingOptionTb != null)
{                                                
    pricingOptionTb .DirectPrice = item.DirectPrice;                
    pricingOptionTb .Increment = item.BidIncrement;
    pricingOptionTb .WarningCap = item.BidWarningCap;
}
else 
    context.PricingOptions.Add(PricingOption);

However it's not clear what "PricingOption" is here given you were iterating over a set of DTO. Where this issue commonly appears is when using Automapper to construct an entity from a DTO thinking that because it has an existing ID, adding it to the DbContext will be interpreted as an update. Unfortunately EF only tracks updates against tracked entity references so if you give it a different reference to the same data it will treat it as an insert.

On a side note: When loading an entity where you expect 0 or 1 entity to come back, use SingleOrDefault rather than FirstOrDefault. This provides an assertion that only 1 record should be found (or not). When using the First* methods you should always be expecting and allowing for the possibility of multiple matches, and that being the case always include an OrderBy* clause to ensure your execution and selection are predictable.

Update: To handle update or insert scenarios you would want something like:

 Entities.PricingOption pricingOptionTb = context.PricingOptions.Where(a => a.ID.Equals(item.ID)).SingleOrDefault();

if (pricingOptionTb != null)
{                                                
    pricingOptionTb .DirectPrice = item.DirectPrice;                
    pricingOptionTb .Increment = item.BidIncrement;
    pricingOptionTb .WarningCap = item.BidWarningCap;
}
else 
{
    pricingOptionTb = new PricingOption
    {
        DirectPrice = item.DirectPrice,
        Increment = item.BidIncrement,
        WarningCap = item.BidWarningCap
    }
    context.PricingOptions.Add(pricingOptionTb);
}

Ideally though you should either separate Add vs Update calls explicitly as separate methods and guard against Add requests where a row already exists, or Updates where a row doesn't exist, or consider inspecting the passed in ID or a state value on the DTO. I.e. If ID = 0 it is a new record, otherwise treat it as an existing item that should exist in the DB.

if (item.ID == 0) // New item.
{
    // Consider adding a validation check for duplicate pricing options.
   var duplicatePricingOption == context.PricingOptions
       .Any(x => x.DirectPrice == item.DirectPrice
           && x.Increment == item.Increment
           && x.WarningCap == item.WarningCap);
   if (duplicatePricingOption)
       throw new ArgumentException("An existing pricing option exists for this combination.");

   var pricingOption = new PricingOption
   {
       DirectPrice = item.DirectPrice,
       Increment = item.BidIncrement,
       WarningCap = item.BidWarningCap
   };
   context.PricingOptions.Add(pricingOption);
}
else
{
    var pricingOption = context.PricingOptions.Where(a => a.ID.Equals(item.ID)).Single();

    pricingOption.DirectPrice = item.DirectPrice;                
    pricingOption.Increment = item.BidIncrement;
    pricingOption.WarningCap = item.BidWarningCap;
}

context.SaveChanges();

edited Mar 16, 2021 at 23:21

answered Mar 16, 2021 at 21:23

Steve Py

36.4k4 gold badges34 silver badges58 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

whisk Over a year ago

I updated the code so that it reflects the correct context. pricingOptionTb instead of PricingOption.

Steve Py Over a year ago

That code wouldn't work if no existing row is found because pricingOptionTb would be Null in that case. Did you add the missing else condition around the Add?

whisk Over a year ago

All valid points and a lot of information that I didn't think about and or aware of. I think I was using the Add() incorrectly thinking it was adding the data to the object when I should have been using something else. I am looking to just update the given fields.

whisk Over a year ago

so I really don't even need to Add() anything? just assign it to the pricingOption object?

Steve Py Over a year ago

Yes, if you are updating an existing object and EF is set up the default way (with change tracking enabled) you just fetch it, update the values and call SaveChanges .Add is used to add a new entity to the context, and when you have IDs that are recognized as Identity (auto-generated) this can result in a doubling up since EF will ignore any existing ID on the entity and generate a new one. So if that is an Update only scenario it is better to remove the Add all-together and use Single rather than SingleOrDefault unless it is "ok" that an item might not be found.

wale A · Accepted Answer · 2021-03-16 21:52:29Z

1

You should only use the Add method for new objects. In order to update existing objects, you need to call the Update method.

foreach (var item in pricingOptionDTO)
 {
     Entities.PricingOption pricingOptionTb = context.PricingOptions.Where(a => a.ID.Equals(item.ID)).FirstOrDefault();

        if (pricingOptionTb != null)
           {                                                
            pricingOptionTb .DirectPrice = item.DirectPrice;                
            pricingOptionTb .Increment = item.BidIncrement;
            pricingOptionTb .WarningCap = item.BidWarningCap;

            context.Set<PricingOptions>().Update(pricingOptionTb);
           }
         context.SaveChanges(); 
   }

You can also set the state of the object to 'Modified'

edited Mar 16, 2021 at 21:52

answered Mar 16, 2021 at 21:45

wale A

811 silver badge7 bronze badges

1 Comment

Steve Py Over a year ago

Update would only be needed if the DbContext is set up to not track changes on the entities. If change tracking is used (on by default) then it's better not to use Update or setting the EntityState to Modified so that EF generates UPDATE statements only if values changed, and only for the values that changed. Using Update / Modified entity state results in an UPDATE statement for all columns even if nothing had actually changed.

vander · Accepted Answer · 2021-03-17 03:11:09Z

0

You need to do it this way:

if (pricingOptionTb != null)
{                                                
    pricingOptionTb.DirectPrice = item.DirectPrice;                
    pricingOptionTb.Increment = item.BidIncrement;
    pricingOptionTb.WarningCap = item.BidWarningCap;

    context.Set<Entities.PricingOption>().Attach(pricingOptionTb);
    context.Entry(entity).State = EntityState.Modified;
    context.SaveChanges();
}

answered Mar 17, 2021 at 3:11

vander

18710 bronze badges

Collectives™ on Stack Overflow

C# ASP.Net MVC Updating Multiple rows in database is creating extra rows

3 Answers 3

5 Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

5 Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related