[RFC] Database Query Exception Handling

[CarsonF]

Premise

In ~65% of our DB queries we are only looking for a single row. We use Query.first() to execute the query and return the first row. However that return type is nullable, since the result could be empty. We have these categories of use cases for handling that null:

1. Ignore it. list queries not-null ! assert it and move on.
   

   cord-api-v3/src/components/user/user.repository.ts

   Lines 211 to 212 in 08c2f0a

   	.first();
   	return result!; // result from paginate() will always have 1 row.

2. throw a NotFoundException with a custom message (bad user input)
   

   cord-api-v3/src/components/user/unavailability/unavailability.service.ts

   Lines 100 to 105 in 08c2f0a

   	if (!result) {
   	throw new NotFoundException(
   	'Could not find user associated with unavailability',
   	'user.unavailability'
   	);
   	}

3. throw a ServerException with a custom message (bad DB query or something)
   

   cord-api-v3/src/components/user/user.repository.ts

   Lines 101 to 103 in 08c2f0a

   	if (!result) {
   	throw new ServerException('Failed to create user');
   	}

   cord-api-v3/src/components/user/user.repository.ts

   Lines 351 to 354 in 08c2f0a

   	.first();
   	if (!result) {
   	throw new ServerException('Failed to assign organization to user');
   	}

4. Wrap a single DB query to provide a nicer & known message
   

   cord-api-v3/src/components/user/user.repository.ts

   Line 99 in 08c2f0a

   throw new ServerException('Failed to create user', e);

5. Wrap multiple DB queries & service methods
   

   cord-api-v3/src/components/story/story.service.ts

   Lines 59 to 66 in 08c2f0a

   	return await this.readOne(result.id, session);
   	} catch (exception) {
   	this.logger.error(`Could not create story`, {
   	exception,
   	userId: session.userId,
   	});
   	throw new ServerException('Could not create story', exception);
   	}

   
   Similar to above however if create fails to set some data that the read query is expecting, the read query could return no rows, and in this case we know that problem is actually with the create query.

Analysis

1. I'm comfortable with this. Though it would be good to make it runtime safe.
2. Necessary. We need to throw these specific errors somehow/somewhere.
3. I think the check is good, but I'm not sure the custom message is necessary.
   Esp during dev & query authoring this could happen.
   I believe in the beginning, when this pattern was established, we weren't capturing stack traces correctly on database queries. Now we do, so a standardized/generic throw could still give enough info to debug.
4. Similar to above I'm not sure the custom message adds value. i.e.
   mutation createUser -> { code: "Server", message: "Failed to create user" }
mutation createUser -> { code: "Server", message: "Server Error" }
   I think we can just infer the first error message. If we are worried about what is shown to users, that should probably be a static custom message in the UI codebase.
   On the flip side, is the first error message nicer? Yes.
   It's also could be possible to just do this automatically based on the mutation name.
   Also something to think about for both this and # 3 is if they are ever hit. These are unexpected server errors, I'd like to think that this is a very rare occurrence.
5. This actually makes a logical difference because we want to send back a server error, not a client one. Though this doesn't feel like the best way to do it. If this is just to confirm a DB query is written correctly it should probably be done with a test instead.

Solution?

1.  Query.exactlyOne()

   which throws if there's not 1 row. Easy swap.
2. I think if we were to do anything different here it should be a unique shortcut method for a "single row or not found". Maybe:

    Query.firstOrNotFound(label?: 'user', field?: 'internId')

3. exactlyOne() will work here too. Custom message handling can be addressed in # 4
4. I can't decide.
5. Round trip in tests and move read queries out of try/catch and address custom message in # 4. There are a few cases where we do lots of things in the try/catch block, like several event handlers. I think even in this context it still makes sense to not wrap it.

┆Issue is synchronized with this Monday item by Unito