clean up use of public_error_from_diesel*

[davepacheco]

When running database queries in datastore.rs, we use the public_error_from_diesel* family of functions from nexus/src/db/error.rs to translate database errors into appropriate external API errors. There are a lot of possible cases.

Every database query can result in:

• a 503 error (for a connection problem or a problem running the query at the database)
• a 500 error (if we've run into some unknown problem, which should never happen unless there's a bug -- an example might be if we fail to deserialize a record from the database)

If those are the only two errors that can happen, callers can use public_error_from_diesel_pool_shouldnt_fail() to handle errors. This function explicitly checks for the 503 case and turns anything else into a 500.

Other errors are also possible. These depend on the kind of query we're running. Most queries fall into one of these buckets:

1. INSERT statements that are associated with a "create" operation. These can usually produce a 400-level error indicating a validation problem (e.g., a Project already exists with that name). Errors are handled for these queries using public_error_from_diesel_pool_create, which takes extra arguments that are used to construct a useful "name already exists" message.
2. SELECT statements that are expected to return exactly one item (e.g., lookup of any resource, like an Instance), UPDATE statements that are expected to update one item, and DELETE statements that are expected to delete one item. These produce a 404 when the expected item isn't found. Errors are handled for these queries using public_error_from_diesel_pool, which takes extra arguments that are used to construct a useful "not found" message.
3. SELECT statements that list a page of items (e.g., listing Instances in a Project). These generally don't produce any 400-level error. (You could argue that they should produce a 404 if you, say, try to list Instances in a Project that doesn't exist. But that's generally handled before you get to this query -- some other query already had to map a Project name to an id, and that would have failed if the Project didn't exist. We could check it here also, but it doesn't buy us anything. If you somehow hit this case, that means the Project did exist at one point in your request, so it's valid for us to report 0 Instances found inside it rather than a 404 error.)

I've left out INSERT, UPDATE, and DELETE statements that may affect 0 or more rows because they're not relevant here.

Today, case 3 (SELECTs that can return 0 or more results and still be valid) tend to use public_error_from_diesel_pool with a special LookupType::Other argument with the string "Listing All" or something like that. I don't believe this case can ever happen. For us to produce a 404, we need to get back DieselError::NotFound, which means we've used first_async() or get_result_async() when running the query, which is the way we tell Diesel that we expect one result. Naturally, we never do this when we're expecting 0 or more results.

This change:

• changes most of the list functions in datastore.rs to use public_error_from_diesel_pool_shouldnt_fail rather than public_error_from_diesel_pool. To summarize what I said above: the shouldnt_fail version is intended for cases where a 500 or 503 can still happen, but 400-level errors can't. On the other hand, public_error_from_diesel_pool is structured for cases that can return a 404, which these cannot.
• removes LookupType::Other, since it's no longer used. This makes sense: all lookups are either by id or name. With one exception, the only cases where LookupType::Other was used were cases that couldn't produce a 404 anyway and so shouldn't need to construct a LookupType. In the exceptional case, it was easy to make the appropriate 404 directly.
• renames public_error_from_diesel_pool to public_error_from_diesel_pool_lookup, which I think better reflects that this function is intended for the specific case where you expect exactly one result.
• adds a variant of this function called public_error_from_diesel_pool_authz. Rather than accepting a LookupType, this accepts an authz::ApiResource, which can be used to construct the appropriate 404 error. (Implementors of ApiResource know their own LookupType already. This function avoids callers having to specify it again.)

[smklein]

I like this change - thanks for cleaning it up! - but I still feel bothered by the name public_error_from_diesel_pool_shouldnt_fail. It's weird to call that on an Error type, right? It implies that there shouldn't be an error, but there obviously is one (even if we're claiming it should only be 500-level...)

What do you think about renaming it to public_error_from_diesel_pool_list?

[davepacheco]

I like this change - thanks for cleaning it up! - but I still feel bothered by the name public_error_from_diesel_pool_shouldnt_fail. It's weird to call that on an Error type, right? It implies that there shouldn't be an error, but there obviously is one (even if we're claiming it should only be 500-level...)

What do you think about renaming it to public_error_from_diesel_pool_list?

I hear you about that. But public_error_from_diesel_pool_list isn't right either. This function is also used for some INSERT statements (for loading the built-in users and roles into the database). What unifies the callers of this function is that they cannot fail as a result of user error. They can only fail because we're either buggy or unavailable. ("shouldnt_fail" is short for "shouldnt_fail_under_normal_conditions", not "never_fails".) I'm open to better names but I haven't found one yet!

[smklein]

FWIW, I approved this change, because I don't think we need to hold up the PR on my naming nit.

Other ideas:

• public_error_from_diesel_pool_shouldnt_fail is actually the PoolError -> PublicError case -- it could just be a From impl instead of a separate function? This doesn't invalidate the other translation functions, just makes them more specific.
• Since public_error_from_diesel no longer exists that name could be re-purposed for this?

[davepacheco]

Okay, I landed this as-is for now.

That's an interesting thought. Both of these provide a sort of default conversion from PoolError to PublicError (whether by an explicit conversion in the language or a name that suggests it's the main way to do that conversion). I kind of think we got into a bit of a mess here because we had one function looks like a catch-all, but really there are 3-4 different modes and you really want to think about which mode you're in rather than copying and pasting or picking a default option.

Still, I agree the name here is imprecise and unwieldy.

It might also be the abstraction that's a little fuzzy here. If we had different functions for doing these different types of queries, they could do the appropriate error handling for the query. We could ensure other kinds of consistency here, too. For example: If you use first_async or get_result_async, you want to be using public_error_from_diesel_pool_lookup, since Diesel will return NotFound in that case. If you're not using those, you don't want to be using that function because it can never produce the 404 case. This is not enforced today.

Yet another idea: public_error_from_diesel_pool which accepts an enum like:

enum ErrorKind<'a> {
    ExpectOneAuthz(&'a dyn ApiResource), // "_lookup" variant
    ExpectOneLookup(ResourceType, LookupType), // "_authz" variant
    ExpectNoUserErrors, // "_shouldnt_fail" variant
    ExpectNoConflict(ResourceType, String), // "_create" variant
}

(I know these names are terrible.)

[smklein]

Ooooo I kinda like putting the context in an enum. It centralizes the error conversion (no more "looking for the right function") but it makes it easy to pick a low-cost one. I can toy around with implementing this.