From bf58a4638513a3d5ccc4b5f387cc5b96e140f875 Mon Sep 17 00:00:00 2001 From: Orual Date: Wed, 24 Jun 2026 00:54:06 -0400 Subject: [PATCH] PM-61: typed error classification --- src/appview/writes.rs | 187 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 171 insertions(+), 16 deletions(-) diff --git a/src/appview/writes.rs b/src/appview/writes.rs index 05dd5f7..c76de4b 100644 --- a/src/appview/writes.rs +++ b/src/appview/writes.rs @@ -19,11 +19,15 @@ use jacquard_axum::{ExtractXrpc, XrpcResponse}; use jacquard_common::deps::smol_str::SmolStr; use jacquard_common::types::blob::{BlobRef, MimeType}; use jacquard_common::types::collection::Collection; +use jacquard_common::types::collection::RecordError; use jacquard_common::types::ident::AtIdentifier; use jacquard_common::types::recordkey::{RecordKey, Rkey}; use jacquard_common::types::string::{AtUri, Cid, Datetime, Did}; use jacquard_common::types::value::to_data; +use jacquard_common::xrpc::XrpcError; +use jacquard_common::xrpc::atproto::CreateRecordError; use polymodel_api::app_bsky::actor::profile::Profile as BskyProfile; +use polymodel_api::com_atproto::repo::get_record::GetRecordError; use polymodel_api::com_atproto::repo::strong_ref::StrongRef; use polymodel_api::space_polymodel::actor::ProfileView; use polymodel_api::space_polymodel::actor::bootstrap_profile::{ @@ -530,7 +534,7 @@ pub(super) async fn ensure_polymodel_profile( let existing_remote = match agent.get_record::(&profile_uri).await { Ok(resp) => match resp.into_output() { Ok(output) => Some(output), - Err(e) if display_is_not_found(&e) => None, + Err(e) if into_output_is_not_found(&e) => None, Err(e) => return Err(internal(format!("profile record decode failed: {e}"))), }, Err(e) if client_error_is_not_found(&e) => None, @@ -568,7 +572,7 @@ pub(super) async fn ensure_polymodel_profile( let bsky = match agent.get_record::(&bsky_uri).await { Ok(resp) => match resp.into_output() { Ok(output) => output.value, - Err(e) if display_is_not_found(&e) => empty_bsky_profile(), + Err(e) if into_output_is_not_found(&e) => empty_bsky_profile(), Err(e) => return Err(internal(format!("bsky profile decode failed: {e}"))), }, Err(e) if client_error_is_not_found(&e) => empty_bsky_profile(), @@ -1674,8 +1678,7 @@ fn agent_error(operation: &str, err: jacquard::client::AgentError) -> AppError { } fn client_error(operation: &str, err: jacquard_common::error::ClientError) -> AppError { - let text = format!("{err:?}"); - if text.contains("Auth") || text.contains("Unauthorized") || text.contains("401") { + if err.is_auth() { tracing::warn!(operation, error = %err, "authenticated PDS read failed authorization"); return unauthorized("authenticated PDS read was not authorized"); } @@ -1684,17 +1687,37 @@ fn client_error(operation: &str, err: jacquard_common::error::ClientError) -> Ap } fn client_error_is_not_found(err: &jacquard_common::error::ClientError) -> bool { - let text = format!("{err:?}"); - text.contains("RecordNotFound") || text.contains("not found") || text.contains("404") + err.is_not_found() +} + +/// Trait for XRPC error enums that carry a `RecordNotFound` variant. +trait XrpcNotFound: core::error::Error { + fn is_record_not_found(&self) -> bool; +} + +impl XrpcNotFound for GetRecordError { + fn is_record_not_found(&self) -> bool { + matches!(self, GetRecordError::RecordNotFound(_)) + } +} + +impl XrpcNotFound for RecordError { + fn is_record_not_found(&self) -> bool { + matches!(self, RecordError::RecordNotFound(_)) + } } /// A `getRecord` for a missing record can come back as a successful HTTP /// response whose body decodes (via `into_output()`) to an XRPC `RecordNotFound` -/// error rather than a transport-level `Err`. Detect that from the error's -/// `Display` so the caller can treat the record as absent instead of failing. -fn display_is_not_found(err: &E) -> bool { - let text = err.to_string(); - text.contains("RecordNotFound") || text.contains("not found") || text.contains("404") +/// error rather than a transport-level `Err`. Detect that via the typed +/// `RecordNotFound` variant, with a `Generic` fallback for non-conforming +/// envelopes, so the caller can treat the record as absent. +fn into_output_is_not_found(err: &XrpcError) -> bool { + match err { + XrpcError::Xrpc(e) => e.is_record_not_found(), + XrpcError::Generic(g) => g.error.as_str() == "RecordNotFound", + _ => false, + } } /// An empty `app.bsky.actor.profile` used when an actor has no Bluesky profile @@ -1716,17 +1739,20 @@ fn empty_bsky_profile() -> BskyProfile { } fn agent_error_is_auth(err: &jacquard::client::AgentError) -> bool { - format!("{err:?}").contains("Auth") + err.is_auth() } fn agent_error_is_not_found(err: &jacquard::client::AgentError) -> bool { - let text = format!("{err:?}"); - text.contains("RecordNotFound") || text.contains("not found") || text.contains("404") + err.client_error().is_some_and(|c| c.is_not_found()) } fn agent_error_is_conflict(err: &jacquard::client::AgentError) -> bool { - let text = format!("{err:?}"); - text.contains("AlreadyExists") || text.contains("InvalidSwap") || text.contains("409") + err.source_downcast::().is_some_and(|e| { + matches!( + e, + CreateRecordError::AlreadyExists(_) | CreateRecordError::InvalidSwap(_) + ) + }) || err.client_error().is_some_and(|c| c.is_conflict()) } #[cfg(test)] @@ -2081,4 +2107,133 @@ mod tests { 2 ); } + + // --- PM-61 typed error classification helpers --- + + #[test] + fn client_error_auth_maps_to_unauthorized() { + let err = jacquard_common::error::ClientError::auth( + jacquard_common::error::AuthError::TokenExpired, + ); + let result = client_error("test op", err); + assert!(matches!(result, AppError::Unauthorized(_))); + } + + #[test] + fn client_error_http_401_maps_to_unauthorized() { + let err = jacquard_common::error::ClientError::http(http::StatusCode::UNAUTHORIZED, None); + let result = client_error("test op", err); + assert!(matches!(result, AppError::Unauthorized(_))); + } + + #[test] + fn client_error_other_maps_to_internal() { + let err = jacquard_common::error::ClientError::http( + http::StatusCode::INTERNAL_SERVER_ERROR, + None, + ); + let result = client_error("test op", err); + assert!(!matches!(result, AppError::Unauthorized(_))); + } + + #[test] + fn client_error_is_not_found_typed() { + let err = jacquard_common::error::ClientError::http(http::StatusCode::NOT_FOUND, None); + assert!(client_error_is_not_found(&err)); + + let err = jacquard_common::error::ClientError::http(http::StatusCode::OK, None); + assert!(!client_error_is_not_found(&err)); + } + + #[test] + fn into_output_not_found_via_typed_get_record_error() { + let err: XrpcError = XrpcError::Xrpc(GetRecordError::RecordNotFound(None)); + assert!(into_output_is_not_found(&err)); + } + + #[test] + fn into_output_not_found_via_generic_code() { + let generic = jacquard_common::xrpc::GenericXrpcError { + error: "RecordNotFound".into(), + message: None, + nsid: "com.atproto.repo.getRecord", + method: "GET", + http_status: http::StatusCode::BAD_REQUEST, + }; + let err: XrpcError = XrpcError::Generic(generic); + assert!(into_output_is_not_found(&err)); + } + + #[test] + fn into_output_not_found_via_typed_record_error() { + let err: XrpcError = XrpcError::Xrpc(RecordError::RecordNotFound(None)); + assert!(into_output_is_not_found(&err)); + } + + #[test] + fn into_output_not_found_false_for_auth_error() { + let err: XrpcError = + XrpcError::Auth(jacquard_common::error::AuthError::TokenExpired); + assert!(!into_output_is_not_found(&err)); + } + + #[test] + fn agent_error_auth_typed() { + let err = + jacquard::client::AgentError::auth(jacquard_common::error::AuthError::TokenExpired); + assert!(agent_error_is_auth(&err)); + } + + #[test] + fn agent_error_auth_from_wrapped_client_401() { + let err = jacquard::client::AgentError::from(jacquard_common::error::ClientError::auth( + jacquard_common::error::AuthError::TokenExpired, + )); + assert!(agent_error_is_auth(&err)); + } + + #[test] + fn agent_error_not_found_from_client_404() { + let err = jacquard::client::AgentError::from(jacquard_common::error::ClientError::http( + http::StatusCode::NOT_FOUND, + None, + )); + assert!(agent_error_is_not_found(&err)); + } + + #[test] + fn agent_error_conflict_from_already_exists() { + let err = jacquard::client::AgentError::sub_operation( + "create record", + CreateRecordError::AlreadyExists(None), + ); + assert!(agent_error_is_conflict(&err)); + } + + #[test] + fn agent_error_conflict_from_invalid_swap() { + let err = jacquard::client::AgentError::sub_operation( + "create record", + CreateRecordError::InvalidSwap(None), + ); + assert!(agent_error_is_conflict(&err)); + } + + #[test] + fn agent_error_conflict_from_client_409() { + let err = jacquard::client::AgentError::from(jacquard_common::error::ClientError::http( + http::StatusCode::CONFLICT, + None, + )); + assert!(agent_error_is_conflict(&err)); + } + + #[test] + fn agent_error_not_conflict_for_internal_error() { + let err = jacquard::client::AgentError::from(jacquard_common::error::ClientError::http( + http::StatusCode::INTERNAL_SERVER_ERROR, + None, + )); + assert!(!agent_error_is_conflict(&err)); + } } -- 2.51.2