Make all single-row database calls return an Option. #4617
Conversation
- Diesel ordinarily throws an error when no results are returned for a single fetch, which is a bit confusing. This PR ensures that the missing value cases are all caught, and wrapped with new LemmyErrors, rather than diesel errors. - Fixes #4601
dessalines
requested review from
Nutomic,
phiresky,
dullbananas and
SleeplessOne1917
as code owners
dessalines
changed the title
| .filter(community::actor_id.eq(object_id)) | ||
| .first::<Community>(conn) | ||
| .await | ||
| .ok() |
There was a problem hiding this comment.
@Nutomic these were the hacks I was referring to. Without this ok and map, it would've thrown an error.
Was able to get rid of this by using .optional()
| let community = Community::read(&mut context.pool(), community_id).await?; | ||
| let community = Community::read(&mut context.pool(), community_id) | ||
| .await? | ||
| .ok_or(LemmyErrorType::CouldntFindCommunity)?; |
There was a problem hiding this comment.
Better to do this ok_or inside of Community::read, instead of adding it to every single callsite. Same for person, comment etc.
There was a problem hiding this comment.
I'd rather keep LemmyErrors and diesel errors separate, otherwise we'd have to redo all the functions in db_schema. And one of the main points of this was to allow call sites to do their own handling of the option returned. For 90% of cases, the None / missing value can be turned into a lemmy CouldntFind error, but some of them don't convert it to an error, and might just choose to ignore the None case.
I know its more verbose, but I think its more flexible to move all of the missing-single-row errors out of a diesel error, and give the option to convert them to lemmy errors everywhere else.
There was a problem hiding this comment.
Do we really need separate errors for CouldntFindCommunity, CouldntFindPost etc? Why dont we use a single error NotFound instead? Then we can add a much shorter conversion method Option.to_err().
There was a problem hiding this comment.
I second @Nutomic's suggestion. There are already a staggering number of error types, so I'd rather avoid adding more than is necessary.
There was a problem hiding this comment.
A single NotFound error would be a good idea if api endpoints only try to read one type of thing that could throw the error
There was a problem hiding this comment.
There were already a few CouldntFindX errors, probably for cases where you go to a community or person and it can't find it.
Anyway I already did the work, so I don't see the point of going from specific errors to less specific errors.
Also we could simplify them to NotFound in a later PR, when we do breaking changes, because I believe removing the existing ones would be a breaking change.
NotFounderror when no results are returned for a single fetch, which is a bit confusing. This PR ensures that the missing value cases are all caught (by making all these single-row -fetches anOption), and wrapped with new LemmyErrors, rather than diesel errors..optional()to database fetches that shouldn't throw an error for zero-result single-row-fetches. #4601