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

  • 2
    Sharing your research helps everyone. Tell us what you've tried and why it didn’t meet your needs. This demonstrates that you’ve taken the time to try to help yourself, it saves us from reiterating obvious answers, and most of all it helps you get a more specific and relevant answer. Also see How to Ask – gnat Sep 19 at 10:09
  • 5
    As others have mentioned, there exists the possibility that a record could be inserted or deleted concurrently with your checking for NotFound. For that reason, checking first seems like an unacceptable solution. If you are concerned about writing Postgres-specific exception handling that is not portable to other database backends, try to structure the exception handler in such a way that core functionality can be extended by database-specific classes (SQL, Postgres, etc) – billrichards Sep 19 at 13:16
  • 3
    Looking through the comments, I need to say this: stop thinking in platitudes. "Fail fast" is not an isolated, out of context rule that can or should be followed blindly. It's a rule of thumb. Always analyze what you're actually trying to achieve and then consider any technique in light of whether it helps you achieve that goal or not. "Fail fast" helps you prevent unintended side effects. And besides, "fail fast" really means, "fail as soon as you can detect there is a problem." Both techniques fail as soon as a problem is detected, so you must look at other considerations. – jpmc26 Sep 19 at 20:59
  • 1
    @Konrad what do exceptions have to do with it? Stop thinking of race conditions as something that lives in your code: it is a property of the universe. Anything, anything that touches a resource it doesn't completely control (e.g. direct memory access, shared memory, database, REST API, filesystem, etc. etc.) more than once and expects it to be unchanged has a potential race condition. Heck, we deal with this in C which doesn't even have exceptions. Just don't ever branch on the state of a resource you don't control if at least one of the branches messes with the state of that resource. – Jared Smith Sep 20 at 14:05
  • 1
    For a lengthier exposition, see Mr.Mindor's answer: it's really good. – Jared Smith Sep 20 at 14:08
up vote 3 down vote accepted

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

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.

  • 33
    checking and fail is not faster than just "trying" and hope for the best. Former implies 2 operations to be implemented and performed by your system and 2 by the DB, while the latest only implies one of them. Checking is delegated to the DB server. It also implies one less hop into the network and one less task to be attended by the DB. We might think that one more query to the DB is affordable, but we often forget to think in big. Think in high concurrency triggering the query over and over a hundred times. It could dup the whole traffic to the DB. If that matters is up to you to decide. – Laiv Sep 19 at 10:39
  • 6
    @Konrad My position is that the default-correct choice is one query that will fail on its own, and it is the separate query pre-flighting approach that has the burden of proof to justify itself. As for "become a problem": so you are using transactions or otherwise ensuring that you are safe against ToCToU errors, right? It's not obvious to me from the code posted that you are, but if you are not, then it has already become a problem the way that a ticking bomb becomes a problem long before it actually explodes. – mtraceur Sep 19 at 16:55
  • 4
    @Konrad EF Core is not going to implicitly put both your check and the insert into one transaction, you will have to explicitly request that. Without the transaction, checking first is pointless as the database state can change between the the check and insert anyway. Even with a transaction, you might not prevent the database from changing under your feet. We ran in to an issue a few years ago using EF with Oracle where although the db supports it, Entity was not triggering locking of the read records within a transaction, and only the insert was treated as transactional. – Mr.Mindor Sep 19 at 20:55
  • 3
    "Checking for uniqueness and then setting is an antipattern" I would not say this. It depends strongly on whether you can assume no other modifications are happening and on whether the check produces some more useful result (even just an error message that actually means something to the reader) when it does not exist. With a database handling concurrent web requests, no, you can't guarantee other modifications aren't happening, but there are cases when it's a reasonable assumption. – jpmc26 Sep 19 at 21:06
  • 5
    Checking for uniqueness first does not eliminate the need to handle possible failures. On the other hand, if an action would require performing several operations, checking whether all are likely to succeed before starting any of them is often better than performing actions that may likely need to be rolled back. Doing the initial checks may not avoid all situations where a rollback would be necessary, but it could help reduce the frequency of such cases. – supercat Sep 20 at 16:24

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.

  • 1
    There are cases when you need 2nd query when one class depend on another, so you have no choice in cases like that. – Konrad Sep 19 at 11:01
  • 4
    But not here. And database queries can be quite clever, so I generally doubt the “no choice”. – gnasher729 Sep 19 at 11:04
  • 1
    I think it also depends on the application, if you create it just for a few users then it shouldn't make a difference and code is more readable with 2 queries. – Konrad Sep 19 at 11:06
  • 21
    You are assuming that your DB is storing inconsistent data. In other words, looks like you don't trust in your DB and the consistency of the data. If that were the case, you have a really big problem and your solution is a walkaround. A palliative solution fated to be overruled sooner than later. There could be cases where you are forced to consume a DB out of your control and management. From other applications. In those cases, I would consider such validations. In any case, @gnasher is right, yours is not failing fast or it's not what we understand as fail fast. – Laiv Sep 19 at 11:58

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.

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.

  • 2
    "show the end user the same generic error message for every error that occurs." that was the main reason, formatting the exception for end-user looks like a horrible thing to do.. – Konrad Sep 20 at 16:11
  • 1
    In any reasonable database system, you should be able to programmatically find out why something has failed. It shouldn't be necessary to parse an exception message. And more generally: who says that an error message needs to be displayed to the user at all? You can fail the first insert and retry in a loop until you succeed (or up to some limit of retries or time). And in fact, backoff-and-retry is something you're going to want to implement eventually anyway. – Daniel Pryden Sep 20 at 18:40

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.