0

I started learning C# and I want to update my model using the [HttpPost] annotation. I tried removing the [FromBody]Item itm parameter and the fields on the repository but it's not working either. Below is my code.

Controller:

       [HttpPost("{id}")]
        public ActionResult<Item> UpdateItem([FromBody]Item itm, int id)
        {
            var getItem = _repository.GetItemById(id);
            if (getItem == null)
            {
                return NotFound();
            }

            _repository.UpdateItem(itm);
            _repository.SaveChanges();

            return Ok(getItem);
        }

Repository:

        public void UpdateItem(Item itm)
        {
            if (itm == null)
            {
                throw new ArgumentNullException(nameof(itm));
            }
            var itemToUpdate = this.GetItemById(itm.Id);

            if (itm.Name != null)
            {
                itemToUpdate.Name = itm.Name;
            }
            itemToUpdate.Price = itm.Price;
            itemToUpdate.Condition = itm.Condition;
            itemToUpdate.Size = itm.Size;
            itemToUpdate.DateSold = itm.DateSold;
            itemToUpdate.SellMethod = itm.SellMethod;
            
            _context.Items.Update(itemToUpdate);
        }

Interface:

void UpdateItem(Item itm);

Model:

        public int Id { get; set; }
        [Required]
        public string Name { get; set; }
        public int Price { get; set; }
        public string Condition { get; set; }
        public string Size { get; set; }
        public string DateSold { get; set; }
        public string SellMethod { get; set; }
