Every error response in the { error, code, details? } format (step 2)
Moves every remaining hand-built error response onto ApiError: ~300
sites across faces, image, video, files, memories, versions,
duplicates, favorites, libraries, auth, clip/unified search, prefs,
reels, tts, insight chat, persona chat, knowledge and personas.
- 500s: the body was the error's text (anyhow / diesel / {:?}); now
ApiError::internal logs it and sends a generic message.
- Each 4xx/502/503 has a stable code and a message written for users;
statuses are unchanged. Shared helpers per module (faces_unavailable,
person_not_found, unknown_library, insight_not_found, ...).
- Pre-format top-level fields kept where clients read them: upload 409
duplicate_of / content_hash / library_name.
- PersonaChatError converts into ApiError in one place.
- Multipart upload errors use the format (error::multipart_config).
- Guard test: no non-test code builds HttpResponse::<error>() by hand.
Closes #202
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
1836f1b9ca
commit
431c3a7492
23 files changed
+894
-542
No files matched your search
+115
@@ -74,6 +74,29 @@ impl ApiError {
|
||||
Self::new(StatusCode::CONFLICT, code, message)
|
||||
}
|
||||
|
||||
pub fn forbidden(code: &'static str, message: impl Into<String>) -> Self {
|
||||
Self::new(StatusCode::FORBIDDEN, code, message)
|
||||
}
|
||||
|
||||
pub fn gone(code: &'static str, message: impl Into<String>) -> Self {
|
||||
Self::new(StatusCode::GONE, code, message)
|
||||
}
|
||||
|
||||
pub fn too_many_requests(code: &'static str, message: impl Into<String>) -> Self {
|
||||
Self::new(StatusCode::TOO_MANY_REQUESTS, code, message)
|
||||
}
|
||||
|
||||
/// 502: a service this server depends on (Apollo, ComfyUI, an LLM, SMS-API)
|
||||
/// failed or answered badly.
|
||||
pub fn bad_gateway(code: &'static str, message: impl Into<String>) -> Self {
|
||||
Self::new(StatusCode::BAD_GATEWAY, code, message)
|
||||
}
|
||||
|
||||
/// 503: a feature that isn't configured, or a dependency that's down.
|
||||
pub fn unavailable(code: &'static str, message: impl Into<String>) -> Self {
|
||||
Self::new(StatusCode::SERVICE_UNAVAILABLE, code, message)
|
||||
}
|
||||
|
||||
/// A 500 for an unexpected failure. `cause` is logged, never sent.
|
||||
pub fn internal(cause: impl fmt::Debug) -> Self {
|
||||
log::error!("Internal error: {:?}", cause);
|
||||
@@ -105,6 +128,12 @@ impl ApiError {
|
||||
self
|
||||
}
|
||||
|
||||
/// The response, for handlers that return `HttpResponse` rather than
|
||||
/// `ApiResult`.
|
||||
pub fn into_response(self) -> HttpResponse {
|
||||
self.error_response()
|
||||
}
|
||||
|
||||
pub fn code(&self) -> &'static str {
|
||||
self.code
|
||||
}
|
||||
@@ -215,6 +244,25 @@ pub fn path_config() -> web::PathConfig {
|
||||
})
|
||||
}
|
||||
|
||||
/// Multipart upload failures (malformed form, over the size limit).
|
||||
pub fn multipart_config(total_limit: usize) -> actix_multipart::form::MultipartFormConfig {
|
||||
actix_multipart::form::MultipartFormConfig::default()
|
||||
.total_limit(total_limit)
|
||||
.error_handler(|err, req: &HttpRequest| {
|
||||
log::warn!("Upload error on {} {}: {}", req.method(), req.uri(), err);
|
||||
let api = if err.to_string().contains("limit") {
|
||||
ApiError::new(
|
||||
StatusCode::PAYLOAD_TOO_LARGE,
|
||||
"body_too_large",
|
||||
"The upload is too large",
|
||||
)
|
||||
} else {
|
||||
ApiError::bad_request("invalid_upload", format!("Invalid upload: {}", err))
|
||||
};
|
||||
actix_web::error::InternalError::from_response(err, api.error_response()).into()
|
||||
})
|
||||
}
|
||||
|
||||
/// The App's default service: an unknown route answers in the same format.
|
||||
pub async fn unknown_route() -> HttpResponse {
|
||||
ApiError::not_found("unknown_route", "No such endpoint").error_response()
|
||||
@@ -268,6 +316,73 @@ mod tests {
|
||||
assert!(!body.to_string().contains("secret"), "{body}");
|
||||
}
|
||||
|
||||
/// Every error response goes through `ApiError` (#202). A handler that
|
||||
/// builds one by hand (`HttpResponse::NotFound()...`) skips the format;
|
||||
/// this fails with the file and line so it can be converted.
|
||||
#[test]
|
||||
fn no_handler_builds_an_error_response_by_hand() {
|
||||
const BUILDERS: &[&str] = &[
|
||||
"BadRequest",
|
||||
"Unauthorized",
|
||||
"Forbidden",
|
||||
"NotFound",
|
||||
"MethodNotAllowed",
|
||||
"Conflict",
|
||||
"Gone",
|
||||
"PayloadTooLarge",
|
||||
"UnprocessableEntity",
|
||||
"TooManyRequests",
|
||||
"RangeNotSatisfiable",
|
||||
"InternalServerError",
|
||||
"NotImplemented",
|
||||
"BadGateway",
|
||||
"ServiceUnavailable",
|
||||
"GatewayTimeout",
|
||||
];
|
||||
fn walk(dir: &std::path::Path, out: &mut Vec<std::path::PathBuf>) {
|
||||
for entry in std::fs::read_dir(dir).unwrap() {
|
||||
let path = entry.unwrap().path();
|
||||
if path.is_dir() {
|
||||
walk(&path, out);
|
||||
} else if path.extension().is_some_and(|e| e == "rs") {
|
||||
out.push(path);
|
||||
}
|
||||
}
|
||||
}
|
||||
let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src");
|
||||
let mut files = Vec::new();
|
||||
walk(&src, &mut files);
|
||||
|
||||
let mut offenders = Vec::new();
|
||||
for file in files {
|
||||
let name = file.to_string_lossy();
|
||||
if name.ends_with("tests.rs") || name.ends_with("/error.rs") {
|
||||
continue;
|
||||
}
|
||||
let text = std::fs::read_to_string(&file).unwrap();
|
||||
// Test modules may assert on hand-built responses.
|
||||
let code = match text.find("#[cfg(test)]\nmod ") {
|
||||
Some(i) => &text[..i],
|
||||
None => &text[..],
|
||||
};
|
||||
for (n, line) in code.lines().enumerate() {
|
||||
if line.trim_start().starts_with("//") {
|
||||
continue;
|
||||
}
|
||||
for b in BUILDERS {
|
||||
if line.contains(&format!("HttpResponse::{b}()")) {
|
||||
offenders.push(format!("{}:{}: {}", name, n + 1, line.trim()));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
offenders.is_empty(),
|
||||
"build these with crate::error::ApiError instead:\n{}",
|
||||
offenders.join("\n")
|
||||
);
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct Q {
|
||||
#[allow(dead_code)]
|
||||
|
||||
Reference in new issue
Block a user