_context.SaveChanges() works but await _context.SaveChangesAsync() doesn't

Question

I'm struggling to understand something. I have a .Net Core 2.2 Web API, with a MySQL 8 database, and using the Pomelo library to connect to MySQL Server.

I have a PUT action method that looks like this:

// PUT: api/Persons/5
[HttpPut("{id}")]
public async Task<IActionResult> PutPerson([FromRoute] int id, Person person)
{
    if (id != person.Id)
    {
        return BadRequest();
    }

    _context.Entry(person).State = EntityState.Modified;

    try
    {
        _context.SaveChanges(); // Works
        // await _context.SaveChangesAsync(); // Doesn't work
    }
    catch (DbUpdateConcurrencyException)
    {
        if (!PersonExists(id))
        {
            return NotFound();
        }
        else
        {
            throw;
        }
    }

    return NoContent();
}

As per my comments in the code snippet above, when I call _context.SaveChanges(), it works (i.e. it updates the relevant record in the MySQL database, and returns a 1) but when I call await _context.SaveChangesAsync(), it doesn't work (it does not update the record, and it returns a 0). It's not throwing an exception or anything - it just doesn't update the record.

Any ideas?

Answer 1

As I said in my comment above, EF Core has no true sync methods. The sync methods (e.g. SaveChanges) merely block on the async methods (e.g. SaveChangesAsync). As such, it's impossible that SaveChanges would work, if SaveChangesAsync doesn't, as the former just proxies to the latter. There's some other issue at at play here, which is not evident from the code you've provided.

However, the reason I'm writing this as an answer is that the way you're doing this, in general, is wrong, and I believe by doing it right, the problem may disappear. You should never and I mean never directly save an instance created from the request body directly into your database. This provides an attack vector that would allow a malicious user to alter your database in undesirable ways. You've covered that partially by checking the id has not been modified, but a user could still alter things they should not be allowed to.

That security vulnerability aside, there's a practical reason not to do it this way. An API serves as an anti-corruption layer, but only if you decouple your entity from the object the client interacts with. When you use your entity directly, you're tightly coupling your database to your API layer, such that any change at the database level necessitates a new version of your API, and worse, provides no opportunity for deprecating the previous version. All clients must immediately update or their implementations will break. By exposing a DTO class instead to your client, the database can evolve independently of the API, as you can add any anti-corruption logic necessary to bridge the gap between the two.

Long and short, this is how your method should be structured:

// PUT: api/Persons/5
[HttpPut("{id}")]
public async Task<IActionResult> PutPerson([FromRoute] int id, PersonModel model)
{
     // not necessary if using `[ApiController]`
    if (!ModelState.IsValid)
        return BadRequest();

    var person = await _context.People.FindAsync(id);
    if (person == null)
        return NotFound();

    // map `model` onto `person`

    try
    {
        await _context.SaveChangesAsync();
    }
    catch (DbUpdateConcurrencyException)
    {
        // use an optimistic concurrency strategy from:
        // https://learn.microsoft.com/en-us/ef/core/saving/concurrency#resolving-concurrency-conflicts
    }

    return NoContent();
}

I wanted to keep the code straight-forward, but for handling optimistic concurrency, I'd actually recommend using the Polly exception handling library. You can set up retry policies with that which can keep trying to make the update after error correction. Otherwise, you'd need try/catch within try/catch within try/catch, etc. Also, the DbUpdateConcurrencyException is something you should always handle in some way, so re-throwing it makes no sense.