Norseback
  • 193
  • 2
  • 4
  • 16
  • Do you use a form on your view to post this information? And if so how did you define that form? With razor you can create a form, if you put all the input elements inside of it, it posts the information to the correct controller (based on element names) as an object. – Freek W. Jul 29 '21 at 07:19
  • Putting a "repository" over a DbContext is an **anti**pattern. What's the point of that `_context.Items.Update(itemToUpdate)` ? You already updated `itemToUpdate`. The entity is already being tracked. All you need now is to call `_context.SaveChanges` to persist that and *all* other pending changes. Which shows another problem of the "repository" antipattern - a DbContext is a Unit-of-Work. `SaveChanges` will persist all pending changes, for this and any other entity. You can end up executing 42 DELETEs and 17 UDPATEs on various entities, not just `Items` – Panagiotis Kanavos Jul 29 '21 at 07:46
  • BTW where is `SaveChanges`? As the docs say, [DbSet.Update](https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.dbset-1.update?view=efcore-5.0) `Begins tracking the given entity and entries reachable from the given entity using the Modified state by default,`. It doesn't store changes in the database. That's what `SaveChanges` does – Panagiotis Kanavos Jul 29 '21 at 07:47
  • @PanagiotisKanavos There's not one single way how to do it, and repository pattern is still valid, for example to prevent a massive DbContext class with lots of lots of entities. You just have to care a bit more to get it right, like [here](https://stackoverflow.com/questions/64957036/implementing-the-repository-pattern-correctly-with-ef-core) – DanielD Jul 29 '21 at 10:38
  • @DanielD two wrongs don't make a right and the *real* answer to the very link you posted shows that a DDD repository is nothing like the "repositories" people put over a Unit-of-Work DbContext. A DbContext is a UoW for a bounded context. If you have a massive DbContext, you have a design bug. A DDD repository is **specialized**, dealing with just the entities in its bounded context. Entities plural, not single entities. This is nothing new, the serious problems of putting a single-entity Repo or rather, Data Access Object, over an ORM are known for over a decade. – Panagiotis Kanavos Jul 29 '21 at 11:04
  • @DanielD [Repository is the new Singleton](https://ayende.com/blog/3955/repository-is-the-new-singleton) is from 2009. [Architecting in the pit of doom: The evils of the repository abstraction layer](https://ayende.com/blog/4784/architecting-in-the-pit-of-doom-the-evils-of-the-repository-abstraction-layer) in 2011. Gunnar Peipman's [No need for repositories and unit of work with Entity Framework Core](https://gunnarpeipman.com/ef-core-repository-unit-of-work/) shows the *logical* problems with "repositories". – Panagiotis Kanavos Jul 29 '21 at 11:08
  • What does `GetItemById` do? Does `itm` have a value or is it null? Does `_repository.SaveChanges` call `_context.SaveChanges`? There are too many things missing. The `Repository` code only adds noise and increases the chance of a bug, but the question's code is incomplete. It's impossible to reproduce the problem with just this code. Perhaps it's the `Repository`, perhaps `itm` is null, perhaps the client code posted the wrong JSON payload – Panagiotis Kanavos Jul 29 '21 at 11:50
  • Another helpful [link](https://stackoverflow.com/questions/40853188/frombody-string-parameter-is-giving-null) for the possible mentioned problem, that maybe the item parameter is empty/null. – DanielD Jul 29 '21 at 12:06

1 Answers1

1

First of all verify that you're sending that item correctly:

  • Is the form correct and pointing to that method of your controller?
  • Are you sending that item via the form (have you used the provided methods for this) ?

After that, if you're sending the item in the body of your post request, then verify the item in the method's parameter is available.

EDIT:

Well, as already discussed with Panagiotis you should rather directly use the DbContext itself as it already provides everything you need.

[HttpPost("{id}")]
public ActionResult<Item> UpdateItem(int id, [FromBody]Item itemData)
{
    var foundItem = _dbContext.Items.SingleOrDefault(x => x.Id == id);
    if (foundItem == null)
    {
        return NotFound();
    }

    foundItem.Name = itemData.Name;
    foundItem.Size = itemData.Size;
    // and so on

    _dbContext.SaveChanges();

    return Ok(foundItem);
}

Another way to keep your current structure, but it's not recommended, would be the following:

[HttpPost("{id}")]
public ActionResult<Item> UpdateItem(int id, [FromBody]Item itemData)
{
    var updatedItem = _repository.UpdateItem(id, itemData);
    
    if (updatedItem == null)
    {
        return NotFound();
    }    

    return Ok(updatedItem);
}
public void UpdateItem(int id, Item itemData)
{
    // you can validate parameters and throw errors if e.g. itemData == null

    var originalItem = GetItemById(id); // this should internally get the item e.g. _dbContext.Items.Where(x => x.id == itemData.Id);

    if(originalItem == null)
    {
        return null;
    }

    originalItem.Name = itemData.Name;    
    originalItem.Price = itemData.Price;
    originalItem.Condition = itemData.Condition;
    originalItem.Size = itemData.Size;
    originalItem.DateSold = itemData.DateSold;
    originalItem.SellMethod = itemData.SellMethod;

    SaveChanges(); // guess this will be _dbContext.SaveChanges() instead

    return originalItem;
}

Well, you could also change it to first load the item and then pass the originalItem and the itemData into the UpdateItem method inside your repository. But as you see the better way to directly use the DbContext is more clearer and shorter.

DanielD
  • 326
  • 2
  • 7
  • Actually, the *exact* opposite is needed. There's a reason the docs, courses, books and tutorials show loading an entity, updating it and then *simply calling SaveChanges*. There's a reason they *don't* show any "repository" classes. A DbSet is already a Repository. A DbContext is the Unit-of-Work. All that's needed is to load the entity with `_dbContext.Items.Find` or `FirstOrDefault`, modify it then call `SaveChangges`. No need for `Update` to tell - the entity is *already* tracked, already marked as updated – Panagiotis Kanavos Jul 29 '21 at 07:42
  • @PanagiotisKanavos you should better read next time before downvoting without a valid reason. Yes, usually DbContext and DbSet are enough and already work out of the box, but that's not the questions here. And there's no fault in using a Repository Pattern, actually many people are using it in larger projects to prevent getting a very big DbContext class with lots of lots of DbSets in it. So you provide your DbContext to your Repository for a specific type of object use DbSet there and perform the tasks inside. I also stated that the item was already loaded and should be used to update. – DanielD Jul 29 '21 at 10:32
  • Btw OP just asked why it's not working, not what can be changed/optimized, so no legit reason to downvote this answer, anyway. – DanielD Jul 29 '21 at 10:39
  • The downvote is because the answer is simply wrong. There's no reason to use an `ItemExists ` because `FirstOrDefault` will return an object that's ready to be updated and persisted. It's `ItemExists` that would query twice to get a single object. `_repository.UpdateItem(getItem, itm)` why? The best this code could do is what the action's code is already doing - apply the DTO's values and call `SaveChanges`. There's no need to call `Update` because `getItem` is already tracked and `Modified` – Panagiotis Kanavos Jul 29 '21 at 11:12
  • `Another curiosity is that you're providing an ID parameter` no it's not. The object is already loaded, what's the point of passing that `ID` when `getItem` already has that key? – Panagiotis Kanavos Jul 29 '21 at 11:13
  • 1
    And yes, the best approach would be to simply load the item from DbContext with the provided ID, then simply update those properties with the provided ones from the post body. and then run SaveChanges to store them to database. Just pointed out another solution respecting OP's current layout. You're still free to point out that it's better to use DbContext directly. – DanielD Jul 29 '21 at 11:40
  • 1
    I'm removing the downvote because we have no idea what is going on - does that `GetItemById` return a proper item from the database? Does `Repository.SaveChanges` actually all `_context.SaveChanges`? What does `itm` contain? Is it null, which is a binding problem? Did the client post the correct data? – Panagiotis Kanavos Jul 29 '21 at 11:52
  • @DanielD, I'm explicitly calling the ``SaveChanges`` method under ``DbContext`` for my own brevity. Thanks! Also I'm not using Razor pages or any views for this matter. It's simply a call to the API that I'm testing out – Norseback Jul 29 '21 at 20:38