tenant_status: don't InternalServerError if tenant not found #4337
Conversation
Before this patch, it would use error type `TenantStateError` which has many more error variants than can actually happen with `mgr::get_tenant`. Along the way, I also introduced `SetNewTenantConfigError` because it uses `mgr::get_tenant` and also can only fail in much fewer ways than `TenantStateError` suggests. The new `page_service.rs`'s `GetActiveTimelineError` and `GetActiveTenantError` types were necessary to avoid an `Other` variant on the `GetTenantError`. The context for this PR is me trying to find an alternative to #4291 which is s part of the https://github.com/orgs/neondatabase/projects/38 (async get_value_reconstruct_data). I don't know if this leads to a true alternative for 4291, but, it's a useful cleanup by itself.
Note this also changes the status code to the (correct) 404. Not sure if that's relevant to Console. Context: https://neondb.slack.com/archives/C04PSBP2SAF/p1684746238831449?thread_ts=1684742106.169859&cid=C04PSBP2SAF Atop - #4300 because it cleans up the mgr::get_tenant() error type and I want eyes on that PR.
|
Thanks that's what I asked today |
There was a problem hiding this comment.
So, I've now understood how this patch works and why this patch avoids the stacktrace on the path returning 404.
The change looks a bit sneaky, but the Result::<_, ApiError>::Ok err type is required because of the earlier and following ?. For future readers, here are some breadcrumbs that may or may not help understand any of the above.
My approval was conditional on the understanding that #4300 introduced a conversion from the error to ApiError, which correctly sets status 404. However, that conversion still uses anyhow but somehow ends up formatted differently when reporting the error response and, by doing so, avoids the stacktrace? Yes, anyhow::Error lives within ApiError where it will be formatted only with std::fmt::Display ({0}), not std::fmt::Debug ({0:?}).
Compared to previously used ApiError::InternalServerError which is #[error(transparent)] /* thiserror */. In addition to being #[error(transparent)] , the ApiError::InternalServerError is later reported as {:?}, which is different compared to other ApiError variants, which use {:#}.
The different reporting style for ApiError variants is still required, because thiserror does not generate a Debug impl instead, we use #[derive(Debug)]. If the non-InternalServerError variants were reported with {:?}, it would be in the style of NotFound(long cause path and stacktrace for TenantError::NotFound). With {:#} reporting, the formatting uses what has been specified in the #[error("Not Found: {0}")] attribute and reports the 404 as Not Found: {0} where .0 is the anyhow!("tenant {}", tid) from #4300. Formatting anyhow::Error with std::fmt::Display is the same as dumping the outermost cause string. The # in {:#} or the alternate output has been lost at this stage and has no meaning.
992 tests run: 952 passed, 0 failed, 40 skipped (full report)The comment gets automatically updated with the latest test results
83c2f35 at 2023-05-24T17:45:12.338Z :recycle: |
problame
requested review from
lubennikovaav
and removed request for
a team and
lubennikovaav
Note this also changes the status code to the (correct) 404. Not sure if that's relevant to Console.
Context: https://neondb.slack.com/archives/C04PSBP2SAF/p1684746238831449?thread_ts=1684742106.169859&cid=C04PSBP2SAF
Atop
because it cleans up the mgr::get_tenant() error type and I want eyes on that PR.