CODEWITHSUNDEEP
asp.net-mvc
michael
michael

Reputation: 647

Should we check id in POST action edit or not?

I see everywhere (in each tutorial) something like that:

 public ActionResult Edit(int id = 0)
{
    Employee employee = db.Employees.Find(id);
    if (employee == null)
    {
        return HttpNotFound();
    }

    return View(employee);
}

[HttpPost]
public ActionResult Edit(Employee employee)
{
    if (ModelState.IsValid)
    {
        db.Entry(employee).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index");
    }

    return View(employee);
}

So in GET action edit we check if id exists but in POST action edit not. Shouldn't we check also id in POST action edit? I mean something like that:

[HttpPost]
public ActionResult Edit(Employee employee)
{
    // Check if employe exists in database:
    Employee employeeFromDB = db.Employees.Find(employee.id);
    if (employeeFromDB == null)
    {
        return HttpNotFound();
    }

    if (ModelState.IsValid)
    {
        db.Entry(employee).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index");
    }

    return View(employee);
}

Or maybe this isn't necessary because database doesn't allow to save employee with bad id? What do you think?

Upvotes: 0

Views: 143

Answers (3)

Peter Lea
Peter Lea

Reputation: 1751

One thing I would address earlier rather than later is decoupling your entity model from your view, this is not good practice. What if your entities contain sensitive data? or complex models? This data is viewable in the browser. You potentially end up passing additional data to your view and end up with 'fat' models, keep them small and necessary.

With a View Model you can apply validations to them which will handle your problem for you. These errors will then populate when you call ModelState.IsValid and pass them back to the view. I use Fluent Validation instead of the default MVC annotations out of preference. But check out both / similar and see which suits you.

By separating out your validations into your view model you keep your controllers nice and tidy. It's more effort of course as you need to map them back and forth from your database, but it gives you a great deal of flexibility whereby you can extend them, add validations etc.

Upvotes: 1

Andrew Barber
Andrew Barber

Reputation: 40139

It's actually not necessary, for the reason you note; Your code is going to result in an UPDATE command, but if the id of the entity is not valid, you'll get an error. There's no need to explicitly load it again, for purposes of checking the existence of an entity matching the id...

That said, there are possibly other reasons you want to do that. For instance, to make sure the logged-on user is allowed to edit that particular entity.

Upvotes: 2

user1540857
user1540857

Reputation: 194

There is nothing wrong with doing a sanity check imho. If nothing to give you some error during the development phase.

Upvotes: 0

Related Questions