From b795d070dc28302d4c4ba29b8631865688cfdb59 Mon Sep 17 00:00:00 2001 From: James Lal Date: Thu, 7 May 2026 13:01:14 -0600 Subject: [PATCH 1/2] fix(client): typed multi-response handling with ApiError envelope (closes #8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operations that declared multiple responses (e.g. 200 + 400) silently collapsed into a single Response type — the inline-naming function in analysis.rs ignored its status_code argument so the second schema overwrote the first in the schema registry. The generated client then tried to deserialize success bodies into the error shape. Fix the naming collision and reshape the error story: - ApiError envelope always carries status + headers + raw body, so callers can inspect what the server actually sent without hacking the generated code (the original side-tangent in #8). - ApiOpError = Transport(HttpError) | Api(ApiError) is the new return type for every generated operation method. - Per-operation error enums are emitted when an operation declares non-2xx body schemas; otherwise ApiOpError falls back so the body is still inspectable as JSON. - Response handler reads the body to a string before any typed parse, so deserialization failures preserve the bytes. Breaking: generated method signatures change from HttpResult to Result>. The HttpError::Http variant and from_status helper are removed from generated code (replaced by ApiError). Bump to 0.2.0 per CLAUDE.md "no backwards compat" policy. End-to-end compile tests cover the toy multi-response spec plus the anthropic and openai fixtures so the new shape is exercised against realistic schemas, not just string-search assertions. Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 2 +- Cargo.toml | 2 +- examples/generated/client.rs | 447 +++++++++++++++++++++------ examples/generated/mod.rs | 4 +- examples/generated/types.rs | 17 +- src/analysis.rs | 30 +- src/client_generator.rs | 240 +++++++++++++- src/generator.rs | 129 +++++--- src/http_error.rs | 60 ++++ src/lib.rs | 2 +- tests/full_client_generation_test.rs | 32 +- tests/http_error_test.rs | 45 +-- tests/inline_response_schema_test.rs | 107 +++++++ tests/multi_response_client_test.rs | 219 +++++++++++++ tests/operation_generation_test.rs | 28 +- 15 files changed, 1142 insertions(+), 222 deletions(-) create mode 100644 tests/multi_response_client_test.rs diff --git a/Cargo.lock b/Cargo.lock index b595233..a59f204 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -936,7 +936,7 @@ checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" [[package]] name = "openapi-to-rust" -version = "0.1.15" +version = "0.2.0" dependencies = [ "clap", "heck", diff --git a/Cargo.toml b/Cargo.toml index 4920935..83e5eef 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "openapi-to-rust" -version = "0.1.15" +version = "0.2.0" edition = "2024" rust-version = "1.85.0" authors = ["James Lal"] diff --git a/examples/generated/client.rs b/examples/generated/client.rs index 9c3a7b0..245e0fd 100644 --- a/examples/generated/client.rs +++ b/examples/generated/client.rs @@ -2,9 +2,17 @@ //! //! This file contains the HTTP client implementation for GET, POST, etc. //! Do not edit manually - regenerate using the appropriate script. +#![allow(clippy::format_in_format_args)] +#![allow(clippy::let_unit_value)] use super::types::*; use thiserror::Error; -/// HTTP client errors that can occur during API requests +/// Transport-level errors: failures where we never received an +/// inspectable HTTP response from the server. +/// +/// HTTP responses with non-2xx status codes are surfaced as +/// [`ApiError`] inside [`ApiOpError::Api`], not here, so callers can +/// always inspect status, headers, and the raw body when the server +/// actually responded. #[derive(Error, Debug)] pub enum HttpError { /// Network or connection error (from reqwest) @@ -16,12 +24,6 @@ pub enum HttpError { /// Request serialization error #[error("Failed to serialize request: {0}")] Serialization(String), - /// Response deserialization error - #[error("Failed to deserialize response: {0}")] - Deserialization(String), - /// HTTP error response (4xx, 5xx) - #[error("HTTP error {status}: {message}")] - Http { status: u16, message: String, body: Option }, /// Authentication error #[error("Authentication error: {0}")] Auth(String), @@ -36,46 +38,92 @@ pub enum HttpError { Other(String), } impl HttpError { - /// Create an HTTP error from a status code and message - pub fn from_status( - status: u16, - message: impl Into, - body: Option, - ) -> Self { - Self::Http { - status, - message: message.into(), - body, - } - } /// Create a serialization error pub fn serialization_error(error: impl std::fmt::Display) -> Self { Self::Serialization(error.to_string()) } - /// Create a deserialization error - pub fn deserialization_error(error: impl std::fmt::Display) -> Self { - Self::Deserialization(error.to_string()) + /// Check if this transport error is retryable + pub fn is_retryable(&self) -> bool { + matches!(self, Self::Network(_) | Self::Middleware(_) | Self::Timeout) } - /// Check if this is a client error (4xx) +} +/// Envelope returned for any HTTP response that we received but +/// couldn't (or didn't) treat as a successful typed result. +/// +/// Includes both non-2xx responses and 2xx responses whose body +/// failed to deserialize into the expected success type. `status`, +/// `headers`, and `body` are always populated so callers can +/// inspect what the server sent without modifying the generated +/// code. `typed` carries the parsed per-operation error variant +/// when the body matched a declared schema. +#[derive(Debug, Clone)] +pub struct ApiError { + pub status: u16, + pub headers: reqwest::header::HeaderMap, + pub body: String, + pub typed: Option, + pub parse_error: Option, +} +impl ApiError { pub fn is_client_error(&self) -> bool { - matches!(self, Self::Http { status, .. } if * status >= 400 && * status < 500) + (400..500).contains(&self.status) } - /// Check if this is a server error (5xx) pub fn is_server_error(&self) -> bool { - matches!(self, Self::Http { status, .. } if * status >= 500 && * status < 600) + (500..600).contains(&self.status) } - /// Check if this error is retryable + /// Retry guidance for the response. Mirrors the previous + /// HttpError logic for backwards-compatible retry middleware. pub fn is_retryable(&self) -> bool { + matches!(self.status, 429 | 500 | 502 | 503 | 504) + } +} +impl std::fmt::Display for ApiError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "API error {}: {}", self.status, self.body) + } +} +impl std::error::Error for ApiError {} +/// Result error type returned by every generated operation method. +/// +/// `Transport` covers failures where we never got an inspectable +/// response (network, timeout, middleware, request-side +/// serialization). `Api` covers any case where the server *did* +/// respond — the envelope always carries status + headers + raw +/// body even when the typed deserialize fails. +#[derive(Debug, Error)] +pub enum ApiOpError { + #[error(transparent)] + Transport(#[from] HttpError), + #[error(transparent)] + Api(ApiError), +} +impl ApiOpError { + /// Returns the API envelope when this is an `Api` variant. + pub fn api(&self) -> Option<&ApiError> { match self { - Self::Network(_) => true, - Self::Middleware(_) => true, - Self::Timeout => true, - Self::Http { status, .. } => matches!(status, 429 | 500 | 502 | 503 | 504), - _ => false, + Self::Api(e) => Some(e), + Self::Transport(_) => None, } } + /// True when the underlying error came from the server (i.e. + /// any `Api` variant) rather than the transport layer. + pub fn is_api_error(&self) -> bool { + matches!(self, Self::Api(_)) + } } -/// Result type for HTTP operations +impl From for ApiOpError { + fn from(e: reqwest::Error) -> Self { + Self::Transport(HttpError::Network(e)) + } +} +impl From for ApiOpError { + fn from(e: reqwest_middleware::Error) -> Self { + Self::Transport(HttpError::Middleware(e)) + } +} +/// Result alias for transport-only error paths (e.g. helpers that +/// don't have a per-operation error type). Generated operation +/// methods use [`ApiOpError`] directly. pub type HttpResult = Result; /// Retry configuration for HTTP requests #[derive(Debug, Clone)] @@ -162,17 +210,22 @@ impl HttpClient { self } } +impl Default for HttpClient { + fn default() -> Self { + Self::new() + } +} impl HttpClient { ///POST /todos - pub async fn create_todo(&self, request: CreateTodoRequest) -> HttpResult { - let url = format!("{}{}", self.base_url, "/todos"); + pub async fn create_todo( + &self, + request: CreateTodoRequest, + ) -> Result> { + let request_url = format!("{}{}", self.base_url, "/todos"); let mut req = self .http_client - .post(url) - .body( - serde_json::to_vec(&request) - .map_err(|e| HttpError::serialization_error(e))?, - ) + .post(request_url) + .body(serde_json::to_vec(&request).map_err(HttpError::serialization_error)?) .header("content-type", "application/json"); if let Some(api_key) = &self.api_key { req = req.bearer_auth(api_key); @@ -182,23 +235,66 @@ impl HttpClient { } let response = req.send().await?; let status = response.status(); + let status_code = status.as_u16(); + let headers = response.headers().clone(); + let body_text = response + .text() + .await + .map_err(|e| ApiOpError::Transport(HttpError::Network(e)))?; if status.is_success() { - let body = response - .json() - .await - .map_err(|e| HttpError::deserialization_error(e))?; - Ok(body) + match serde_json::from_str(&body_text) { + Ok(body) => Ok(body), + Err(e) => { + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers: headers, + body: body_text, + typed: None, + parse_error: Some( + format!("failed to deserialize 2xx response body: {}", e), + ), + }), + ) + } + } } else { - let status_code = status.as_u16(); - let message = status.canonical_reason().unwrap_or("Unknown error"); - let body = response.text().await.ok(); - Err(HttpError::from_status(status_code, message, body)) + let typed: Option; + let parse_error: Option; + match status_code { + _ => { + match serde_json::from_str::(&body_text) { + Ok(v) => { + typed = Some(v); + parse_error = None; + } + Err(e) => { + typed = None; + parse_error = Some(e.to_string()); + } + } + } + } + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers, + body: body_text, + typed, + parse_error, + }), + ) } } ///DELETE /todos/{id} - pub async fn delete_todo(&self) -> HttpResult<()> { - let url = format!("{}{}", self.base_url, "/todos/{id}"); - let mut req = self.http_client.delete(url); + pub async fn delete_todo( + &self, + id: impl AsRef, + ) -> Result<(), ApiOpError> { + let request_url = format!( + "{}{}", self.base_url, format!("/todos/{}", id.as_ref()) + ); + let mut req = self.http_client.delete(request_url); if let Some(api_key) = &self.api_key { req = req.bearer_auth(api_key); } @@ -207,23 +303,53 @@ impl HttpClient { } let response = req.send().await?; let status = response.status(); + let status_code = status.as_u16(); + let headers = response.headers().clone(); + let body_text = response + .text() + .await + .map_err(|e| ApiOpError::Transport(HttpError::Network(e)))?; if status.is_success() { - let body = response - .json() - .await - .map_err(|e| HttpError::deserialization_error(e))?; - Ok(body) + let _ = body_text; + let _ = headers; + Ok(()) } else { - let status_code = status.as_u16(); - let message = status.canonical_reason().unwrap_or("Unknown error"); - let body = response.text().await.ok(); - Err(HttpError::from_status(status_code, message, body)) + let typed: Option; + let parse_error: Option; + match status_code { + _ => { + match serde_json::from_str::(&body_text) { + Ok(v) => { + typed = Some(v); + parse_error = None; + } + Err(e) => { + typed = None; + parse_error = Some(e.to_string()); + } + } + } + } + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers, + body: body_text, + typed, + parse_error, + }), + ) } } ///GET /todos/{id} - pub async fn get_todo(&self) -> HttpResult { - let url = format!("{}{}", self.base_url, "/todos/{id}"); - let mut req = self.http_client.get(url); + pub async fn get_todo( + &self, + id: impl AsRef, + ) -> Result> { + let request_url = format!( + "{}{}", self.base_url, format!("/todos/{}", id.as_ref()) + ); + let mut req = self.http_client.get(request_url); if let Some(api_key) = &self.api_key { req = req.bearer_auth(api_key); } @@ -232,23 +358,63 @@ impl HttpClient { } let response = req.send().await?; let status = response.status(); + let status_code = status.as_u16(); + let headers = response.headers().clone(); + let body_text = response + .text() + .await + .map_err(|e| ApiOpError::Transport(HttpError::Network(e)))?; if status.is_success() { - let body = response - .json() - .await - .map_err(|e| HttpError::deserialization_error(e))?; - Ok(body) + match serde_json::from_str(&body_text) { + Ok(body) => Ok(body), + Err(e) => { + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers: headers, + body: body_text, + typed: None, + parse_error: Some( + format!("failed to deserialize 2xx response body: {}", e), + ), + }), + ) + } + } } else { - let status_code = status.as_u16(); - let message = status.canonical_reason().unwrap_or("Unknown error"); - let body = response.text().await.ok(); - Err(HttpError::from_status(status_code, message, body)) + let typed: Option; + let parse_error: Option; + match status_code { + _ => { + match serde_json::from_str::(&body_text) { + Ok(v) => { + typed = Some(v); + parse_error = None; + } + Err(e) => { + typed = None; + parse_error = Some(e.to_string()); + } + } + } + } + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers, + body: body_text, + typed, + parse_error, + }), + ) } } ///GET /todos - pub async fn list_todos(&self) -> HttpResult<()> { - let url = format!("{}{}", self.base_url, "/todos"); - let mut req = self.http_client.get(url); + pub async fn list_todos( + &self, + ) -> Result> { + let request_url = format!("{}{}", self.base_url, "/todos"); + let mut req = self.http_client.get(request_url); if let Some(api_key) = &self.api_key { req = req.bearer_auth(api_key); } @@ -257,29 +423,70 @@ impl HttpClient { } let response = req.send().await?; let status = response.status(); + let status_code = status.as_u16(); + let headers = response.headers().clone(); + let body_text = response + .text() + .await + .map_err(|e| ApiOpError::Transport(HttpError::Network(e)))?; if status.is_success() { - let body = response - .json() - .await - .map_err(|e| HttpError::deserialization_error(e))?; - Ok(body) + match serde_json::from_str(&body_text) { + Ok(body) => Ok(body), + Err(e) => { + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers: headers, + body: body_text, + typed: None, + parse_error: Some( + format!("failed to deserialize 2xx response body: {}", e), + ), + }), + ) + } + } } else { - let status_code = status.as_u16(); - let message = status.canonical_reason().unwrap_or("Unknown error"); - let body = response.text().await.ok(); - Err(HttpError::from_status(status_code, message, body)) + let typed: Option; + let parse_error: Option; + match status_code { + _ => { + match serde_json::from_str::(&body_text) { + Ok(v) => { + typed = Some(v); + parse_error = None; + } + Err(e) => { + typed = None; + parse_error = Some(e.to_string()); + } + } + } + } + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers, + body: body_text, + typed, + parse_error, + }), + ) } } ///PUT /todos/{id} - pub async fn update_todo(&self, request: UpdateTodoRequest) -> HttpResult { - let url = format!("{}{}", self.base_url, "/todos/{id}"); + pub async fn update_todo( + &self, + id: impl AsRef, + request: UpdateTodoRequest, + ) -> Result> { + let request_url = format!( + "{}{}", self.base_url, format!("/todos/{}", id.as_ref()) + ); let mut req = self .http_client - .put(url) - .body( - serde_json::to_vec(&request) - .map_err(|e| HttpError::serialization_error(e))?, - ) + .put(request_url) + .body(serde_json::to_vec(&request).map_err(HttpError::serialization_error)?) .header("content-type", "application/json"); if let Some(api_key) = &self.api_key { req = req.bearer_auth(api_key); @@ -289,17 +496,55 @@ impl HttpClient { } let response = req.send().await?; let status = response.status(); + let status_code = status.as_u16(); + let headers = response.headers().clone(); + let body_text = response + .text() + .await + .map_err(|e| ApiOpError::Transport(HttpError::Network(e)))?; if status.is_success() { - let body = response - .json() - .await - .map_err(|e| HttpError::deserialization_error(e))?; - Ok(body) + match serde_json::from_str(&body_text) { + Ok(body) => Ok(body), + Err(e) => { + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers: headers, + body: body_text, + typed: None, + parse_error: Some( + format!("failed to deserialize 2xx response body: {}", e), + ), + }), + ) + } + } } else { - let status_code = status.as_u16(); - let message = status.canonical_reason().unwrap_or("Unknown error"); - let body = response.text().await.ok(); - Err(HttpError::from_status(status_code, message, body)) + let typed: Option; + let parse_error: Option; + match status_code { + _ => { + match serde_json::from_str::(&body_text) { + Ok(v) => { + typed = Some(v); + parse_error = None; + } + Err(e) => { + typed = None; + parse_error = Some(e.to_string()); + } + } + } + } + Err( + ApiOpError::Api(ApiError { + status: status_code, + headers, + body: body_text, + typed, + parse_error, + }), + ) } } } diff --git a/examples/generated/mod.rs b/examples/generated/mod.rs index eab4700..113f3af 100644 --- a/examples/generated/mod.rs +++ b/examples/generated/mod.rs @@ -1,8 +1,10 @@ //! Generated API modules -//! +//! //! This module exports all generated API types and clients. //! Do not edit manually - regenerate using the appropriate script. +#![allow(unused_imports)] + pub mod types; pub mod client; diff --git a/examples/generated/types.rs b/examples/generated/types.rs index 915eec5..abaf315 100644 --- a/examples/generated/types.rs +++ b/examples/generated/types.rs @@ -3,26 +3,29 @@ //! This file contains all the generated types for the API. //! Do not edit manually - regenerate using the appropriate script. #![allow(clippy::large_enum_variant)] +#![allow(clippy::format_in_format_args)] +#![allow(clippy::let_unit_value)] #![allow(unreachable_patterns)] use serde::{Deserialize, Serialize}; +pub type ListTodosResponse = Vec; #[derive(Debug, Clone, Deserialize, Serialize)] -pub struct CreateTodoRequest { +pub struct Todo { + ///Whether the todo is completed + #[serde(default)] + pub completed: bool, ///Optional description #[serde(skip_serializing_if = "Option::is_none")] pub description: Option, + ///Unique identifier for the todo + pub id: String, ///The todo item title pub title: String, } #[derive(Debug, Clone, Deserialize, Serialize)] -pub struct Todo { - ///Whether the todo is completed - #[serde(default)] - pub completed: bool, +pub struct CreateTodoRequest { ///Optional description #[serde(skip_serializing_if = "Option::is_none")] pub description: Option, - ///Unique identifier for the todo - pub id: String, ///The todo item title pub title: String, } diff --git a/src/analysis.rs b/src/analysis.rs index 282b76e..5b2c5e9 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -3662,14 +3662,32 @@ impl SchemaAnalyzer { Ok(op_info) } - /// Generate a type name for an inline response schema - fn generate_inline_response_type_name(&self, operation_id: &str, _status_code: &str) -> String { + /// Generate a type name for an inline response schema. + /// + /// 200 (the canonical success status) keeps the unsuffixed `{Op}Response` + /// name so simple specs and existing snapshots are unchanged. Every other + /// status code is disambiguated by suffix so that multi-response operations + /// (e.g. 200 + 400) don't collide in the schema registry — see issue #8. + fn generate_inline_response_type_name(&self, operation_id: &str, status_code: &str) -> String { use heck::ToPascalCase; - // Convert operation_id to PascalCase and append Response - // e.g., "app.skills" -> "AppSkillsResponse" - // e.g., "getUser" + "200" -> "GetUserResponse" let base_name = operation_id.replace('.', "_").to_pascal_case(); - format!("{}Response", base_name) + let suffix = Self::status_code_suffix(status_code); + format!("{}Response{}", base_name, suffix) + } + + /// Map an OpenAPI status code key to a suffix for generated type names. + /// + /// "200" → "" (unchanged, the dominant case) + /// "201", "400", "404" → "201", "400", "404" + /// "default" → "Default" + /// "4XX" / "4xx" → "4xx" (lowercased range form) + fn status_code_suffix(status_code: &str) -> String { + match status_code { + "" | "200" => String::new(), + "default" | "Default" => "Default".to_string(), + other if other.chars().all(|c| c.is_ascii_digit()) => other.to_string(), + other => other.to_ascii_lowercase(), + } } /// Generate a type name for an inline request body schema diff --git a/src/client_generator.rs b/src/client_generator.rs index b23a6b6..5c9cb43 100644 --- a/src/client_generator.rs +++ b/src/client_generator.rs @@ -368,8 +368,18 @@ impl CodeGenerator { } } - /// Generate HTTP operation methods for the client + /// Generate HTTP operation methods for the client. + /// + /// Emits per-operation typed error enums (one variant per declared non-2xx + /// response with a body schema) BEFORE the `impl HttpClient` block so the + /// generated method signatures can reference them. pub fn generate_operation_methods(&self, analysis: &SchemaAnalysis) -> TokenStream { + let op_error_enums: Vec = analysis + .operations + .values() + .filter_map(|op| self.generate_op_error_enum(op)) + .collect(); + let methods: Vec = analysis .operations .values() @@ -377,12 +387,93 @@ impl CodeGenerator { .collect(); quote! { + #(#op_error_enums)* + impl HttpClient { #(#methods)* } } } + /// Generate the per-operation typed error enum, if the op has any non-2xx + /// responses with a body schema. Returns None when the op has no declared + /// error bodies — those operations use `ApiOpError` so + /// the raw response body is still inspectable. + fn generate_op_error_enum(&self, op: &OperationInfo) -> Option { + let variants: Vec<(String, String)> = op + .response_schemas + .iter() + .filter(|(code, _)| !code.starts_with('2')) + .map(|(code, schema)| (code.clone(), schema.clone())) + .collect(); + + if variants.is_empty() { + return None; + } + + let enum_ident = self.op_error_enum_ident(op); + let variant_decls: Vec = variants + .iter() + .map(|(code, schema)| { + let variant_ident = Self::op_error_variant_ident(code); + let payload_ty_name = self.to_rust_type_name(schema); + let payload_ty = + syn::Ident::new(&payload_ty_name, proc_macro2::Span::call_site()); + quote! { #variant_ident(#payload_ty) } + }) + .collect(); + + let doc = format!( + "Typed error responses for `{}`. One variant per declared non-2xx response.", + op.operation_id + ); + + Some(quote! { + #[doc = #doc] + #[derive(Debug, Clone)] + pub enum #enum_ident { + #(#variant_decls,)* + } + }) + } + + /// Type name (Ident) for the per-op error enum, e.g. `ListTodosApiError`. + fn op_error_enum_ident(&self, op: &OperationInfo) -> syn::Ident { + use heck::ToPascalCase; + let name = format!( + "{}ApiError", + op.operation_id.replace('.', "_").to_pascal_case() + ); + syn::Ident::new(&name, proc_macro2::Span::call_site()) + } + + /// Variant name for a status code: "400" → Status400, "default" → Default, + /// "4XX" → Status4xx. + fn op_error_variant_ident(status_code: &str) -> syn::Ident { + let raw = match status_code { + "default" | "Default" => "Default".to_string(), + other if other.chars().all(|c| c.is_ascii_digit()) => format!("Status{other}"), + other => format!("Status{}", other.to_ascii_lowercase()), + }; + syn::Ident::new(&raw, proc_macro2::Span::call_site()) + } + + /// Token stream for the type plugged into `ApiOpError` for an op: + /// either the per-op enum, or `serde_json::Value` for ops with no + /// declared error body schemas. + fn op_error_type_token(&self, op: &OperationInfo) -> TokenStream { + if op + .response_schemas + .iter() + .any(|(code, _)| !code.starts_with('2')) + { + let ident = self.op_error_enum_ident(op); + quote! { #ident } + } else { + quote! { serde_json::Value } + } + } + /// Generate a single operation method fn generate_single_operation_method(&self, op: &OperationInfo) -> TokenStream { let method_name = self.get_method_name(op); @@ -393,7 +484,8 @@ impl CodeGenerator { let query_params = self.generate_query_params(op); let response_type = self.get_response_type(op); let has_response_body = self.get_success_response_schema(op).is_some(); - let error_handling = self.generate_error_handling(has_response_body); + let op_error_type = self.op_error_type_token(op); + let error_handling = self.generate_error_handling(op, has_response_body); let url_construction = self.generate_url_construction(path, op); let doc_comment = self.generate_operation_doc_comment(op); @@ -402,7 +494,7 @@ impl CodeGenerator { pub async fn #method_name( &self, #request_param - ) -> HttpResult<#response_type> { + ) -> Result<#response_type, ApiOpError<#op_error_type>> { #url_construction let mut req = self.http_client @@ -679,31 +771,153 @@ impl CodeGenerator { /// Generate error handling. /// - /// When `has_response_body` is false the endpoint returns no JSON body - /// (e.g. 204 No Content) and we skip deserialization entirely. - fn generate_error_handling(&self, has_response_body: bool) -> TokenStream { + /// Always reads the response body to a string before attempting any typed + /// deserialization, so the raw body and headers are preserved on the error + /// path even when JSON parsing fails. On 2xx the body is parsed into the + /// success type; on non-2xx the body is parsed into the matching variant + /// of the per-operation error enum (when one is declared) and wrapped in + /// `ApiError`. + fn generate_error_handling(&self, op: &OperationInfo, has_response_body: bool) -> TokenStream { + let op_error_type = self.op_error_type_token(op); + let success_branch = if has_response_body { quote! { - let body = response.json().await - .map_err(HttpError::deserialization_error)?; - Ok(body) + match serde_json::from_str(&body_text) { + Ok(body) => Ok(body), + Err(e) => Err(ApiOpError::Api(ApiError { + status: status_code, + headers: headers, + body: body_text, + typed: None, + parse_error: Some(format!( + "failed to deserialize 2xx response body: {}", + e + )), + })), + } } } else { quote! { + let _ = body_text; + let _ = headers; Ok(()) } }; + let error_match_arms = self.generate_error_match_arms(op); + quote! { let status = response.status(); + let status_code = status.as_u16(); + let headers = response.headers().clone(); + let body_text = response.text().await + .map_err(|e| ApiOpError::Transport(HttpError::Network(e)))?; if status.is_success() { #success_branch } else { - let status_code = status.as_u16(); - let message = status.canonical_reason().unwrap_or("Unknown error"); - let body = response.text().await.ok(); - Err(HttpError::from_status(status_code, message, body)) + let typed: Option<#op_error_type>; + let parse_error: Option; + #error_match_arms + Err(ApiOpError::Api(ApiError { + status: status_code, + headers, + body: body_text, + typed, + parse_error, + })) + } + } + } + + /// Generate the match arms that select which per-op error variant to + /// deserialize the response body into based on the runtime status code. + fn generate_error_match_arms(&self, op: &OperationInfo) -> TokenStream { + let arms: Vec = op + .response_schemas + .iter() + .filter(|(code, _)| !code.starts_with('2')) + .filter_map(|(code, schema)| { + let variant_ident = Self::op_error_variant_ident(code); + let payload_ty_name = self.to_rust_type_name(schema); + let payload_ty = + syn::Ident::new(&payload_ty_name, proc_macro2::Span::call_site()); + let enum_ident = self.op_error_enum_ident(op); + + let pattern = match code.as_str() { + "default" | "Default" => return None, // handled in fallback + other if other.chars().all(|c| c.is_ascii_digit()) => { + let n: u16 = other.parse().ok()?; + quote! { #n } + } + // Range like "4XX" / "5XX" — fall through to generic + // for now; declared-range handling is a follow-up. + _ => return None, + }; + + Some(quote! { + #pattern => { + match serde_json::from_str::<#payload_ty>(&body_text) { + Ok(v) => { + typed = Some(#enum_ident::#variant_ident(v)); + parse_error = None; + } + Err(e) => { + typed = None; + parse_error = Some(e.to_string()); + } + } + } + }) + }) + .collect(); + + // Fallback for "default" or undeclared status codes: try to parse + // as `serde_json::Value` for inspectability when the op's error + // type is generic, otherwise leave typed = None. + let has_typed_enum = op + .response_schemas + .iter() + .any(|(code, _)| !code.starts_with('2') && !matches!(code.as_str(), "default" | "Default")); + + let default_arm = if has_typed_enum { + quote! { + _ => { + typed = None; + parse_error = None; + } + } + } else { + // No typed enum — op_error_type is serde_json::Value. + quote! { + _ => { + match serde_json::from_str::(&body_text) { + Ok(v) => { + typed = Some(v); + parse_error = None; + } + Err(e) => { + typed = None; + parse_error = Some(e.to_string()); + } + } + } + } + }; + + if arms.is_empty() { + // No declared status arms — just the fallback. + quote! { + match status_code { + #default_arm + } + } + } else { + quote! { + match status_code { + #(#arms)* + #default_arm + } } } } diff --git a/src/generator.rs b/src/generator.rs index 987b0c6..572a7c4 100644 --- a/src/generator.rs +++ b/src/generator.rs @@ -381,7 +381,13 @@ impl CodeGenerator { quote! { use thiserror::Error; - /// HTTP client errors that can occur during API requests + /// Transport-level errors: failures where we never received an + /// inspectable HTTP response from the server. + /// + /// HTTP responses with non-2xx status codes are surfaced as + /// [`ApiError`] inside [`ApiOpError::Api`], not here, so callers can + /// always inspect status, headers, and the raw body when the server + /// actually responded. #[derive(Error, Debug)] pub enum HttpError { /// Network or connection error (from reqwest) @@ -396,18 +402,6 @@ impl CodeGenerator { #[error("Failed to serialize request: {0}")] Serialization(String), - /// Response deserialization error - #[error("Failed to deserialize response: {0}")] - Deserialization(String), - - /// HTTP error response (4xx, 5xx) - #[error("HTTP error {status}: {message}")] - Http { - status: u16, - message: String, - body: Option, - }, - /// Authentication error #[error("Authentication error: {0}")] Auth(String), @@ -426,51 +420,108 @@ impl CodeGenerator { } impl HttpError { - /// Create an HTTP error from a status code and message - pub fn from_status(status: u16, message: impl Into, body: Option) -> Self { - Self::Http { - status, - message: message.into(), - body, - } - } - /// Create a serialization error pub fn serialization_error(error: impl std::fmt::Display) -> Self { Self::Serialization(error.to_string()) } - /// Create a deserialization error - pub fn deserialization_error(error: impl std::fmt::Display) -> Self { - Self::Deserialization(error.to_string()) + /// Check if this transport error is retryable + pub fn is_retryable(&self) -> bool { + matches!(self, Self::Network(_) | Self::Middleware(_) | Self::Timeout) } + } - /// Check if this is a client error (4xx) + /// Envelope returned for any HTTP response that we received but + /// couldn't (or didn't) treat as a successful typed result. + /// + /// Includes both non-2xx responses and 2xx responses whose body + /// failed to deserialize into the expected success type. `status`, + /// `headers`, and `body` are always populated so callers can + /// inspect what the server sent without modifying the generated + /// code. `typed` carries the parsed per-operation error variant + /// when the body matched a declared schema. + #[derive(Debug, Clone)] + pub struct ApiError { + pub status: u16, + pub headers: reqwest::header::HeaderMap, + pub body: String, + pub typed: Option, + pub parse_error: Option, + } + + impl ApiError { pub fn is_client_error(&self) -> bool { - matches!(self, Self::Http { status, .. } if *status >= 400 && *status < 500) + (400..500).contains(&self.status) } - /// Check if this is a server error (5xx) pub fn is_server_error(&self) -> bool { - matches!(self, Self::Http { status, .. } if *status >= 500 && *status < 600) + (500..600).contains(&self.status) } - /// Check if this error is retryable + /// Retry guidance for the response. Mirrors the previous + /// HttpError logic for backwards-compatible retry middleware. pub fn is_retryable(&self) -> bool { + matches!(self.status, 429 | 500 | 502 | 503 | 504) + } + } + + impl std::fmt::Display for ApiError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "API error {}: {}", self.status, self.body) + } + } + + impl std::error::Error for ApiError {} + + /// Result error type returned by every generated operation method. + /// + /// `Transport` covers failures where we never got an inspectable + /// response (network, timeout, middleware, request-side + /// serialization). `Api` covers any case where the server *did* + /// respond — the envelope always carries status + headers + raw + /// body even when the typed deserialize fails. + #[derive(Debug, Error)] + pub enum ApiOpError { + #[error(transparent)] + Transport(#[from] HttpError), + + #[error(transparent)] + Api(ApiError), + } + + impl ApiOpError { + /// Returns the API envelope when this is an `Api` variant. + pub fn api(&self) -> Option<&ApiError> { match self { - Self::Network(_) => true, - Self::Middleware(_) => true, - Self::Timeout => true, - Self::Http { status, .. } => { - // Retry on 429 (rate limit), 500, 502, 503, 504 - matches!(status, 429 | 500 | 502 | 503 | 504) - } - _ => false, + Self::Api(e) => Some(e), + Self::Transport(_) => None, } } + + /// True when the underlying error came from the server (i.e. + /// any `Api` variant) rather than the transport layer. + pub fn is_api_error(&self) -> bool { + matches!(self, Self::Api(_)) + } + } + + // Direct From impls so `?` works without going through HttpError + // first. Rust's `?` only chains a single `From` conversion. + impl From for ApiOpError { + fn from(e: reqwest::Error) -> Self { + Self::Transport(HttpError::Network(e)) + } + } + + impl From for ApiOpError { + fn from(e: reqwest_middleware::Error) -> Self { + Self::Transport(HttpError::Middleware(e)) + } } - /// Result type for HTTP operations + /// Result alias for transport-only error paths (e.g. helpers that + /// don't have a per-operation error type). Generated operation + /// methods use [`ApiOpError`] directly. pub type HttpResult = Result; } } diff --git a/src/http_error.rs b/src/http_error.rs index ef6f470..f4afaa8 100644 --- a/src/http_error.rs +++ b/src/http_error.rs @@ -203,3 +203,63 @@ impl HttpError { /// Result type for HTTP operations pub type HttpResult = Result; + +/// Envelope for an API response we received but couldn't (or didn't) treat as success. +/// +/// `ApiError` is returned whenever the server actually responded — whether the +/// status was non-2xx, or the 2xx body failed to deserialize into the expected +/// type. `status`, `headers`, and `body` are always populated so callers can +/// inspect what the server actually sent without having to hack the generated +/// client. `typed` is `Some(_)` when the raw body was successfully parsed into a +/// per-operation error type; `parse_error` records why parsing failed when not. +#[derive(Debug, Clone)] +pub struct ApiError { + pub status: u16, + pub headers: reqwest::header::HeaderMap, + pub body: String, + pub typed: Option, + pub parse_error: Option, +} + +impl ApiError { + pub fn is_client_error(&self) -> bool { + (400..500).contains(&self.status) + } + + pub fn is_server_error(&self) -> bool { + (500..600).contains(&self.status) + } +} + +impl std::fmt::Display for ApiError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "API error {}: {}", self.status, self.body) + } +} + +impl std::error::Error for ApiError {} + +/// Result error type for generated operation methods. +/// +/// `Transport` covers failures where the request never produced a response we +/// can inspect (network, timeout, middleware, request-side serialization). +/// `Api` covers any case where the server *did* respond — the envelope always +/// carries status + headers + raw body even when the typed deserialize fails. +#[derive(Debug, thiserror::Error)] +pub enum ApiOpError { + #[error(transparent)] + Transport(#[from] HttpError), + + #[error(transparent)] + Api(ApiError), +} + +impl ApiOpError { + /// Convenience accessor: if this is an Api variant, return the envelope. + pub fn api(&self) -> Option<&ApiError> { + match self { + Self::Api(e) => Some(e), + Self::Transport(_) => None, + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 5f30292..6970263 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,7 +18,7 @@ pub use config::ConfigFile; pub use error::GeneratorError; pub use generator::{CodeGenerator, GeneratedFile, GenerationResult, GeneratorConfig}; pub use http_config::{AuthConfig, HttpClientConfig, RetryConfig}; -pub use http_error::{HttpError, HttpResult}; +pub use http_error::{ApiError, ApiOpError, HttpError, HttpResult}; pub use openapi::{OpenApiSpec, Schema, SchemaType}; pub type Result = std::result::Result; diff --git a/tests/full_client_generation_test.rs b/tests/full_client_generation_test.rs index 9b8acb3..3a0dab5 100644 --- a/tests/full_client_generation_test.rs +++ b/tests/full_client_generation_test.rs @@ -402,7 +402,7 @@ fn test_full_client_includes_errors() { .generate_http_client(&analysis) .expect("Failed to generate HTTP client"); - // Verify all error types are present + // Verify transport-error machinery is present assert!( client_code.contains("pub enum HttpError"), "Should include HttpError enum" @@ -416,22 +416,34 @@ fn test_full_client_includes_errors() { "Should include Serialization error variant" ); assert!( - client_code.contains("Deserialization(String)"), - "Should include Deserialization error variant" + client_code.contains("pub type HttpResult"), + "Should include HttpResult type alias" ); + + // Verify the new typed-response error envelope and operation result + // wrapper are emitted (replaces the old `HttpError::Http { ... }` + // variant — see issue #8 for the design). assert!( - client_code.contains("Http {"), - "Should include Http error variant with status and message" + client_code.contains("pub struct ApiError"), + "Should include ApiError envelope struct" ); assert!( - client_code.contains("pub type HttpResult"), - "Should include HttpResult type alias" + client_code.contains("pub enum ApiOpError"), + "Should include ApiOpError enum" + ); + assert!( + client_code.contains("Transport(#[from] HttpError)"), + "ApiOpError should have Transport variant carrying HttpError" ); - // Verify helper methods + // Verify helper methods on the new envelope + assert!( + client_code.contains("pub fn is_client_error"), + "Should include is_client_error helper" + ); assert!( - client_code.contains("pub fn from_status"), - "Should include from_status helper" + client_code.contains("pub fn is_server_error"), + "Should include is_server_error helper" ); assert!( client_code.contains("pub fn is_retryable"), diff --git a/tests/http_error_test.rs b/tests/http_error_test.rs index 44bd7d0..b5b659f 100644 --- a/tests/http_error_test.rs +++ b/tests/http_error_test.rs @@ -247,13 +247,11 @@ fn test_generated_error_code() { let client_content = &client_file.expect("client file").content; - // Verify the generated code contains HttpError enum + // Transport-level error machinery assert!( client_content.contains("pub enum HttpError"), "Generated code should contain HttpError enum" ); - - // Verify error variants assert!( client_content.contains("Network(#[from] reqwest::Error)"), "Generated code should contain Network variant" @@ -262,26 +260,6 @@ fn test_generated_error_code() { client_content.contains("Serialization(String)"), "Generated code should contain Serialization variant" ); - assert!( - client_content.contains("Deserialization(String)"), - "Generated code should contain Deserialization variant" - ); - assert!( - client_content.contains("Http {"), - "Generated code should contain Http variant" - ); - assert!( - client_content.contains("status: u16"), - "Generated code should contain status field in Http variant" - ); - assert!( - client_content.contains("message: String"), - "Generated code should contain message field in Http variant" - ); - assert!( - client_content.contains("body: Option"), - "Generated code should contain body field in Http variant" - ); assert!( client_content.contains("Auth(String)"), "Generated code should contain Auth variant" @@ -299,18 +277,25 @@ fn test_generated_error_code() { "Generated code should contain Other variant" ); - // Verify helper methods + // New typed-response envelope (replaces the old HttpError::Http variant + // and from_status helper — see issue #8). assert!( - client_content.contains("pub fn from_status"), - "Generated code should contain from_status method" + client_content.contains("pub struct ApiError"), + "Generated code should contain ApiError struct" ); assert!( - client_content.contains("pub fn serialization_error"), - "Generated code should contain serialization_error method" + client_content.contains("pub enum ApiOpError"), + "Generated code should contain ApiOpError enum" ); assert!( - client_content.contains("pub fn deserialization_error"), - "Generated code should contain deserialization_error method" + client_content.contains("Transport(#[from] HttpError)"), + "ApiOpError should have Transport variant carrying HttpError" + ); + + // Helpers on the new envelope + assert!( + client_content.contains("pub fn serialization_error"), + "Generated code should contain serialization_error method" ); assert!( client_content.contains("pub fn is_client_error"), diff --git a/tests/inline_response_schema_test.rs b/tests/inline_response_schema_test.rs index f00f4b6..b91d809 100644 --- a/tests/inline_response_schema_test.rs +++ b/tests/inline_response_schema_test.rs @@ -120,6 +120,113 @@ fn test_inline_object_response_analyzed() { insta::assert_debug_snapshot!(op.response_schemas); } +#[test] +fn test_multiple_inline_responses_have_distinct_schemas() { + // Regression test for issue #8: when an operation declares multiple responses + // with inline schemas (e.g. 200 success + 400 error), each must produce a + // distinct registered schema. Previously the inline naming function ignored + // the status code, so the 400 schema overwrote the 200 in the schema registry + // and the generated client tried to deserialize success bodies as the error + // shape. + let spec = serde_json::json!({ + "openapi": "3.1.0", + "info": { "title": "Test API", "version": "1.0.0" }, + "paths": { + "/todos": { + "get": { + "operationId": "listTodos", + "responses": { + "200": { + "description": "Success", + "content": { + "application/json": { + "schema": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { "type": "string" }, + "title": { "type": "string" } + }, + "required": ["id", "title"] + } + } + } + } + }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "error": { "type": "string" } + }, + "required": ["error"] + } + } + } + } + } + } + } + }, + "components": { + "schemas": { + "Placeholder": { + "type": "object", + "properties": { "id": { "type": "string" } } + } + } + } + }); + + let mut analyzer = SchemaAnalyzer::new(spec).expect("Failed to create analyzer"); + let analysis = analyzer.analyze().expect("Failed to analyze"); + + let op = analysis + .operations + .get("listTodos") + .expect("Operation not found"); + + // Both status codes must be present. + let s200 = op + .response_schemas + .get("200") + .expect("200 response missing from response_schemas"); + let s400 = op + .response_schemas + .get("400") + .expect("400 response missing from response_schemas"); + + // The two responses must map to *distinct* registered schemas. + assert_ne!( + s200, s400, + "200 and 400 inline response schemas collided into the same name: {s200}" + ); + + // Both names must actually exist in the schema registry. + assert!( + analysis.schemas.contains_key(s200), + "200 schema {s200} not registered" + ); + assert!( + analysis.schemas.contains_key(s400), + "400 schema {s400} not registered" + ); + + // Sanity: the registered schemas should differ structurally — the 200 is an + // array wrapper / object with id+title, the 400 is an object with `error`. + let analyzed_200 = &analysis.schemas[s200]; + let analyzed_400 = &analysis.schemas[s400]; + assert_ne!( + format!("{:?}", analyzed_200.schema_type), + format!("{:?}", analyzed_400.schema_type), + "200 and 400 schemas resolved to the same SchemaType — collision not actually fixed" + ); +} + #[test] fn test_ref_response_still_works() { // Ensure we don't break the existing $ref handling diff --git a/tests/multi_response_client_test.rs b/tests/multi_response_client_test.rs new file mode 100644 index 0000000..9f4b4d0 --- /dev/null +++ b/tests/multi_response_client_test.rs @@ -0,0 +1,219 @@ +//! End-to-end test for the multi-response error envelope work in issue #8. +//! +//! These tests generate a complete client (types + http_client) for a spec +//! that exercises the new code paths — multiple responses with different +//! schemas, declared error bodies, and ops with no error bodies — and then +//! invoke `cargo check` on the generated crate to verify the emitted code is +//! not just syntactically valid Rust but actually compiles against the +//! reqwest / serde / thiserror runtime it depends on. + +use openapi_to_rust::{CodeGenerator, GeneratorConfig, SchemaAnalyzer}; +use std::fs; +use std::path::Path; +use std::process::Command; +use tempfile::TempDir; + +const CLIENT_CARGO_TOML: &str = r#" +[package] +name = "{name}" +version = "0.1.0" +edition = "2021" + +[dependencies] +serde = { version = "1.0", features = ["derive"] } +serde_json = "1.0" +reqwest = { version = "0.12", features = ["json", "multipart"] } +reqwest-middleware = { version = "0.4", features = ["multipart"] } +thiserror = "1.0" +tokio = { version = "1.0", features = ["full"] } +validator = { version = "0.20", features = ["derive"] } +"#; + +/// Generate types + http_client for a spec, write a compilable crate to a +/// temp dir, run `cargo check`, and panic if it fails. Returns the temp dir +/// so callers can keep it alive long enough for inspection on failure. +fn assert_generates_compiling_client(name: &str, spec: serde_json::Value) -> TempDir { + let mut analyzer = SchemaAnalyzer::new(spec).expect("analyzer"); + let mut analysis = analyzer.analyze().expect("analyze"); + + let config = GeneratorConfig { + module_name: name.to_string(), + enable_async_client: true, + tracing_enabled: false, + ..Default::default() + }; + let generator = CodeGenerator::new(config); + let types_code = generator.generate(&mut analysis).expect("generate types"); + let client_code = generator + .generate_http_client(&analysis) + .expect("generate http client"); + + let temp = TempDir::new().expect("temp dir"); + let temp_path = temp.path(); + + fs::write( + temp_path.join("Cargo.toml"), + CLIENT_CARGO_TOML.replace("{name}", name), + ) + .expect("write Cargo.toml"); + + let src = temp_path.join("src"); + fs::create_dir_all(&src).expect("create src"); + fs::write(src.join("types.rs"), types_code).expect("write types.rs"); + fs::write(src.join("client.rs"), client_code).expect("write client.rs"); + fs::write(src.join("lib.rs"), "pub mod types;\npub mod client;\n").expect("write lib.rs"); + + let output = Command::new("cargo") + .arg("check") + .arg("--quiet") + .current_dir(temp_path) + .output() + .expect("run cargo check"); + + if !output.status.success() { + eprintln!( + "STDOUT:\n{}\n\nSTDERR:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + panic!("Generated client for `{name}` failed to compile"); + } + temp +} + +fn multi_response_spec() -> serde_json::Value { + serde_json::json!({ + "openapi": "3.1.0", + "info": { "title": "Multi-Response Test", "version": "1.0.0" }, + "paths": { + "/todos": { + "get": { + "operationId": "listTodos", + "responses": { + "200": { + "description": "Success", + "content": { + "application/json": { + "schema": { + "type": "array", + "items": { "$ref": "#/components/schemas/Todo" } + } + } + } + }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { "$ref": "#/components/schemas/BadRequest" } + } + } + } + } + } + }, + "/ping": { + "get": { + "operationId": "ping", + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { "type": "string" } + } + } + } + } + } + } + }, + "components": { + "schemas": { + "Todo": { + "type": "object", + "properties": { + "id": { "type": "string" }, + "title": { "type": "string" } + }, + "required": ["id", "title"] + }, + "BadRequest": { + "type": "object", + "properties": { + "error": { "type": "string" } + }, + "required": ["error"] + } + } + } + }) +} + +#[test] +fn test_generated_multi_response_client_compiles() { + let _temp = assert_generates_compiling_client("multi_response_client", multi_response_spec()); +} + +/// The anthropic and openai fixtures are realistic-shape API specs whose +/// generated clients must compile end-to-end. These regression tests catch +/// codegen breakage in non-trivial schema territory (oneOfs, discriminated +/// unions, etc) that the toy multi_response_spec doesn't exercise. +#[test] +fn test_generated_anthropic_client_compiles() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures/anthropic.yml"); + let raw = fs::read_to_string(&path).expect("read anthropic fixture"); + let spec: serde_json::Value = + serde_yaml::from_str(&raw).expect("parse anthropic fixture"); + let _temp = assert_generates_compiling_client("anthropic_client", spec); +} + +#[test] +fn test_generated_openai_client_compiles() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures/openai-responses.json"); + let raw = fs::read_to_string(&path).expect("read openai fixture"); + let spec: serde_json::Value = + serde_json::from_str(&raw).expect("parse openai fixture"); + let _temp = assert_generates_compiling_client("openai_client", spec); +} + +#[test] +fn test_generated_client_emits_per_op_error_enum() { + let spec = multi_response_spec(); + let mut analyzer = SchemaAnalyzer::new(spec).expect("analyzer"); + let analysis = analyzer.analyze().expect("analyze"); + + let config = GeneratorConfig { + module_name: "multi_response_client".to_string(), + enable_async_client: true, + ..Default::default() + }; + let generator = CodeGenerator::new(config); + let client_code = generator + .generate_http_client(&analysis) + .expect("generate http client"); + + // The op with a 400 schema should get its own typed error enum. + assert!( + client_code.contains("pub enum ListTodosApiError"), + "Expected ListTodosApiError enum in generated client. Code:\n{client_code}" + ); + assert!( + client_code.contains("Status400(BadRequest)"), + "Expected Status400(BadRequest) variant. Code:\n{client_code}" + ); + + // The signature should plug the per-op error type into ApiOpError. + assert!( + client_code.contains("ApiOpError"), + "list_todos signature should use ApiOpError. Code:\n{client_code}" + ); + + // The op without any non-2xx schemas should fall back to serde_json::Value. + assert!( + client_code.contains("ApiOpError"), + "ping signature should use ApiOpError. Code:\n{client_code}" + ); +} diff --git a/tests/operation_generation_test.rs b/tests/operation_generation_test.rs index d751cba..8f7d779 100644 --- a/tests/operation_generation_test.rs +++ b/tests/operation_generation_test.rs @@ -60,8 +60,8 @@ fn test_generate_get_operation() { assert!(result_str.contains("get_user")); // Verify HTTP method is GET assert!(result_str.contains(". get (request_url)")); - // Verify response type - assert!(result_str.contains("HttpResult < User >")); + // Verify response type — see issue #8 for the ApiOpError envelope. + assert!(result_str.contains("Result < User , ApiOpError <")); // Verify no request body parameter assert!(!result_str.contains("request :")); } @@ -103,8 +103,8 @@ fn test_generate_post_operation() { // Verify request body serialization (middleware-compatible) assert!(result_str.contains("serde_json :: to_vec (& request)")); assert!(result_str.contains("application/json")); - // Verify response type - assert!(result_str.contains("HttpResult < User >")); + // Verify response type — see issue #8 for the ApiOpError envelope. + assert!(result_str.contains("Result < User , ApiOpError <")); } #[test] @@ -271,8 +271,9 @@ fn test_method_with_response_type() { let result = generator.generate_operation_methods(&analysis); let result_str = result.to_string(); - // Verify response type is correctly mapped - assert!(result_str.contains("HttpResult < DataResponse >")); + // Verify response type is correctly mapped — see issue #8 for the + // ApiOpError envelope. + assert!(result_str.contains("Result < DataResponse , ApiOpError <")); } #[test] @@ -299,12 +300,13 @@ fn test_method_without_response_type() { let result = generator.generate_operation_methods(&analysis); let result_str = result.to_string(); - // Verify unit type for no response - assert!(result_str.contains("HttpResult < () >")); + // Verify unit type for no response — see issue #8 for the ApiOpError + // envelope. + assert!(result_str.contains("Result < () , ApiOpError <")); // Verify no deserialization for empty response (should use Ok(()) directly) assert!( - !result_str.contains("deserialization_error"), + !result_str.contains("failed to deserialize 2xx response body"), "Empty response type should not attempt JSON deserialization" ); assert!( @@ -339,11 +341,13 @@ fn test_error_handling_generation() { let result = generator.generate_operation_methods(&analysis); let result_str = result.to_string(); - // Verify error handling logic + // Verify error handling logic — the new envelope reads the body to a + // string before deserializing and wraps non-2xx (and 2xx parse failures) + // in `ApiOpError::Api(ApiError { ... })`. See issue #8. assert!(result_str.contains("let status = response . status ()")); assert!(result_str.contains("if status . is_success ()")); - assert!(result_str.contains("HttpError :: deserialization_error")); - assert!(result_str.contains("HttpError :: from_status")); + assert!(result_str.contains("response . text ()")); + assert!(result_str.contains("ApiOpError :: Api (ApiError")); } #[test] From 12c333f3f0d044e3fea50f5839fd27b185c084bf Mon Sep 17 00:00:00 2001 From: James Lal Date: Thu, 7 May 2026 13:04:49 -0600 Subject: [PATCH 2/2] style: cargo fmt Co-Authored-By: Claude Opus 4.7 (1M context) --- src/client_generator.rs | 13 +++++-------- tests/multi_response_client_test.rs | 12 ++++-------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/client_generator.rs b/src/client_generator.rs index 5c9cb43..949b157 100644 --- a/src/client_generator.rs +++ b/src/client_generator.rs @@ -417,8 +417,7 @@ impl CodeGenerator { .map(|(code, schema)| { let variant_ident = Self::op_error_variant_ident(code); let payload_ty_name = self.to_rust_type_name(schema); - let payload_ty = - syn::Ident::new(&payload_ty_name, proc_macro2::Span::call_site()); + let payload_ty = syn::Ident::new(&payload_ty_name, proc_macro2::Span::call_site()); quote! { #variant_ident(#payload_ty) } }) .collect(); @@ -840,8 +839,7 @@ impl CodeGenerator { .filter_map(|(code, schema)| { let variant_ident = Self::op_error_variant_ident(code); let payload_ty_name = self.to_rust_type_name(schema); - let payload_ty = - syn::Ident::new(&payload_ty_name, proc_macro2::Span::call_site()); + let payload_ty = syn::Ident::new(&payload_ty_name, proc_macro2::Span::call_site()); let enum_ident = self.op_error_enum_ident(op); let pattern = match code.as_str() { @@ -875,10 +873,9 @@ impl CodeGenerator { // Fallback for "default" or undeclared status codes: try to parse // as `serde_json::Value` for inspectability when the op's error // type is generic, otherwise leave typed = None. - let has_typed_enum = op - .response_schemas - .iter() - .any(|(code, _)| !code.starts_with('2') && !matches!(code.as_str(), "default" | "Default")); + let has_typed_enum = op.response_schemas.iter().any(|(code, _)| { + !code.starts_with('2') && !matches!(code.as_str(), "default" | "Default") + }); let default_arm = if has_typed_enum { quote! { diff --git a/tests/multi_response_client_test.rs b/tests/multi_response_client_test.rs index 9f4b4d0..84d11f0 100644 --- a/tests/multi_response_client_test.rs +++ b/tests/multi_response_client_test.rs @@ -161,21 +161,17 @@ fn test_generated_multi_response_client_compiles() { /// unions, etc) that the toy multi_response_spec doesn't exercise. #[test] fn test_generated_anthropic_client_compiles() { - let path = Path::new(env!("CARGO_MANIFEST_DIR")) - .join("tests/fixtures/anthropic.yml"); + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/anthropic.yml"); let raw = fs::read_to_string(&path).expect("read anthropic fixture"); - let spec: serde_json::Value = - serde_yaml::from_str(&raw).expect("parse anthropic fixture"); + let spec: serde_json::Value = serde_yaml::from_str(&raw).expect("parse anthropic fixture"); let _temp = assert_generates_compiling_client("anthropic_client", spec); } #[test] fn test_generated_openai_client_compiles() { - let path = Path::new(env!("CARGO_MANIFEST_DIR")) - .join("tests/fixtures/openai-responses.json"); + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/openai-responses.json"); let raw = fs::read_to_string(&path).expect("read openai fixture"); - let spec: serde_json::Value = - serde_json::from_str(&raw).expect("parse openai fixture"); + let spec: serde_json::Value = serde_json::from_str(&raw).expect("parse openai fixture"); let _temp = assert_generates_compiling_client("openai_client", spec); }