Should I check if something exists in the db and fail fast or wait for db exception

Question

Having two classes:

public class Parent 
{
    public int Id { get; set; }
    public int ChildId { get; set; }
}

public class Child { ... }

When assigning ChildId to Parent should I check first if it exists in the DB or wait for the DB to throw an exception?

For example (using Entity Framework Core):

NOTE these kinds of checks are ALL OVER THE INTERNET even on official Microsoft's docs: https://docs.microsoft.com/en-us/aspnet/mvc/overview/getting-started/getting-started-with-ef-using-mvc/handling-concurrency-with-the-entity-framework-in-an-asp-net-mvc-application#modify-the-department-controller but there is additional exception handling for SaveChanges

also, note that the main intent of this check was to return friendly message and known HTTP status to the user of the API and not to completely ignore database exceptions. And the only place exception be thrown is inside SaveChanges or SaveChangesAsync call... so there won't be any exception when you call FindAsync or Any. So if child exists but was deleted before SaveChangesAsync then concurrency exception will be thrown.

I did this due to a fact that foreign key violation exception will be much harder to format to display "Child with id {parent.ChildId} could not be found."

public async Task<ActionResult<Parent>> CreateParent(Parent parent)
{
    // is this code redundant?
   // NOTE: its probably better to use Any isntead of FindAsync because FindAsync selects *, and Any selects 1
    var child = await _db.Children.FindAsync(parent.ChildId);
    if (child == null)
       return NotFound($"Child with id {parent.ChildId} could not be found.");

    _db.Parents.Add(parent);    
    await _db.SaveChangesAsync();        

    return parent;
}

versus:

public async Task<ActionResult<Parent>> CreateParent(Parent parent)
{
    _db.Parents.Add(parent);
    await _db.SaveChangesAsync();  // handle exception somewhere globally when child with the specified id doesn't exist...  

    return parent;
}

The second example in Postgres will throw 23503 foreign_key_violation error: https://www.postgresql.org/docs/9.4/static/errcodes-appendix.html

The downside of handling exceptions this way in ORM like EF is that it will work only with a specific database backend. If you ever wanted to switch to SQL server or something else this will not work anymore because the error code will change.

Not formatting the exception properly for the end-user could expose some things you don't want anybody but developers to see.

Related:

https://stackoverflow.com/questions/6171588/preventing-race-condition-of-if-exists-update-else-insert-in-entity-framework

https://stackoverflow.com/questions/4189954/implementing-if-not-exists-insert-using-entity-framework-without-race-conditions

https://stackoverflow.com/questions/308905/should-there-be-a-transaction-for-read-queries

Answer 1

Rather a confused question, but YES you should check first and not just handle a DB exception.

First of all, in your example you are at the data layer, using EF directly on the database to run SQL. You code is equivalent to running

select * from children where id = x
//if no results, perform logic
insert into parents (blah)

The alternative you are suggesting is:

insert into parents (blah)
//if exception, perform logic

Using the exception to execute conditional logic is slow and universally frowned upon.

You do have a race condition and should use a transaction. But this can be fully done in code.

using (var transaction = new TransactionScope())
{
    var child = await _db.Children.FindAsync(parent.ChildId);
    if (child == null) 
    {
       return NotFound($"Child with id {parent.ChildId} could not be found.");
    }

    _db.Parents.Add(parent);    
    await _db.SaveChangesAsync();        
    transaction.Complete();

    return parent;
}

The key thing is to ask yourself:

"Do you expect this situation to occur?"

If not, then sure, insert away and throw an exception. But just handle the exception like any other error that might occur.

If you do expect it to occur, then it is NOT exceptional and you should check to see if the child exists first, responding with the appropriate friendly message if it doesn't.

Edit - There's a lot of controversy over this it seems. Before you downvote consider:

A. What if there were two FK constraints. Would you advocate parsing the exception message to work out which object was missing?

B. If you have a miss, only one SQL statement is run. It's only hits which incur the extra expense of a second query.

C. Usually Id would be a surrogate key, It's hard to imagine a situation where you know one and you aren't pretty sure it's on the DB. Checking would be odd. But what if its a natural key the user has typed in? That could have a high chance of not being present

Answer 2

Checking for uniqueness and then setting is an antipattern; it can always happen that the ID is inserted concurrently between checking time and writing time. Databases are equipped to deal with this problem through mechanisms like constraints and transactions; most programming languages aren't. Therefore, if you value data consistency, leave it to the expert (the database), i.e. do the insert and catch an exception if it occurs.

Answer 3

I think what you call “fail fast” and what I call it is not the same.

Telling the database to make a change and handling the failure, that is fast. Your way is complicated, slow and not particularly reliable.

That technique of yours is not fail fast, it is “preflighting”. There are sometimes good reasons, but not when you use a database.

Answer 4

This started as a comment but grew too large.

No, as the other answers have stated, this pattern should not be used.*

When dealing with systems that use asynchronous components, there will always be a race condition where the database (or file system, or other async system) may change between the check and the change. A check of this type is simply not a reliable way prevent the the type of error you don't want to handle.
Worse than not being sufficient, at a glance it gives the impression that it should prevent the duplicate record error giving a false sense of security.

You need the error handling anyway.

In comments you've asked what if you need data from multiple sources.
Still No.

The fundamental issue does not go away if what you want to check becomes more complex.

You still need the error handling anyway.

Even if this check were a reliable way to prevent the particular error you are trying to guard against, other errors can still occur. What happens if you lose connection to the database, or it runs out of space, or?

You very probably still need other database related error handling anyway. The handling of this particular error should probably be a small piece of it.

If you need data to determine what to change, you obviously will need to collect it from somewhere. (depending on what tools you are using there are probably better ways than separate queries to collect it) If, in examining the data you collected, you determine you don't need to make the change after all, great, don't make the change. This determination is completely separate from error handling concerns.

You still need the error handling anyway.

I know I'm being repetitive but I feel it is important to make this clear. I've cleaned up this mess before.

It will fail eventually. When it does fail it will be difficult and time consuming to get to the bottom of. Resolving issues that arise from race conditions is hard. They don't happen consistently, so it will be difficult or even impossible to reproduce in isolation. You didn't put in the proper error handling to begin with so you won't likely have much to go on: Maybe an end user's report of some cryptic text (which hey you were trying to prevent from seeing in the first place.) Maybe a stack trace that points back to that function that when you look at it blatantly denies the error should even be possible.

*There may be valid business reasons to perform these exists checks, such as to prevent the application from duplicating expensive work, but it is not a suitable replacement for proper error handling.

Answer 5

I think a secondary thing to note here - one of the reasons you want this is so that you can format an error message for the user to see.

I would heartily recommend that you:

a) show the end user the same generic error message for every error that occurs.

b) log the actual exception somewhere that only the developers can access (if on a server) or somewhere that can be sent to you by error reporting tools (if client deployed)

c) don't try and format the error exception details that you log unless you can add more useful information. You don't want to have accidentally 'formatted' away the one piece of useful information that you would have been able to use to track an issue down.

---

In short - exceptions are full of very useful technical information. None of this should be for the end user and you lose this information at your peril.